Stop the tests losing entries to a race in their own harness - #50
Merged
Conversation
ScanDirectory and ConvertFiles invoke onEntry concurrently from worker threads and document that the caller must synchronise. Both production callers use a ConcurrentBag. The tests passed List<T>.Add. Measured before fixing: over 200 files, 3 runs in 40 collected fewer entries than were reported, losing one or two each time. In a test that is worse than in the product. A dropped entry does not throw; it quietly removes a file from what the test then asserts about, so the test still passes while asserting less than it claims. It surfaced in CI as AFileChangedAfterTheConfirmation_StopsTheWholeRun reporting Converted instead of PlanWentStale - not because the staleness check failed, but because the file modified after planning had never reached the plan, and FindStaleFiles only looks at files a plan schedules. EntrySink collects into a ConcurrentBag and enumerates in path order, so tests reading several entries also get a fixed order. Every onEntry callback in the suite now uses it. The stale-plan test now states that both files reached the plan. That assertion is the one that should fail if this ever regresses, rather than the outcome quietly changing. 432 passing. Co-Authored-By: Claude Opus 5 <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.
The CI flake was the test harness losing entries
AFileChangedAfterTheConfirmation_StopsTheWholeRunfailed on master reportingConvertedwhere it expectedPlanWentStale.The staleness check did not fail. The file modified after planning had never reached the plan, and
FindStaleFilesonly inspects files a plan actually schedules — so there was nothing to find stale.Cause
ScanDirectoryandConvertFilesinvokeonEntryconcurrently from worker threads, and say so:Both production callers use a
ConcurrentBag. The tests passedList<T>.Add.Measured, not assumed
A throwaway probe scanning 200 files, 40 times, comparing a
Listcollector against aConcurrentBagover identical scans:Entries were being dropped. It did not reproduce in 12 local runs of the failing test, which is what a narrow race looks like.
Why this is worse in a test than in the product
A dropped entry does not throw. It quietly removes a file from what the test then asserts about, so the test still passes while asserting less than it claims. Any of these could have been silently weakened; only the one whose subject was the dropped file failed visibly.
Fix
EntrySinkcollects into aConcurrentBagand enumerates in path order, so tests reading several entries also get a fixed order. EveryonEntrycallback in the suite now uses it — 20 test files.The stale-plan test now asserts that both files reached the plan. That is the assertion that should fail if this regresses, rather than the outcome quietly changing.
No production code is touched. The concurrent
onEntrycontract is correct and documented; the tests were not honouring it.422 passing.
🤖 Generated with Claude Code