yuseok89 commented on code in PR #74145:
URL: https://github.com/apache/airflow/pull/74145#discussion_r4192418198
##########
providers/ftp/tests/unit/ftp/sensors/test_ftp.py:
##########
@@ -93,3 +96,94 @@ def test_poke_ignore_transient_error(self, mock_hook):
assert not op.poke(None)
assert op.poke(None)
+
+ @pytest.mark.parametrize(("sensor_cls", "hook_cls"), [(FTPSensor,
FTPHook), (FTPSSensor, FTPSHook)])
Review Comment:
Some new cases seem to overlap. `FTPSSensor` only overrides `_create_hook`,
so the FTPS parametrization runs the same code. This `test and
test_poke_retries_temporary_reply_from_ftp_client` cover the same retry path.
The `keeps_raise_policy` cases pass without the fix. Could we keep only the
cases that fail without the change?
##########
providers/ftp/tests/unit/ftp/sensors/test_ftp.py:
##########
@@ -93,3 +96,94 @@ def test_poke_ignore_transient_error(self, mock_hook):
assert not op.poke(None)
Review Comment:
This test and the two above use `error_perm("434 ...")`, but ftplib raises
`error_temp` for 4xx replies. Switching them to `error_temp` makes
`test_poke_ignore_transient_error` fail on main and pass with this fix. Could
we update these instead of adding new sensor tests? That would keep the test
diff small and stop the existing tests from relying on a reply ftplib never
produces.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
