Run the realm-server tests that were never running - #5996
Conversation
Found while validating the CS-12731 weights generator against a real merged junit report: the report has no suites for several files that exist on disk. Two mechanisms, both silent. Between them 24 tests were being filtered out of every shard, and three whole files — 26 more tests — were never loaded at all. `tests/index.ts` loads from a hand-maintained `ALL_TEST_FILES`. A file missing from it is never parsed — the sharder still assigns it and still packs a weight for it, and it reports nothing. Three files were in that state: `load-links-batching-test.ts`, `server-endpoints/delegate-session-test.ts`, and `shard-assignment-test.ts`, which arrived with the weighted sharding, so the invariants it pins were not being checked either. delegate-session also imports `module` and `test` as named exports, which qunit's CJS entry does not provide, and names its module with `basename(__filename)`, undefined in an ES module — it would have thrown on load had it ever been registered. Both corrected to the form the rest of the suite uses. The second mechanism is the module filter. Each shard parses every file and narrows what *runs* with a QUnit name filter built from its assigned files, `^(?:a-test\.ts|b-test\.ts)(?:\s>\s|:)`, matched against `<module>: <test>`. A module titled `X | qualifier` is followed by neither separator, so it selects nothing — indistinguishable from a file with no tests. `node-realm-test.ts | file stat probing` has never run for that reason. CS-12731 then added four more while retitling files so the weights generator could attribute them, which stopped `cpu-profiler-affinity-gate-test.ts` and `prerender-artifact-sink-test.ts` from running at all. The filter now admits ` | ` as a third separator, which fixes the pre-existing case too. `ALL_TEST_FILES` and the filter helpers move to `tests/helpers/suite-registry.ts` so a test can import them. Two tests in `shard-assignment-test.ts` cover the two failures: every file on disk is registered exactly once and nothing registered is missing, and the filter selects every top-level module title declared anywhere in the suite. Both mutation-checked — restoring either bug fails exactly one of them, naming the file and module. Verified locally with the pg harness: every qualified module now runs and passes, `node-realm-test.ts | file stat probing` among them. The two newly-registered files need the Matrix homeserver, which a local run does not have; their endpoint and helpers both exist, so CI is the check. Fixes CS-12759. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Host Test Results 1 files ±0 1 suites ±0 1h 58m 17s ⏱️ +7s Results for commit afe7c5d. ± Comparison against earlier commit db48f45. Realm Server Test Results 1 files ±0 199 suites ±0 1h 4m 11s ⏱️ - 10m 11s Results for commit afe7c5d. ± Comparison against earlier commit db48f45. |
The test counted every boxel_index instance query shaped `i.url = $1` as a per-link lookup and demanded zero. One such query is the search path's own: assembling the result runs attachRealmInfo → getRealmInfo → parseRealmInfo, which overlays the indexed RealmConfig card with a single-URL instance() read the first time after indexing has cleared the realm-info cache. That overlay landed two days before this test was written, so the assertion has been red since it first ran — which, until this branch registered the file, was never. Bind-match on the fixture's link targets instead, so only queries that actually resolve a link are counted either way. The batched `IN (...)` path binds those same URLs and still counts as batched. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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. |
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Went after the two silent-drop mechanisms and the guards added for them: registry parity, the module-name filter and its new separator, the retitled delegate-session file, and the loadLinks counting change. I didn't re-review the weighted-sharding machinery these tests sit beside beyond what the new tests touch.
No blocking issues. Both new tests are mutation-checked from the branch: reverting the | separator fails the filter test on exactly the six qualified titles it should (cpu-profiler-affinity-gate ×2, node-realm ×1, prerender-artifact-sink ×3), and the parity test compares 195 registered entries against 195 files on disk with no drift in either direction. The published report for the head commit lists 199 testsuites, which is exactly the number of top-level module titles declared across the suite, so every declared module is producing a suite. parseRealmInfo's indexed-RealmConfig overlay is the single i.url = $1 instance lookup the loadLinks comment attributes it to, and it is cached until indexing clears it — the target-prefix exclusion tightens that test rather than loosening it.
Recommendations, all non-blocking:
ALL_TEST_FILESis now a set-equal duplicate ofdiscoverTestFiles(); decide whether load order is load-bearing and delete the list if it isn't — comment on the declaration.- Restate four of the new comments without the incident's dates and counts — comment on the header.
- Drop
basename(__filename)fromdeclaredModuleTitles' accepted forms now that no file uses it — file comment onshard-assignment-test.ts.
Follow-up, not this PR. Nothing yet checks the symptom you diagnosed from. Both new tests pin the inputs — the registry is complete, the filter can express every title — but a shard that dies mid-run, or a module that registers no tests, still yields a merged report with a missing suite behind a green tick. The merge job already produces realm-server.xml and createResolver already maps a suite name to a file, so failing that job when a discovered file has no suite covers the outcome directly and is a handful of lines.
Adjacent, out of scope. load-links-batching-test.ts's header comment names the test by a private ticket ID. That file has never run in CI before this PR, so the comment is effectively new prose to every reader of it; worth restating as the invariant when someone next touches the file.
Generated by Claude Code
tests/index.ts loaded test files from a hand-maintained array while the sharder walked the disk, and the only thing keeping the two in step was a test asserting they were equal as sets. A list a test proves redundant carries no information except its order, and that order was never chosen: it is the sequence the original import statements accreted in. QUnit under Node runs modules in registration order, and the shards run disjoint, weight-packed subsets that shift as the weights refresh, so no test can rely on another file's position anyway. Derive the load list from the same walk the sharder uses. A file the sharder assigns is then by construction a file the runner parses, and adding a test file no longer requires an edit to a 195-line list to get its shard back to green. The parity test goes with the list. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The title checker in shard-assignment-test.ts read both `basename(import.meta.filename)` and `basename(__filename)` as a file naming its module after itself. No test file uses the second form, and none can: `__filename` is undefined in ES module scope, so a file titled that way throws at load and, because every file is loaded before any test runs, takes its whole shard down with it — while the checker blesses the title. The error message already offers only the first form; the patterns now match it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Four comments in the new test prose described the incident that prompted them — a count of missing tests, a day, which files were affected — and the loadLinks test's header led with a ticket number. Restate each as the invariant it guards: what a filter that cannot express a title does, why the qualifier form is the one most easily left out, why the batching counter matches on the fixture's link targets rather than on query shape, and what loadLinks batches. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
All three addressed: On the follow-up: agreed that the two tests pin inputs, not the outcome. Failing the merge job when a discovered test file has no testsuite in the merged report is the right shape and belongs in its own change. The resolver already maps a suite name to a file, so it is a short script over |
2 participants
Claude: Found while validating the CS-12731 weights generator against a real merged junit report: the report has no suites for several files that exist on disk. Two mechanisms, both silent. Between them 24 tests were being filtered out of every shard, and three whole files — 26 more tests — were never loaded at all.
tests/index.tsloads from a hand-maintainedALL_TEST_FILES. A file missing from it is never parsed — the sharder still assigns it and still packs a weight for it, and it reports nothing. Three files were in that state:load-links-batching-test.ts,server-endpoints/delegate-session-test.ts, andshard-assignment-test.ts, which arrived with the weighted sharding, so the invariants it pins were not being checked either. delegate-session also importsmoduleandtestas named exports, which qunit's CJS entry does not provide, and names its module withbasename(__filename), undefined in an ES module — it would have thrown on load had it ever been registered. Both corrected to the form the rest of the suite uses.The second mechanism is the module filter. Each shard parses every file and narrows what runs with a QUnit name filter built from its assigned files,
^(?:a-test\.ts|b-test\.ts)(?:\s>\s|:), matched against<module>: <test>. A module titledX | qualifieris followed by neither separator, so it selects nothing — indistinguishable from a file with no tests.node-realm-test.ts | file stat probinghas never run for that reason. CS-12731 then added four more while retitling files so the weights generator could attribute them, which stoppedcpu-profiler-affinity-gate-test.tsandprerender-artifact-sink-test.tsfrom running at all. The filter now admits|as a third separator, which fixes the pre-existing case too.ALL_TEST_FILESand the filter helpers move totests/helpers/suite-registry.tsso a test can import them. Two tests inshard-assignment-test.tscover the two failures: every file on disk is registered exactly once and nothing registered is missing, and the filter selects every top-level module title declared anywhere in the suite. Both mutation-checked — restoring either bug fails exactly one of them, naming the file and module.Verified locally with the pg harness: every qualified module now runs and passes,
node-realm-test.ts | file stat probingamong them. The two newly-registered files need the Matrix homeserver, which a local run does not have; their endpoint and helpers both exist, so CI is the check.Fixes CS-12759.