test: guard the Rstest pool lists, strip publish-only plugins from pools, restore state between tests (#566 §3) - #570
Merged
Conversation
…ols, restore state between tests (#566 §3) Pools are defined by subtraction and nothing checked the lists, the package build's publish-time plugins ran in every pool, and no pool restored mocks or stubs between tests. This lands the interim guards from issue #566 section 3 without the test-file reorganization. - rstest-pool-lists.test.ts: every path in rstest.integration-tests.ts exists, every glob matches, no entry repeats, no file is collected by two lists, and no `packages/**/tests` test file is left without a pool. The lists were clean; the orphan check found runtime-playground.browser.test.tsx, runnable only through the dead configs below, and it is deleted with them. - rstest.rslib.ts: `modifyLibConfig` drops `plugin-publint` and `agent-bundle:esm-node-globals` by plugin name and the publish-only `tools.rspack` hook; `source.define` and the tsconfig stay. Resolved config verified with DEBUG=rstest before and after. - `restoreMocks`, `clearMocks`, `unstubEnvs`, `unstubGlobals` on every pool through the shared config (`rstestHygiene`), spread into the route-unit and projection pools that build from the shipped helper. The ten `Object.defineProperty(globalThis, '__AGENT_BUNDLE_VERSION__')` stubs were dead — the compiled test bundle carries `.version("0.1.0")` from `source.define` — and are removed. - Deleted rstest.runtime-playground.config.ts, rstest.runtime-playground.browser.config.ts and rstest.setup.browser.ts: never referenced by a script or workflow, and they fail to load (`node_modules/react` is not resolvable at the workspace root). - ci.yml: note that Rstest already enables the `github-actions` reporter under GITHUB_ACTIONS, so no --reporter flag is needed.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
commit: |
… devDependencies - rstest-pool-lists.test.ts: normalize separators (Windows), prune what Rstest's default exclude prunes, and forbid test files under hidden paths so fs.globSync (no dot support) and Rstest (dot: true) agree. - Remove @rstest/browser and @rstest/browser-react from the root devDependencies: their only consumer was the deleted browser-mode runtime-playground config.
…tations of the deleted browser test
- rstest-pool-lists.test.ts: derive the orphan set from *.{test,spec}.*
filtered by the same regex Rstest's default include accepts, so
.mtsx/.ctsx/.mjsx/.cjsx files are no longer invisible to the guard.
- docs: regenerate the RSC runtime topology (pnpm docs:runtime-topology;
it was already stale on main) and fix the fidelity-ledger citation.
ScriptedAlchemy
enabled auto-merge (squash)
September 5, 2026 00:50
ScriptedAlchemy
disabled auto-merge
September 5, 2026 00:55
…verage into pools that run runtime-playground.browser.test.tsx never had a pool. Its reset-fencing scenario was already covered by runtime-playground.e2e.test.ts; the two uncovered interactions move to where they can honestly run: - Flight tab download (client rejects a non-octet-stream response with an alert; the real route yields runtime-run-<id>.flight.bin) -> the real- browser e2e, plus the UI consequence of real spans carrying no details. - Diagnostics span-details toggle (renders only when a span carries details, which no producer sets today) -> the SSR unit test, with a fabricated span.
…tch span toggles order-independently The deleted browser-mode test's first scenario also proved a rejected reset focuses the request alert, hands the controls back, leaves the state version untouched, and lets the next attempt through. The e2e now stubs the first reset response (an invalid state wrapper the client refuses) and asserts exactly that before the real reset it already exercised.
ScriptedAlchemy
enabled auto-merge (squash)
September 5, 2026 01:21
This was referenced Sep 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lands the three P2s and the in-scope P3s of #566 §3 (tests) without the test-file reorganization — that is scoped as a follow-up at the bottom, not executed, because several in-flight PRs add or edit tests. No test file is moved or renamed.
packages/agent-bundle/tests/mcp-session-service.test.ts(the fixed-sleeps P3) andpackages/rsc-runtime/src/**are untouched.Per-item resolution
P2 — pools defined by subtraction → interim guard
packages/agent-bundle/tests/rstest-pool-lists.test.ts(new, unit pool). It imports every export ofrstest.integration-tests.tsand asserts, per list: every literal path exists as a file, every glob still matches at least one file, no entry repeats; across lists: no file is collected by two lists; and across the tree: nopackages/**/tests/**/*.{test,spec}.{ts,tsx,…}file is left without a pool (a file the unit include glob does not match runs only if a list names it). The unit include glob moved to a shared export,workspaceTestFileGlob(rstest.integration-tests.ts:8), imported byrstest.unit.config.ts:20andrstest.config.ts:15and by the guard, so the check and the configs cannot drift.Two things make the guard's view provably equal to Rstest's: paths are normalized to
/(Windows), and one assertion forbids test files under a hidden path — Rstest globs withdot: true, Node'sfs.globSyncnever enters a dot path and has no option to, so the two views agree exactly while no.dir/x.test.tsexists, and the guard names the offender the moment one appears (verified by plantingtests/.hidden/planted.test.ts).Result on
maintoday: all 108 entries — 103 literal paths and 5 globs — are current; no duplicates. Nothing in the lists needed fixing. The orphan check did find one file no pool collected:packages/workbench/tests/runtime-playground.browser.test.tsx(see the P3 below). Verified the guard fails with a readable diff on a fake stale entry, on a cross-list duplicate, on the restored orphan, and on a planted hidden test file; 31 assertions, ~360 ms.P2 — production plugins in test builds →
rstest.rslib.tsmodifyLibConfig(rstest.rslib.ts:70-85) now destructurespluginsandtoolsout of the lib config, filterspluginsby name againstpublishOnlyPlugins(rstest.rslib.ts:30), and dropstools.rspack. Exact names were taken from the plugin objects, not the issue text:pluginPublint().nameisplugin-publint(the issue guessedrsbuild:publint), and the shim isagent-bundle:esm-node-globals. Kept, and commented as kept:source.define(the__AGENT_BUNDLE_VERSION__identifier insrc/cli.ts:726must resolve at compile time) andsource.tsconfigPath(repointed at the workspace tsconfig).Verified with
DEBUG=rstest(dist/.rstest-temp/.rsbuild/rstest.config.mjs+rspack.config.rstest.mjs):pluginsplugin-publint,agent-bundle:esm-node-globals[]tools.rspacknode.__dirname=false)undefinedsource.define.__AGENT_BUNDLE_VERSION__'"0.1.0"''"0.1.0"'(kept)node{ __dirname: false, __filename: false }ignoreWarnings[/Module not found/, /Can't resolve 'source-map-support'/u][/Module not found/]That is the entire Rspack-level diff. Publint's
onAfterBuildnever fired under Rstest 0.11.10 (no publint output anywhere in the debug log), so it was registered-but-inert against the workspace root's privatepackage.json; it stays out so a runner change cannot arm it. The shim'sprocessAssetsscan did run over every test chunk (it prepended the shim toprepack.test.ts's chunk because fixture strings contain__filename).P2 — restore between tests
Object.defineProperty(globalThis, '__AGENT_BUNDLE_VERSION__', …)stub is dead: with the stub removed,inspect-state.test.tspasses and the compiled test bundle indist/.rstest-tempcontains.version("0.1.0")— the identifier is replaced bysource.defineat compile time; no__AGENT_BUNDLE_VERSION__survives in any compiled.mjs. The same dead line existed at 10 sites in 7 files; all removed (cli.test.ts×3,install.test.ts×2,eval-cli.test.ts,inspect-state.test.ts,prepack.test.ts,route-graph.test.ts,uninstall.test.ts). The one test that exercises--version(cli.test.ts:247) spawns the builtdist/cli.jsand never depended on the stub.rstestHygiene(rstest.rslib.ts:48-53):restoreMocks,clearMocks,unstubEnvs,unstubGlobalsalltrue, merged into the sharedwithAgentBundleRslibConfig()result (rstest.rslib.ts:87) so the unit, integration, packed, evidence, mcp-conformance and whole-workspace configs get it, and spread intorstest.route-unit.config.ts:21andrstest.projection.config.ts:25, which build from the shippedagentBundleRstest()helper instead (the helper's behaviour for consumers is unchanged). Confirmed in the resolved config of the unit and route-unit pools. Note: Rstest dispatchesrestoreMocksahead ofclearMocks(if/else if); itsmockRestoreismockReset+ original implementation, so calls are cleared too —clearMocksis documented as the floor ifrestoreMocksis ever relaxed.Order-dependent tests revealed and fixed: none. Every pool passed on the first run with the flags on. The reason is structural, not luck: the repo's tests use dependency injection, not the mocking API — the whole tree has three
rs.fn()calls (hooks.test.ts, each created inside its test body) and zerors.spyOn/rs.stubEnv/rs.stubGlobal, so there was nothing left behind for the flags to restore. The onlyglobalThismutation in a unit-pool test was the dead version stub above. The five directprocess.envmutations (projection/mcp-in-memory.test.ts,projection/script-dispatch.test.ts,route-unit/render-route.test.ts,support/durability.ts,support/host-install.ts) are all save/restore pairs with cleanup infinallyand live in isolated pools; left as they are.P3 —
rstest.runtime-playground*.config.ts→ deletedgit grep -l 'rstest.runtime-playground' -- ':!repos'matched only two historical plan documents underdocs/superpowers/plans/; nopackage.jsonscript or workflow ever referenced either config (git log -Soverpackage.json,.github,scriptsis empty), and both fail to load today (realpathSync('node_modules/react')→ ENOENT at the workspace root). Deleted both, plusrstest.setup.browser.ts, whose only importers they were.packages/workbench/tests/runtime-playground.browser.test.tsx(256 lines) — with its uncovered scenarios ported, not dropped. Deleting a test because no pool collected it is a different call from deleting dead configs, so the evidence, in order:RuntimePlaygroundis mounted atpackages/workbench/src/main.tsx:785(sixsrcfiles reference it).pagefrom@rstest/browser,renderfrom@rstest/browser-react; it executes inside the browser). The "existing browser pool" —lifecycles-page.browser.test.tsxand friends in the integration list — is the Node-side@rstest/playwrightruntime, a different runtime. Its only configs were the two dead ones, which fail to load today (realpathSync('node_modules/react')→ ENOENT); wiring a Browser Mode pool (react alias, Chrome launch, a CI leg) is new infrastructure, out of scope here.runtime-playground.e2e.test.ts(reset confirmation, disabled controls, focus restoration, the exact reset request, state-version change). (2) "downloads the selected Flight payload and toggles trace span details" — not covered anywhere, so it is ported in20184079: the Flight download (failure alertRuntime Flight response is not valid.on a non-octet-stream response, then a realruntime-run-<id>.flight.bindownload via Playwright'sdownloadevent) joins the first e2e scenario; the span-details toggle renders only when a span carriesdetails, which no producer sets today (the sole producer,examples/rsc-agent-runtime/src/dev/serialize-inspection.ts:204-210, emitsid/phase/startedAt/status; the HMR e2e asserts exactly that shape), so it is ported to the SSR unit testruntime-playground.test.tswith a fabricated span, and the e2e asserts the real-world consequence (no toggle on the Diagnostics tab). The reviewer's round 4 then found that scenario (1) also proved a rejected reset recovers (alert focused, controls handed back, state untouched, next attempt succeeds) — not covered by the e2e's happy path — so398eb3b2ports that too, by stubbing the first reset response with an invalid state wrapper the client refuses. One substitution: the original asserted focus stayed on the alert after acontroller.dispatchtab change; a real tab click moves focus to the tab, so the e2e asserts the alert stays visible and the status is not focused. What is no longer asserted: the fabricated fixture's exactreadRunFlight/readRunDocumentcall ledgers and its synthetic "Rendered · 1 / 1" document text — the real e2e exercises their functional equivalents against a real server. Both files pass: e2e 3/3 in real Chrome (four runs, 30–40 s), unit 33/33.git grep -n -E 'rstest\.runtime-playground|rstest\.setup\.browser|@rstest/browser-react|@rstest/browser"' -- .github package.json→ no hits; the same over the whole tracked tree minusrepos/,docs/superpowers/plans/(historical plans) and the lockfile → no hits. Every remainingrstest.*.config.tsis run by a root script orscripts/run-packed-tests.mjs.docs/architecture/rsc-runtime-workbench.mdis regenerated (pnpm docs:runtime-topology; it was already stale onmainby 12 unrelated entries, which ride along) anddocs/assets/rsc-runtime-workbench/fidelity-ledger.md:60now cites the surviving e2e only. The root devDependencies@rstest/browserand@rstest/browser-reacthad no other consumer and are removed (examples/mcp-appdeclares its own@rstest/browserfor its live browser-app pool;@rstest/playwrightstays — 18 Workbench e2e importers).Coverage decision: not added. The finding's rationale ("no coverage runs anywhere") is true, but the thresholds in the deleted config (90/90/90/85 over three workbench files) were never calibrated against a real run and there is no measured baseline for
packages/*/src/**, socoverage.include+ thresholds would either fail CI on day one or be numbers picked to pass; V8 collection also adds wall time to the largest per-PR leg. Follow-up: run@rstest/coverage-v8once locally withcoverage.include: ['packages/*/src/**'], record the baseline, then add thresholds a few points under it.P3 —
--reporter=github-actions, shardingAlready satisfied:
@rstest/core0.11.10 defaultsreportersto['default', 'github-actions']whenGITHUB_ACTIONS === 'true'(getDefaultReporters), and no config here setsreporters. Verified by running the guard against a fake stale entry withGITHUB_ACTIONS=true(agent env vars unset, since Rstest switches to themdreporter when it detects an agent): it emits::error file=…/rstest-pool-lists.test.ts,line=86,col=60,title=…. Recorded as a comment abovepnpm testinci.yml:161-163so it isn't re-audited. Sharding is not a one-line change with the current matrix (PRs run a single Node 24 leg; sharding needs a shard dimension,--reporter=blob, and arstest merge-reportsjob), so it is proposed below instead.Timing (unit pool, this machine: 96 cores, load avg 30–60 from other work; 3 runs each)
build)build)Wall time is within noise; the reporter's build phase — where the plugin removal lives — is ~13 % shorter. "After" includes the new 30-assertion guard file (≈0.4 s of glob work). Test count 3576 → 3606.
Gates run locally (all green)
On the branch merged with
mainat #567:pnpm build,pnpm typecheck,pnpm lint,pnpm test=test:unit(3620 passed, 6 skipped; 3621 after the port) +test:route-unit(83) +test:projection(172) +test:integration(1067 passed, 33 skipped; re-run green after the port);pnpm test:packed(27, 1 skipped); evidence pool (1); mcp-conformance lane (1) — every pool the shared config reaches.Changeset
skip-changeset: only test files, Rstest configs, a CI comment, and the private workspace-rootpackage.json(two devDependencies removed,@rstest/browserand@rstest/browser-react, whose only consumer was the deleted browser-mode config;examples/mcp-appdeclares its own) change. No file under a publishable package'ssrc/orpackage.jsonis touched.Proposed follow-up: directory-based pools (not in this PR)
Move suites so membership is the directory, not a list — one move per pool so each PR is mechanical:
then one
rstest.config.ts:pnpm testbecomesrstest run(+ the twoagentBundleRstest()pools), CI can--project integration --shard=${{ matrix.shard }}/N --reporter=blobacross the Node matrix andrstest merge-reportsonce, andrstest.integration-tests.tsplus this PR's guard are deleted — the directory is the guard. Sequence: (1) integration, (2) packed, (3) evidence/conformance, (4) collapse configs. Blocked until the in-flight test PRs land.Self-review
Reviewer:
generalPurposesubagent on gpt-5.6-sol-medium (thechange-risk-reviewertype could not run — it depends on the TraceDecay MCP, which is unavailable here — so the same model was given the fullorigin/main...HEADdiff inline), asked for concrete merge risks only; two rounds, plus four read-only lanes on the second round (PR-body fact-check, stale-reference hunt, Rstest-API verification of the follow-up sketch on gpt-5.6-terra-medium, CI/conflict snapshot).Round 1 (on
8ee774cc's parent) — 3 findings, all fixed in8ee774cc:fs.globSyncnever enters dot paths while Rstest globs withdot: true, so a hidden test file would run under Rstest but be invisible to the guard. → Added a dot-inclusive walk that forbids test files under hidden paths, making the two views provably equal; verified it names a plantedtests/.hidden/planted.test.ts.node_modules/distexclusion and cross-list overlap could miss. →toPosixon every path fromfs.globSync, theexcludecallback, and the walk.@rstest/browserand@rstest/browser-reactorphaned in root devDependencies after the browser-mode config deletion. → Removed; lockfile refreshed;pnpm install --frozen-lockfileclean. No peer of@rstest/playwrightdepends on them.Round 2 (on
5d50f8d0, merged withmain) — 2 findings, both fixed inc4160395; round-1 fixes confirmed correct:?(c|m)[jt]s?(x), so.mtsx/.ctsx/.mjsx/.cjsxtest files had no guard. → The orphan set is now*.{test,spec}.*filtered by the same regex the hidden-path walk uses, so there is one definition of "test file".docs/architecture/rsc-runtime-workbench.md:122anddocs/assets/rsc-runtime-workbench/fidelity-ledger.md:60still cited the deletedruntime-playground.browser.test.tsx. → Regenerated / rewritten (see the P3 above).Round 3 (on
c4160395) — both round-2 findings confirmed fixed (the orphan regex matched all 24 positive extension cases and rejected all negatives against picomatch;pnpm check:runtime-topologypasses; the e2e test does assert focus restoration and selected states); no new findings.Lane results folded in: 8 of 18
file:line/count citations in this description had drifted after the edits (rstest.rslib.tsranges ×4, the projection spread line,cli.test.tsline, "108 literal paths" → 103 literals + 5 globs, 30 → 31 assertions) — all corrected above. Every Rstest API claim in the follow-up sketch (projects/defineProject,--project,--shard,--reporter=blob,rstest merge-reports,getDefaultReporters, restore-before-clear dispatch, tinyglobbydot: true+ default excludes,modifyLibConfig) was verified against the installed 0.11.10. No other tracked file names a deleted artifact or removed dependency. Dismissed: none. Open review threads: none. Conflict exposure noted: #571 also editsrstest.integration-tests.tsand the lockfile — whichever lands second re-merges.Round 4 (on
20184079, the coverage port) — 1 should-fix, 1 nit, both fixed in398eb3b2: the deleted test's rejected-reset recovery path was still unported → ported to the e2e (see the P3 above); the SSR span-toggle regex depended on React's attribute order → it now matches any<button …>… span details</button>and readsaria-expandedseparately. E2E flakiness, SSR typing, and helper hygiene: no findings.Round 5 (on
398eb3b2) — both round-4 items confirmed fixed; the tab-click focus substitution accepted ("a real click correctly transfers focus to the tab; retaining the alert and excluding success-status focus is the honest browser equivalent"); route/unroute ordering, request recorder, and second-confirmation state judged sound; no new findings; verdict: the deleted.tsxis now adequately compensated by stronger real-browser coverage.Coordinator question (delete vs re-attach
runtime-playground.browser.test.tsx): answered under the P3 above — live surface, unrunnable in any existing pool (Browser Mode ≠ the Node-side@rstest/playwrightpool), one of two scenarios already covered, the uncovered interactions ported into the unit and e2e files that do run, no remaining reference in.github/workflows/*.ymlor the rootpackage.json(verbatimgit greprecorded there).Gate note: while the branch was being verified, a parallel
test:integration:runreported 5 failed files (emitted-artifact-effect-surface,host-install-proof,lifecycles.e2e,provider-typegen,cli), allERR_MODULE_NOT_FOUND dist/cli-runtime.js/Rspack build failed— a self-inflicted race withpnpm test's chainedpnpm buildrewritingdist/mid-run, not a regression; the serialpnpm testleg on the same commit passed all four pools.