Repository navigation
Shard the backend suite across three runners - #547
Conversation
This comment has been minimized.
This comment has been minimized.
`backend-tests` is the longest gate on every PR and the first thing every deploy waits for: 129s, of which a single pytest invocation is 107s. Its setup is only 18s, so splitting the tests across runners divides nearly all of it, and public-repo runners are free. Three shards rather than two because the combine job runs serially after them and costs about 18s whatever the count, which would leave two at ~98s. Past three the other gate jobs set the run's floor and a fourth buys nothing. The threshold is the awkward part. A shard measures a third of the suite, so it cannot apply `--cov-fail-under` to its own figure; each overrides it away and uploads its coverage fragment, and the new `backend-coverage` job combines the three and applies the 55 once. `pyproject.toml` keeps the number, so a local `pytest` still enforces the gate exactly as it did. The failure mode here is quiet rather than red: a combine that reads fewer fragments than ran reports a plausible figure over part of the suite. Hence the fragment count in the job, and `tests/test_backend_coverage_gate.py` pinning that the shards disclaim the threshold, that the combine applies the number `addopts` names, that the number is not zero, that a deploy waits on it, that the omit below still matches a real temp path, and that the combine job reads with the coverage version the shards wrote with. Reporting on a different runner from the one that measured is what the new `[tool.coverage.report] omit` entry is for. pytest-cov exports `COVERAGE_PROCESS_START`, so a subprocess a test spawns measures under this config, and `source = ["."]` resolves against that subprocess's cwd: `tests/test_deploy_scope.py` runs `deploy-scope.py` inside a throwaway git repo and its `app.py` lands in the data as an unrun file. That was harmless while the report ran beside the tests that made it, and fatal once it does not, because `coverage report` stops at "No source for code". pytest-split cuts the tests into contiguous chunks of equal recorded duration rather than bin-packing them into balanced ones, so each file's tests stay together and in collection order. This suite has tests that pass or fail on what shares their process, and the least disturbance to that is the split worth having. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
749e1db to
66cc8d2
Compare
|
Claude finished @Babissimo's task in 22m 28s —— View job Tasks
Review of PR #547 (updated pass, commit 66cc8d2)Since the previous review on this branch, 1. Still open from the previous pass: required status checks can't be verified from the repo, and this diff makes it easy to get wrong 2. 3. Still open from the previous pass: 4. Non-blocking: real but unaddressed duplicated compute in 5. Minor:
Checked and correct, not flagging:
|
|
Thanks. Taken one, answered three. 1. Duplicated 2. Clearing The suggestion is based on an incomplete picture of where the measurement comes from. It names the On the glob being "anchored to one directory depth": it isn't, and this run demonstrates it. Locally it matches 3. Required status checks. Checked before writing the change: there are none. 4. Lighter toolchain in |
With the backend suite sharded (#547), Web (dashboard) became the job that sets the gate: in #547's second run the backend path finished at 94 s and the dashboard at 105 s. It runs lint (13 s), typecheck (7 s), tests (39-62 s) and build (5 s) one after another, so the tests wait on 25 s of work they do not depend on. The tests now have their own matrix entry, beside one for lint, typecheck and build. On a slow runner that entry is about 13 s of setup plus the 60 s test step, which puts it below the backend path in both of #547's runs and takes the dashboard off the critical path. The other workspaces keep one entry each; their scripts take 20 s together and splitting them would buy nothing. Splitting rather than sharding with vitest --shard, because the split alone is enough while the backend path stays above ~75 s, and sharding would add a runner for no gain today. Unlike pytest, vitest's cost does divide by shard: its reported environment, import and test times are summed over three workers (about 162 s against a 61 s wall), and a shard loads only its own files. So --shard is the next step if the backend path ever drops below this entry. The job keeps its name, web-build, so staging's needs list is unchanged. Which entry runs which script is now decided by step conditions, and a mistake there fails nothing: a script left out runs nowhere, and one run twice silently undoes the split. tests/test_web_build_matrix.py pins that every workspace in package.json has an entry, that each script it defines runs in exactly one, that the dashboard's test entry runs nothing else, and that a split entry's name says which half it is. All nine mutations tried against the workflow turn it red. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With the backend suite sharded (#547), Web (dashboard) became the job that sets the gate: on main's run for #547 the backend path finished at 95 s and the dashboard at 119 s. It runs lint (13 s), typecheck (7 s), tests (39-75 s) and build (5 s) one after another, so the tests wait on 25 s of work they do not depend on. The tests now have their own matrix entry, beside one for lint, typecheck and build. On a slow runner that entry is about 13 s of setup plus the test step, which is below the backend path and takes the dashboard off the critical path. The other workspaces keep one entry each; their scripts take 20 s together and splitting them would buy nothing. Splitting rather than sharding with vitest --shard, because the split alone is enough while the backend path stays above this entry, and sharding would add a runner for no gain today. Unlike pytest, vitest's cost does divide by shard: timed at three workers, the whole suite took 100 s and each third 37-41 s, which fits about 8 s fixed against 92 s divisible. So --shard is the next step if the backend path ever drops below this entry. The job keeps its name, web-build, so staging's needs list is unchanged. Which entry runs which script is now decided by step conditions, and a mistake there fails nothing: a script left out runs nowhere, one run twice silently undoes the split, and the two halves trading conditions keeps every count right while the tests land back on the checks entry. tests/test_web_build_matrix.py pins that every workspace in package.json has an entry, that each lint, typecheck, test and build script it defines runs in exactly one, that the dashboard is split in two, that each half runs only its own scripts, and that a split entry's name says which half it is. All eleven mutations tried against the workflow turn it red. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Three follow-ups to #547, where the shards' coverage is combined and the threshold applied on a runner that measured nothing. The omit test checked only test_deploy_scope.py's shallow tmp_path shape, a repo's top-level app.py. test_migrations.py also puts measured files in the report, a copied core/ two levels deeper and under an xdist worker's popen-gwN directory. A glob narrowed to a fixed depth, or to the serial basetemp layout, still matched the shallow path and would have passed while backend-coverage died on "No source for code". The test now asserts both shapes; both of those narrowings turn it red. --splits named the shard count as a literal beside the matrix list it has to equal. It now reads strategy.job-total, so the matrix alone sets the count, and the test pins that --splits and --group follow the matrix rather than comparing two literals. The fragment count was checked by a whitespace-sensitive substring, where the rest of the file parses numbers with _threshold(). It uses that now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Three follow-ups to #547, where the shards' coverage is combined and the threshold applied on a runner that measured nothing. The omit test checked only test_deploy_scope.py's shallow tmp_path shape, a repo's top-level app.py. test_migrations.py also puts measured files in the report, a copied core/ two levels deeper and under an xdist worker's popen-gwN directory. A glob narrowed to a fixed depth, or to the serial basetemp layout, still matched the shallow path and would have passed while backend-coverage died on "No source for code". The test now asserts both shapes; both of those narrowings turn it red. --splits named the shard count as a literal beside the matrix list it has to equal. It now reads strategy.job-total, so the matrix alone sets the count, and the test pins that --splits and --group follow the matrix. job-total counts every combination, so the test also pins that shard is the matrix's only axis: a second one would multiply --splits while --group still spanned only the shards, leaving part of the suite unrun. The fragment count was checked by a whitespace-sensitive substring, where the rest of the file parses numbers with _threshold(). It uses that now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With the backend suite sharded (#547), Web (dashboard) became the job that sets the gate: on main's run for #547 the backend path finished at 95 s and the dashboard at 119 s. It runs lint (13 s), typecheck (7 s), tests (39-75 s) and build (5 s) one after another, so the tests wait on 25 s of work they do not depend on. The tests now have their own matrix entry, beside one for lint, typecheck and build. On a slow runner that entry is about 13 s of setup plus the test step, which is below the backend path and takes the dashboard off the critical path. The other workspaces keep one entry each; their scripts take 20 s together and splitting them would buy nothing. Splitting rather than sharding with vitest --shard, because the split alone is enough while the backend path stays above this entry, and sharding would add a runner for no gain today. Unlike pytest, vitest's cost does divide by shard: timed at three workers, the whole suite took 100 s and each third 37-41 s, which fits about 8 s fixed against 92 s divisible. So --shard is the next step if the backend path ever drops below this entry. The job keeps its name, web-build, so staging's needs list is unchanged. Which entry runs which script is now decided by step conditions, and a mistake there fails nothing: a script left out runs nowhere, one run twice silently undoes the split, and the two halves trading conditions keeps every count right while the tests land back on the checks entry. tests/test_web_build_matrix.py pins that every workspace in package.json has an entry, that each lint, typecheck, test and build script it defines runs in exactly one, that the dashboard is split in two, that each half runs only its own scripts, and that a split entry's name says which half it is. All eleven mutations tried against the workflow turn it red. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Three follow-ups to #547, where the shards' coverage is combined and the threshold applied on a runner that measured nothing. The omit test checked only test_deploy_scope.py's shallow tmp_path shape, a repo's top-level app.py. test_migrations.py also puts measured files in the report, a copied core/ two levels deeper and under an xdist worker's popen-gwN directory. A glob narrowed to a fixed depth, or to the serial basetemp layout, still matched the shallow path and would have passed while backend-coverage died on "No source for code". The test now asserts both shapes; both of those narrowings turn it red. --splits named the shard count as a literal beside the matrix list it has to equal. It now reads strategy.job-total, so the matrix alone sets the count, and the test pins that --splits and --group follow the matrix. job-total counts every combination, so the test also pins that shard is the matrix's only axis: a second one would multiply --splits while --group still spanned only the shards, leaving part of the suite unrun. The fragment count was checked by a whitespace-sensitive substring, where the rest of the file parses numbers with _threshold(). It uses that now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… gaps (#549) * ci: run the dashboard's tests in a web-build entry of their own With the backend suite sharded (#547), Web (dashboard) became the job that sets the gate: on main's run for #547 the backend path finished at 95 s and the dashboard at 119 s. It runs lint (13 s), typecheck (7 s), tests (39-75 s) and build (5 s) one after another, so the tests wait on 25 s of work they do not depend on. The tests now have their own matrix entry, beside one for lint, typecheck and build. On a slow runner that entry is about 13 s of setup plus the test step, which is below the backend path and takes the dashboard off the critical path. The other workspaces keep one entry each; their scripts take 20 s together and splitting them would buy nothing. Splitting rather than sharding with vitest --shard, because the split alone is enough while the backend path stays above this entry, and sharding would add a runner for no gain today. Unlike pytest, vitest's cost does divide by shard: timed at three workers, the whole suite took 100 s and each third 37-41 s, which fits about 8 s fixed against 92 s divisible. So --shard is the next step if the backend path ever drops below this entry. The job keeps its name, web-build, so staging's needs list is unchanged. Which entry runs which script is now decided by step conditions, and a mistake there fails nothing: a script left out runs nowhere, one run twice silently undoes the split, and the two halves trading conditions keeps every count right while the tests land back on the checks entry. tests/test_web_build_matrix.py pins that every workspace in package.json has an entry, that each lint, typecheck, test and build script it defines runs in exactly one, that the dashboard is split in two, that each half runs only its own scripts, and that a split entry's name says which half it is. All eleven mutations tried against the workflow turn it red. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: close three gaps in the backend coverage gate's pins Three follow-ups to #547, where the shards' coverage is combined and the threshold applied on a runner that measured nothing. The omit test checked only test_deploy_scope.py's shallow tmp_path shape, a repo's top-level app.py. test_migrations.py also puts measured files in the report, a copied core/ two levels deeper and under an xdist worker's popen-gwN directory. A glob narrowed to a fixed depth, or to the serial basetemp layout, still matched the shallow path and would have passed while backend-coverage died on "No source for code". The test now asserts both shapes; both of those narrowings turn it red. --splits named the shard count as a literal beside the matrix list it has to equal. It now reads strategy.job-total, so the matrix alone sets the count, and the test pins that --splits and --group follow the matrix. job-total counts every combination, so the test also pins that shard is the matrix's only axis: a second one would multiply --splits while --group still spanned only the shards, leaving part of the suite unrun. The fragment count was checked by a whitespace-sensitive substring, where the rest of the file parses numbers with _threshold(). It uses that now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
The shard split reads .test_durations, last written at #547, and group 3 now runs about 15% lighter than the other two. Serial, so no xdist worker's first test absorbs the session fixture setup, and under coverage as the shards run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The shard split reads .test_durations, last written at #547, and group 3 now runs about 15% lighter than the other two. Serial, so no xdist worker's first test absorbs the session fixture setup, and under coverage as the shards run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
Shards
backend-testsacross three runners and applies the coverage threshold once, in a newbackend-coveragejob behind them.ClickUp: Shard the backend suite across two runners and split the dashboard tests from its lint and build
Measured effect, across five runs
main#514main#539main#534About 25 s off the median, not the ~55 s the ticket projected. Single runs vary by 15 s or more either way, so the two sharded runs are a range rather than a figure.
The projection assumed the 107 s pytest step divides by the shard count. It does not: every shard collects all 4656 tests, imports the app in four xdist workers and starts coverage, and none of that divides. Fitting the two measured points, 107 s unsharded against 67 s for a third of the tests, puts the per-shard fixed cost near 47 s and the divisible work near 60 s. A fourth shard would buy about 5 s and no number of shards takes the pytest step below ~47 s, so the remaining lever is that fixed cost, not more runners.
One consequence worth recording: in run 2 the backend path finished at 94 s and
Web (dashboard)set the gate at 105 s. The two are now comparable, so the ticket's other half is on the critical path and does have something to buy, which was not true before this change.Why
backend-testsis the longest gate on every PR and the first thing every deploy waits for. Three shards rather than two because the combine job runs serially after them and costs about 16 s whatever the count; past three the per-shard fixed cost dominates and a fourth buys very little.The coverage seam
A shard measures a third of the suite, so it cannot apply
--cov-fail-underto its own figure. Each overrides it to 0 and uploads its data fragment;backend-coveragecombines the three and applies the 55 once.pyproject.tomlkeeps the number, so a localpytestenforces the gate exactly as before.The failure mode here is quiet rather than red: a combine that reads fewer fragments than ran reports a plausible figure over part of the suite. Hence the fragment count in the job, and
tests/test_backend_coverage_gate.py, which pins that the shards disclaim the threshold, that the combine applies the numberaddoptsnames, that the number is not zero, that a deploy waits on it, and that the temp-path omit still matches a real path.The reported coverage figure moves, 88% to 93%
This looks like the gate weakening and is the opposite.
mainreports 15460 statements at 88%; this PR reports 14171 at 93%. Every one of the 1289 statements in that gap is pytest temp-directory content thatmaincounts and this PR omits:app.pyfiles fromtests/test_deploy_scope.py's temp git repos.tests/test_migrations.py'sold_image, whichshutil.copytreescore/andmigrations/into a tmp_path.maintherefore counts a second, almost entirely uncovered copy of the codebase, which is what was holding the figure down.Nothing appears in this PR's report that is absent from
main's, and the per-file statement counts agree for every shared file except the two #534 grew.Why the omit was needed at all
tests/test_deploy_scope.pyandtests/test_migrations.pyrun subprocesses whose cwd is a tmp_path. pytest-cov exportsCOVERAGE_PROCESS_START, so those subprocesses measure under the same config, andsource = ["."]resolves against their cwd. Harmless while the report runs in the same job, where the paths still exist. Fatal to a combine job on another runner:coverage reportstops atNo source for codeand exits non-zero. It failed exactly this way before the fix.Two details that are easy to get wrong, and why the test checks the glob against a real filename rather than by string match:
*does not cross a path separator, so*pytest-of-*does not match.[report], not[run].[run] omitfilters what is measured, and these files are added back by the source walk at report time.[report] omitis additive, so the existing[run]omissions still apply.Verification
The shards partition the suite exactly: full collection 4656, groups of 1636 / 1277 / 1743 summing to 4656, no pairwise overlap, nothing unassigned. On CI the three shards ran 4716 tests between them against
main's 4710, the difference being the six added here.Locally, an unsharded run and the three shards combined both report 14171 statements and 4716 passed; only the missed count differs, 981 against 984, being three lines covered solely because the whole suite shared one session.
pre-commit run --all-filesgreen, and the workflow-reading tests at 96 passed, including the cases #546 added.🤖 Generated with Claude Code