Remove redundant tests from Task SDK#68504
Conversation
eladkal
added
the
changelog:skip
shahar1
force-pushed
the
remove-redundant-task-sdk-tests
branch
from
9e28a55 to
996b89e
Compare
| assert MinimumCount(5).n == 5 | ||
|
|
||
| def test_eq_same_n(self): | ||
| assert MinimumCount(5) == MinimumCount(5) |
There was a problem hiding this comment.
I understand removing test_eq_same_n on the grounds that __eq__ is generated by attrs, rather than implemented explicitly by Airflow.
However, this test also protects the value-equality semantics of MinimumCount. For example, changing the class to the following would make MinimumCount(5) != MinimumCount(5), while the remaining test_neq_different_n would still pass:
@attrs.define(frozen=True, eq=False)
class MinimumCount(WaitPolicy):
...- rm
test_eq_same_n&test_hash_consistent
- keep
test_eq_same_n&test_hash_consistent
Since the test can detect a change in the class's observable behavior, should we keep one positive equality test to make this contract explicit?
Lee-W
left a comment
Lee-W
left a comment
There was a problem hiding this comment.
I feel we should keep P1 cases. We've been bitten by lib from time to time. P2 might be able to be removed in some cases, but it also ensure the value is not changed during object creation. totally agree with P5
related: apache#68502 These tests are exact or near-exact duplicates of sibling tests that already assert the same behavior, so they add maintenance cost without adding coverage: - test_mask_secret_with_list / test_mask_secret_with_iterable duplicate each other and are subsumed by the parametrized test_mask_secret_with_objects. - test_build_task_group_with_prefix_functionality duplicates test_build_task_group_with_prefix. - test_override_dag_default_args is byte-identical to test_default_args. Tests that guard against a library or attrs-generated behavior change (equality, hashing, repr, exception-hierarchy catches, attribute round-trips) are kept, per review feedback. Generated-by: Claude Code (Opus 4.8)
shahar1
force-pushed
the
remove-redundant-task-sdk-tests
branch
from
a3eec60 to
3d26f48
Compare
5 participants
Human Summary
related: #68502.
This PR removes tests that are exact or near-exact duplicates of sibling tests
already asserting the same behavior — they add maintenance cost without adding
coverage.
Following review feedback, tests that guard against a library or attrs-generated
behavior change (equality, hashing, repr, exception-hierarchy catches, attribute
round-trips) are kept; only true duplicates are removed. Summarizing table is in
the "AI Summary".
AI Summary
Click Here
Legend — patterns:
test_build_task_group_with_prefix_functionalitytest_build_task_group_with_prefix: same Dag structure, same 7 assertions, only the Dag name string and inline comments differtest_override_dag_default_argstest_default_args(same Dag, sameTaskGroupdefault_args, same three owner assertions); only the name and docstring differtest_mask_secret_with_listtest_mask_secret_with_objects, which already covers a list input alongside dict and string casestest_mask_secret_with_iterabletest_mask_secret_with_list: identical setupexample_dict = ["test"]and identical assertion — claims to test an iterable but uses the same listWas generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 4.8) following the guidelines