Repository navigation
fix(test): make two Windows-flaky tests deterministic - #158
Merged
Merged
Conversation
TestMenubarCacheSlowBuildServesCachedThenUpdates compared generated_at with .After, but Windows' clock resolution can stamp the cached build and the background rebuild with the same instant, so the assertion failed only there. Store different data while the rebuild is gated and assert the post-build snapshot carries it, which proves the rebuild replaced the cached snapshot without depending on wall-clock granularity. Keep only a non-decreasing timestamp check. TestHistoryAccountSwitchFailureMarksStale piped its script to node through stdin. Node reads a stdin script until EOF before executing any of it, so a child that never saw EOF printed nothing and sat until the harness killed it - a bare "exit status 1" after the full 15s timeout on Windows. Run node scripts from a temp file through one shared helper instead, which needs no EOF handshake, and report stdout and stderr (plus an explicit timeout message) instead of a bare exit error. Convert the sibling app.js node tests to the same helper and drop their process.exit(0) so the runner also proves the script drains on its own.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
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
Two tests failed intermittently and only on the Windows CI job (run 39778619812, PR #156), while main's own CI passed.
1.
TestMenubarCacheSlowBuildServesCachedThenUpdatesThe test proved the rebuild replaced the cached snapshot by checking
fresh.GeneratedAt.After(cached.GeneratedAt). Windows' system clock resolution is coarse enough that the cached build and the gated background rebuild receive the same timestamp, so.Afterlegitimately fails there.The test now stores different Cursor data (
total_usage75%) while the rebuild is gated, then asserts the post-build snapshot carries that data, and that it was served from the cache rather than a third build. That proves the rebuilt snapshot replaced the cached one without relying on wall-clock granularity. The timestamp assertion is kept only as a non-decreasing check. Verified it still fails if the store step is mutated to keep the stale cache.The other
TestMenubarCache*,TestMenubarPrebuild*andTestMenubarRebuildGaptests were checked: none order two builds bygenerated_at.2.
TestHistoryAccountSwitchFailureMarksStaleThe script was piped to node on stdin. Node reads a script from stdin until EOF before executing any of it (reproduced locally: a 3s-delayed EOF printed nothing for 3s). When the child did not see EOF in time it emitted no assertion output at all and simply sat until the 15s
CommandContextdeadline killed it. On Windows Go reports the killed child asexit status 1, which hid both the cause and the absence of diagnostics.All node scripts in these history tests now run from a temp file via a shared
runNodeScripthelper: no EOF handshake, stdout and stderr always captured, and an explicit "did not finish within 15s" message for a hang. Paths are not embedded, sojsStringLiteralis not needed. The sibling tests (history_stale_mark_test.go,history_never_blank_test.go,history_chart_state_test.go,history_provider_stale_test.go) were converted too, and theirprocess.exit(0)calls dropped so the helper also proves the script drains on its own.Test plan
go test -count=30onTestMenubarCache*,TestMenubarPrebuild*,TestMenubarRebuildGap,TestHistoryAccountSwitchFailureMarksStale- passgofmt -l .clean,GOOS=windows go vet ./internal/web/clean./app.sh --smokepasses;go test -race -timeout 25m ./internal/web/passes