2 Commits

Author	SHA1	Message	Date
Zachary Lowden	e81bcc973f	fix(test-cache): normalise the leading slash POSIX fileURLToPath adds to a Windows file URL (#4981) ... * fix(test-cache): normalise the leading slash POSIX fileURLToPath adds to a Windows file URL `scripts/__tests__/test-cache-core.test.ts` has been red on every pull request opened since 07:13Z today, failing one assertion of 32: × gives the same path for the same file in two worktrees, URL or path AssertionError: expected null to be 'src/a.ts' Root cause, established by executing the function rather than reading it: `fileURLToPath` is platform-dependent. Given `file:///C:/Dev/wt/two/src/a.ts` it returns `C:\Dev\wt\two\src\a.ts` on Windows but `/C:/Dev/wt/two/src/a.ts` on POSIX — with a leading slash. Neither drive-letter comparison in `toRel` can see past that slash, so the path stops matching `root` and `isAbsolute()` returns null instead of the relative path. The test asserts the Windows result; CI runs Linux. The fix normalises the leading slash away immediately after the conversion, so the drive-letter forms below it read the same shape whichever platform resolved the URL. It is one line in the shared prefix of every path reaching this function, which is why the diff carries more comment than code. Verified: - red at the pre-fix commit, green at HEAD. With the fix reverted and the test file unchanged: 2 failed | 32 passed. With the fix: 34 passed. - Six control cases executed before and after — both POSIX spellings, a POSIX path outside the root, an already-relative path, the Windows non-URL path, and a Windows path outside the root. All unchanged by the fix. Only the failing case moves. Two tests added, and they are labelled from what they were MEASURED to do at the pre-fix commit rather than from what they were written to do. The first draft called both "invariant"; the run showed the second one fails at base, so it is regression coverage and is now named that. The POSIX one does pass at base and stays labelled an invariant guard, so nobody counts it as regression coverage it does not provide. Scope note: this is a fix for a defect on main, deliberately kept out of the unrelated PR that surfaced it. #4971, which introduced the test, merged at 07:13:43Z with this same shard already failing on its own head commit. One judgement worth flagging for review: a POSIX path whose first segment is literally a single letter and a colon (`/C:/…`) would now be rewritten. That spelling is pathological on POSIX and cannot be produced by `fileURLToPath` from a non-Windows URL, but it is the one input whose handling this changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(test-cache): scope the normalisation to the file:// branch, and drop a subsumed test Round 0 of the pre-merge audit found two things, both acted on here. Neither is a correctness defect in the shipped fix — CI was fully green at 20282b3e77, 19 entries, 18 success, 1 skipped, shard 2 passing. 1. NARROWED (R1). The normalisation ran on every input, so it also rewrote a genuinely POSIX path whose first segment is a letter and a colon. Measured: toRel('/C:/notes/x.md', '/C:') returned 'notes/x.md' before and null after. That was the one behaviour change the PR body had to flag for review. Only a file:// id can carry the platform artefact, so only a file:// id needs the repair. Moving it inside that branch satisfies the requirement exactly and leaves every non-URL input byte-for-byte as it was. The flagged behaviour change is gone rather than documented. Measured across all three variants on 8 cases: base fails only the Windows file:// URL case; the unscoped draft fixes that but breaks the POSIX pseudo-drive case; the narrowed version is correct on all 8. 2. DELETED a test I added (the "regression" equality at :93-103). The audit ran a mutation table I had not. Against four mutants — normalisation deleted, inverted, prefix-compare broken, and "strip any leading slash" — that test killed only the first, which the pre-existing assertion at :71-72 already kills, and it PASSED both the inverted and broken-prefix mutants. An equality with no anchor is satisfied when both sides return null, which is the property I had described as its strength. It is subsumed, and strictly weaker than the assertion that surfaced the bug. A comment now records why it was removed, so it is not re-added as an apparent improvement. The invariant guard survives and is unchanged: it is the only thing in the file that kills the "strip any leading slash" mutant, and every other toRel assertion here is Windows-shaped while CI and every Linux/macOS dev run POSIX. A new invariant guard pins the POSIX pseudo-drive case the narrowing protects. Re-verified after the change, because an audit fix resets the verification gate: 34 passed at HEAD; at the pre-fix commit 1 failed | 33 passed, the single failure being the genuine regression guard at :71-72. That is a cleaner control than the previous revision, where two failed because the subsumed test failed alongside it. Round 0 reports and does not move the ladder; its advisory verdict was safe to merge, and the requirement survived questioning — the Windows fixture is this repo's only Windows coverage for toRel, since no workflow runs vitest on a Windows runner, so platform-gating or deleting it would zero that coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-09-19 14:22:18 -05:00
Justin Maier	2df0241a4f	feat(test-cache): skip unit test files unchanged since they last passed (#4971) ... * feat(test-cache): shadow-record what a content-keyed result cache would skip Phase 1 of a test-result cache: nothing is skipped. A vitest reporter keys each test file on the content of every first-party module it depends on, plus the inputs no import records (lockfile, vitest config, tsconfig, node, platform), and records files that passed in full. A later run whose key matches is one the cache WOULD skip; if that file then fails, the key missed a dependency and the run is logged as a false skip. Dependencies come from vite's server-side ssr module graph, not diagnostic().importDurations: on a fixture, importDurations missed an `await import()` made inside a test body, and the ssr graph caught it cold and warm. The graph also leaves out the subtree behind a vi.mock factory, which never executes, while keeping the mocked module itself. The store lives in the COMMON git dir, shared by every worktree, and keys use repo-relative paths, so one tree's green run covers every tree whose files are identical. Files that read the filesystem, spawn processes, or import a non-literal specifier always run (243 of 1,880, 6.5% of modelled worker time). The queue gains a hot-configurable cache mode (`test config --cache shadow`, TEST_CACHE_MODE in the skill .env). In shadow mode a queued unit run gets the primary checkout's reporter appended, plus `--reporter=default` when the caller named none, so turning it on never strips a run's normal output. Fixture sequence, each step as predicted: cold 0 skipped; unchanged 1/1; runtime-imported dep changed 0; unchanged again 1/1; dep behind a mock changed still 1/1; env-driven failure with an unchanged key flagged as 1 false skip. 89-file yardstick, cold then warm: 89/89 passed both times, warm would skip 85/89 (98% of worker time), 0 false skips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(test-cache): skip unit test files unchanged since they last passed Turns the shadow recorder into a real cache. Before a queued unit run, a custom sequencer re-fingerprints each test file's recorded dependencies and drops the files whose key still matches a recorded pass; vitest runs exactly the list the sequencer returns. Reusing the OLD dependency list is sound: gaining a dependency means editing a file already in the list, which changes the key. A key covers the test file, every first-party module it imports (from vite's module graph, in whichever environment loaded it, so happy-dom files count too), every file or directory it read at runtime (a setup-file fs tracker, directories fingerprinted by their whole subtree), and the lockfile, configs, node, platform, vitest and the cache's own code. Records are shared by every worktree through the common git dir, up to 8 per test file, so two trees on different code both stay fast. Still always run: files that spawn, glob, or import a computed specifier (19 files, 0.7% of modelled worker time). Known blind spot: environment variables are not in the key. A random ~5% of skippable files run anyway. If one fails, the cache predicted a pass it could not deliver; it writes TRIPPED.json and runs everything until a human removes it. Modes off|shadow|on are hot-configurable on the queue (`test config --cache on`); never on in CI, never applied to a named-files run. Measured, 89-file yardstick: cold 294s, warm 24s (85 skipped, 3 re-sampled, 0 false skips). Fixture scenarios: runtime-imported dep edited, fixture file edited, dependency of a happy-dom test edited each re-ran exactly the affected file; an env-driven failure was flagged as a false skip and tripped the cache. Fixes found by those checks: a fully cached run exited 1 ("no test files"); 44 happy-dom files were never cached; a builtin heuristic dropped top-level directories like `src` from the key. Full unit suite, cache off: Test Files 1 failed | 1904 passed | 3 skipped (1908); the one failure is rest-error-envelope-ledger, which fails identically on origin/main CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(test-cache): close three false-skip paths found by review Adversarial review of af72b46854 confirmed three ways a failing test could be skipped green, each reproduced on a fixture: - An input edited while the run was in flight was fingerprinted at the end and recorded as the version that passed. Records are now refused for any input modified after the run started (directories by their subtree). - A run that failed on an unhandled error still recorded its files, because the module state stays "passed". Nothing is recorded from a run with unhandled errors, or an interrupted one. - A computed import in a HELPER (`import(/* @vite-ignore */ file)`, the form pending-review-mute.test.ts uses) was invisible to the key. Every first-party module in the closure is now scanned, the comment form is matched, and importing child_process/worker_threads/cluster in the closure keeps a file uncached however it is called. Also: a false skip now deletes that file's records, so clearing TRIPPED cannot revive it; TRIPPED is written first and atomically, and an unreadable marker reads as tripped; package.json files and pnpm's installed lock are in the salt; a sibling that would shadow an import (foo.ts beside foo/index.ts) changes the key; the sequencer skips nothing on a file-filtered run or when the cache reporter is not loaded; messages go to stderr; a malformed sample rate falls back to 5%; the fs tracker loads first and covers access/open/ realpath/readlink. Two regressions inside this round, caught by a positive control that ordinary tests still record: scanning the fs tracker's own closure (which imports child_process) marked every test uncacheable, and a call-name pattern matched `regex.exec(` in src/__tests__/setup.ts. The tracker is excluded as instrumentation, and process use is detected by import, not call name. Fixture battery, positive control first (recorded 2, then skipped 2): edited mid-run not recorded and next run fails as it should; leaked rejection blocks recording; helper computed import and namespaced child_process not recorded; shadowing file re-runs; false skip trips and forgets. 89-file yardstick: cold 63s, warm 14s, 84 skipped, 0 false skips; the two files kept uncached are pending-review-mute (the reviewer's case) and a computed-import hook. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(test-cache): close the false skips the second review found Re-review of fbe832cbdc confirmed three more false skips and one breakage: - A dependency deleted or renamed mid-run was recorded as `missing`, which the next run matched. changedSince now asks the parent directory, whose mtime moves on a removal, and a module that was in the graph but is gone at the end refuses the record outright. - `createRequire(...)('child_process')`, `process.getBuiltinModule(...)` and `from"node:child_process"` walked past the import-syntax patterns. The module name is now matched as a string anywhere, plus the common spawn wrappers. The graph-level builtin check is deleted: builtins never enter vite's graph, so it could not fire. - The tracker's wrappers dropped properties living on the function, so `fs.realpathSync.native` (called by next off-Windows) vanished with the cache on. Own properties are copied and `.native` is wrapped. - An unhandled error now blocks only the file vitest attributes it to (VITEST_TEST_PATH, verified present on a leaked rejection); an unattributed one still blocks the run. The key is taken before the change check, closing the window between them. A TRIPPED rename that EPERMs writes in place instead of aborting before records are forgotten. The reporter and sequencer now have their own tests, driven with fake vitest objects (scripts/__tests__/test-cache-reporter.test.ts), led by a positive control that an ordinary file IS recorded. 13 revert controls, each red on its own named test. The changedSince test now actually moves one input past the run start per case, and covers deletion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(test-cache): stop the round-2 fixes from making the cache inert Third review of 3101975fe1 found no new false skips, but two regressions from the previous round that left the cache recording NOTHING on the real repo — every test ran, safely, and none was ever skipped: - The process check matched a bare quoted 'cluster', an ordinary value in the Redis and telemetry code every test's setup reaches: 1880/1880 unit tests uncacheable (36/1880 without it). It now matches the module name only in import-shaped positions: from, import(, require(, getBuiltinModule(, and a call on a call (createRequire(...)('...')). - The deletion check read a missing PARENT as "changed". Every test probes __snapshots__/<file>.snap in a directory that usually never existed, so every record was refused. It now asks the nearest existing ancestor, which still moves when a file or a whole subtree is removed. The per-run memo on the change check is restored (~19s of synchronous work at the end of a full run without it). The fake-driven positive control stayed green through both, because the fakes modelled neither the snapshot probe nor a setup closure mentioning 'cluster'. Added: - a fake control shaped like a real file (snapshot probe + that closure); - scripts/__tests__/test-cache-e2e.test.ts, which runs REAL vitest with the real sequencer, reporter and tracker over a one-file fixture twice and asserts recorded 1, then ran 0 / skipped 1 (3.8s). Reverting either fix reddens both. The e2e test's first control did NOT redden, which exposed that the tracker exclusion covered the whole scripts/test-cache/ directory — including the fixture's own setup file. It now excludes exactly scripts/test-cache/fs-tracker.mjs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-09-19 01:13:43 -06:00