test: make timing-sensitive tests deterministic - #1330
Merged
Dumbris merged 3 commits intoSep 21, 2026
Merged
Conversation
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
… rewrite
Code review of the deflaking changes found the hermeticity fixes dropped
guarantees rather than relocating them:
- runDockerInfo's real exec.CommandContext(...).Run() invocation shape had
zero test coverage (both docker-recovery tests injected a fake runner).
Add TestRunDockerInfo_ExecutesResolvedBinary and
TestRunDockerInfo_PropagatesNonZeroExit, which exercise the real subprocess
path directly and hermetically (no PATH tricks, no wall clock beyond a
generous timeout).
- The quadratic-time regression guard for MaskText was replaced by a
benchmark nothing in CI ever runs. Add
TestMaskTextScalesLinearlyOnAPayloadFullOfSecrets, which compares relative
growth across payload sizes instead of an absolute wall-clock ceiling, so
it stays sensitive to a real O(n^2) regression without the flakiness that
motivated removing the original timing assertion.
- Unify the two DI idioms checkDockerAvailability used for sibling fakes
(dockerResolverFn package var vs dockerInfoRunner struct field with a
nil-fallback) into one: dockerInfoRunnerFn, a package var mirroring
dockerResolverFn. Removes the dead nil-fallback branch entirely.
- Drop the leftover t.Setenv("PATH", ...) and stale comment in
TestCheckDockerAvailability_UsesShellwrapResolver — the injected runner
never execs, so PATH was no longer load-bearing for that test.
- Extract secretsPayload() so the correctness test, the new complexity
guard, and the benchmark build the same payload shape instead of three
copies that could silently drift apart.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… validate full argv Cross-review (codex/gpt-5.6-sol) round 1 finding: the fake docker script only checked $1 == "info", so it would still exit 0 if the --format flag or the Go template string were deleted or corrupted, silently defeating the test's stated purpose of catching that exact regression. Require all three arguments match exactly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Dumbris
changed the title
Member
|
Thanks for tackling the flaky tests here, @lux-liang — the deflaking approach (injecting the docker command runner, moving the perf check off wall-clock) is exactly right for the CI-stability problem. I pushed two small follow-up commits on top to close a couple of test-coverage gaps a review surfaced:
Everything's been cross-model reviewed clean and stress-tested under |
Dumbris
approved these changes
Sep 21, 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
3 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.
Summary
Closes #1163
Verification
go test ./internal/upstream -run '^TestCheckDockerAvailability_(UsesShellwrapResolver|FallbackOnResolverFailure)$' -count=100go test -race ./internal/upstream -run '^TestCheckDockerAvailability_(UsesShellwrapResolver|FallbackOnResolverFailure)$' -count=100go test ./internal/security -run '^TestMaskText(HandlesAPayloadFullOfSecrets|MasksBeyondTheDetectionCap)$' -count=20go test -race ./internal/security -run '^TestMaskTextHandlesAPayloadFullOfSecrets$' -count=10go test ./internal/upstream ./internal/securitygo test -race ./internal/upstream ./internal/securitygo vet ./internal/upstream ./internal/securitygolangci-lint run --new-from-rev=origin/main ./internal/upstream/... ./internal/security/...