chore(sensor): use Go 1.25 synctest for queue_test.go #18440
Conversation
Apply testing/synctest package to make queue tests deterministic and faster. Changes: - Replace time.Sleep with synctest.Wait() for goroutine synchronization - Use fake clock for time.After/time.AfterFunc (runs instantly) - Fix pre-existing goroutine leak detected by synctest's strict tracking - Preserve original table-driven test structure Test execution time reduced from ~3.5s to ~0s (instant).
|
Skipping CI for Draft Pull Request. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider using t.Cleanup in each subtest to stop the testStopper/queueStopper and call synctest.Wait(), so that goroutines are reliably cleaned up even if the test fails or bails out early.
- There is a fair bit of repeated setup/teardown logic across the cases in TestPauseAndResume (creating stoppers, queue, final Stop/Wait), which could be factored into a small helper to reduce duplication and make the intended lifecycle easier to see.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using t.Cleanup in each subtest to stop the testStopper/queueStopper and call synctest.Wait(), so that goroutines are reliably cleaned up even if the test fails or bails out early.
- There is a fair bit of repeated setup/teardown logic across the cases in TestPauseAndResume (creating stoppers, queue, final Stop/Wait), which could be factored into a small helper to reduce duplication and make the intended lifecycle easier to see.
## Individual Comments
### Comment 1
<location> `sensor/common/detector/queue/queue_test.go:37-46` </location>
<code_context>
+func noPull(t *testing.T, q *Queue[*string], stopper concurrency.Stopper) {
</code_context>
<issue_to_address>
**issue (testing):** noPull helper does not distinguish between a blocked Pull and an early-unblocked Pull that returns nil
The helper’s comment says it verifies that Pull() blocks when the queue is paused, but it only fails when a non-nil item is received. If Pull() returns nil early (e.g., due to the queue stopping), the goroutine exits without sending on ch and the test times out, treating this as success. To avoid this mismatch, either explicitly assert that Pull() is still blocked (e.g., by checking the goroutine is still running) or rename/reword the helper and add a separate test for the nil-while-paused case.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| func noPull(t *testing.T, q *Queue[*string], stopper concurrency.Stopper) { | ||
| t.Helper() | ||
| ch := make(chan *string) | ||
| go func() { | ||
| defer close(ch) | ||
| select { | ||
| case item := <-q.Pull(): | ||
| ch <- item | ||
| // Pull returned - either got an item or queue was stopped (nil). | ||
| // Use nested select to avoid blocking if nobody reads from ch. | ||
| if item != nil { |
There was a problem hiding this comment.
issue (testing): noPull helper does not distinguish between a blocked Pull and an early-unblocked Pull that returns nil
The helper’s comment says it verifies that Pull() blocks when the queue is paused, but it only fails when a non-nil item is received. If Pull() returns nil early (e.g., due to the queue stopping), the goroutine exits without sending on ch and the test times out, treating this as success. To avoid this mismatch, either explicitly assert that Pull() is still blocked (e.g., by checking the goroutine is still running) or rename/reword the helper and add a separate test for the nil-while-paused case.
|
Images are ready for the commit at 12f4a20. To use with deploy scripts, first |
2 participants
Description
Apply Go 1.25's
testing/synctestpackage (https://pkg.go.dev/testing/synctest) tosensor/common/detector/queue/queue_test.goto make concurrent tests deterministic and significantly faster.What changed
synctest.Test()which provides a fake clock and goroutine trackingtime.Sleepwithsynctest.Wait()for goroutine synchronizationtime.Afterandtime.AfterFuncnow advance instantly when goroutines blocksynctestdetected (orphaned goroutines after timeout)Why
Performance comparison
AI disclosure
This change was developed with AI assistance. The user reviewed the code, identified issues with test coverage preservation and improper use of buffered channels, and directed corrections. The final implementation was validated by the user.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
Ran baseline measurements (5 runs) before changes:
Applied synctest changes and verified all 6 test scenarios pass:
Ran post-change measurements (5 runs) to confirm speedup:
Verified test structure preserved:
TestPauseAndResumewith 6 subtests matching original names