dima-asana commented on a change in pull request #5308: [AIRFLOW-4549] skipped
tasks should be ok for wait_for_downstream
URL: https://github.com/apache/airflow/pull/5308#discussion_r285941524
##########
File path: tests/models/test_dagrun.py
##########
@@ -560,3 +562,87 @@ def with_all_tasks_removed(dag):
dagrun.verify_integrity()
flaky_ti.refresh_from_db()
self.assertEqual(State.NONE, flaky_ti.state)
+
Review comment:
I'm not really sure where to put these integration-like tests. They only
check functionality on task instances. But they require having DagRuns for the
tasks because of how `previous_ti` works
(https://github.com/apache/airflow/blob/e1c1a8dad0e01a7baa50ebe02e748429d33241fd/airflow/models/taskinstance.py#L533-L540).
They also require something to actually try running the tasks (I used
dag.run). And the exception they raise is because of scheduler deadlock as
expected. So I could rationalize putting this here, SchedulerJobTest, or
TaskInstanceTest. Let me know if someone more familiar with the code has a
preference.
I'm also not sure they're strictly necessary. I think this code is
reasonably unit tested through the combination of
https://github.com/apache/airflow/blob/e1c1a8dad0e01a7baa50ebe02e748429d33241fd/tests/ti_deps/deps/test_prev_dagrun_dep.py
and
https://github.com/apache/airflow/blob/5acc221a0993296b00430626de107515acb9c2d4/tests/models/test_taskinstance.py#L519-L545.
At the same time, it might be more straightforward to follow this example in
an actual Dag like here, and the task_instance.run() part of this is only
tested in accidentally tangent unit tests.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
