test: make the test suite faster and sleep-free#3399
Merged
Conversation
newTestSession now injects a millisecond-scale backoff so the 429 rate-limit test no longer sleeps ~4.4s through go-containerregistry's default 1s+3s retry delays. Assisted-By: Claude (Anthropic)
Channels, require.Eventually, gotest.tools poll.WaitOn, fake clocks, and backdated TTL entries replace every racy sleep in the test suite. A new forbidigo rule in .golangci.yml bans time.Sleep in tests going forward; the three remaining calls in source_loader_test.go are fake-time inside synctest bubbles and carry nolint directives. One small production change: askpass.go gains a promptWaiters counter so the serialization test can sync without sleeping. Assisted-By: Claude
docker-agent
reviewed
Jul 1, 2026
docker-agent
left a comment
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This PR cleanly replaces 44 time.Sleep calls across 22 test files with deterministic synchronization primitives. No bugs were found in the changed code.
Key areas reviewed:
askpass.goproduction change —promptArrivals atomic.Int32is correctly placed and used with atomic operations. No race conditions introduced.- Channel-based synchronization — All channels in elicitation, server, and supervisor tests are appropriately buffered or closed; no goroutine leaks or deadlocks detected.
require.Eventually/poll.WaitOn— Timeout values (1–2 s with ms polling) are reasonable for in-process test conditions.- Rate-limit retry backoff —
Backoff{Duration: time.Millisecond, Factor: 1, Steps: 3}correctly removes multi-second backoff while preserving retry-path coverage. forbidigolint rule — Correctly scoped to*_test.gofiles; preventstime.Sleepfrom silently re-entering the test suite.
The three intentional time.Sleep calls inside synctest bubbles with //nolint directives are appropriate and well-documented.
aheritier
added
area/ci
melmennaoui
approved these changes
Jul 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The test suite contained 44
time.Sleepcalls spread across 22 test files. These sleeps slowed every run, introduced timing-dependent flakiness on loaded CI machines, and gave no actual guarantee of ordering — a fast machine might still race past them. This change removes all of them, replacing each with a deterministic synchronization primitive appropriate to the situation.Depending on the test, the replacement is a channel gate,
require.Eventually,gotest.tools/poll.WaitOn,time.AfterFunc, a fake clock, or a backdated TTL entry. For the prompt-serialization test inpkg/tools/builtin/shell, a small production change was needed:askpass.gogained an atomic counter (promptArrivals) so the test can wait for a prompt to arrive rather than sleeping. The 429 rate-limit test inpkg/remotealso dropped go-containerregistry's default retry backoff (which slept ~4.4 s total) in favour of a millisecond-scale backoff with the same retry count. The three remainingtime.Sleepcalls are fake-time advances insidetesting/synctestbubbles and carry//nolintdirectives explaining why they are intentional. Aforbidigolint rule now banstime.Sleepin*_test.gofiles so the pattern cannot silently return.Wall-clock speedups are most visible in the packages that leaned on sleep the most:
pkg/runtimedrops from ~9 s to ~3 s andpkg/serverfrom ~5 s to ~2 s in isolation. The full suite was run with-raceon all touched packages;golangci-lintand the new custom cop are clean.