Shard realm-server tests by duration instead of file count - #5966
Conversation
`shard-test-modules.cjs` dealt an alphabetical list round-robin, which balances how many files each shard gets rather than how long they take. Across four main runs that produced a 5x spread — 266s to 1544s — and the Realm Server job waits on the slowest. Because the order shifts whenever a file is added or renamed, the slow shard is not a fixed one: it moved from shard 1 to shard 2 between 08-31 and 09-01, which is why this reads as "shard N got slow" rather than as a balance problem. Packing heaviest-first into the lightest shard flattens it. Against measured weights the projected spread is 10s, and the slowest shard's test time falls from ~1300s to ~637s. With the ~197s of per-shard setup that is roughly 25 minutes down to 14, at no machine-time cost — the same work, redistributed. Two supporting changes. The junit reporter recorded `data.module`, which QUnit 2.x's testEnd payload does not have — it provides name, suiteName, fullName, runtime and status. Every testcase therefore landed in a suite called "default", making the report useless for attribution. It now reads `fullName[0]`, which is the outermost module, and every test file opens with `module(basename(import.meta.filename), …)` — so a suite name is now a file name, exactly and without inference. `generate-test-module-timings.mjs` turns a merged junit report into the weights file, keeping prior values for files a run did not cover and refusing to write when attribution drops below 90% — which is what a regression back to "default" would look like. The committed weights were derived from run 33535224316 by differencing the timestamps on its TAP lines, since no attributed junit exists until the reporter fix has run. They cover 183 of 189 files; the remaining six take DEFAULT_WEIGHT. Regenerate them from junit once this has landed. With no weights file present the old round-robin split is used unchanged — verified to produce byte-identical assignments for all six shards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Host Test Results 1 files ± 0 1 suites ±0 2h 6m 54s ⏱️ + 2m 16s Results for commit 971fa52. ± Comparison against earlier commit fd078e5. For more details on these errors, see this check. Realm Server Test Results 1 files ± 0 190 suites +4 1h 8m 49s ⏱️ -50s Results for commit 971fa52. ± Comparison against earlier commit fd078e5. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eeaa178630
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex caught that the generator maps suite names to files through a
basename-only table, while the 44 nested test files declare path-qualified
titles — `realm-endpoints/info-test.ts`, not `info-test.ts`. Every one of
them missed. That is 1,280s of the 3,794s the weights file accounts for, so
attribution landed at 66.2%, under the generator's own 90% floor: the
regeneration command documented at the top of the file would have exited 1
without ever writing a weight.
Three shapes are in use, and now all three resolve — a bare basename, a
path-qualified name, and either followed by ` | qualifier` where one file
declares more than one top-level module (`node-realm-test.ts | file stat
probing`). Dropping a qualifier is a retry rather than a guess: the
shortened name still has to name a file on disk before it is accepted.
Four files had top-level modules named after nothing in particular
("prerender artifact-sink gates"), which no lookup can resolve. They are
retitled to name their file, so the convention the reporter and the
generator both document is now true of every file rather than of 141 of
189. Their 34 tests pass unchanged.
The generator also now lists files it has never measured. Those are packed
at DEFAULT_WEIGHT, so a genuinely slow one distorts a shard for as long as
it stays invisible, and coverage staying above the floor makes that easy to
miss — six files are in that position today.
Verified by building a junit report from the actual module titles of all 189
files, with each file's committed weight split across its suites: 100.0%
attributed, 189 measured, and all 183 known weights reproduced exactly,
which also exercises the summing of a file's several suites.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Reviewed the packing algorithm, the attribution chain from testEnd through the junit report to the weights file, and the failure modes of each — not the six retitled modules' test bodies beyond confirming they still name their files.
No blocking issues. The headline claims all check out against the branch: the pack is an exact partition (189 files, each assigned once), the projected spread is 10.0s with a 637.3s slowest shard, and with the weights file removed the assignment is byte-identical to main's round-robin for all six shards. Every finding below is about what happens when this machinery degrades, not about whether it works.
The three shapes of suite name from the resolved thread are handled — I rebuilt the resolution against the actual module titles of all 189 files and every one resolves to its own file, including the | qualifier retry.
Recommendations, in the order I'd act on them:
- Pin the "every top-level module is named after its file" convention with a test — it is what makes attribution work at all, and it drifts (see the generator comment).
- Add the partition test the matrix twin already has for the same algorithm (comment on
assignByWeight). - Fail on an unparseable weights file instead of silently reverting to round-robin (comment on
loadTimings). DEFAULT_WEIGHT's "roughly the median" is off by ~5x, and> 0conflates measured-zero with unmeasured (two comments on those lines).moduleNameFor'ssuiteNamefallback is unreachable for the case it documents (comment on the reporter).
CI is green across all six shards; observed wall clocks were 13m10s–15m47s, so the improvement is real and the residual spread is setup jitter rather than packing.
Adjacent, out of scope: nothing regenerates these weights. The matrix suite commits its spec-timings.json from CI behind a staleness guard and a drift gate (--min-drift-seconds, --shard-count); here regeneration is a manual gh run download plus a script run, so the weights decay as 189 files churn and each new file packs at DEFAULT_WEIGHT. Not this PR's job, but it is what decides whether this stays flat six months from now.
Generated by Claude Code
… no measurement Review follow-ups on the duration-weighted split. All five findings held up against the branch; the two tests are the substance. `tests/shard-assignment-test.ts` pins the property CI silently depends on: every shard computes the assignment for all shards and keeps its own bucket, so nothing at runtime checks that the buckets agree. `index % totalShards` was self-evidently a partition; a bin-pack recomputed on six machines is not, and the cost of it being wrong is a file that runs twice or not at all. It covers shard counts 1, 2, 3, 4, 6, 7 and 30, the round-robin bootstrap, and reproducibility under a reordered discovery. The same file pins the module-title convention, which is what makes a junit suite name resolvable to a file and which drifts on its own — four files had free-form titles when weighting was introduced, and the only signal was a console.warn in a manual run. Both scripts assert it in prose; now it fails a test. Checked by mutation: a free-form title, a nested file dropping its directory prefix, a pack returning one bucket for every shard, and a dropped sort tiebreak each fail exactly one test with a legible message. The weight rule now distinguishes a file measured at zero from one never measured. The seed run resolved whole seconds, so a recorded 0.0 means "under the resolution of the clock", not "free" — and it also stands in for the per-file import and hook cost a sum of per-test runtimes cannot see. Charging both cases DEFAULT_WEIGHT put 270s of load that does not exist onto the pack: 54 of 183 files. Measured weights now take a 1s floor, and only an absent measurement takes the default. Projected per-shard load drops from 677.3s to 645.0-646.0s, and the numbers now describe the real suite. DEFAULT_WEIGHT's comment claimed it was roughly the median. The median of the committed weights is 1.1s; 5 is about the 60th percentile. The value is right — an unmeasured file is unknown rather than small — but the wrong reason invites someone to "correct" it down and make new tests nearly free. A weights file that exists but does not parse now throws instead of falling back to round-robin. The file is committed and generated, which is what a merge resolves badly, and the fallback made that invisible: green CI, back to a 5x spread, nothing on stderr. An absent file still bootstraps. The reporter's `suiteName` fallback could not fire for the case it documented. A test declared outside any module has fullName `[testName]` and no suiteName at all (verified against qunit 2.26.0), so `fullName[0]` was truthy and such a test got a testsuite named after itself — unresolvable, eroding the generator's attribution budget under a name that reads like a module. It now lands in "default", which is what makes it visible. Two smaller things. A coverage failure caused by colliding basenames printed an empty "Unattributed suites:" list and then pointed at the reporter, the wrong place to look; ambiguous names are now listed there too. And suite-name resolution moved to scripts/test-module-names.mjs, shared with the test and re-exporting discovery from the splitter so the two cannot disagree about which files exist. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
[Claude Code 🤖] All five taken, in 971fa52 — replies on each thread, all resolved. Two things that belong here rather than on a thread. A correction to the PR description. The "projected spread is 10.0s, slowest shard ~637s" was computed with a weight rule the script never implemented (zeros staying zero). Under the rule as written the pack was perfectly balanced — 677.3s on all six shards — but 270s of that was phantom, because 54 files measured at 0.0 were each charged The adjacent item is filed: CS-12753 — Realm-server shard weights have no automated refresh. Agreed it is what decides whether this stays flat; realm-server's version should be simpler than the host's, since with no Percy and no index cache a main push run and a PR run are the same shape, so there is no equivalent of CS-12582's run-shape filtering to get right. New in this push: |
2 participants
This rebalances the realm server tests like the other sharded suites:
mainClaude:
shard-test-modules.cjsdealt an alphabetical list round-robin, which balances how many files each shard gets rather than how long they take. Across four main runs that produced a 5x spread — 266s to 1544s — and the Realm Server job waits on the slowest. Because the order shifts whenever a file is added or renamed, the slow shard is not a fixed one: it moved from shard 1 to shard 2 between 08-31 and 09-01, which is why this reads as "shard N got slow" rather than as a balance problem.Packing heaviest-first into the lightest shard flattens it. Against measured weights the projected spread is 10s, and the slowest shard's test time falls from ~1300s to ~637s. With the ~197s of per-shard setup that is roughly 25 minutes down to 14, at no machine-time cost — the same work, redistributed.
Two supporting changes.
The junit reporter recorded
data.module, which QUnit 2.x's testEnd payload does not have — it provides name, suiteName, fullName, runtime and status. Every testcase therefore landed in a suite called "default", making the report useless for attribution. It now readsfullName[0], which is the outermost module, and every test file opens withmodule(basename(import.meta.filename), …)— so a suite name is now a file name, exactly and without inference.generate-test-module-timings.mjsturns a merged junit report into the weights file, keeping prior values for files a run did not cover and refusing to write when attribution drops below 90% — which is what a regression back to "default" would look like.The committed weights were derived from run 33535224316 by differencing the timestamps on its TAP lines, since no attributed junit exists until the reporter fix has run. They cover 183 of 189 files; the remaining six take DEFAULT_WEIGHT. Regenerate them from junit once this has landed.
With no weights file present the old round-robin split is used unchanged — verified to produce byte-identical assignments for all six shards.