Skip to content

OCPBUGS-86511: Fix flaky TestAsyncCache backend test #16495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jhadvig wants to merge 1 commit into
base: main
Choose a base branch
from
Open

OCPBUGS-86511: Fix flaky TestAsyncCache backend test #16495

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 17 additions & 26 deletions pkg/serverutils/asynccache/asyncccache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,49 +28,40 @@ func TestAsyncCache(t *testing.T) {
return &testItem{ctx: ctx, t: time.Now()}, nil
}

initializationRetryInterval = 5 * time.Millisecond
initializationTimeout = 10 * time.Millisecond

Comment on lines +31 to +33
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Restore overridden package-level timing vars with t.Cleanup.

This test mutates global knobs and leaves them changed, which can make other tests order-dependent/flaky. Save prior values and restore them in cleanup.

Suggested fix 
+	oldInitializationRetryInterval := initializationRetryInterval
+	oldInitializationTimeout := initializationTimeout
 	initializationRetryInterval = 5 * time.Millisecond
 	initializationTimeout = 10 * time.Millisecond
+	t.Cleanup(func() {
+		initializationRetryInterval = oldInitializationRetryInterval
+		initializationTimeout = oldInitializationTimeout
+	})
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
initializationRetryInterval = 5 * time.Millisecond
initializationTimeout = 10 * time.Millisecond
oldInitializationRetryInterval := initializationRetryInterval
oldInitializationTimeout := initializationTimeout
initializationRetryInterval = 5 * time.Millisecond
initializationTimeout = 10 * time.Millisecond
t.Cleanup(func() {
initializationRetryInterval = oldInitializationRetryInterval
initializationTimeout = oldInitializationTimeout
})
🤖 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/serverutils/asynccache/asyncccache_test.go` around lines 31 - 33, The
test mutates package-level timing variables initializationRetryInterval and
initializationTimeout and does not restore them; save their current values at
the start of the test and register a t.Cleanup that restores them (use the same
variable names) so other tests aren’t affected; update the test in
asyncccache_test.go to capture prevRetry := initializationRetryInterval and
prevTimeout := initializationTimeout and call t.Cleanup(func() {
initializationRetryInterval = prevRetry; initializationTimeout = prevTimeout })
immediately after changing them.

c, err := NewAsyncCache(context.Background(), 2*time.Second, cacheTime)
require.NoError(t, err)

initializationRetryInterval = 5 * time.Millisecond
initializationTimeout = 10 * time.Millisecond
// test that initialization was successful
item := c.GetItem()
if item.t.IsZero() {
t.Error("expected non-zero time")
}
require.False(t, item.t.IsZero(), "expected non-zero time")

time.Sleep(1 * time.Second)
if item.isContextCancelled() {
t.Error("expected usable context")
}
require.False(t, item.isContextCancelled(), "expected usable context")

timedCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
timedCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
c.Run(timedCtx)

// test the values get properly changed
var matches int
var changed bool
for i := 0; i < 3; i++ {
newItem := c.GetItem()
if newItem == item {
matches++
} else {
changed = true
}
// wait.UntilWithContext fires runCache immediately — let it settle
time.Sleep(100 * time.Millisecond)
item = c.GetItem()

time.Sleep(1 * time.Second)
}
// Cache should return the same item between reloads
require.Equal(t, item, c.GetItem(), "expected same item before next reload")

// Cache should eventually return a different item after a reload cycle
require.Eventually(t, func() bool {
return c.GetItem() != item
}, 5*time.Second, 200*time.Millisecond, "expected cache to refresh")

require.Greater(t, matches, 0)
require.True(t, changed)
cancel()

// test that the cache returns error properly
// Initialization returns error when caching func fails
errorCaching := func(ctx context.Context) (bool, error) {
return false, fmt.Errorf("test error")
}

_, err = NewAsyncCache(context.Background(), 2*time.Second, errorCaching)
require.Error(t, err)
}