dx(test): opt-in worker-count override for solo local vitest runs - #1964
dx(test): opt-in worker-count override for solo local vitest runs#1964thymikee wants to merge 4 commits into
Conversation
Size Report
npm unpacked components
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. Top changed packed filesNo changed packed files. |
|
Not ready at
The rest of the change is focused and matches #1962, but these two items block readiness. |
|
Both items addressed. 1. Guidance budget (
2. Verified after both changes: |
|
CI status on Reproduction without any of my commits. Checked out
Root cause. Stryker's dry run executes the suite inside its sandbox (
The same test passes in the normal lanes on this PR ( Not fixing it here, since it is unrelated to #1962 and the fix is a judgment call I would rather not make inside a DX PR — the options I see are excluding the ratchet from the mutation test scope in Everything else on this PR is green and both of your review items are addressed. |
|
Prior findings are addressed, but this is not ready at |
resolveVitestMaxWorkers() caps local runs at 2 workers so parallel worktrees and spawn-heavy tests keep headroom, but a solo run that owns the machine pays 6x on a 12-core host for no benefit. Add AGENT_DEVICE_VITEST_MAX_WORKERS to opt in to a higher cap. It is clamped to os.cpus().length so a runaway value can't oversubscribe the host, and it is a no-op in CI (CI already derives its own worker count). A missing, blank, non-numeric, non-integer, or non-positive value falls through to the existing default cap rather than throwing. Default (unset) behavior is unchanged. Closes #1962
docs/agents/testing.md sits at a 10,000-byte per-file ceiling enforced by check:agent-guidance, and the first phrasing pushed it to 10,065. Restate the override in one tighter bullet that leads with the "solo run only" caveat, which is the constraint a reader most needs.
Node documents cpus().length as unfit for sizing application parallelism: it ignores CPU affinity and cgroup limits, so it can report a pool wider than the process may actually use. Clamping against it would inflate the very ceiling this override's safety clamp exists to enforce. availableParallelism() honors those constraints, so the clamp now means what it claims on constrained hosts. Test updated to match.
Review feedback: a new test file beside the resolver, plus its entry in vitest.config.ts's unit-core include list, is a change to test discovery that the mutation lane's `vitest related` graph reads. Fold the override cases into src/__tests__/hermetic-env-setup.test.ts, which is already in the unit suite and already imports the resolver, and drop the config edit entirely so this PR no longer touches test discovery at all. Same six assertions, no coverage lost.
062f499 to
8360daa
Compare
|
Code-review clean at |
|
Done at Change made as asked. Rebased onto Measured mutation scope, by calling the lane's own
Lane run, same command, same machine:
So I could not reproduce the 2 → 816 expansion at this head — I get 804 on both sides, and the lane goes red on Why the 26 fire. In the sandbox The Proposed fix, not landed. You said the mutation-lane fix is a separate call, so I have not touched it. Everything else is green: 30 pass, 2 fail (both these lanes). |
Stryker runs the suite from a sandbox copy under `.tmp/stryker/`, so a test that asserts about the repository checkout itself — its files on disk, or its git history — reads a repository that does not exist, and no answer it gives there means anything. `src/__tests__/test-file-size-ratchet.test.ts` is such a gate, and it fails in the sandbox for two independent reasons. Stryker's `disableTypeChecks` default prepends `// @ts-nocheck` to every copied file, so all 26 pinned files read one line longer than they are; and the sandbox has no `origin/main`, so the gate's history-backed half cannot resolve its merge-base. Fixing either leaves the other — turning off the type-check rewrite was tried first, and CI then failed on the missing ref. The gate's own `.tmp` skip entry cannot save it either: that is matched relative to `REPO_ROOT`, which inside the sandbox *is* the sandbox. Excluding it hides nothing, for the reason this module's header already gives: a mutant only an excluded test could kill shows up as a survivor. A repo-shape gate cannot kill a kernel mutant at all — it asserts about file lengths and history, never about behavior under mutation. Derived rather than listed, matching the neighbouring thread-hostile exclusion, so a future sibling gate is caught the same way. Nothing was wrong with the ratchet or with #1964; before #1969 rewired the module graph, nothing pulled this gate into a lane's scope and the trap stayed hidden. Verified with `pnpm mutation:run --modules kernel-errors`: scope 804 -> 803 test files (exactly this one), dry run clean, lane `pass` in 560s, score 75.2% (188 killed / 62 survived / 250). `pnpm mutation:test` 39/39. `stryker.config.json` is deliberately untouched, so the config content hash the report and lane envelope carry is unchanged and scores stay comparable. Unblocks #1964, whose two mutation checks fail on main's tip without its code.
Stryker runs the suite from a sandbox copy under `.tmp/stryker/`, so a test that asserts about the repository checkout itself — its files on disk, or its git history — reads a repository that does not exist, and no answer it gives there means anything. `src/__tests__/test-file-size-ratchet.test.ts` is such a gate, and it fails in the sandbox for two independent reasons. Stryker's `disableTypeChecks` default prepends `// @ts-nocheck` to every copied file, so all 26 pinned files read one line longer than they are; and the sandbox has no `origin/main`, so the gate's history-backed half cannot resolve its merge-base. Fixing either leaves the other — turning off the type-check rewrite was tried first, and CI then failed on the missing ref. The gate's own `.tmp` skip entry cannot save it either: that is matched relative to `REPO_ROOT`, which inside the sandbox *is* the sandbox. Excluding it hides nothing, for the reason this module's header already gives: a mutant only an excluded test could kill shows up as a survivor. A repo-shape gate cannot kill a kernel mutant at all — it asserts about file lengths and history, never about behavior under mutation. Derived rather than listed, matching the neighbouring thread-hostile exclusion, so a future sibling gate is caught the same way. Nothing was wrong with the ratchet or with #1964; before #1969 rewired the module graph, nothing pulled this gate into a lane's scope and the trap stayed hidden. Verified with `pnpm mutation:run --modules kernel-errors`: scope 804 -> 803 test files (exactly this one), dry run clean, lane `pass` in 560s, score 75.2% (188 killed / 62 survived / 250). `pnpm mutation:test` 39/39. `stryker.config.json` is deliberately untouched, so the config content hash the report and lane envelope carry is unchanged and scores stay comparable. Unblocks #1964, whose two mutation checks fail on main's tip without its code.
|
Code review is clean: the local-only worker override is focused, correctly clamped/validated, ignored in CI, covered in the existing hermetic config suite, documented at the testing-procedure owner, and has 0 B package impact. The two required mutation checks remain red, however. They are deterministic baseline/dependency failures—not this diff or infrastructure: Stryker’s dry run includes the repo size-ratchet test, whose sandbox-rewritten copies gain a line, and the aggregate job then has no mutation report. Land a corrected structural fix for #1977, rebase/update this branch onto it, and rerun mutation CI. No #1964 code changes are otherwise required. |
Stryker runs the suite from a sandbox copy under `.tmp/stryker/`, so a test that asserts about the repository checkout itself — its files on disk, or its git history — reads a repository that does not exist. `test-file-size-ratchet.test.ts` is such a gate, and it fails there for two independent reasons: `disableTypeChecks` (Stryker's default) prepends `// @ts-nocheck` to every copied file, so all 26 pinned files read one line longer than they are; and the sandbox has no `origin/main`, so the gate's history-backed half cannot resolve its merge-base. Fixing either leaves the other. Its own `.tmp` skip entry cannot help: that is matched relative to `REPO_ROOT`, which inside the sandbox *is* the sandbox. Move it to `scripts/__tests__/` and include it explicitly in `unit-core`, the address the repo already uses for maintained gates that are not `src` tests. `KERNEL_TEST_FILE_RE` admits only root/package `src` tests, and its comment already names `scripts/__tests__` as unreachable by construction — so the gate leaves every mutation lane by virtue of where it lives, with no classifier to recognise it and nothing to keep in sync. This replaces the source-text scanner of the previous revision, which was the wrong boundary: it sniffed for a single-quoted `walk-files` import or the string `origin/main`, so a behavioral test could match and be silently excluded while an equivalent repo gate using double quotes, another walker, or another base ref would be missed. The scanner, its test, and its justifying comment are all gone. `REPO_ROOT` and the walked roots are unchanged — both addresses are two levels below the repo root, and `TEST_ROOTS` already included `scripts`, so the gate measures exactly what it did before. The one new assertion pins the invariant this now depends on: `isKernelTestFile` accepts root/package `src` tests and rejects `scripts/__tests__`. Widening that pattern would silently pull the gate back into every lane. Verified with `pnpm mutation:run --modules kernel-errors`: scope 804 -> 803 test files, dry run clean, lane `pass` at stage complete, score 74.8% (187 killed / 63 survived / 250) — unchanged. `pnpm mutation:test` 39/39, `pnpm check:layering` 181/181, `typecheck`, `lint`, `format` clean. `stryker.config.json` is untouched, so scores stay comparable. Unblocks #1964, whose two mutation checks fail on main's tip without its code.
1 participant
Summary
resolveVitestMaxWorkers()caps local Vitest runs at 2 workers so parallel worktrees and spawn-heavy tests keep host headroom. That default is correct, but a run that owns the whole machine pays 6x on a 12-core host for no benefit.This adds an opt-in override:
AGENT_DEVICE_VITEST_MAX_WORKERS=<n> pnpm test:unit.process.env, clamped toos.availableParallelism()(a value like999is not honored literally).availableParallelism()rather thancpus().length: Node documents the latter as unfit for sizing parallelism because it ignores CPU affinity and cgroup limits, which would inflate the very ceiling this clamp exists to enforce.CI === 'true'— CI already derives its own worker count from the isolated runner's CPU pool.DEFAULT_VITEST_MAX_WORKERS = 2, same CI branch).Also added a one-line mention in
docs/agents/testing.md's "Speed rules" section, phrased to lead with the "solo run only" caveat and to stay under that file's 10,000-byte agent-guidance budget.Catches / Evidence / Cost / Kill-criterion
scripts/lib/vitest-concurrency.ts+ a doc sentence indocs/agents/testing.md.What I verified
resolveVitestMaxWorkers(), so I addedscripts/lib/vitest-concurrency.test.ts(wired into theunit-coreVitest project's explicit include list, matching the file's existing convention) covering: unset preserves default, CI ignores the override even when both signals are present, a valid override is honored, an override above available parallelism is clamped, and non-numeric / zero / negative / non-integer overrides fall through to the default.scripts/lib/vitest-concurrency.test.tsandsrc/__tests__/hermetic-env-setup.test.ts(the latter has a pre-existing direct test of this function).pnpm typecheck,pnpm lint,pnpm format:check,pnpm build,pnpm check:agent-guidance(4/4) — all clean.vitest.config.tsmodule with the env var set and confirmedtest.maxWorkerson the resolved config object —4for=4, and12(not999) for=999on this 12-core host. This proves the var reaches Vitest's config and that the clamp applies there, not just in the resolver in isolation. Also ran a livevitest run --project unit-coresubset with the override set, which passed.pnpm check:affectedfails open to the full 54-check gate set for this diff (touchesvitest.config.tsandscripts/lib/, both flaggedworkflow-tooling), which is the documented behavior for tooling/config changes, not a regression.Closes #1962