You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "itz-puneet (via GitHub)" <gi...@apache.org> on 2026/09/06 22:32:53 UTC
[PR] Add tests for sftp ConnectionNotOpenedException [airflow]
itz-puneet opened a new pull request, #72605:
URL: https://github.com/apache/airflow/pull/72605
closes: #72268
`providers/sftp/src/airflow/providers/sftp/exceptions.py` had no test module and was not covered
indirectly either — nothing under any `providers/*/tests/` directory imported it — so it sat on the
`OVERLOOKED_TESTS` allowlist in `airflow-core/tests/unit/always/test_project_structure.py`.
The module declares a single exception, `ConnectionNotOpenedException`. As the issue points out,
asserting the class declaration on its own would be worth very little, so the tests target the
behaviour that raises it instead: the `handle_connection_management` decorator in
`providers/sftp/src/airflow/providers/sftp/hooks/sftp.py`. That decorator is the exception's only
raise site, and no test in `providers/sftp/tests/` exercised that branch either.
What the new tests pin down:
- an unmanaged call (`use_managed_conn=False`) with no open connection raises
`ConnectionNotOpenedException`, and the message points the caller at `hook.get_managed_conn()`
- an unmanaged call with a connection already open delegates through to the wrapped function,
forwards its arguments, returns its result, and does not open a managed connection
- a managed call (`use_managed_conn=True`) opens a managed connection instead of raising, sets it on
the hook for the duration of the call, and releases it — including when the wrapped call raises
- `ConnectionNotOpenedException` derives from `AirflowException`, which matters because callers catch
the base class and narrowing it later would be a breaking change for them
The decorator runs against a small stub hook exposing only the three attributes it touches
(`use_managed_conn`, `conn`, `get_managed_conn`), so the tests need no SFTP server, no database and
no network, and run in milliseconds.
`providers/sftp/tests/unit/sftp/test_exceptions.py` is also removed from `OVERLOOKED_TESTS`, per the
definition of done in the issue.
---
**Gen-AI disclosure:** I used an AI coding assistant while drafting these tests. I reviewed the
decorator's behaviour against the source myself and checked each assertion fails against a
deliberately broken decorator before submitting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add tests for sftp ConnectionNotOpenedException [airflow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #72605:
URL: https://github.com/apache/airflow/pull/72605#discussion_r3945479057
##########
providers/sftp/tests/unit/sftp/test_exceptions.py:
##########
@@ -0,0 +1,114 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from collections.abc import Generator
+from contextlib import contextmanager
+from typing import Any
+
+import pytest
+
+from airflow.providers.common.compat.sdk import AirflowException
+from airflow.providers.sftp.exceptions import ConnectionNotOpenedException
+from airflow.providers.sftp.hooks.sftp import handle_connection_management
+
+
+class FakeSFTPHook:
+ """
+ Minimal stand-in for :class:`~airflow.providers.sftp.hooks.sftp.SFTPHook`.
+
+ ``handle_connection_management`` only reads ``use_managed_conn`` and ``conn`` and calls
+ ``get_managed_conn``, so the decorator can be exercised without reaching an SFTP server.
+ """
+
+ def __init__(self, *, use_managed_conn: bool, conn: Any = None) -> None:
+ self.use_managed_conn = use_managed_conn
+ self.conn = conn
+ self.managed_conn = "managed-conn"
+ self.managed_conn_opened = 0
+ self.managed_conn_released = 0
+ self.conn_seen_by_wrapped: Any = None
+
+ @contextmanager
+ def get_managed_conn(self) -> Generator[str, None, None]:
+ self.managed_conn_opened += 1
+ try:
+ yield self.managed_conn
+ finally:
+ self.managed_conn_released += 1
+
+ @handle_connection_management
+ def do_work(self, path: str, recursive: bool = False) -> str:
+ self.conn_seen_by_wrapped = self.conn
+ return f"{path}:{recursive}"
+
+ @handle_connection_management
+ def do_failing_work(self) -> None:
+ raise RuntimeError("wrapped call failed")
+
+
+class TestConnectionNotOpenedException:
+ def test_subclasses_airflow_exception(self) -> None:
+ # Callers catch the base class, so narrowing this later would be a breaking change.
+ assert issubclass(ConnectionNotOpenedException, AirflowException)
+
+ def test_keeps_its_message(self) -> None:
+ assert str(ConnectionNotOpenedException("connection is not open")) == "connection is not open"
+
Review Comment:
`test_keeps_its_message` is asserting Python's default exception message behavior (i.e., `Exception.__str__`) rather than provider logic, so it doesn't meaningfully guard against regressions in `ConnectionNotOpenedException` (it has no custom behavior). Consider removing this test and keeping the message assertion on the actual raise site in `handle_connection_management` (which you already cover below).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add tests for sftp ConnectionNotOpenedException [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #72605:
URL: https://github.com/apache/airflow/pull/72605#issuecomment-5562648562
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](https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [prek-hooks]( https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#prerequisites-for-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](https://github.com/apache/airflow/blob/main/airflow-core/docs/howto/custom-operator.rst) Consider adding an example Dag that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst) 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](https://www.apache.org/foundation/policies/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]( https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#coding-style-and-best-practices).
- 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add tests for sftp ConnectionNotOpenedException [airflow]
Posted by "itz-puneet (via GitHub)" <gi...@apache.org>.
itz-puneet commented on code in PR #72605:
URL: https://github.com/apache/airflow/pull/72605#discussion_r3945624349
##########
providers/sftp/tests/unit/sftp/test_exceptions.py:
##########
@@ -0,0 +1,114 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from collections.abc import Generator
+from contextlib import contextmanager
+from typing import Any
+
+import pytest
+
+from airflow.providers.common.compat.sdk import AirflowException
+from airflow.providers.sftp.exceptions import ConnectionNotOpenedException
+from airflow.providers.sftp.hooks.sftp import handle_connection_management
+
+
+class FakeSFTPHook:
+ """
+ Minimal stand-in for :class:`~airflow.providers.sftp.hooks.sftp.SFTPHook`.
+
+ ``handle_connection_management`` only reads ``use_managed_conn`` and ``conn`` and calls
+ ``get_managed_conn``, so the decorator can be exercised without reaching an SFTP server.
+ """
+
+ def __init__(self, *, use_managed_conn: bool, conn: Any = None) -> None:
+ self.use_managed_conn = use_managed_conn
+ self.conn = conn
+ self.managed_conn = "managed-conn"
+ self.managed_conn_opened = 0
+ self.managed_conn_released = 0
+ self.conn_seen_by_wrapped: Any = None
+
+ @contextmanager
+ def get_managed_conn(self) -> Generator[str, None, None]:
+ self.managed_conn_opened += 1
+ try:
+ yield self.managed_conn
+ finally:
+ self.managed_conn_released += 1
+
+ @handle_connection_management
+ def do_work(self, path: str, recursive: bool = False) -> str:
+ self.conn_seen_by_wrapped = self.conn
+ return f"{path}:{recursive}"
+
+ @handle_connection_management
+ def do_failing_work(self) -> None:
+ raise RuntimeError("wrapped call failed")
+
+
+class TestConnectionNotOpenedException:
+ def test_subclasses_airflow_exception(self) -> None:
+ # Callers catch the base class, so narrowing this later would be a breaking change.
+ assert issubclass(ConnectionNotOpenedException, AirflowException)
+
+ def test_keeps_its_message(self) -> None:
+ assert str(ConnectionNotOpenedException("connection is not open")) == "connection is not open"
+
Review Comment:
Agreed — removed in cfd5a59. The class has no custom `__str__`, so that assertion was testing the standard library; the message is still pinned where it matters, at the raise site in `test_raises_when_unmanaged_and_no_connection_is_open`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add tests for sftp ConnectionNotOpenedException [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on PR #72605:
URL: https://github.com/apache/airflow/pull/72605#issuecomment-5732660375
Thanks for your contribution!
Unfortunately there's another [PR ](https://github.com/apache/airflow/pull/72456) for this issue that I find more suitable for merging at this point.
Feel free to create PRs for other GitHub issues!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add tests for sftp ConnectionNotOpenedException [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 closed pull request #72605: Add tests for sftp ConnectionNotOpenedException
URL: https://github.com/apache/airflow/pull/72605
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org