fix(test): rebuild dist/ once before every vitest run - #2555
Merged
Conversation
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 <file>` — 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
Contributor
Greptile SummaryThe PR ensures Vitest rebuilds compiled JavaScript before collecting tests, preventing WASM integration tests from using stale extractor code.
Confidence Score: 5/5The PR appears safe to merge, with the setup consistently covering repository Vitest invocations and no actionable failures identified. The build command does not recurse into tests or clean required artifacts, all identified Vitest commands inherit the root configuration, and the npm spawning pattern matches existing cross-platform repository code. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Vitest
participant Setup as globalSetup
participant Build as npm run build
participant Tests
User->>Vitest: Start test run
Vitest->>Setup: Execute once
Setup->>Build: Rebuild dist/
Build-->>Setup: Updated compiled artifacts
Setup-->>Vitest: Complete
Vitest->>Tests: Collect and execute tests
Tests->>Tests: WASM worker loads fresh dist/
Reviews (1): Last reviewed commit: "fix(test): rebuild dist/ once before eve..." | Re-trigger Greptile |
Contributor
Codegraph Impact Analysis1 functions changed → 0 callers affected across 0 files
|
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Summary
The WASM engine's worker always loads the compiled
dist/domain/wasm-worker-entry.js, even when a test importssrc/*.tsdirectly (Node's worker_threads loader doesn't apply vitest/ts-node transforms).pretestonly runsnpm run doctor(no build), andnpm install'spreparescript buildsdist/once, not on every subsequent extractor edit — so editing an extractor undersrc/silently exercised stale compiled code in every WASM-engine integration test, while parser-level tests (which importsrc/domain/parser.tsdirectly) saw the live edit.Why this bites: the two disagree silently, and the failure is maximally misleading. 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, and would reasonably be reported as one.
Fix
Added a vitest
globalSetup(scripts/vitest-global-setup.ts) that runsnpm run buildonce before the whole test run — regardless of invocation style. This matters because a pretest-only fix (addingnpm run buildto thepretestnpm-lifecycle script) would only covernpm test, not a directnpx vitest run <file>— which bypassespretestentirely and is how individual test files get iterated on during actual development.A hand-rolled staleness check was tried first and reverted
My first attempt compared
dist/domain/wasm-worker-entry.js's mtime against every.tsfile undersrc/, throwing if any were newer. This is wrong for this codebase:tsconfig.jsonsetsincremental: true, sotscskips re-emitting an output file whose compiled content wouldn't change.dist/domain/wasm-worker-entry.js's own mtime reflects when that specific file was last actually recompiled — not when the project was last built — and is routinely older than unrelatedsrc/files even in a fully up-to-date build. Confirmed via local verification: this produced false positives on essentially every run, failing 114 test files.Just running the build sidesteps this entirely by deferring 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 (not once per test file).Tests
tests/unit/vitest-global-setup.test.ts: mocksnode:child_process'sexecFileSyncand verifies the setup hook invokesnpm run buildwith the correctcwd.doctorand confirmed the test fails; restored and confirmed it passes.npm test): 337 files / 5399 tests pass in ~57s (unchanged from baseline — the one-time rebuild cost is negligible against the full suite's runtime).Closes #2439