ferruzzi opened a new issue, #71577: URL: https://github.com/apache/airflow/issues/71577
### Body This is a direct follow-up to https://github.com/apache/airflow/issues/68374 with all the same reasoning: We have a lot of unit tests which start by cleaning the DB in the setup phase. This is a clear indication that _something_ is leaking artifacts into the database causing other tests to fail. All unit test fixtures which create a row should own that row and clean it up. A blanket bd cleanup in the unit test's teardown should be an exception when there is no fixture, not the norm. No unit test should require pre-cleaning in setup in order to avoid another test's garbage. As before, I'll get to this eventually, but I'm putting up an Issue in case someone wants to get to it before I do. After @seanghaeli pointed out other spots we didn't clean up, I set Claude on the task of finding any remaining unit tests that match the pre-cleaning pattern and it found A LOT. The numbers below are unverified Claude output, but should serve as a solid launching point. --- ``` Tables cleared in setup position, by frequency: 91 clear_db_runs 7 clear_db_pools 53 clear_db_dags 7 clear_db_logs 26 clear_db_assets 7 clear_db_import_errors 18 clear_db_connections 6 clear_db_deadline 17 clear_db_serialized_dags 5 clear_db_backfills 14 clear_db_jobs 4 clear_db_apdr / pakl / callbacks / triggers 9 clear_db_variables 3 clear_db_revoked_tokens / xcom 9 clear_db_dag_bundles 2 clear_db_deadline_alert 8 clear_db_connection_tests 1 clear_db_dag_warnings / dag_code / task_reschedule 8 clear_db_teams ``` --- Notably, `dag_maker.cleanup()` has the same gap for assets that it had for bundles before #69093: it deletes `AssetEvent` but not the `AssetModel` / `AssetActive` / `AssetAliasModel` rows a Dag's schedule creates. Fixing it the same way (track what was created, delete when unreferenced, FK-safe order) should account for the ~26 `clear_db_assets` pre-cleans on its own. ### Proposal I'm using that term very loosely, these are just Thoughts. Cleaning up the codebase is not very useful without enforcing the change, so some ideas in addition to the actual clean up: 1. A new enforcer: I don't know if this is a pre-commit or a ruff check or what this actually looks like, but we should be preventing tests from calling db_clean in the setup. If a new test needs that to pass, something is already wrong and we're just masking an existing issue. 2. A new "leak detector": This might be in the form of a fixture which snapshots the pre- and post-test table row counts and reports if there is any variation? Maybe include it in the existing "this test needs a db" fixture? 3. Several tests create `Job` rows by hand and nothing cleans them up; we might want a `job_maker` fixture akin to `dag_maker` which tracks those creations and cleans up on teardown? Similarly, it doesn't look like anything owns tracking and cleaning up Connections, Variables, or Pools. maybe a similar pattern can be employed there? We can either add the enforcer ahead of time with an allowlist of existing cases which we clean as we go, or we need to clean before that lands. I kind of like the former as it doubles as a tracker for remaining work and closes the floodgate so we're not stuck making another pass later to catch the issues that were merged while we were working. ### Triage Rules: How do you tell a deletable site from a load-bearing one? I see three cases: - Already covered by `dag_maker` (it deletes `DagRun`, `TaskInstance`, `DagVersion`, `XCom`, `DagModel`, `TaskMap`, plus its own bundles, and `SerializedDagModel` / `DeadlineAlert` cascade off `dag_version.id` and `serialized_dag.id`). Delete the pre-clean, unless the test bypasses `dag_maker` via `DagBag`, example Dags, or a hand-built `DagModel`. - Genuine fixture gap (assets today). Fix the fixture first, then delete. - Nothing owns the table (`Job`, `Log`, `Pool`, `Variable`, `Connection`, `ConnectionTest`, `RevokedToken`, `Backfill`). Load-bearing until an owning fixture exists; do not delete yet. --- Once we have the framework in place to prevent regression, here is what Claude reports for the current violations that need to be fixed. Included here for a sense of scope only, I have not validated the numbers and naturally this will drift between now and then anyway: ``` ┌───────────────────────────────────────────────────┬───────┬───────┬────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ Batch │ Sites │ Files │ Notes │ ├───────────────────────────────────────────────────┼───────┼───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ api_fastapi/execution_api/versions/ │ 42 │ 12 │ Almost entirely clear_db_runs. Most mechanical batch; good starter. │ ├───────────────────────────────────────────────────┼───────┼───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ api_fastapi/core_api/routes/ui/ │ 39 │ 10 │ Includes the apdr / pakl partition tables. │ ├───────────────────────────────────────────────────┼───────┼───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ api_fastapi/core_api/routes/public/ │ 34 │ 14 │ Blocked on Pool / Variable / Connection / Log ownership. │ ├───────────────────────────────────────────────────┼───────┼───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ api_fastapi misc (common/, auth/, conftest.py, │ 10 │ 5 │ │ │ core_api/test_app.py) │ │ │ │ ├───────────────────────────────────────────────────┼───────┼───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ models/ │ 46 │ 18 │ Blocked on the dag_maker asset fix. Also has one leftover bundle/team site from #68374. │ ├───────────────────────────────────────────────────┼───────┼───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ cli/commands/ │ 24 │ 9 │ Heavy on connections and variables; blocked on Connection / Variable ownership. │ ├───────────────────────────────────────────────────┼───────┼───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ dag_processing/ │ 15 │ 3 │ Includes two leftover bundle/team sites from #68374. │ ├───────────────────────────────────────────────────┼───────┼───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ jobs/ │ 12 │ 2 │ test_scheduler_job.py + test_triggerer_job.py. Blocked on Job ownership. │ ├───────────────────────────────────────────────────┼───────┼───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ utils/ │ 10 │ 4 │ │ ├───────────────────────────────────────────────────┼───────┼───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ core misc: always/, api/, assets/, core/, state/, │ 19 │ 10 │ One combined batch. Includes one leftover team site in ti_deps/. │ │ ti_deps/, timetables/ │ │ │ │ ├───────────────────────────────────────────────────┼───────┼───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ providers: standard 8, celery 6, snowflake 3, │ 19 │ 8 │ Version-gated behind AIRFLOW_V_3_0_PLUS in places, so this batch carries provider compat rules the core batches do │ │ common.io 2 │ │ │ not. Also has the last leftover bundle site from #68374. │ ├───────────────────────────────────────────────────┼───────┼───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ Total │ 270 │ 95 │ │ └───────────────────────────────────────────────────┴───────┴───────┴────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ ``` ### Committer - [x] I acknowledge that I am a maintainer/committer of the Apache Airflow project. -- 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]
