Skip to content

test: guard the Rstest pool lists, strip publish-only plugins from pools, restore state between tests (#566 §3) - #570

Merged
ScriptedAlchemy merged 9 commits into
mainfrom
test/rstest-hygiene
Sep 5, 2026
Merged

test: guard the Rstest pool lists, strip publish-only plugins from pools, restore state between tests (#566 §3)#570
ScriptedAlchemy merged 9 commits into
mainfrom
test/rstest-hygiene

Conversation

@ScriptedAlchemy

@ScriptedAlchemy ScriptedAlchemy commented Sep 5, 2026

Copy link
Copy Markdown
Owner

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) and packages/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 of rstest.integration-tests.ts and 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: no packages/**/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 by rstest.unit.config.ts:20 and rstest.config.ts:15 and 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 with dot: true, Node's fs.globSync never enters a dot path and has no option to, so the two views agree exactly while no .dir/x.test.ts exists, and the guard names the offender the moment one appears (verified by planting tests/.hidden/planted.test.ts).

Result on main today: 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.ts

modifyLibConfig (rstest.rslib.ts:70-85) now destructures plugins and tools out of the lib config, filters plugins by name against publishOnlyPlugins (rstest.rslib.ts:30), and drops tools.rspack. Exact names were taken from the plugin objects, not the issue text: pluginPublint().name is plugin-publint (the issue guessed rsbuild:publint), and the shim is agent-bundle:esm-node-globals. Kept, and commented as kept: source.define (the __AGENT_BUNDLE_VERSION__ identifier in src/cli.ts:726 must resolve at compile time) and source.tsconfigPath (repointed at the workspace tsconfig).

Verified with DEBUG=rstest (dist/.rstest-temp/.rsbuild/rstest.config.mjs + rspack.config.rstest.mjs):

resolved key before after
plugins plugin-publint, agent-bundle:esm-node-globals []
tools.rspack function (ignoreWarnings + node.__dirname=false) undefined
source.define.__AGENT_BUNDLE_VERSION__ '"0.1.0"' '"0.1.0"' (kept)
rspack node { __dirname: false, __filename: false } unchanged — Rstest's own default
rspack ignoreWarnings [/Module not found/, /Can't resolve 'source-map-support'/u] [/Module not found/]

That is the entire Rspack-level diff. Publint's onAfterBuild never 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 private package.json; it stays out so a runner change cannot arm it. The shim's processAssets scan did run over every test chunk (it prepended the shim to prepack.test.ts's chunk because fixture strings contain __filename).

P2 — restore between tests

  • The Object.defineProperty(globalThis, '__AGENT_BUNDLE_VERSION__', …) stub is dead: with the stub removed, inspect-state.test.ts passes and the compiled test bundle in dist/.rstest-temp contains .version("0.1.0") — the identifier is replaced by source.define at 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 built dist/cli.js and never depended on the stub.
  • rstestHygiene (rstest.rslib.ts:48-53): restoreMocks, clearMocks, unstubEnvs, unstubGlobals all true, merged into the shared withAgentBundleRslibConfig() result (rstest.rslib.ts:87) so the unit, integration, packed, evidence, mcp-conformance and whole-workspace configs get it, and spread into rstest.route-unit.config.ts:21 and rstest.projection.config.ts:25, which build from the shipped agentBundleRstest() helper instead (the helper's behaviour for consumers is unchanged). Confirmed in the resolved config of the unit and route-unit pools. Note: Rstest dispatches restoreMocks ahead of clearMocks (if/else if); its mockRestore is mockReset + original implementation, so calls are cleared too — clearMocks is documented as the floor if restoreMocks is 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 zero rs.spyOn/rs.stubEnv/rs.stubGlobal, so there was nothing left behind for the flags to restore. The only globalThis mutation in a unit-pool test was the dead version stub above. The five direct process.env mutations (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 in finally and live in isolated pools; left as they are.

P3 — rstest.runtime-playground*.config.ts → deleted

git grep -l 'rstest.runtime-playground' -- ':!repos' matched only two historical plan documents under docs/superpowers/plans/; no package.json script or workflow ever referenced either config (git log -S over package.json, .github, scripts is empty), and both fail to load today (realpathSync('node_modules/react') → ENOENT at the workspace root). Deleted both, plus rstest.setup.browser.ts, whose only importers they were.

⚠️ Also deleted: 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:

  • The surface is live. RuntimePlayground is mounted at packages/workbench/src/main.tsx:785 (six src files reference it).
  • The test is unrunnable in any existing pool. It is an Rstest Browser Mode test (page from @rstest/browser, render from @rstest/browser-react; it executes inside the browser). The "existing browser pool" — lifecycles-page.browser.test.tsx and friends in the integration list — is the Node-side @rstest/playwright runtime, 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.
  • Coverage audit of its two scenarios. (1) "mounts Runtime controls and fences reset through its correlated success" — already covered, in a real browser against a real fixture server, by 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 in 20184079: the Flight download (failure alert Runtime Flight response is not valid. on a non-octet-stream response, then a real runtime-run-<id>.flight.bin download via Playwright's download event) joins the first e2e scenario; the span-details toggle renders only when a span carries details, which no producer sets today (the sole producer, examples/rsc-agent-runtime/src/dev/serialize-inspection.ts:204-210, emits id/phase/startedAt/status; the HMR e2e asserts exactly that shape), so it is ported to the SSR unit test runtime-playground.test.ts with 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 — so 398eb3b2 ports 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 a controller.dispatch tab 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 exact readRunFlight/readRunDocument call 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.
  • Nothing else names it or the removed pieces. 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 minus repos/, docs/superpowers/plans/ (historical plans) and the lockfile → no hits. Every remaining rstest.*.config.ts is run by a root script or scripts/run-packed-tests.mjs.
  • Its two remaining prose citations are fixed: docs/architecture/rsc-runtime-workbench.md is regenerated (pnpm docs:runtime-topology; it was already stale on main by 12 unrelated entries, which ride along) and docs/assets/rsc-runtime-workbench/fidelity-ledger.md:60 now cites the surviving e2e only. The root devDependencies @rstest/browser and @rstest/browser-react had no other consumer and are removed (examples/mcp-app declares its own @rstest/browser for its live browser-app pool; @rstest/playwright stays — 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/**, so coverage.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-v8 once locally with coverage.include: ['packages/*/src/**'], record the baseline, then add thresholds a few points under it.

P3 — --reporter=github-actions, sharding

Already satisfied: @rstest/core 0.11.10 defaults reporters to ['default', 'github-actions'] when GITHUB_ACTIONS === 'true' (getDefaultReporters), and no config here sets reporters. Verified by running the guard against a fake stale entry with GITHUB_ACTIONS=true (agent env vars unset, since Rstest switches to the md reporter when it detects an agent): it emits ::error file=…/rstest-pool-lists.test.ts,line=86,col=60,title=…. Recorded as a comment above pnpm test in ci.yml:161-163 so 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 a rstest merge-reports job), so it is proposed below instead.

Timing (unit pool, this machine: 96 cores, load avg 30–60 from other work; 3 runs each)

run 1 run 2 run 3 median
before (wall) 44.71 s 44.04 s 46.30 s 44.71 s
after (wall) 46.72 s 44.24 s 44.36 s 44.36 s
before (reporter build) 1378 ms 1423 ms 1330 ms 1378 ms
after (reporter build) 1237 ms 1192 ms 996 ms 1192 ms

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 main at #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-root package.json (two devDependencies removed, @rstest/browser and @rstest/browser-react, whose only consumer was the deleted browser-mode config; examples/mcp-app declares its own) change. No file under a publishable package's src/ or package.json is 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:

packages/agent-bundle/tests/
  unit/            # today's default pool (build-free, process-free; isolate: false)
  integration/     # integrationTestFiles (builds, processes, browsers; isolate: true)
  packed/          # packedTestFiles (+ release/ for packedReleaseOnlyTestFiles)
  route-unit/      # already a directory
  projection/      # already a directory
  conformance/     # mcpConformanceTestFiles
packages/workbench/tests/{unit,integration,evidence}/
packages/rsc-runtime/tests/{unit,integration}/

then one rstest.config.ts:

export default defineConfig({
  extends: withAgentBundleRslibConfig(),          // hygiene + reduced lib config
  projects: [
    defineProject({ name: 'unit', include: ['packages/*/tests/unit/**/*.test.ts'], isolate: false }),
    defineProject({ name: 'integration', include: ['packages/*/tests/integration/**/*.test.ts'],
      globalSetup: ['./rstest.integration.setup.ts'], pool: { maxWorkers }, testTimeout: 30_000 }),
    defineProject({ name: 'packed', include: ['packages/*/tests/packed/**/*.test.ts'] }),
    defineProject({ name: 'evidence', include: ['packages/workbench/tests/evidence/**'], pool: { maxWorkers: 1 } }),
    // route-unit and projection stay separate runs: they need the `react-server`
    // process condition (`pool.execArgv`), which a project cannot change per worker.
  ],
});

pnpm test becomes rstest run (+ the two agentBundleRstest() pools), CI can --project integration --shard=${{ matrix.shard }}/N --reporter=blob across the Node matrix and rstest merge-reports once, and rstest.integration-tests.ts plus 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: generalPurpose subagent on gpt-5.6-sol-medium (the change-risk-reviewer type could not run — it depends on the TraceDecay MCP, which is unavailable here — so the same model was given the full origin/main...HEAD diff 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 in 8ee774cc:

  1. should-fixfs.globSync never enters dot paths while Rstest globs with dot: 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 planted tests/.hidden/planted.test.ts.
  2. should-fix — paths not normalized for Windows separators; node_modules/dist exclusion and cross-list overlap could miss. → toPosix on every path from fs.globSync, the exclude callback, and the walk.
  3. should-fix@rstest/browser and @rstest/browser-react orphaned in root devDependencies after the browser-mode config deletion. → Removed; lockfile refreshed; pnpm install --frozen-lockfile clean. No peer of @rstest/playwright depends on them.

Round 2 (on 5d50f8d0, merged with main) — 2 findings, both fixed in c4160395; round-1 fixes confirmed correct:

  1. should-fix — the orphan glob listed 8 extensions but Rstest's default include accepts ?(c|m)[jt]s?(x), so .mtsx/.ctsx/.mjsx/.cjsx test 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".
  2. should-fixdocs/architecture/rsc-runtime-workbench.md:122 and docs/assets/rsc-runtime-workbench/fidelity-ledger.md:60 still cited the deleted runtime-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-topology passes; 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.ts ranges ×4, the projection spread line, cli.test.ts line, "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, tinyglobby dot: 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 edits rstest.integration-tests.ts and the lockfile — whichever lands second re-merges.

Round 4 (on 20184079, the coverage port) — 1 should-fix, 1 nit, both fixed in 398eb3b2: 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 reads aria-expanded separately. 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 .tsx is 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/playwright pool), 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/*.yml or the root package.json (verbatim git grep recorded there).

Gate note: while the branch was being verified, a parallel test:integration:run reported 5 failed files (emitted-artifact-effect-surface, host-install-proof, lifecycles.e2e, provider-typegen, cli), all ERR_MODULE_NOT_FOUND dist/cli-runtime.js / Rspack build failed — a self-inflicted race with pnpm test's chained pnpm build rewriting dist/ mid-run, not a regression; the serial pnpm test leg on the same commit passed all four pools.

…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.
@ScriptedAlchemy ScriptedAlchemy added the skip-changeset PR changes a publishable package but ships no observable change; changeset not required label Sep 5, 2026
@changeset-bot

changeset-bot Bot commented Sep 5, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 6db9696

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@pkg-pr-new

pkg-pr-new Bot commented Sep 5, 2026

Copy link
Copy Markdown
npm i https://pkg.pr.new/ScriptedAlchemy/agent-bundle@570
npm i https://pkg.pr.new/ScriptedAlchemy/agent-bundle/create-agent-bundle@570
npm i https://pkg.pr.new/ScriptedAlchemy/agent-bundle/rsc-markdown-stream@570
npm i https://pkg.pr.new/ScriptedAlchemy/agent-bundle/@agent-bundle/runtime@570

commit: 6db9696

… 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
ScriptedAlchemy enabled auto-merge (squash) September 5, 2026 00:50
…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
ScriptedAlchemy enabled auto-merge (squash) September 5, 2026 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changeset PR changes a publishable package but ships no observable change; changeset not required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant