diff --git a/docs/agents/pull-requests.md b/docs/agents/pull-requests.md index bccf054af3..41645e35d2 100644 --- a/docs/agents/pull-requests.md +++ b/docs/agents/pull-requests.md @@ -6,9 +6,9 @@ code-quality/dead-code risk is relevant, CI guards are green, no conflict markers or unmerged paths remain. - A local unit-only run is not CI-green. Use `pnpm test:unit` for the repo unit bundle, or - `vitest run --project unit-core --project subprocess-stub` directly. The **Integration Tests** - and **Coverage** jobs run the `provider-integration` project — verify those green on the actual - PR head. + `vitest run --project unit-core --project subprocess-stub --project fuzz-worker` directly. + The **Integration Tests** and **Coverage** jobs run the `provider-integration` project — + verify those green on the actual PR head. - Device-facing behavior is not merge-ready without real simulator/emulator/device evidence for the changed path. Fixture-backed tests prove contracts; they do not replace a live run that creates or observes the artifact/state the feature claims to handle. If live verification is blocked, diff --git a/package.json b/package.json index 85f4ea931b..6f3914b759 100644 --- a/package.json +++ b/package.json @@ -171,10 +171,11 @@ "test-app:maestro:ios": "pnpm test-app:maestro --platform ios", "test-app:maestro:android": "pnpm test-app:maestro --platform android", "test": "pnpm test:unit", - "test:unit": "vitest run --project unit-core --project subprocess-stub", + "test:unit": "vitest run --project unit-core --project subprocess-stub --project fuzz-worker", "test:maestro-compat": "vitest run --project unit-core packages/maestro src/daemon/adapters/maestro src/compat/__tests__/replay-input.test.ts", - "test:coverage": "vitest run --coverage", - "test:coverage:ci": "vitest run --coverage", + "test:coverage": "vitest run --coverage --project=!fuzz-worker && pnpm test:fuzz-worker", + "test:coverage:ci": "vitest run --coverage --project=!fuzz-worker && pnpm test:fuzz-worker", + "test:fuzz-worker": "AGENT_DEVICE_COVERAGE_SHARD= AGENT_DEVICE_COVERAGE_MERGE= vitest run --project fuzz-worker", "test:integration:provider": "vitest run --project provider-integration", "test:integration:progress": "node --experimental-strip-types scripts/integration-progress.ts", "test:integration:progress:check": "node --experimental-strip-types scripts/integration-progress.ts --check", diff --git a/scripts/gate/audit-coverage.test.ts b/scripts/gate/audit-coverage.test.ts index e3d2df430c..a6a8daf82b 100644 --- a/scripts/gate/audit-coverage.test.ts +++ b/scripts/gate/audit-coverage.test.ts @@ -95,8 +95,9 @@ test('a path filter that excludes a category fails, though the check still runs assert.ok(found.some((message) => /selects "daemon-wire-compat"/.test(message))); }); -// The Coverage lane runs a bare `vitest run --coverage`, which runs every project the config -// declares — so an unrun project is only representable once that script names its projects. +// The Coverage lane's two legs together run every project the config declares — the instrumented +// one takes `--project=!fuzz-worker` and the nested `test:fuzz-worker` takes the rest — so an unrun +// project is still only representable once that script names its projects positively. const projectScoped = (projects: readonly string[]): string => `vitest run --coverage ${projects.map((name) => `--project ${name}`).join(' ')}`; diff --git a/scripts/gate/declarations.ts b/scripts/gate/declarations.ts index d49d98db7a..e3a6a1f7b9 100644 --- a/scripts/gate/declarations.ts +++ b/scripts/gate/declarations.ts @@ -2,7 +2,9 @@ // A script whose Vitest/node-test invocation the loader cannot read, mapped to the units it // really runs. Empty right now: the one entry was a coverage wrapper, and `test:coverage:ci` -// is a plain `vitest run --coverage` again, which the loader reads directly. +// is back to shapes the loader reads directly. It is no longer a single bare run — it is a +// negated `--project` leg plus a nested script whose body carries an env prefix — but +// scripts/gate/model.ts resolves both, so the units still come from the script itself. export const OPAQUE_RUNNERS: Readonly> = {}; export const REPORTING_SCRIPTS: Readonly> = { diff --git a/scripts/gate/model.test.ts b/scripts/gate/model.test.ts index 455b979f98..7ba63f4712 100644 --- a/scripts/gate/model.test.ts +++ b/scripts/gate/model.test.ts @@ -65,6 +65,31 @@ test('a bare Vitest run spans every configured project', () => { ]); }); +test('a negated --project subtracts from the configured set, so the skipped one is not credited', () => { + assert.deepEqual( + scriptUnits('cov', scriptModel({ cov: 'vitest run --coverage --project=!subprocess-stub' })), + ['vitest:unit-core'], + ); +}); + +// The real `test:coverage:ci` shape: the second leg is a nested script whose body carries an env +// prefix (it blanks the coverage-shard switches). Both indirections have to survive, or the lane +// stops owning the project it hands to that leg. +test('the two halves of test:coverage:ci together still own every project', () => { + assert.deepEqual( + scriptUnits( + 'test:coverage:ci', + scriptModel({ + 'test:coverage:ci': + 'vitest run --coverage --project=!subprocess-stub && pnpm test:subprocess-stub', + 'test:subprocess-stub': + 'AGENT_DEVICE_COVERAGE_SHARD= AGENT_DEVICE_COVERAGE_MERGE= vitest run --project subprocess-stub', + }), + ), + ['vitest:unit-core', 'vitest:subprocess-stub'], + ); +}); + test('aggregates expand transitively, so a lane running the aggregate owns its parts', () => { const units = scriptUnits( 'check:all', diff --git a/scripts/gate/model.ts b/scripts/gate/model.ts index 493947b0b2..16f4ce2893 100644 --- a/scripts/gate/model.ts +++ b/scripts/gate/model.ts @@ -70,9 +70,24 @@ function vitestArgs(parts: readonly string[]): { const RUNNER_TOKENS = /^(?:pnpm|exec|vitest|run)$/; +/** + * Vitest's own `--project` semantics: bare names select, `!name` excludes, and a run with only + * exclusions starts from every configured project. The model has to read the negated form or it + * would credit a lane with a project it skips — `test:coverage:ci` runs `--project=!fuzz-worker` + * and hands that project to a second, uninstrumented invocation. + */ +function selectedProjects(named: readonly string[], projects: readonly string[]): string[] { + const excluded = new Set( + named.filter((name) => name.startsWith('!')).map((name) => name.slice(1)), + ); + const included = named.filter((name) => !name.startsWith('!')); + const base = included.length > 0 ? included : projects; + return base.filter((name) => !excluded.has(name)); +} + function vitestUnits(parts: readonly string[], projects: readonly string[]): Unit[] { const { named, files } = vitestArgs(parts); - const selected = named.length > 0 ? named : projects; + const selected = named.length > 0 ? selectedProjects(named, projects) : projects; const suffix = files.length > 0 ? `@${files.join(',')}` : ''; return selected.map((project) => `vitest:${project}${suffix}`); } diff --git a/vitest.config.ts b/vitest.config.ts index e54ac6703c..ae6ba7f6da 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -8,14 +8,51 @@ import slowTestGateReporter from './scripts/vitest-slow-test-reporter.ts'; // them one at a time to bound that contention; per-file `process.env` isolation is // already delivered by `pool: forks` + `isolate: true` on every project. // Membership and the project's deletion test live in issue #1823. -export const SUBPROCESS_STUB_TESTS: readonly string[] = [ +const SUBPROCESS_STUB_TESTS: readonly string[] = [ // Stubs npx plus the package managers and spawns a real Metro dev server per case. 'src/__tests__/client-metro.test.ts', // The SUT is the subprocess watchdog: a node subprocess per case, one hangs on purpose (#1414). 'scripts/fuzz/harness.test.ts', - // Replays the fuzz corpus through that same worker watchdog, waiting its per-case budget. +]; + +// The fuzz corpus replay, which must not run under V8 coverage instrumentation. +// +// #1824 found two causes behind `Worker exited unexpectedly`. Shape (A) — a test signalling +// a fabricated pid that landed on a sibling fork — was fixed at the source in #1854. Shape +// (B) was left open: one fork dies mid-file, alone, with no test attributed. Scanning every +// failed Coverage job across the 120 CI runs after #1854 merged found the signature five +// times, and the vanished file was this one all five (plus #1866's, so six for six) — 23% of +// Coverage failures in that window. The uninstrumented unit lane has never lost it. +// +// So the coverage run skips this project and a second, uninstrumented Vitest invocation owns +// it — see `test:coverage:ci`. That costs no coverage at all, which is measured rather than +// assumed: the cases execute inside worker threads, a separate isolate the fork's inspector +// session never instruments, so this file reports the same lines with or without it. +// +// The second leg goes through `test:fuzz-worker`, which blanks AGENT_DEVICE_COVERAGE_SHARD and +// AGENT_DEVICE_COVERAGE_MERGE — the sharding switches read just below. ci.yml sets them as +// *job*-level env over a single `gate: unit-ci` step, so without the blanking both legs inherit +// them and the shard dies: Vitest refuses `--shard=1/2` over a one-file project ("must be a +// smaller than count of test files"), and the blob reporter overwrites the instrumented shard's +// report on its way out, leaving the Coverage Report job nothing to merge. Measured, not +// reasoned: the unguarded leg leaves a 1.4 kB blob holding only this project plus that error. +// +// Membership is by demonstrated failure, not by a property of the code. In particular it is +// NOT "constructs a `node:worker_threads` Worker": `session-replay-runtime-maestro.test.ts` +// does exactly that and stays in `unit-core`, instrumented and green. The proximate cause was +// never reproduced — what these entries share is an observed record of vanishing from the +// Coverage lane, and that record is the only thing that admits a file here. A new entry needs +// its own run URLs; a theory about workers is not enough. +const FUZZ_WORKER_TESTS: readonly string[] = [ + // Replays the fuzz corpus through the worker watchdog, waiting its per-case budget (#1414). 'scripts/fuzz/corpus-replay.test.ts', ]; +/** + * Everything the serialized projects own, which is what the fast lane must not also collect. + * The two lists above stay module-local: this union is the whole cross-file surface, and the + * mutation lane wants exactly it — every test the root config declines to run in parallel. + */ +export const SERIALIZED_TESTS: readonly string[] = [...SUBPROCESS_STUB_TESTS, ...FUZZ_WORKER_TESTS]; // Imported by vitest.mutation.config.ts so the two lanes cannot drift: a guard // added here must reach the Stryker sandbox too. @@ -142,7 +179,7 @@ export default defineConfig({ // The Maestro conformance oracle runs via `node --test` in its own CI // job (scripts/maestro-conformance), like the layering guard. ], - exclude: [...SUBPROCESS_STUB_TESTS], + exclude: [...SERIALIZED_TESTS], setupFiles: SETUP_FILES, }, }, @@ -156,6 +193,19 @@ export default defineConfig({ maxWorkers: 1, }, }, + { + test: { + // Same serialization as its sibling above, for the same contention reason: the + // per-case watchdog budget is real wall clock. The project exists so the coverage + // run can leave it out, not to run it differently. + name: 'fuzz-worker', + include: [...FUZZ_WORKER_TESTS], + setupFiles: SETUP_FILES, + fileParallelism: false, + isolate: true, + maxWorkers: 1, + }, + }, { test: { name: 'provider-integration', diff --git a/vitest.mutation.config.ts b/vitest.mutation.config.ts index 09e3b2bcaa..5555841e2c 100644 --- a/vitest.mutation.config.ts +++ b/vitest.mutation.config.ts @@ -3,7 +3,7 @@ import { fileURLToPath } from 'node:url'; import { defineConfig } from 'vitest/config'; import { readTestScope, threadHostileTestFiles } from './scripts/mutation/test-scope.ts'; import { workspaceSourceAliases } from './scripts/mutation/workspace-aliases.ts'; -import { SETUP_FILES, SUBPROCESS_STUB_TESTS } from './vitest.config.ts'; +import { SERIALIZED_TESTS, SETUP_FILES } from './vitest.config.ts'; const repoRoot = path.dirname(fileURLToPath(import.meta.url)); @@ -24,7 +24,7 @@ const workspaceAliases = workspaceSourceAliases(repoRoot); // (`vitest related` over the mutated files) and hands it over through // AGENT_DEVICE_MUTATION_TEST_FILES; the fallback is the deterministic unit suite, // which keeps `pnpm exec stryker run` usable by hand. Excluded either way: the -// subprocess-stub group and the CLI-capture tests — see +// serialized groups (subprocess-stub and fuzz-worker) and the CLI-capture tests — see // scripts/mutation/test-scope.ts for why, and why excluding them cannot hide a // surviving mutant. const scope = readTestScope(); @@ -35,7 +35,7 @@ export default defineConfig({ }, test: { include: scope ?? ['src/**/*.test.ts', 'packages/*/src/**/*.test.ts'], - exclude: [...SUBPROCESS_STUB_TESTS, ...threadHostileTestFiles(repoRoot), '**/node_modules/**'], + exclude: [...SERIALIZED_TESTS, ...threadHostileTestFiles(repoRoot), '**/node_modules/**'], setupFiles: [...SETUP_FILES], }, });