[MAINTENANCE] Create BigQuery test tables in the configured dataset and sweep tables, not datasets - #12024
Draft
joshua-stauffer wants to merge 3 commits into
Draft
[MAINTENANCE] Create BigQuery test tables in the configured dataset and sweep tables, not datasets#12024joshua-stauffer wants to merge 3 commits into
joshua-stauffer wants to merge 3 commits into
Conversation
…w one per test BigQuery calls its schemas "datasets", so the harness's per-test schema became a per-test dataset. Datasets are project-level objects, which makes them an expensive unit of test isolation: every test config paid a CREATE SCHEMA on setup and a DROP SCHEMA on teardown, and any run killed before teardown left an orphan that only an out-of-band sweep could find. That bought no isolation the suite did not already have. Generated table names carry a uuid4-derived suffix, so concurrent runs -- across xdist workers, Python versions, and branches -- cannot collide however many of them share a dataset. Tests now create their tables directly in the dataset the connection already points at. Teardown drops the tables and nothing else, and there is no longer a class of leaked resource that lives outside the one dataset CI is configured with. Dataset teardown was the largest remaining cost in the suite, holding the four slowest entries at 20-29s each. Schema-qualified coverage on BigQuery is not currently exercised by any test. Should one need it, passing `schema_name` while use_schema is False raises, so the requirement surfaces as a clear error rather than silently doing nothing.
With tests no longer creating a dataset apiece, there are no stale datasets left to find -- only stale tables, and only ever inside the one dataset CI is configured with. Narrowing the sweep to match removes every reason the old project-wide search was awkward. Discovery is now `tables.list` against the configured dataset. That is scoped to a single dataset, so it runs with a credential that can see only that dataset and never pays to enumerate a project which may hold many thousands of unrelated datasets. It also sidesteps the two problems with the previous INFORMATION_SCHEMA.SCHEMATA query: that view is region-scoped, so anything created in another location was invisible to it, and reading it required project-wide metadata access that a CI credential should not need. `tables.list` reports creationTime per table, so age filtering needs no follow-up request per candidate. The age threshold is an injectable parameter so it can be exercised without waiting an hour. The match is deliberately narrow: the fixed prefix, an optional label, and an anchored uuid4-derived suffix. The configured dataset is shared with whatever else lives there, so a table someone created on purpose must never be a candidate. Deletion tolerates a table disappearing between listing and deleting, which happens whenever a run reaches its own teardown mid-sweep. Also drops the `py3XX_i*` pattern, which nothing in the codebase produces.
✅ Deploy Preview for niobium-lead-7998 canceled.
|
The module annotated a parameter as `python_bigquery.Client`. Annotations are evaluated at import time, and outside the BigQuery test lane that name is a NotImported sentinel which raises on attribute access, so merely importing the module failed wherever the optional dependency was absent -- including the unit test lane, which collects the whole tests tree. `from __future__ import annotations` defers the annotation, matching how the rest of the codebase refers to optional dependencies in signatures. The tests now stand in for the module rather than reaching through it. Patching `python_bigquery.Client` cannot work against the sentinel, which raises from both __getattr__ and __setattr__; and patch's own default path inspects what it is replacing via `hasattr(original, "__func__")`, where NotImported raises ModuleNotFoundError rather than the AttributeError hasattr would absorb. Passing `new=` explicitly skips that inspection. `NotFound` is likewise substituted with a local exception, since it is a sentinel in the same environments. That keeps the behaviour under test -- a table vanishing mid-sweep is tolerated -- but would no longer notice the guard being widened to catch everything, so there is now a test that an unexpected delete failure propagates instead of being logged as a routine skip.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two commits. The BigQuery test harness stops creating a dataset per test config, and the cleanup script stops looking for stale datasets and looks for stale tables instead. The second follows from the first: once no test creates a dataset, there are no stale datasets to find.
Why
BigQuery calls its schemas "datasets", so the harness's
use_schema = Truemeant a project-level object created and dropped per test config — aCREATE SCHEMAon setup and aDROP SCHEMAon teardown, plus an orphan whenever a run was killed before teardown.That bought no isolation the suite didn't already have. Generated table names carry a uuid4-derived suffix (
expectation_test_table[_label]_<10 hex>), so concurrent runs cannot collide however many share a dataset — across xdist workers, Python versions, or branches.It was also the largest remaining cost. After #12020 the BigQuery suite runs in 3:36, and the four slowest entries in that run are all teardown, at 20–29s each — which is
DROP TABLEplusDROP SCHEMA. This removes the second half.Nothing exercises schema-qualified access on BigQuery today:
test_schemas.pycovers databricks, snowflake and sql_server, andtest_sql_datasources.pycovers sqlite, postgresql, trino and sql_server. No BigQuery test setsschema_name, and none sets an explicittable_name. Should that need arise, passingschema_namewhileuse_schemais False raises, so it surfaces as a clear error rather than silently doing nothing.The sweeper
Discovery moves to
tables.listagainst the configured dataset. Compared to the previous project-wideINFORMATION_SCHEMA.SCHEMATAquery:INFORMATION_SCHEMAis region-scoped, so anything created in another location was invisible to it and never cleaned up.tables.listreportscreationTime, so age filtering is free.The match is deliberately narrow — fixed prefix, optional label, anchored 10-hex suffix — because the configured dataset is shared with whatever else lives there and a deliberately-created table must never be a candidate. Deletion tolerates a table vanishing between listing and deleting, which happens whenever a run reaches its own teardown mid-sweep.
Also drops the
py3XX_i*pattern, which nothing in the codebase produces any more.Testing
21 new tests in
tests/scripts/test_cleanup_big_query.py, covering the name match (13 cases including near-misses like a 9- and 11-char suffix, a non-hex suffix, and an unanchored prefix), the age threshold and its boundary, unknown creation time, dataset-scoping, and the delete path includingNotFound.I mutation-tested them rather than trusting green mocks — each of these was caught by exactly the intended test:
>→>=on the age comparisontest_table_exactly_at_the_age_threshold_is_left_alonetest_cleanup_deletes_each_stale_tablecreatedas staletest_table_with_unknown_creation_time_is_left_aloneLocal runs:
-m unit4380 passed (baseline 4359, +21 = exactly the new tests) ·-m sqlite539 passed · ruff clean · mypy clean on the changed files under CI's flags on Python 3.10.The
use_schemachange has no unit test — there's no existing test module for the harness configs, and assertingschema is Nonewould test little. I verified it directly instead:use_schemaFalse,schemaNone, connection string resolving to the configured dataset, the table's SQLAlchemy schema None, andschema_nameraising. The real verification is the BigQuery marker job.Not yet verified
The BigQuery marker job has not run against this branch — it's gated on
draft == false. That job is the only meaningful test of this change, and it now takes ~4 minutes, so marking this ready is worth doing before merge. I'd expect green with a visible drop in teardown time.One transition note
After this merges, nothing sweeps
gx_ci_test_*datasets any more, because nothing creates them. If a run is in flight across the merge it could leave one final orphan with no sweeper to collect it. Worth one check afterwards:Anything listed can be removed with
bq rm -r -f -d <dataset>.