Skip to content

Download each Google Sheet once per run, not once per xdist worker - #130

Merged
gaurav merged 7 commits into
mainfrom
cache-sheet-downloads
Sep 1, 2026
Merged

Download each Google Sheet once per run, not once per xdist worker#130
gaurav merged 7 commits into
mainfrom
cache-sheet-downloads

Conversation

@gaurav

@gaurav gaurav commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

pytest --target ci-es -n auto tests/nameres/ intermittently died before running anything:

ERROR tests/nameres/test_blocklist.py - requests.exceptions.ConnectTimeout: H...
ERROR gw2 - Different tests were collected between gw0 and gw2
ERROR gw1 - Different tests were collected between gw0 and gw1
14 errors in 1.28s

test_blocklist.py builds its parametrization in pytest_generate_tests, which runs in
every xdist worker, and it was the one collection-time sheet download with no cache: -n auto fired 14 simultaneous requests at Google's unauthenticated gviz endpoint with a 10s
timeout and no retry. Any one of them failing makes that worker collect a different set of
tests, and xdist then aborts the whole run. dashboard.yaml runs -n 8 against every
target daily, so this was live in CI, where it surfaces as the "Target run broke"
annotation with no results written for that target.

GoogleSheetTestCases already had the fix — a FileLocked download cached under
cache_dir() — so this PR lifts that into sources.google_sheets.fetch_sheet_csv() and
routes both sheets through it. Verified by logging each fetch during a --target ci-es -n auto tests/nameres/ run: 14 blocklist downloads before, 1 after, with the same
165/162/117/1950 outcome split.

Two other things this turned up

A failed download printed the sheet ID. requests puts the request URL in its exception
message, and for an unauthenticated CSV export that URL is the capability that grants
access to the sheet. A transient Google error therefore wrote it into pytest's output — and
dashboard.yaml runs pytest on a public repository, so that is a public Actions log. The
helper now re-raises with the status line only. from None rather than from e is the
substance of it: pytest prints an exception chain in full, so suppressing the context is
what actually stops the original message being published. No rotation was needed — this was
found by injecting a failure locally, not in a published log.

The start-of-run cache sweep was globbing the wrong directory. cache_dir() read
BABEL_VALIDATION_CACHE_DIR straight from os.environ, but nothing loads .env except
resolve_sheet_id(). So its answer changed part-way through a process: tests/conftest.py
asks at import time and got ~/.cache/babel-validation, while the downloads ask afterwards
and got the override. For anyone with the override set, pytest_configure's gsheet_*.csv
sweep has been deleting nothing, and the caches it exists to clear only aged out on the
one-hour TTL.

That is a second, independent route to the same error, and one that needs no network
failure: a cache that expires between one worker's collection and the next leaves half the
run reading the old sheet and half re-downloading it. It also left
unlink_if_exists()'s containment check comparing against a cache_dir() that could move
under it, which raises and aborts the run outright. cache_dir() now loads .env itself,
once per process — not per call, because load_dotenv() re-sets a key that has been
deleted, which would make the override impossible to unset in a test.

Gotchas worth keeping

  • The cache key now covers the tab name as well as the sheet ID. Two tabs of one sheet are
    two different CSVs and must not share a file. Existing gsheet_*.csv are orphaned by the
    new key, which costs one download.
  • The sheet ID is no longer kept on the GoogleSheetTestCases instance. Nothing read it,
    and that object's str() reaches assertion messages the dashboard publishes.
  • Both new invariants were checked by mutation — reverting from None to from e, and
    putting the uncached requests.get back — with __pycache__ cleared between runs.
  • The blocklist CSV fixture is built with the csv module: several real column names
    contain a comma or a quote, and a hand-written header silently shifts every field by one.

Testing

  • pytest -m unit — 201 passed (was 192).
  • pytest --target ci-es -n auto tests/nameres/ — same failure/pass split as before the
    change, no xdist errors.
  • pytest tests --target dev -n 8 -k <no match> — the whole tree collects identically
    across 8 workers.

The remaining tests/nameres failures against ci-es are findings about NameRes ES, not
defects in this repo; they are what the validate-nameres-es branch is for.

gaurav and others added 7 commits September 1, 2026 14:54
GoogleSheetTestCases was the only place that knew how to download a Google
Sheet without hitting the network once per xdist worker: it hashed the sheet
ID into a cache file under cache_dir() and took a FileLock around the fetch.
That logic is not specific to the test-case sheet, and the next caller needs
it, so move it to sources.google_sheets.fetch_sheet_csv().

