From 3b193907b65dc42bdaa7f07eb9f69fad4496f4fe Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Mon, 17 Aug 2026 04:37:27 -0600 Subject: [PATCH] fix(test): rebuild dist/ once before every vitest run (#2439) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The WASM engine's worker always loads the compiled dist/domain/wasm-worker-entry.js, even when a test imports src/*.ts directly. pretest only runs `npm run doctor` (no build), and `npm install`'s prepare script builds dist/ once, not on every subsequent extractor edit — so an edit silently exercised stale compiled code while parser-level tests saw the live change. In PR #2432 this presented as "the native engine is correct and WASM reproduces the old buggy behaviour", indistinguishable from a genuine engine-parity bug. Added a vitest globalSetup that runs `npm run build` once before the whole run, regardless of invocation style (`npm test` or a direct `npx vitest run ` — the latter bypasses pretest entirely, which a pretest-only fix would have missed). A hand-rolled mtime-based staleness check was tried first and reverted: tsconfig.json sets `incremental: true`, so tsc skips re-emitting an output file whose compiled content wouldn't change. dist/domain/wasm-worker-entry.js's own mtime reflects when THAT file was last actually recompiled, not when the project was last built, and is routinely older than unrelated src/ files even in a fully up-to-date build — comparing raw mtimes produced false positives on essentially every run (confirmed: 114 failing test files during local verification). Just running the build defers to tsc's own incremental engine, which is the only thing that actually knows what's stale, instead of reimplementing it. A no-op incremental rebuild costs about a second, dominated by process startup rather than compilation. docs check acknowledged Impact: 1 functions changed, 0 affected --- scripts/vitest-global-setup.ts | 45 ++++++++++++++++++++++++++ tests/unit/vitest-global-setup.test.ts | 43 ++++++++++++++++++++++++ vitest.config.ts | 7 ++++ 3 files changed, 95 insertions(+) create mode 100644 scripts/vitest-global-setup.ts create mode 100644 tests/unit/vitest-global-setup.test.ts diff --git a/scripts/vitest-global-setup.ts b/scripts/vitest-global-setup.ts new file mode 100644 index 000000000..02b5d552e --- /dev/null +++ b/scripts/vitest-global-setup.ts @@ -0,0 +1,45 @@ +#!/usr/bin/env node +/** + * Vitest `globalSetup` — runs once before the whole test run, regardless of + * whether it was invoked via `npm test` or a direct `npx vitest run `. + * + * Issue #2439: the WASM engine always parses through the COMPILED + * `dist/domain/wasm-worker-entry.js`, even when a test imports `src/*.ts` + * directly — so editing an extractor under `src/` silently exercises stale + * compiled code until `dist/` is rebuilt. `pretest` only runs `npm run + * doctor`, which doesn't rebuild, and `npm install`'s `prepare` script + * builds `dist/` once, not on every subsequent edit. In PR #2432 this + * manifested as "the native engine is correct and WASM reproduces the old + * buggy behaviour" — indistinguishable from a genuine engine-parity bug. + * + * A hand-rolled staleness check (comparing dist/'s mtime against src/'s) + * was tried and reverted: `tsconfig.json` sets `incremental: true`, so tsc + * skips re-emitting an output file whose compiled content wouldn't change — + * `dist/domain/wasm-worker-entry.js`'s own mtime reflects when THAT file + * was last actually recompiled, not when the project was last built, and is + * routinely older than unrelated src/ files even in a fully up-to-date + * build. That produced false positives on every run. Just running the + * build is simpler and correct, because it defers to tsc's own incremental + * engine — the only thing that actually knows what's stale — instead of + * reimplementing it. + * + * A no-op incremental rebuild costs about a second, dominated by process + * startup rather than compilation, and runs ONCE per test invocation here + * (not once per test file), so the added cost is negligible against a + * multi-minute full suite run. + */ + +import { execFileSync } from 'node:child_process'; +import os from 'node:os'; +import { fileURLToPath } from 'node:url'; + +const repoRoot = fileURLToPath(new URL('..', import.meta.url)); + +// npm on Windows is npm.cmd; Node refuses to spawn .cmd/.bat without a shell. +// Safe with shell: true here since the argv is a fixed literal, never +// user-controlled input (matches scripts/doctor.ts's NPM_SHELL convention). +const NPM_SHELL = os.platform() === 'win32'; + +export default function setup(): void { + execFileSync('npm', ['run', 'build'], { cwd: repoRoot, stdio: 'inherit', shell: NPM_SHELL }); +} diff --git a/tests/unit/vitest-global-setup.test.ts b/tests/unit/vitest-global-setup.test.ts new file mode 100644 index 000000000..f9247f78d --- /dev/null +++ b/tests/unit/vitest-global-setup.test.ts @@ -0,0 +1,43 @@ +/** + * Unit test for scripts/vitest-global-setup.ts (issue #2439). + * + * The WASM engine's worker always loads COMPILED dist/, even when a test + * imports src/*.ts directly, so an edit to an extractor silently exercises + * stale compiled code until dist/ is rebuilt. This globalSetup hook rebuilds + * dist/ once before the whole vitest run (regardless of `npm test` vs. a + * direct `npx vitest run `) rather than reimplementing tsc's own + * incremental staleness detection — see that file's doc comment for why a + * hand-rolled mtime comparison was tried and reverted. + */ + +import { fileURLToPath } from 'node:url'; +import { describe, expect, it, vi } from 'vitest'; + +vi.mock('node:child_process', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + execFileSync: vi.fn(), + }; +}); + +// Mirrors exactly how scripts/vitest-global-setup.ts derives its own cwd — +// fileURLToPath(new URL(...)) is not guaranteed trailing-slash-equivalent to +// path.resolve() across platforms, and this test only cares that setup() +// passes ITS OWN computed root through unchanged. +const repoRoot = fileURLToPath(new URL('../..', import.meta.url)); + +describe('vitest global setup (#2439)', () => { + it('runs npm run build in the repo root before the test run starts', async () => { + const { execFileSync } = await import('node:child_process'); + const setup = (await import('../../scripts/vitest-global-setup.js')).default; + + setup(); + + expect(execFileSync).toHaveBeenCalledWith( + 'npm', + ['run', 'build'], + expect.objectContaining({ cwd: repoRoot, stdio: 'inherit' }), + ); + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index c78f12ae3..075ee84b7 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -18,6 +18,13 @@ export default defineConfig({ testTimeout: 30000, hookTimeout: 30000, exclude: ['**/node_modules/**', '**/.git/**', '**/.claude/**'], + // Issue #2439: rebuild dist/ once before the whole run (regardless of + // `npm test` vs. a direct `npx vitest run `), so the WASM engine's + // worker — which always loads compiled dist/, even when tests import + // src/*.ts directly — never silently runs stale extraction logic. See + // scripts/vitest-global-setup.ts for why this isn't a hand-rolled + // staleness check. + globalSetup: './scripts/vitest-global-setup.ts', // Ensure child processes spawned by tests (e.g. CLI integration tests) // can load .ts files via Node's built-in type stripping. env: {