CBG-5969: fix staticcheck single-case select warnings - #8519
Conversation
- Enable staticcheck rule S1000 in .golangci-strict.yml - Replace single-case select inside db/change_cache_test.go with a simple channel receive - Remove redundant loop and select on ticker.C in tools/cache_perf_tool/main.go
|
Droid finished @torcolvin's task —— View job |
There was a problem hiding this comment.
Pull request overview
This PR addresses staticcheck S1000 (“single-case select”) warnings by simplifying channel/ticker waits to direct receives, and updates the strict golangci-lint configuration accordingly.
Changes:
- Replace single-case
selectblocks with direct channel receives in a tool and a unit test. - Remove the
.golangci-strict.ymlexemption forS1000so future violations are flagged.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/cache_perf_tool/main.go | Simplifies the test-duration wait logic (removes single-case select). |
| db/change_cache_test.go | Simplifies feed consumption loop (removes single-case select). |
| .golangci-strict.yml | Re-enables staticcheck rule S1000 enforcement. |
| ticker := time.NewTicker(*timeToRun) | ||
| defer ticker.Stop() | ||
|
|
||
| outerloop: | ||
| for { | ||
| select { | ||
| case <-ticker.C: | ||
| cancelFunc(errors.New("test duration complete")) | ||
| break outerloop | ||
| } | ||
| } | ||
| <-ticker.C | ||
| cancelFunc(errors.New("test duration complete")) |
| go func() { | ||
| for feedClosed == false { | ||
| select { | ||
| case entry, ok := <-feed: | ||
| if ok { | ||
| // feed sends nil after each continuous iteration | ||
| if entry != nil { | ||
| log.Println("Changes entry:", entry.Seq) | ||
| changes.lock.Lock() | ||
| changes.entries = append(changes.entries, entry) | ||
| changes.lock.Unlock() | ||
| } | ||
| } else { | ||
| log.Println("Closing feed") | ||
| feedClosed = true | ||
| entry, ok := <-feed | ||
| if ok { | ||
| // feed sends nil after each continuous iteration |
There was a problem hiding this comment.
+1 if we weren't changing much here - but this whole test is a bit weirdly structured and seems overly reliant on timing/sleeps? I'm not sure how this is not flaky.
I think this actually warrants a rewrite using synctest to simulate the time and probably a waitgroup goroutine to drain the feed?
![factory-droid[bot]](https://avatars.githubusercontent.com/in/358017?s=60&v=4){kind=small}
bbrks
left a comment
bbrks
left a comment
There was a problem hiding this comment.
I very much dislike these two areas of code already - what do we want to do about them? The changes to satisfy the linter don't go far enough IMO
| @@ -225,14 +225,8 @@ func main() { | |||
| ticker := time.NewTicker(*timeToRun) | |||
| defer ticker.Stop() | |||
|
|
|||
| outerloop: | |||
| for { | |||
| select { | |||
| case <-ticker.C: | |||
| cancelFunc(errors.New("test duration complete")) | |||
| break outerloop | |||
| } | |||
| } | |||
| <-ticker.C | |||
There was a problem hiding this comment.
Why is this not just a time.Sleep()?
| go func() { | ||
| for feedClosed == false { | ||
| select { | ||
| case entry, ok := <-feed: | ||
| if ok { | ||
| // feed sends nil after each continuous iteration | ||
| if entry != nil { | ||
| log.Println("Changes entry:", entry.Seq) | ||
| changes.lock.Lock() | ||
| changes.entries = append(changes.entries, entry) | ||
| changes.lock.Unlock() | ||
| } | ||
| } else { | ||
| log.Println("Closing feed") | ||
| feedClosed = true | ||
| entry, ok := <-feed | ||
| if ok { | ||
| // feed sends nil after each continuous iteration |
There was a problem hiding this comment.
+1 if we weren't changing much here - but this whole test is a bit weirdly structured and seems overly reliant on timing/sleeps? I'm not sure how this is not flaky.
I think this actually warrants a rewrite using synctest to simulate the time and probably a waitgroup goroutine to drain the feed?
- tools/cache_perf_tool/main.go: replace the one-shot ticker+receive with time.Sleep, since only a single wake-up is needed. - db/change_cache_test.go TestChannelRace: drop the redundant feedClosed flag in favor of ranging over the feed channel, and replace the fixed time.Sleep-then-assert waits (which didn't allow for slow processing) with require.EventuallyWithT polling. The mid-test sleeps that deliberately land within a specific cache-processing race window are left as-is, since they encode the scenario under test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace timing-dependent sleeps with a LeakyBucket write callback that reliably delays sequence 6's visibility, and drop the background drain goroutine/polling for a simple synchronous drainFeed helper. Update the expected sequences to match the real compound-ID backfill behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CBG-5969: fix staticcheck single-case select warnings
silences warnings for gopls especially.
Integration Tests