fix(test-harness): invalidate generated tests when the generator changes - #49
Merged
Conversation
main is red. #48 passed locally and failed in CI, and this is why. The harness generates a .test.js per scenario, baking the {bin} command into the generated file at generation time. Generation is incremental: fileEntryCache tracks the scenario files and the generated outputs, and only regenerates scenarios whose inputs changed. Nothing tracked the generator itself. So #48 changed which command the tests run, no scenario file moved, and CI restored a previously generated tests/ directory together with the cache that declared it current. Result: the suites kept executing the old packaged-binary path, which no longer exists: /bin/sh: 1: /__w/.../packages/cli/binaries/spectral: not found Two layers, because either alone leaves a hole. generate-tests.ts now tracks __filename alongside the scenarios. If the generator changed, every scenario is regenerated rather than only those with changed inputs. The CI cache key now includes hashFiles('test-harness/**') — and, critically, so does the restore-keys PREFIX. The prefix was the actual hole: an exact-key miss fell back to test-harness-<os>-<node>-, which happily restored tests generated by an older generator. Verified by reproducing the failure locally before fixing it. The subtlety is that it only appears at steady state: after a run with an empty tests/ directory, the next run sees the generated files as newly tracked and regenerates anyway. It takes three runs to reach the state CI restores into. control, no fix, steady-state cache, generator changed -> dist/index.js (stale) with fix, same cache, generator changed -> dist/FIXED.js (correct) Harness still passes 65/66 locally. The 66th is help-no-document, whose own faketty.sh uses util-linux `script -qfec` syntax that BSD script rejects; it fails identically with a trivial command and is unrelated. Refs #35, #29, #33 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second Windows attempt, and the first one was wrong for a reason worth recording.
win.ts wraps every command in:
powershell -Command "& { cd '<cwd>';<command>;echo LASTEXITCODE=$LASTEXITCODE }"
The whole thing is already inside double quotes. So emitting {bin} as
"C:\Program Files\nodejs\node.exe" "...\dist\index.js"
terminated that wrapper, and PowerShell saw an unquoted path that split on its
space:
& : The term 'C:\Program' is not recognized as the name of a cmdlet...
The call operator I added was firing correctly; the quoting around it was not
surviving. This never came up before because the old {bin} was a single path with
no spaces (binaries/spectral.exe), so it needed no quoting at all.
Quoting is now platform-aware: PowerShell single quotes are literal and pass
through the wrapper intact, while POSIX shells keep double quotes. win.ts
recognises either quote character when deciding to prefix the call operator.
Also fixes the prettier error the lint job caught on the previous commit.
Local state unchanged: 65/66 on macOS, the 66th being help-no-document's BSD
`script` incompatibility.
Windows remains unverified locally and cannot be verified locally. CI is the only
proof, and this is the second attempt at it — stated plainly rather than dressed
up as confidence.
Refs #35, #29
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Refs #35, #29, #33.
mainis red; this is the fix.#48 passed locally and failed in CI. You asked me to verify, I did, and I still missed this — worth being plain about, because the reason I missed it is the interesting part.
What went wrong
The harness generates a
.test.jsper scenario and bakes the{bin}command into the generated file. Generation is incremental —fileEntryCachetracks the scenario files and the generated outputs, regenerating only scenarios whose inputs changed. Nothing tracked the generator itself.So #48 changed which command the tests run, no scenario file moved, and CI restored a previously generated
tests/directory alongside a cache declaring it current. The suites kept executing the old binary path:That is why
test-node (18.20.8)regressed from passing to failing — it wasn't Windows-specific at all.Why local verification didn't catch it
The bug only appears at steady state. After a run with an empty
tests/directory, the next run sees the generated files as newly tracked and regenerates everything regardless. It takes three runs to reach the state CI restores into — and I had been deleting the cache between runs, which is precisely the condition that hides it.My first attempt at a control run even seemed to disprove the bug, for exactly this reason. I only got a true reading after forcing steady state.
The experiment
Reproduced the CI failure locally first, then fixed it, then re-confirmed. Harness still passes 65/66 — the 66th is
help-no-document, whose ownfaketty.shuses util-linuxscript -qfecthat BSDscriptrejects, unrelated and pre-existing.Two layers, because either alone leaves a hole
generate-tests.tsnow tracks__filenamealongside the scenarios. A changed generator regenerates every scenario.The CI cache key now includes
hashFiles('test-harness/**')— and so does therestore-keysprefix. That prefix was the real hole: an exact-key miss fell back totest-harness-<os>-<node>-, which cheerfully restored tests built by an older generator. Adding the hash to the key alone would not have helped.Worth drawing out
This is a cache that could silently serve stale executable content across a semantic change — the same shape as the
{{ checksum }}constant-key defect in #29, and adjacent to the reproducibility concern in #15. Caches that key on the wrong thing do not fail loudly; they hand you yesterday's answer.Given the day's record I would rather this one ran CI before merging.