Dev-iL commented on code in PR #73403:
URL: https://github.com/apache/airflow/pull/73403#discussion_r4146261028
##########
airflow-core/tests/unit/api_fastapi/execution_api/conftest.py:
##########
Review Comment:
This, as well as the associated `usefixtures` is no longer necessary
following #73554 that dropped this fixture and gave every test client its own
async engine.
<details><summary>More info by AI</summary>
<p>
#73554 removed this fixture from `main` and made the Execution API `client`
depend on `async_db_engine`, which configures a fresh engine and disposes it
through `client.portal` at teardown.
**Verification**: This branch rebases onto current `main` without conflicts,
and on the rebased branch the fixture now runs before `async_db_engine`: pytest
sets up `usefixtures` names ahead of argument fixtures, so `async_db_engine`
records the reconfigured engine as the "previous" one and restores it
afterwards, while the original engine is replaced without `dispose()`. With the
fixture and all twelve tags removed, the tests for the converted routes (plus
`execution_api/test_app.py`, 64 in total) pass on SQLite (`aiosqlite`),
PostgreSQL (`psycopg_async`) and MySQL (`aiomysql`) through Breeze.
</p>
</details>
##########
airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_assets.py:
##########
@@ -108,3 +113,27 @@ def test_asset_uri_not_found(self, client):
"reason": "not_found",
}
}
+
+
[email protected]("reconfigure_async_db_engine")
+class TestGetAssetAsyncQueries:
Review Comment:
Apologies, I can only provide an AI comment to this.
> The DB-backed tests already fail when an `await` is missing, so these
mocks add coupling rather than coverage. Removing the `await` from
`get_asset_by_name` (`assets.py:43`), `get_dr_count` (`dag_runs.py:265`) and
`get_start_date` (`task_reschedules.py:45`) and running only the existing
non-mock tests fails 10 of 11: `AssetResponse` validation of a coroutine,
`ResponseValidationError` on `/count` (the `or 0` passes the truthy coroutine
through) and on `/start_date`. Patching `AsyncSession.scalar` class-wide also
breaks the test on an equivalent rewrite such as
`execute(...).scalar_one_or_none()`, and the by-uri case returns the same
`AssetModel` whatever filter the handler uses. Drop `TestGetAssetAsyncQueries`
and the three `test_awaits_async_*` tests in `test_dag_runs.py:523,552` and
`test_task_instances.py:3448`, and note in the description that the existing
DB-backed contract tests are the regression net for this refactor.
--
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]
