dstandish commented on code in PR #39351:
URL: https://github.com/apache/airflow/pull/39351#discussion_r1586546901
##########
tests/sensors/test_base.py:
##########
@@ -284,62 +284,59 @@ def test_ok_with_reschedule(self, make_sensor,
time_machine, task_reschedules_fo
if ti.task_id == DUMMY_OP:
assert ti.state == State.NONE
- def test_fail_with_reschedule(self, make_sensor, time_machine):
+ def test_fail_with_reschedule(self, make_sensor, time_machine, session):
sensor, dr = make_sensor(return_value=False, poke_interval=10,
timeout=5, mode="reschedule")
+ def _get_tis():
+ tis = dr.get_task_instances(session=session)
+ yield next(x for x in tis if x.task_id == SENSOR_OP)
Review Comment:
and i feel sometimes we overdo the dryness in tests (and in general)
like in this PR i remove the helper inner function `assert_ti_state` which
was a kind of "dry" way of doing the asserts but... it actually made it harder
to see what was being asserted and to see where the failure was. incidentally
that function def was repeated in the distinct test functions in the same way
_get_tis is in my PR.
and when you add more shared layers you either have to make assumptions
about what different tests will need or build out a sufficiently complicated
interface.
e.g. in this case if i made `_get_tis` dry, then my calls would have to
change to something like
```
sensor_ti, dummy_ti = _get_tis(dr, [DUMMY_OP, SENSOR_OP], session)
```
which is just more noise and more complicated compared to
```
sensor_ti, dummy_ti = _get_tis()
```
i dunno. i don't think it's _wrong_ to DRY it in this way, but i also think
it's reasonable to not do 🤷
BUT IF YOU FEEL STRONGLY let me know and i'll just do it so we can move on
:)
--
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]
