fix(tests): Address review feedback on test infra changes by sentry-junior[bot] · Pull Request #127948 · getsentry/sentry

sentry-junior · 2026-10-17T18:39:42Z

Fixes from the retro on #127938, #127939, #127941, #127942 and #127943. Scheduling is unchanged: --dist=loadfile, deterministic order, and xdist 3.8.0 with loadscopereorder = False.

Filtered sharded runs (first commit)

Since #127943, pytest-selected.py plans shards by file before pytest applies -m/-k. Snuba's CI runs sentry's tests with PYTEST_ADDOPTS="-k 'not __In' -m snuba_ci" over 16 shards, so most shards collected 0 tests, exited 5, and cancelled the rest (e.g. getsentry/snuba#8556). With -m/-k in the args or PYTEST_ADDOPTS, every shard now collects everything and keeps its hash share, as before #127943. Sentry's own CI passes neither, so it keeps per-file sharding. Snuba checks out sentry master, so Snuba PRs pick this up on their next run. getsentry/snuba#8556's rerun (attempt 2, started after this merged at 300ebe3) went green, all 16 sentry shards included.

I ran Snuba's collection locally (make test-python-ci args plus the filter, --collect-only, 16 shards). Unsharded, 78 tests are selected:

	Shards with 0 tests	Tests per shard	Union == unsharded
#127943	8 of 16	0–23	yes
this PR	0 of 16	2–8	yes, each test once

Fixes

