Skip to content

Commit d7feb3c

Browse files
wormeymanclaude
andauthored
ci: shard the test phase across 3 runners (#118)
`verify` took 9m03s as a single job on PR #116, and the test phase is ~95% of it. Splitting the phases into a `static` job and a 3-way sharded `tests` matrix cuts the gate to roughly 3-4 minutes. Nothing about what is checked changes. WHY SHARDING AND NOT A SETTING. Measured locally: the suite is 497s of CPU in 68s of wall, ~7.5x on 12 cores (8 performance + 4 efficiency). Raising parallelism on one box is already exhausted - CLAUDE.md records maxWorkers 4/8/11 at 74.7/61.7/61.8s against a 61.2s default. More machines is the only remaining lever. Two other candidates were measured and rejected: - `isolate: false`: 66 of 171 files FAIL. The same files pass individually with --no-isolate, so it is cross-file module-state pollution, not a misconfiguration. It also only bought 7.6% (68.24s -> 63.07s). - `environment: node` by default (only ~30 of 164 spec files touch document/window): worth ~3s. Real but not worth the churn on its own. WHY THREE, MEASURED NOT GUESSED. vitest splits by FILE, so one file is an unsplittable floor, and `test/previewAgreement.spec.ts` is 67s of the 68s suite. Slowest-shard wall, which is what the gate waits on: N=3 55.7 / 25.0 / 18.0 -> 55.7s 159 runner-seconds N=4 53.5 / 37.1 / 23.4 / 16.9 -> 53.5s 211 runner-seconds N=4 is 2.3s faster - inside run-to-run noise - for 33% more runner minutes, because both are pinned by the same file. So 3 is the cost-optimal point, not merely an adequate one. Raise it only after splitting previewAgreement into its three independent tests, and re-measure rather than assuming. The shards are unbalanced (55.7 vs 18.0) because the split is by file count, not duration. That costs nothing here: even a perfect balance could not beat the 67s file floor. ANTI-DRIFT. The old single job ran `pnpm run verify` verbatim so CI could not drift from local. That rule is kept by a different mechanism: this workflow names only package.json SCRIPTS, never the underlying commands, and `verify`, `verify:static` and `verify:shard` all compose the same `verify:lint`. There is still one definition of each phase. REQUIRED CHECK. Ruleset `EJ` requires a check named `verify`, so the aggregating job keeps that id - a required check that never appears blocks every PR forever. It asserts `needs.*.result` explicitly, because a `needs:` job whose dependency failed is SKIPPED, and a skipped required check does not block a merge. No ruleset change is needed. NO BLOB REPORTS. `--reporter=blob` works, but `vp test --merge-reports` does not merge - it re-runs, reporting 114 files from a 57-file shard's blob. The directory stays gitignored anyway: running that reporter by hand drops a ~500 KB JSON in the repo root that `vp check`'s format step trips over. Claude-Session: https://claude.ai/code/session_018AB7J1qK6kSBnJJmgDqMst Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent cc44aa7 commit d7feb3c

3 files changed

Lines changed: 90 additions & 9 deletions

File tree

.github/workflows/verify.yml

Lines changed: 78 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,31 @@
22
# verify` only ran when a human remembered to (or at deploy time, which can be
33
# days after the breaking merge) - see issue #54.
44
#
5-
# This job deliberately runs `pnpm run verify` VERBATIM rather than re-listing
6-
# its four phases as separate steps. `verify` is the single definition of "the
7-
# repo is consistent"; splitting it here would create a second definition that
8-
# can silently drift from package.json. Note it is `verify`, not `check` -
9-
# `check` is `vp check --fix` and CI must never rewrite files.
5+
# `verify` no longer runs as one job, because the test phase is ~95% of it and
6+
# a runner is ~3x slower than a dev machine: the single job measured 9m03s on
7+
# PR #116. The phases are now split across a `static` job and a sharded `tests`
8+
# matrix that run in parallel, with a `verify` job aggregating them.
9+
#
10+
# The anti-drift rule that motivated the old VERBATIM comment still holds, and
11+
# is now enforced differently: this file names only package.json SCRIPTS
12+
# (`verify:static`, `verify:shard`), never the underlying commands. `verify`,
13+
# `verify:static` and `verify:shard` are all composed from the same
14+
# `verify:lint` script, so there is still exactly one definition of each phase
15+
# and CI cannot drift from local. Do not inline `vp check` etc. here.
16+
# Note it is never `check` - that is `vp check --fix` and CI must never rewrite
17+
# files.
18+
#
19+
# WHY THE JOB NAMED `verify` STILL EXISTS: ruleset `EJ` requires status checks
20+
# named `verify` and `build`. A required check that never appears blocks every
21+
# PR forever, so the aggregating job below MUST keep the id `verify`. Renaming
22+
# it needs a ruleset PUT in the same change - see CLAUDE.md on the two-step.
23+
#
24+
# WHY 3 SHARDS, MEASURED NOT GUESSED: vitest splits by FILE, so a single file
25+
# is an unsplittable floor. `test/previewAgreement.spec.ts` is 67s of the 68s
26+
# local suite (~200s on a runner). Ideal-split time is CI_test/N, so N=3 is
27+
# where that first drops below the floor - and N=4+ buys literally nothing
28+
# until that file is split into its three independent tests. Raise N only
29+
# after splitting it, and re-measure rather than assuming.
1030
#
1131
# The production build runs too, in the separate `build` job below - see its
1232
# own comment for why it is not a phase inside `verify`.
@@ -52,15 +72,41 @@ permissions:
5272
contents: read
5373

5474
jobs:
55-
verify:
75+
# Everything in `verify` that is NOT the app test suite: `vp check`,
76+
# `check:vue`, and `preview:test`. ~24s on a runner, so it finishes long
77+
# before any shard and costs nothing to run as its own job.
78+
static:
5679
runs-on: ubuntu-latest
57-
# `verify` is ~60-90s of work; 15 minutes is a hang detector, not a budget.
5880
timeout-minutes: 15
5981
env:
6082
# `preview:test` shells out to wrangler (`wrangler types --check`). Keep it
6183
# from phoning home for telemetry on a runner where nobody can answer the
6284
# opt-in prompt.
6385
WRANGLER_SEND_METRICS: "false"
86+
steps:
87+
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
88+
- uses: pnpm/action-setup@0ebf47130e4866e96fce0953f49152a61190b271 # v6.0.9
89+
- uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0
90+
with:
91+
node-version-file: .node-version
92+
cache: pnpm
93+
- run: pnpm install --frozen-lockfile
94+
- run: pnpm run verify:static
95+
96+
# The app test suite, split across runners. Each shard is a separate machine,
97+
# which is the ONLY way to raise the parallelism ceiling - `maxWorkers` was
98+
# already measured as a dead end on one box (see CLAUDE.md).
99+
tests:
100+
runs-on: ubuntu-latest
101+
# A shard is ~1/3 of a ~9-minute suite. 15 minutes stays a hang detector.
102+
timeout-minutes: 15
103+
strategy:
104+
# One failing shard must not cancel the others: a red `verify` should say
105+
# everything that is broken, not just whichever shard tripped first.
106+
fail-fast: false
107+
matrix:
108+
shardIndex: [1, 2, 3]
109+
shardTotal: [3]
64110
steps:
65111
# Third-party actions are pinned to a full commit SHA, never a moving tag.
66112
# The trailing comment names the release each SHA is, and Renovate updates
@@ -89,7 +135,31 @@ jobs:
89135
# one that reports `Scope: all 3 workspace projects`.
90136
- run: pnpm install --frozen-lockfile
91137

92-
- run: pnpm run verify
138+
- run: pnpm run verify:shard -- --shard=${{ matrix.shardIndex }}/${{ matrix.shardTotal }}
139+
140+
# The required status check. Ruleset `EJ` requires a check named `verify`, so
141+
# this job must keep that id even though the work now happens above.
142+
#
143+
# `!cancelled()` rather than `always()`: a cancelled run (superseded push, per
144+
# the concurrency group) should stay cancelled, not be converted into a
145+
# failure. But a FAILED dependency must fail this job - a `needs:` job whose
146+
# dependency failed is otherwise skipped, and a skipped required check does
147+
# not block a merge. Hence the explicit result assertions.
148+
verify:
149+
needs: [static, tests]
150+
if: ${{ !cancelled() }}
151+
runs-on: ubuntu-latest
152+
timeout-minutes: 5
153+
steps:
154+
- name: Assert every phase passed
155+
env:
156+
STATIC: ${{ needs.static.result }}
157+
TESTS: ${{ needs.tests.result }}
158+
run: |
159+
echo "static=$STATIC tests=$TESTS"
160+
# `tests` is the matrix aggregate: 'success' only if every shard was.
161+
[ "$STATIC" = "success" ] || { echo "::error::static phase: $STATIC"; exit 1; }
162+
[ "$TESTS" = "success" ] || { echo "::error::test shards: $TESTS"; exit 1; }
93163
94164
# `verify` is check + type-check + tests. None of them build, so a change that
95165
# passes all three and breaks the production build reached `main` unnoticed

.gitignore

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,14 @@ vite.config.ts.timestamp-*
2121
coverage/
2222
test-results/
2323

24+
# Vitest blob reports. Nothing here uses `--reporter=blob` - the sharded CI path
25+
# deliberately does not, because `vp test --merge-reports` does not merge, it
26+
# re-runs (measured: a 57-file shard's blob came back reporting 114 files). This
27+
# stays ignored anyway because running that reporter by hand drops a ~500 KB
28+
# JSON in the repo root, which `vp check`'s format step would then trip over.
29+
.vitest-reports/
30+
.vitest-attachments/
31+
2432
# Lighthouse CLI report output (--output html|json|csv) - a stray report left
2533
# in the repo root once failed `vp check`'s format step and blocked a deploy
2634
*.report.html

package.json

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,10 @@
1313
"check:vue": "vue-tsc --noEmit",
1414
"refs:sync": "bash scripts/sync-factorio-refs.sh",
1515
"verify:deploy": "node scripts/verify-deploy.ts",
16-
"verify": "vp check && pnpm run check:vue && vp run --cache test && pnpm run preview:test",
16+
"verify:lint": "vp check && pnpm run check:vue",
17+
"verify:static": "pnpm run verify:lint && pnpm run preview:test",
18+
"verify:shard": "vp test",
19+
"verify": "pnpm run verify:lint && vp run --cache test && pnpm run preview:test",
1720
"localpreview": "pnpm run preview:dev",
1821
"preview:dev": "concurrently -k -n worker,app -c blue,green \"pnpm run preview:worker\" \"pnpm run preview:app\"",
1922
"preview:worker": "pnpm --filter @fmw/preview-worker exec wrangler dev --var ALLOWED_ORIGIN:http://localhost:5173",

0 commit comments

Comments
 (0)