Skip to content

Add missing tests for sftp exceptions - #72456

Merged
shahar1 merged 2 commits into
apache:mainfrom
vitorantoniazzi:sftp-test-exceptions
Sep 23, 2026
Merged

shahar1 merged 2 commits into
apache:mainfrom
vitorantoniazzi:sftp-test-exceptions

Conversation

@vitorantoniazzi

Copy link
Copy Markdown
Contributor

Summary

exceptions.py in the sftp provider had no tests. Nothing in providers/*/tests/ imported it either, so it wasn't getting covered by accident.

There's not much point in testing the exception class by itself. The one place that raises it is the handle_connection_management decorator, which had no tests of its own, so this covers the decorator and picks up the exception on the way through.

The test walks through four cases. With no open connection and use_managed_conn=False, it raises, and the message points you at hook.get_managed_conn(). With a connection already open, it just delegates and returns. With use_managed_conn=True, it opens a managed connection instead of raising, and sets it on the hook for the length of the call. And the exception subclasses AirflowException, which is worth pinning down since anyone catching the base class is counting on it.

Everything runs against a stub hook, so you don't need an SFTP server to run it.

I also removed the OVERLOOKED_TESTS entry.

Closes: #72268

Test Plan

The four tests pass against apache-airflow-providers-sftp on Airflow 3.3.1 / Python 3.12, importing the real decorator and exception. ruff check and ruff format --check pass using Airflow's own ruff configuration.


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Tests drafted with Claude Code (Opus 5); reviewed and verified by @vitorantoniazzi

providers/sftp/src/airflow/providers/sftp/exceptions.py had no dedicated test
module and was not covered indirectly. The only raise site for
ConnectionNotOpenedException is the handle_connection_management decorator in
the sftp hook, which was untested as well, so the new module drives that
decorator against a stub hook and drops the OVERLOOKED_TESTS entry.

Closes: apache#72268
@boring-cyborg

boring-cyborg Bot commented Sep 2, 2026

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@vitorantoniazzi

Copy link
Copy Markdown
Contributor Author

The workflow runs on this PR have been sitting in action_required since it was opened on Sept 2 — Tests (AMD), CodeQL and Check newsfragment PR number all need a maintainer to approve them, as this is my first contribution to the repo. The only green checks are the two third-party apps that run regardless, so the test suite hasn't actually executed and there's no CI signal to review against yet.

Could a committer approve the workflow runs when convenient? Happy to fix whatever comes up once they do.

This adds the missing exception tests requested in #72268. cc @jroachgolf84

Comment thread providers/sftp/tests/unit/sftp/test_exceptions.py
@shahar1
shahar1 merged commit 9290c26 into apache:main Sep 23, 2026
82 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add missing tests for sftp exceptions

3 participants