kaxil commented on code in PR #72890:
URL: https://github.com/apache/airflow/pull/72890#discussion_r3981021763
##########
airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_task_instances.py:
##########
@@ -216,6 +216,30 @@ def
test_ti_run_context_exposes_consumed_event_partition_key(self, client, sessi
events = response.json()["dag_run"]["consumed_asset_events"]
assert [e["partition_key"] for e in events] == ["2024-01-15"]
+ @mock.patch("sqlalchemy.orm.Session.scalars")
+ def test_ti_run_missing_dagrun_returns_404(self, mock_scalars, client,
session, create_task_instance):
+ """A missing DagRun must surface as a clean 404, not an internal
500."""
+ ti = create_task_instance(
+ task_id="test_ti_run_missing_dagrun",
+ state=State.QUEUED,
+ session=session,
+ )
+ session.commit()
+
+ # Force the DagRun lookup (the only scalars() call before the guard)
to return None.
+ mock_scalars.return_value.unique.return_value.one_or_none.return_value
= None
+
+ response = client.patch(
+ f"/execution/task-instances/{ti.id}/run",
+ json=self.RUN_PAYLOAD,
Review Comment:
This PR carries `backport-to-v3-3-test`, but `RUN_PAYLOAD` was added to this
class on main after the branch cut (the `v3-3-test` copy of this file has no
`RUN_PAYLOAD` at all), and the surrounding hunk is identical there, so the
cherry-pick applies cleanly and then fails with `AttributeError`. Inlining the
payload dict here, as the sibling tests on `v3-3-test` do, keeps the backport
clean.
##########
airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_task_instances.py:
##########
@@ -216,6 +216,30 @@ def
test_ti_run_context_exposes_consumed_event_partition_key(self, client, sessi
events = response.json()["dag_run"]["consumed_asset_events"]
assert [e["partition_key"] for e in events] == ["2024-01-15"]
+ @mock.patch("sqlalchemy.orm.Session.scalars")
Review Comment:
As a decorator this patches `Session.scalars` while `create_task_instance`
runs too, so `bulk_write_to_db` gets a `MagicMock` back from `find_orm_dags`
and only works because `setup_method` cleared the tables first.
`test_ti_run_database_error` further down wraps just the `client.patch(...)`
call in `with mock.patch(...)` after the commit; the same shape here (with
`autospec=True`) keeps the mock out of the fixture setup.
##########
airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_task_instances.py:
##########
@@ -216,6 +216,30 @@ def
test_ti_run_context_exposes_consumed_event_partition_key(self, client, sessi
events = response.json()["dag_run"]["consumed_asset_events"]
assert [e["partition_key"] for e in events] == ["2024-01-15"]
+ @mock.patch("sqlalchemy.orm.Session.scalars")
+ def test_ti_run_missing_dagrun_returns_404(self, mock_scalars, client,
session, create_task_instance):
+ """A missing DagRun must surface as a clean 404, not an internal
500."""
+ ti = create_task_instance(
+ task_id="test_ti_run_missing_dagrun",
+ state=State.QUEUED,
+ session=session,
+ )
+ session.commit()
+
+ # Force the DagRun lookup (the only scalars() call before the guard)
to return None.
+ mock_scalars.return_value.unique.return_value.one_or_none.return_value
= None
+
+ response = client.patch(
+ f"/execution/task-instances/{ti.id}/run",
+ json=self.RUN_PAYLOAD,
+ )
+
+ assert response.status_code == 404
+ assert response.json()["detail"] == {
+ "reason": "not_found",
+ "message": f"DagRun with dag_id={ti.dag_id} and run_id={ti.run_id}
not found.",
Review Comment:
This still expects `not found.` with a trailing period, but the route
message dropped the period in 909a094 when the suggestion above was applied, so
the test fails at HEAD on that one character. I ran it in breeze: fails as
pushed, passes with the period removed here as well.
```suggestion
"message": f"DagRun with dag_id={ti.dag_id} and
run_id={ti.run_id} not found",
```
--
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]