Subtests in the JSON report (perf(tests): Use pytest subtests for parametrized tests with heavy setup #127939). Every subtest report was treated as a rerun. A test with 3 passing subtests got 3 reruns entries, and failure comments only showed "contains 1 failed subtest". json_report_reruns.py now keeps one parent setup/call/teardown and puts subtests in a subtests list with their context and longrepr. Real reruns keep their own subtests. Failure comments show each failed subtest's SUBFAILED line and traceback.
Flush race (perf(tests): Flush transactional test databases with DELETE instead of TRUNCATE #127941). A row committed between the table inventory and the DELETE survived teardown. Cleanup now runs in one transaction that takes TRUNCATE's lock (ACCESS EXCLUSIVE, 1s lock_timeout). If another transaction has touched a table, the lock times out and Django's flush runs, which waits for that transaction the way TRUNCATE always has. That made the pg_stat_activity check redundant, so I removed it.
Relay readiness (perf(tests): Cut Relay readiness wait in relay_server #127942). The wait no longer overshoots its deadline: the request timeout is min(1s, time left) and no probe starts after the deadline. It catches requests.RequestException instead of Exception. The error includes the last status or exception, and the container is removed on failure.
Subtest isolation (perf(tests): Use pytest subtests for parametrized tests with heavy setup #127939). test_buffer.py builds a fresh SpansBuffer for each permutation.
Sharding paths (perf(ci): Collect only each shard's files on full backend runs #127943). Absolute or .. paths also fall back to hash sharding. pytest_shard_plugin raises a usage error if the rootdir isn't the working directory (e.g. --rootdir pointing elsewhere), instead of quietly mismatching paths.

Tests: the JSON report and the Snuba filter run real plugin stacks in a subprocess (the JSON report in-process and with -n 2; the filter checks each test runs exactly once with a Counter). The flush has a writer blocked during cleanup and a writer that's already open at the flush. Relay uses a mocked clock. The JSON report, filter and flush tests fail without their fix.

Declined: an opt-in flag or kill switch for the flush, non-Postgres support, provenance checks for the durations merge, changes to test_client_config.py (no shared state found; reverse order passes all 758 subtests), and docs for running a single subtest.

CI timing

"Run backend test" step across 22 shards, full runs with no test selection (master push and workflow_dispatch):

Run	Sum	Median shard	Max shard
master `a636572` (39051886634)	128.5 min	365 s	436 s
this PR before simplification (39059207673)	123.6 min	342 s	424 s
this PR (39061061527)	131.8 min	382 s	441 s

All 22 shards passed in each run. The spread between the three runs (123.6–131.8 min) is runner noise. Files with transactional tests took 34.9 min on master and 36.4 min here (+4%), while all other files took 297.0 and 304.9 min (+3%). Locally, the lock adds about 18 ms per teardown across all three databases (33 ms against 15 ms without it), while Django's flush takes about 1.14 s. So #127941's saving holds.

via @joshuarli.

--

View Junior Session [Sentry]

Per-file sharding plans whole files before pytest applies marker and keyword filters, so a filtered run puts its few matching files on a few shards. Snuba's CI runs sentry's tests with `-m snuba_ci` (via PYTEST_ADDOPTS) over 16 shards: most shards collected 0 tests, exited with code 5 and cancelled the rest. When -m or -k appear in the args or PYTEST_ADDOPTS, every shard collects everything and keeps its per-test hash share, as before per-file sharding.

- Keep pytest 9 subtest reports out of the JSON report's reruns, and show failed subtests in backend failure comments. - Lock tables while the transactional flush takes its inventory and deletes; fall back to Django's flush when a writer holds them. - Bound Relay readiness probes by the time left and remove the container when readiness fails. - Give each span buffer permutation a fresh SpansBuffer. - Fall back to hash sharding for paths per-file sharding cannot match, and validate merged duration shards. Co-Authored-By: Joshua Li <[email protected]>

github-actions · 2026-10-17T18:40:03Z

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

Keep the fixes and the tests that prove them, and drop the rest: the flush kill switch, test hook and pg_stat_activity check (an ACCESS EXCLUSIVE lock with a timeout covers it), the Relay readiness DI and exception class, the durations provenance checks, the failure-comment subtest cap, and the overlapping tests. The -m/-k and path checks are now one predicate in pytest-selected.py, and pytest_shard_plugin keeps the rootdir check, which only pytest can make. Co-Authored-By: Joshua Li <[email protected]>

@joshuarli

Follow-up to retro feedback on #127938, #127939, #127941, #127942 and #127943. I checked each item against the code before changing anything. Below, each one is marked done or declined. No test case or assertion was dropped or weakened. Scheduling stays on `--dist=loadfile` with deterministic order, and xdist 3.8.0 keeps `loadscopereorder = False`. ### Filtered sharded runs (first commit) Since #127943, `pytest-selected.py` splits test files across shards before pytest applies `-m`/`-k`. Snuba's `ci` job runs sentry master's tests with `PYTEST_ADDOPTS="-k 'not __In' -m snuba_ci"` over 16 shards. As a result, most of those shards collected 0 tests and exited with code 5, and fail-fast cancelled the rest. This breaks the sentry tests job on every Snuba PR (e.g. getsentry/snuba#8556). The first commit falls back to the pre-#127943 per-test hash sharding whenever `-m` or `-k` appears in the args or in `PYTEST_ADDOPTS`. In that case every shard collects everything, and the sentry plugin keeps that shard's hash share after collection. Exit code 5 is still treated as a failure. Sentry's own backend CI passes neither flag, so it keeps per-file sharding. I reproduced Snuba's collection locally against this checkout: same args as `make test-python-ci`, the same `PYTEST_ADDOPTS` filter, `--collect-only`, 16 shards. Unsharded, 78 tests are selected. | | Shards with 0 tests (exit 5) | Tests per shard | Union of shards == unsharded (`Counter`) | |---|---|---|---| | #127943 | 8 of 16, including shards 0 and 12 | 0–23 | yes | | this fix | 0 of 16 | 2–8 | yes | New tests: `tests/sentry/testutils/pytest/test_sentry.py` runs `pytest-selected.py` and the real plugins over a throwaway project with `-m`, `-k`, and Snuba's `PYTEST_ADDOPTS` form. It checks that every shard runs something and that the shards together run each selected test exactly once. Without the fix these cases fail with exit code 5. There are also unit tests for the flag detection. getsentry's CI doesn't pass `-m`/`-k` to its `pytest-selected.py`. ### #127939: subtests in the JSON report (done) The rerun adapter in `json_report_reruns.py` treated every repeated `report.when` under a nodeid as a rerun. I reproduced this with the locked stack (pytest 9, pytest-json-report 1.5.0, rerunfailures, xdist). A test with 3 passing subtests that ran once got 3 `reruns` entries, and its `setup` moved into one of them. When a subtest failed, the only top-level traceback was "contains 1 failed subtest". pytest's exit code was still correct. - The adapter now detects `pytest.SubtestReport`. It keeps one parent `setup`/`call`/`teardown` and stores subtests in a new `subtests` list, each with its `context` (`msg`, repr'd `kwargs`), outcome, duration and longrepr. rerunfailures holds back parent reports until an attempt ends, so subtests are buffered and attached to the `call` they ran in. A genuine rerun still goes to `reruns` and carries that attempt's subtests along. - Outcome: the parent stays `passed` when every subtest passes and becomes `failed` when any subtest fails. I normalized away pytest's "subtests passed" label because `report-backend-test-failures.js` selects `outcome === 'failed'`. - Failure comments now show each failed subtest's context and traceback, up to 5 per test. - `backend-test-durations.py extract` still sums only the parent phases. The parent `call` already includes the subtest loop, so subtest durations are not added again. Since setup no longer moves into `reruns`, it is counted again. - `tests/sentry/testutils/pytest/test_json_report_reruns.py` runs the real plugin stack (json-report, this adapter, rerunfailures, xdist `-n 2`) in a subprocess. It checks that passing subtests make no reruns, a failed subtest keeps its context and assertion traceback, parent setup appears once, and a genuine rerun is recorded. Against the old adapter it fails 6 of 8 cases. ### #127941: transactional flush race (done) The race was real. The table inventory and the open-transaction check were separate statements. A row committed into a table with no pages between those two steps survived teardown. - Each database is now cleaned in one transaction: `lock_timeout = 1s`, then `LOCK TABLE <every managed table> IN EXCLUSIVE MODE` (this blocks writes but allows reads), then inventory, fingerprint check and `DELETE`. A lock timeout or deadlock falls back to Django's flush, which waits for writers the way `TRUNCATE` always has. The open-transaction check stays, both before the lock (so a known-open writer skips the 1s wait) and under it. The lock blocks writers that have already written. A transaction that has only read can still write after the lock is released. - Setting `SENTRY_LEGACY_TRANSACTIONAL_FLUSH=1` keeps Django's teardown. `pytest_unconfigure` restores the original method and clears the seeded-row cache. The cache is keyed by alias plus database `NAME`. - Stable contenttype/permission IDs are unchanged (intentional). - `tests/sentry/testutils/pytest/test_transactional_flush.py` drives `_fixture_teardown` explicitly, so it doesn't depend on test order. It covers: - ORM and raw SQL rows across default/control/secondary, plus a large TOAST value - inserting, updating and deleting seeded contenttype/permission/site rows, which forces the fallback and restores state - the `available_apps` and `serialized_rollback` fallbacks - a second connection that tries to commit into an empty table during cleanup: it blocks on the lock and no row survives - a writer already open at teardown start, cleaned up through the fallback - lock-timeout fallback, the escape hatch, and unconfigure The race tests synchronize on `pg_blocking_pids` / `pg_stat_activity`, not sleeps, and they fail with the lock disabled. - Lock cost, measured locally against the real test schema (254 default, 70 control, 6 secondary tables). A warm teardown across all DBs went from ~15–18 ms to ~23–27 ms. A cold connection went from ~85–92 ms to ~101–112 ms. That is roughly 10–20 ms per teardown against ~1.27 s for Django's flush, so the lock keeps almost all of the saving. CI agrees (see below). - Audits: - No non-internal `pg_trigger` rows exist in any test DB, so DELETE vs TRUNCATE trigger behavior doesn't matter. - The `post_migrate` receivers the skip bypasses are only Django's `create_contenttypes`, `create_permissions` and `create_default_site`. The fingerprint covers all of their rows. - Dead tuples: across 117 transactional teardowns in one session, the first-quarter median was 0.126 s and the last-quarter median 0.116 s. Teardowns did not slow down over the session. - Declined: making the plugin opt-in, separate toggles for DELETE vs the `post_migrate` skip, and non-Postgres support. Sentry tests only run on Postgres, and those backends already take Django's path. ### #127942: Relay readiness (done) `wait_until_ready()` checks the time left before each probe. The request timeout is `min(1s, remaining)`, sleeps are capped by the time left, and no probe starts after the deadline. A 200 from a probe that started before the deadline counts as ready even if it arrives late. It catches `requests.RequestException` instead of `Exception`. The failure message includes the last HTTP status or exception plus the container logs, and the container is removed even if fetching the logs fails. Every test still gets a fresh container. Unit tests with an injected clock, probe and sleep cover: immediate ready, 503→200, connection error→success, permanent 503, request timeout, near-deadline success, and bounded sleep. ### #127939: subtest isolation (done / declined) - `test_buffer.py`: each permutation now builds a fresh `SpansBuffer`, which resets `any_shard_at_limit` and the cached properties. The options and Redis setup stay shared. I re-checked with reverse permutation order and an injected early failure: exactly the injected subtest failed and the rest passed. The redis-cluster variants ran green in the CI run below. - `test_client_config.py`: declined. Each factory clears caches and builds its own users and orgs, and the org/project lookups depend on the request. I found no shared global query or cache, and reverse order already passed all 758 subtests. - Declined: documenting how to reproduce a single subtest. There's no natural place for it, and subtests can't be selected by nodeid. ### #127943: sharding boundaries (done) - Absolute roots, `..` roots, and `--rootdir` or `-c`/`--config-file` (in the args or `PYTEST_ADDOPTS`) produce paths that don't match the sentry plugin's rootdir-relative comparison. `pytest-selected.py` now falls back to the legacy hash sharding for them. `pytest_shard_plugin` raises a usage error if the rootdir isn't the working directory. CI passes the relative root `tests` and keeps the fast path. - Files outside `tests/` were not duplicated across shards. `pytest_collection_modifyitems` already drops them by `SELECTED_TESTS_FILE`; they are only imported. A new test covers this. - `tests/sentry/testutils/pytest/test_sentry.py` runs the real `pytest-selected.py`, sentry plugin and shard plugin over a throwaway project with 3 shards. It asserts that the `Counter` of unsharded nodeids equals the sum of the shard Counters. Cases: relative root, a split file, parametrized and inherited tests, an ignored dir, a root outside `tests/`, absolute root and `--rootdir` (fallback), and an actual `-n 2` run. - Duration files now record schema version, unit, commit, run id, attempt, shard index and shard count. `merge` rejects mixed runs or attempts, missing or duplicate shards, missing provenance, and negative or non-finite values. It writes a `source` block into the merged file. The checked-in weights were not regenerated. - A subtest loop is one pytest item, so `file_weight`/parts can't split it. ### CI timing These numbers come from a full `workflow_dispatch` backend run on this branch (22 shards, no selection, 22/22 green), taken before the `-m`/`-k` fix was added. That fix doesn't touch sentry's own CI path. They're compared with the master push run for a636572, the only master run that includes all five PRs. Each value is the "Run backend test" step: summed across shards, then the median and the slowest shard. | Run | Sum | Median shard | Max shard | |---|---|---|---| | master a636572 ([39051886634](https://github.com/getsentry/sentry/actions/runs/39051886634)) | 128.5 min | 365 s | 436 s | | this branch ([39056799745](https://github.com/getsentry/sentry/actions/runs/39056799745)) | 135.8 min | 378 s | 429 s | The slowest shard didn't change. The summed time is +7.3 min (+5.7%), but that is runner noise, not the lock. Comparing each shard's per-file durations, the 72 files with transactional tests moved +2.6% (+0.9 min), and the 2,452 files that never reach the flush code moved +5.6% (+16.6 min). The four new test files add 46 s in total. At 10–20 ms each across roughly 1,200 transactional tests, the lock costs well under a minute over the whole run, a small fraction of #127941's saving. Master runs before these PRs varied by ±10 min in summed time.  via @joshuarli.    -- [View Junior Session](https://junior-prod.sentry.dev/conversations/agent%3Aaca1f40f54b2553bb3f2b5aa308eca88) [[Sentry]](https://sentry.sentry.io/explore/conversations/agent%3Aaca1f40f54b2553bb3f2b5aa308eca88/?project=4510944073809921)  --------- Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: Joshua Li <[email protected]>

sentry-junior Bot added 2 commits October 17, 2026 18:28

sentry-junior Bot assigned joshuarli Oct 17, 2026

github-actions Bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Oct 17, 2026

joshuarli marked this pull request as ready for review October 17, 2026 19:11

joshuarli requested review from a team as code owners October 17, 2026 19:11

joshuarli approved these changes Oct 17, 2026

View reviewed changes

joshuarli enabled auto-merge (squash) October 17, 2026 19:17

joshuarli merged commit 300ebe3 into master Oct 17, 2026
127 checks passed

joshuarli deleted the jl/test-infra-retro-followups branch October 17, 2026 19:18

sentry-release-bot Bot mentioned this pull request Oct 21, 2026

publish: getsentry/sentry/static/packages/[email protected] getsentry/publish#9959

Closed

2 tasks done

Navigation Menu

Repository navigation

fix(tests): Address review feedback on test infra changes - #127948

fix(tests): Address review feedback on test infra changes#127948
joshuarli merged 3 commits into
masterfrom
jl/test-infra-retro-followups

sentry-junior Bot commented Oct 17, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 17, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

sentry-junior Bot commented Oct 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Filtered sharded runs (first commit)

Fixes

CI timing

Uh oh!

github-actions Bot commented Oct 17, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

sentry-junior Bot commented Oct 17, 2026 •

edited

Loading