[Testing] fix flaky tests in verification fetcher chunk consumer#8588
Conversation
📝 WalkthroughWalkthroughThe consumer verification test updates the sequential and concurrent receive scenarios to coordinate completion with wait groups and mutex-protected slices instead of atomic counters. The test now waits for all queued work to finish before checking consumer completion and asserting collected locators. ChangesChunk consumer verification tests
Estimated review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/verification/fetcher/chunkconsumer/consumer_test.go (1)
120-126: 🩺 Stability & Availability | 🟠 MajorRequire assertions inside goroutines can cause test hangs if errors occur.
In this test, the loop spawns goroutines that call
require.NoErrorandrequire.True(lines 123–124). If an assertion fails inside a spawned goroutine, thetesting.Tcontext does not abort the main test process immediately; instead, the goroutine terminates silently.This execution path prevents
consumer.Check()from running for that specific item (line 125). Sinceconsumer.Check()triggers the callback that callsfinishAll.Done(), thefinishAll.Wait()on line 129 will wait indefinitely for that missing completion, turning a simple assertion failure into a permanent test hang.To fix this, defer the assertions to the main test goroutine:
Suggested fix
+ var storeErrs []string + var mu sync.Mutex for i := 0; i < len(locators); i++ { go func(i int) { ok, err := chunksQueue.StoreChunkLocator(locators[i]) - require.NoError(t, err, fmt.Sprintf("chunk locator %v can't be stored", i)) - require.True(t, ok) + if err != nil { + mu.Lock() + storeErrs = append(storeErrs, fmt.Sprintf("chunk locator %v can't be stored: %v", i, err)) + mu.Unlock() + return + } + if !ok { + mu.Lock() + storeErrs = append(storeErrs, fmt.Sprintf("chunk locator %v was not stored", i)) + mu.Unlock() + return + } consumer.Check() // notify the consumer }(i) } + storeAll.Wait() // Implicitly wait for goroutines to finish if using WaitGroup, otherwise time.Sleep or similar + + // Assert errors back in the main goroutine to fail the test cleanly + for _, e := range storeErrs { + t.Error(e) + } + + // Or use a WaitGroup approach as originally suggested: + // var sg sync.WaitGroup + // ... + // storeErrs := make(chan string, len(locators)) + // ... + // close(storeErrs) + // for e := range storeErrs { require.Fail(t, e) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/verification/fetcher/chunkconsumer/consumer_test.go` around lines 120 - 126, Move the assertions out of the spawned goroutine in consumer_test.go so a failed check cannot prevent progress and hang the test. In the loop that launches the goroutines around StoreChunkLocator and consumer.Check, have the goroutine only capture the result (and any error) or send it back to the main test goroutine, then perform require.NoError and require.True from the main test flow after the goroutines complete. Use the existing consumer.Check and finishAll synchronization points to keep completion signaling reliable.
🧹 Nitpick comments (1)
engine/verification/fetcher/chunkconsumer/consumer_test.go (1)
132-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the locator set, not just the count.
require.Len(t, called, 100)still passes if one locator is processed twice and another is missed. This test now records every locator, so it can validate the concurrent path much more strongly by comparing the collected locators againstlocatorswithout relying on order.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/verification/fetcher/chunkconsumer/consumer_test.go` around lines 132 - 137, The test in consumer_test.go only checks the number of processed callbacks, so it can miss duplicated or skipped locators. Update the assertion in the chunk consumer test around the called collection to verify the full set of recorded locators matches locators, using the existing called slice and the ChunkConsumer/process callback path, rather than relying on require.Len and order. Keep the concurrency-safe collection logic, but compare contents so every expected locator is seen exactly once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@engine/verification/fetcher/chunkconsumer/consumer_test.go`:
- Around line 120-126: Move the assertions out of the spawned goroutine in
consumer_test.go so a failed check cannot prevent progress and hang the test. In
the loop that launches the goroutines around StoreChunkLocator and
consumer.Check, have the goroutine only capture the result (and any error) or
send it back to the main test goroutine, then perform require.NoError and
require.True from the main test flow after the goroutines complete. Use the
existing consumer.Check and finishAll synchronization points to keep completion
signaling reliable.
---
Nitpick comments:
In `@engine/verification/fetcher/chunkconsumer/consumer_test.go`:
- Around line 132-137: The test in consumer_test.go only checks the number of
processed callbacks, so it can miss duplicated or skipped locators. Update the
assertion in the chunk consumer test around the called collection to verify the
full set of recorded locators matches locators, using the existing called slice
and the ChunkConsumer/process callback path, rather than relying on require.Len
and order. Keep the concurrency-safe collection logic, but compare contents so
every expected locator is seen exactly once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bf8b2fd-0fad-4224-83d8-ec6110f6c607
📒 Files selected for processing (1)
engine/verification/fetcher/chunkconsumer/consumer_test.go
4 participants
Fix flaky tests TestProduceConsume/pushing_10_receive_10
Summary by CodeRabbit