Copilot commented on code in PR #64968:
URL: https://github.com/apache/airflow/pull/64968#discussion_r3066473759
##########
airflow-core/tests/unit/models/test_taskinstance.py:
##########
@@ -2605,6 +2605,81 @@ def
test_task_instance_history_is_created_when_ti_goes_for_retry(self, dag_maker
# the new try_id should be different from what's recorded in tih
assert tih[0].task_instance_id == try_id
+ @pytest.mark.parametrize(
+ ("first_ti", "second_ti"),
+ [
+ pytest.param(
+ ("dag_1", "run_1", "task_1", -1),
+ ("dag_2", "run_1", "task_1", -1),
+ id="tasks_with_different_dags",
+ ),
+ pytest.param(
+ ("dag_1", "run_1", "task_1", -1),
+ ("dag_1", "run_2", "task_1", -1),
+ id="tasks_with_different_runs",
+ ),
+ # There are no cases with equal dag_id/run_id because
create_task_instance()
+ # creates a DagRun each time, and DagRun has a unique (dag_id,
run_id) constraint.
+ ],
+ )
+ def test_get_task_instance_disambiguates_by_dag_id(
+ self, create_task_instance, session, first_ti, second_ti
+ ):
+ dag_id_1, run_id_1, task_id_1, map_index_1 = first_ti
+ dag_id_2, run_id_2, task_id_2, map_index_2 = second_ti
+
+ ti1 = create_task_instance(
+ dag_id=dag_id_1,
+ run_id=run_id_1,
+ task_id=task_id_1,
+ map_index=map_index_1,
+ session=session,
+ )
+ ti2 = create_task_instance(
+ dag_id=dag_id_2,
+ run_id=run_id_2,
+ task_id=task_id_2,
+ map_index=map_index_2,
+ session=session,
+ )
+
+ # Regression setup for #64957: if dag_id is ignored, this lookup key
becomes ambiguous.
+ if dag_id_1 != dag_id_2:
+ ambiguous_count = session.scalar(
+ select(func.count())
+ .select_from(TI)
+ .filter_by(run_id=run_id_1, task_id=task_id_1,
map_index=map_index_1)
+ )
+ assert ambiguous_count == 2, "Setup failure: expected two TIs
matching without dag_id filter"
+
+ # This case does not target the original regression directly (run_id
was already filtered),
Review Comment:
PR description mentions adding a regression test named
`test_instance_dag_and_run_id_uniqueness`, but the added test is named
`test_get_task_instance_disambiguates_by_dag_id`. Either update the PR
description to match the implemented test name, or rename the test to match the
description to reduce confusion when searching for the referenced regression
test.
##########
airflow-core/tests/unit/models/test_taskinstance.py:
##########
@@ -2605,6 +2605,81 @@ def
test_task_instance_history_is_created_when_ti_goes_for_retry(self, dag_maker
# the new try_id should be different from what's recorded in tih
assert tih[0].task_instance_id == try_id
+ @pytest.mark.parametrize(
+ ("first_ti", "second_ti"),
+ [
+ pytest.param(
+ ("dag_1", "run_1", "task_1", -1),
+ ("dag_2", "run_1", "task_1", -1),
+ id="tasks_with_different_dags",
+ ),
+ pytest.param(
+ ("dag_1", "run_1", "task_1", -1),
+ ("dag_1", "run_2", "task_1", -1),
+ id="tasks_with_different_runs",
+ ),
+ # There are no cases with equal dag_id/run_id because
create_task_instance()
+ # creates a DagRun each time, and DagRun has a unique (dag_id,
run_id) constraint.
+ ],
+ )
+ def test_get_task_instance_disambiguates_by_dag_id(
Review Comment:
The test name implies it only validates `dag_id` disambiguation, but the
parametrization also includes a `run_id`-only variation
(`tasks_with_different_runs`). Consider renaming to reflect both dimensions
(e.g., `..._by_dag_id_and_run_id`) or splitting into two tests so the name
matches the covered cases.
```suggestion
def test_get_task_instance_disambiguates_by_dag_id_and_run_id(
```
--
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]
