Skip to content

Conversation

@kyungjunleeme
Copy link
Contributor

@kyungjunleeme kyungjunleeme commented Jul 18, 2025

This PR adds the following tests to verify the correct handling of deferrable sensors:

test_short_circuit_operator_skips_deferrable_sensors

  • Ensures that a deferrable sensor downstream of a ShortCircuitOperator is correctly skipped.

test_ensure_tasks_includes_deferrable_sensors_airflow_3x

  • Verifies that _ensure_tasks() properly includes deferrable sensors in its result.

These tests are a follow-up to #53455 and focus specifically on validating deferrable sensor behavior to ensure consistency with non-deferrable operators.

closes #53461
related with #52869


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@kyungjunleeme kyungjunleeme changed the title Test/improve skip logic tests Add tests for deferrable sensor behavior with ShortCircuitOperator and _ensure_tasks Jul 18, 2025
Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no value in testing deferrable for Shortcircuit separately imo

@kyungjunleeme
Copy link
Contributor Author

You're right that the root cause was not the deferrable flag itself, but the type mismatch in _ensure_tasks().
However, since the bug manifested specifically when using deferrable sensors under ShortCircuitOperator, I believe this test still adds value as a regression check.
It helps protect against future issues if version checks or SDK operator typing logic changes again.

@kaxil
Copy link
Member

kaxil commented Jul 19, 2025

You're right that the root cause was not the deferrable flag itself, but the type mismatch in _ensure_tasks(). However, since the bug manifested specifically when using deferrable sensors under ShortCircuitOperator, I believe this test still adds value as a regression check. It helps protect against future issues if version checks or SDK operator typing logic changes again.

I'll have to respectfully disagree. That will be a redundant test.

@kaxil kaxil closed this Jul 19, 2025
@kyungjunleeme
Copy link
Contributor Author

Thank you for your feedback. I completely understand your perspective, and I fully respect your decision to close the PR on the grounds that the test appears redundant at this point.

That said, I personally see potential value in retaining such a test as a regression safeguard—especially considering the subtle interplay between SDK operator typing and future changes in version logic.

I'd like to take some time to investigate this further and better understand whether it truly adds no additional value. If I uncover anything meaningful, I’ll be sure to share it respectfully for further discussion.
I sincerely appreciate your time and thoughtful review.

@kyungjunleeme kyungjunleeme deleted the test/improve-skip-logic-tests branch July 20, 2025 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants