From 46963b3eb25243f595923b5f63a310eb2312fb40 Mon Sep 17 00:00:00 2001 From: dvcdsys Date: Mon, 10 Aug 2026 17:37:00 +0100 Subject: [PATCH] test(cli): wait for FinishIndex instead of racing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three tests in internal/watcher wait for BeginIndex and then assert on FinishIndex. Those are two sequential HTTP calls from the indexer goroutine, so the window between them is small but real, and on a loaded CI runner the assertion lands inside it. It surfaces as "expected FinishIndex to be called" on a test that took 0.02s — not a timeout, an assertion made too early. Adds waitForFinish for the tests that actually care about completion, and keeps waitForCalls for those that only assert BeginIndex. TestDebounce_MultipleEventsOnce had a different problem with the same shape: its deadline was ten times the 80ms debounce interval, which reads generous and is 800ms. A cold macOS runner loses that, and because the flush goroutine then ran after t.TempDir() cleanup, the failure printed as "cannot read a.go: no such file or directory" rather than as the assertion that failed. The property under test is that five events collapse into one flush, not how quickly, so it now waits on patience rather than on the interval. The shared deadline goes to 15s. It exits the instant the condition holds, so a long limit costs nothing when things work. Both failures appeared on macos-latest during the mac-runtime PRs (#229, #230) and passed on rerun; neither is caused by those changes, but the extra parallel load of a new test package is enough to make the races fire. Co-Authored-By: Claude Opus 5 --- cli/internal/watcher/watcher_test.go | 62 ++++++++++++++++------------ 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/cli/internal/watcher/watcher_test.go b/cli/internal/watcher/watcher_test.go index ec02e1a0..57113b2e 100644 --- a/cli/internal/watcher/watcher_test.go +++ b/cli/internal/watcher/watcher_test.go @@ -124,10 +124,32 @@ func newIndexServer(t *testing.T, dir string) (*httptest.Server, *serverCalls) { // waitForCalls polls the mock server counters until BeginIndex has been called // at least target times, or the deadline is reached. func waitForCalls(calls *serverCalls, target int) { - deadline := time.Now().Add(2 * time.Second) + waitForCounter(calls, target, func(c *serverCalls) int { return c.Begin }) +} + +// waitForFinish waits for FinishIndex, which is what a test asserting on +// calls.Finish actually needs. +// +// Waiting on BeginIndex and then asserting on Finish is a race, and it is the +// one this package kept losing on CI: they are two sequential HTTP calls from +// the indexer goroutine, so the window between them is small but real. It shows +// up as "expected FinishIndex to be called" on a test that took 0.02s — not a +// timeout, an assertion made too early. +func waitForFinish(calls *serverCalls, target int) { + waitForCounter(calls, target, func(c *serverCalls) int { return c.Finish }) +} + +// waitForCounter polls until a counter reaches target, or gives up. +// +// The deadline is generous on purpose. It exits the instant the condition holds, +// so a long limit costs nothing when things work and is the difference between a +// green run and a red one on a cold CI runner — which is where this package has +// failed, never locally. +func waitForCounter(calls *serverCalls, target int, get func(*serverCalls) int) { + deadline := time.Now().Add(15 * time.Second) for time.Now().Before(deadline) { calls.mu.Lock() - count := calls.Begin + count := get(calls) calls.mu.Unlock() if count >= target { return @@ -382,17 +404,10 @@ func TestFlushChanges_TriggersIncrementalReindex(t *testing.T) { w.pendingChanges[filepath.Join(dir, "main.go")] = true w.flushChanges() - // Wait for indexing to complete (it runs in a goroutine now) - deadline := time.Now().Add(2 * time.Second) - for time.Now().Before(deadline) { - calls.mu.Lock() - count := calls.Begin - calls.mu.Unlock() - if count >= 1 { - break - } - time.Sleep(10 * time.Millisecond) - } + // Indexing runs in a goroutine, and it ends with FinishIndex — so that is + // what to wait for. Waiting for BeginIndex and then asserting on Finish is + // the race this test used to lose on CI. + waitForFinish(calls, 1) calls.mu.Lock() defer calls.mu.Unlock() @@ -470,7 +485,7 @@ func TestFlushChanges_RecoveryAfterServerDown(t *testing.T) { w2.pendingChanges[filePath] = true w2.flushChanges() - waitForCalls(calls, 1) + waitForFinish(calls, 1) calls.mu.Lock() defer calls.mu.Unlock() @@ -492,7 +507,7 @@ func TestTriggerFullReindex_CallsAPI(t *testing.T) { w.triggerFullReindex() - waitForCalls(calls, 1) + waitForFinish(calls, 1) calls.mu.Lock() defer calls.mu.Unlock() @@ -664,17 +679,12 @@ func TestDebounce_MultipleEventsOnce(t *testing.T) { w.trackChange(filepath.Join(dir, "b.go")) } - // Poll until the debounce timer fires, with a generous deadline. - deadline := time.Now().Add(time.Duration(w.debounceMS*10) * time.Millisecond) - for time.Now().Before(deadline) { - calls.mu.Lock() - n := calls.Begin - calls.mu.Unlock() - if n >= 1 { - break - } - time.Sleep(10 * time.Millisecond) - } + // Wait for the debounce timer to fire. Ten times the debounce interval reads + // like a generous margin and is 800ms — which a cold CI runner loses. The + // property under test is that five events collapse into one flush, not how + // fast the flush arrives, so the wait is bounded by patience rather than by + // the interval. + waitForCalls(calls, 1) calls.mu.Lock() defer calls.mu.Unlock()