test(config): use synctest in TestAddFlags for avoiding nondeterminism #3311
test(config): use synctest in TestAddFlags for avoiding nondeterminism #3311julienrbrt merged 3 commits intoevstack:mainfrom
synctest in TestAddFlags for avoiding nondeterminism #3311Conversation
📝 WalkthroughWalkthrough
ChangesTest Flakiness Fix via Time Freezing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/config/defaults.go (1)
136-137: 💤 Low valuePackage-level mutable
nowUnixis unsafe if tests ever run in parallel.Mutating a bare
varfrom a test goroutine while another goroutine reads it (e.g., via a concurrentDefaultConfig()call) is a data race thatgo test -racewill catch. Currently no tests in this package callt.Parallel(), so there is no active race, but the risk is latent.A minimal guard would use
sync/atomicwith a storedint64, or accept the constraint by adding a comment that parallel tests must not stubnowUnix. As an alternative, passing the seed as a parameter torandString(and accepting it fromDefaultConfig) avoids the global entirely.🛡️ Option A – atomic swap (low-footprint)
-// nowUnix returns the current Unix timestamp; package-level so tests can stub it. -var nowUnix = func() int64 { return time.Now().Unix() } +import "sync/atomic" +import "unsafe" + +// nowUnixFn is the actual function pointer; swapped atomically in tests. +var nowUnixPtr atomic.Pointer[func() int64] + +func init() { + f := func() int64 { return time.Now().Unix() } + nowUnixPtr.Store(&f) +} + +func nowUnix() int64 { return (*nowUnixPtr.Load())() }In tests:
-origNowUnix := nowUnix -nowUnix = func() int64 { return 2_000_000_000 } -t.Cleanup(func() { nowUnix = origNowUnix }) +f := func() int64 { return 2_000_000_000 } +nowUnixPtr.Store(&f) +t.Cleanup(func() { + orig := func() int64 { return time.Now().Unix() } + nowUnixPtr.Store(&orig) +})🤖 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 `@pkg/config/defaults.go` around lines 136 - 137, The package-level mutable var nowUnix must be replaced with an atomic-backed value to avoid races: introduce a package int64 nowUnixVal and replace the var nowUnix func with a getter NowUnix() that returns atomic.LoadInt64(&nowUnixVal), add a test helper SetNowUnixForTest(v int64) that does atomic.StoreInt64(&nowUnixVal), and update all callers (e.g., DefaultConfig and randString) to call NowUnix() instead of invoking the former nowUnix variable; this preserves test stubbing without data races.
🤖 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.
Nitpick comments:
In `@pkg/config/defaults.go`:
- Around line 136-137: The package-level mutable var nowUnix must be replaced
with an atomic-backed value to avoid races: introduce a package int64 nowUnixVal
and replace the var nowUnix func with a getter NowUnix() that returns
atomic.LoadInt64(&nowUnixVal), add a test helper SetNowUnixForTest(v int64) that
does atomic.StoreInt64(&nowUnixVal), and update all callers (e.g., DefaultConfig
and randString) to call NowUnix() instead of invoking the former nowUnix
variable; this preserves test stubbing without data races.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d64c11e7-8bfc-4fc1-b2f6-1932d49513c1
📒 Files selected for processing (2)
pkg/config/config_test.gopkg/config/defaults.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3311 +/- ##
==========================================
- Coverage 60.87% 60.85% -0.03%
==========================================
Files 127 127
Lines 13762 13762
==========================================
- Hits 8378 8375 -3
- Misses 4473 4476 +3
Partials 911 911
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
julienrbrt
left a comment
julienrbrt
left a comment
There was a problem hiding this comment.
Thanks! Let's use the latest libs
| @@ -41,6 +41,12 @@ func TestDefaultConfig(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestAddFlags(t *testing.T) { | |||
There was a problem hiding this comment.
Let's use testing/synctest instead.
There was a problem hiding this comment.
Yeah good suggestion, then we dont need to modify config/defaults.go updated 👍
julienrbrt
changed the title
julienrbrt
changed the title
synctest in TestAddFlags for avoiding nondeterminism
|
Hey @julienrbrt The failing CI jobs look unrelated to my changes, Docker push and the Claude review fail with fork-PR permission errors (denied: installation not allowed to Write organization package, id-token: write missing), and the E2E failure is in TestHASequencerRollingRestartE2E which my PR doesn't touch. |
2 participants
Overview
Fixes a flaky
TestAddFlagscaused by non-deterministic defaults.DA.NamespaceusedrandStringseeded withtime.Now().Unix(), andDefaultConfig()was called twice in the test (once inAddFlags, once for the expected value). If the timestamp second changed between calls, the seeds diverged and the assertion failed.This PR extracts the time source into a package-level
nowUnixvariable indefaults.go, allowing the test to fix the seed and make both calls deterministic.Reproduced locally via
go test ./... -count=1:Summary by CodeRabbit