fix(config): de-flake TestAddFlags by stubbing time source#3311
fix(config): de-flake TestAddFlags by stubbing time source#3311CaelRowley wants to merge 2 commits intoevstack:mainfrom
Conversation
📝 WalkthroughWalkthroughThe PR introduces a package-level ChangesTime Abstraction for Deterministic Testing
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 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)
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.69% 60.71% +0.02%
==========================================
Files 127 127
Lines 13735 13736 +1
==========================================
+ Hits 8336 8340 +4
+ Misses 4494 4492 -2
+ Partials 905 904 -1
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.
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
Release Notes