No behaviour change beyond the cache key, which now covers the tab name as
well as the sheet ID: two tabs of one sheet are two different CSVs and must
not share a cache file. Existing gsheet_*.csv files are orphaned by the new
key, which costs one download; tests/conftest.py sweeps that glob at the
start of every run anyway.

The sheet ID is also no longer kept on the GoogleSheetTestCases instance.
Nothing read it, and this object's str() reaches assertion messages that the
dashboard publishes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
requests puts the request URL into its exception message, and for an
unauthenticated CSV export that URL contains the sheet ID — the capability
that grants access to the sheet. So any transient Google error (a 429, a
connect timeout) printed the ID into pytest's output, and dashboard.yaml runs
pytest on a public repository, where that output is a public Actions log.

Catch RequestException around the fetch and re-raise with only the status
line, or the exception type when there was no response. `from None` rather
than `from e` is the point of the change: pytest prints an exception chain in
full, so a suppressed context would have published the original message
anyway.

Verified against a well-formed but nonexistent sheet ID: the traceback now
carries neither the ID nor docs.google.com.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_blocklist.py builds its parametrization in pytest_generate_tests, which
runs in every xdist worker. The blocklist was the one collection-time sheet
download that did not go through a cache, so `-n auto` fired one request per
worker (14 on this laptop, 8 in dashboard.yaml) at Google's unauthenticated
gviz endpoint, simultaneously, with a 10s timeout and no retry.

Any one of those failing or returning something different from the others
makes that worker collect a different set of tests, and xdist then aborts the
entire run before executing anything:

    ERROR tests/nameres/test_blocklist.py - requests.exceptions.ConnectTimeout
    ERROR gw2 - Different tests were collected between gw0 and gw2

Reproduced by failing the fetch in a subset of workers. Routing it through
fetch_sheet_csv() takes the run from 14 downloads to 1, verified by logging
each fetch during a `--target ci-es -n auto tests/nameres/` run.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two invariants, each verified to fail without its fix (with __pycache__
cleared between runs, since a same-length edit can leave a stale .pyc):

- Repeated downloads hit the network once, for the test-case sheet and for the
  blocklist. The blocklist case is the regression that started this: in an
  xdist run, "the second caller" is another worker.
- A failed download names neither the sheet ID nor docs.google.com. The
  assertion walks the exception chain the way a traceback does — __cause__
  always, __context__ only when `raise ... from None` has not suppressed it —
  because checking str(exc) alone would pass even if the original message,
  URL included, were still attached and printed.

The blocklist CSV is built with the csv module rather than written as a
literal: several of the real column names contain a comma or a quote, and a
hand-written header silently shifts every field by one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cache_dir() read BABEL_VALIDATION_CACHE_DIR straight out of os.environ, but
nothing loads .env except resolve_sheet_id(). So the answer changed part-way
through a process: tests/conftest.py asks at import time, before any sheet has
been resolved, and got ~/.cache/babel-validation, while the downloads ask
afterwards and got the override.

The visible effect is that pytest_configure's start-of-run gsheet_*.csv sweep
has been globbing an empty directory for anyone with the override set. The
caches it is meant to clear were never cleared; they only aged out on the
one-hour TTL. That is its own route to "Different tests were collected between
gw0 and gwN", and one that needs no network failure at all: a cache that
expires between one worker's collection and the next has half the run reading
the old sheet and half re-downloading it.

It also left unlink_if_exists()'s containment check comparing paths against a
cache_dir() that could move under it, which would abort the run outright.

So cache_dir() now loads .env itself. Once per process, not per call:
load_dotenv() leaves an existing key alone but re-sets a deleted one, and
reloading each time would make the override impossible to unset in a test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A collection-time fetch runs once per xdist worker, so it has to be cached, or
one bad response aborts the run with "Different tests were collected". And a
requests exception message carries the URL, which carries the sheet ID — the
next outbound call built from a secret will need the same handling.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The fake load_dotenv wrote os.environ directly. Where there is no .env at all
— CI — the monkeypatch.delenv above it has no previous value to restore, so
that write outlived the test and test_cache_files_live_in_the_cache_dir then
compared the issue cache's directory against a tmp_path. It passed locally
only because .env supplies the key here, giving monkeypatch something to put
back.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@gaurav
gaurav merged commit 9a8f418 into main Sep 1, 2026
2 checks passed
@gaurav
gaurav deleted the cache-sheet-downloads branch September 1, 2026 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant