Skip to content

Commit 31311ed

Browse files
lux-liangDumbrisclaude
authored
test: make timing-sensitive tests deterministic (#1330)
* Make timing-sensitive tests deterministic * fix: restore real-exec/perf-regression coverage lost by hermetic test 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> * fix: make the fake docker in TestRunDockerInfo_ExecutesResolvedBinary 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> --------- Co-authored-by: lux-liang <249971141+lux-liang@users.noreply.github.com> Co-authored-by: Algis Dumbris <a.dumbris@gmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 1e29fe0 commit 31311ed

3 files changed

Lines changed: 156 additions & 58 deletions

File tree

‎internal/security/mask_test.go‎

Lines changed: 62 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -247,30 +247,78 @@ func TestMaskTextMasksBeyondTheDetectionCap(t *testing.T) {
247247
}
248248
}
249249

250-
// Removing the replacement cap makes "a payload stuffed with secrets" the
251-
// worst case, so it must not be quadratic: a full-size activity response
252-
// (64KB, the activity_max_response_size cap) of nothing but distinct tokens
253-
// still has to mask in well under a second.
254-
func TestMaskTextStaysCheapOnAPayloadFullOfSecrets(t *testing.T) {
255-
d := NewDetector(nil)
256-
250+
// secretsPayload builds text of at least targetLen bytes made entirely of
251+
// distinct GitHub-PAT-shaped tokens ("key<i>=ghp_<i zero-padded to 36 digits>"),
252+
// the worst case for masking: every token is a separate match, none share a
253+
// value, so no dedup/caching in the detector can shortcut the work.
254+
func secretsPayload(targetLen int) string {
257255
var b strings.Builder
258-
for i := 0; b.Len() < 64*1024; i++ {
256+
for i := 0; b.Len() < targetLen; i++ {
259257
fmt.Fprintf(&b, "key%d=ghp_%036d\n", i, i)
260258
}
261-
text := b.String()
259+
return b.String()
260+
}
261+
262+
// A full-size activity response made entirely of distinct tokens exercises the
263+
// worst-case masking shape without making correctness depend on host timing.
264+
func TestMaskTextHandlesAPayloadFullOfSecrets(t *testing.T) {
265+
d := NewDetector(nil)
266+
267+
text := secretsPayload(64 * 1024)
262268

263-
start := time.Now()
264269
masked, _ := d.MaskText(text)
265-
elapsed := time.Since(start)
266270

267271
if strings.Contains(masked, "ghp_0000") {
268272
t.Fatal("tokens survived masking")
269273
}
270-
if elapsed > 2*time.Second {
271-
t.Fatalf("masking a %d-byte payload took %s", len(text), elapsed)
274+
}
275+
276+
// TestMaskTextScalesLinearlyOnAPayloadFullOfSecrets guards against
277+
// reintroducing the O(n^2) behavior TestMaskTextStaysCheapOnAPayloadFullOfSecrets
278+
// used to catch (removed because an absolute wall-clock ceiling was flaky under
279+
// CI load — see BenchmarkMaskTextPayloadFullOfSecrets below for the profiling
280+
// counterpart). Instead of a fixed threshold, it compares masking a payload
281+
// against one 4x the size: linear work predicts ~4x the time, so a generous
282+
// growth ceiling still catches real quadratic blowups (~16x) while absorbing
283+
// routine scheduler/GC noise from the comparison running in the same process.
284+
func TestMaskTextScalesLinearlyOnAPayloadFullOfSecrets(t *testing.T) {
285+
if testing.Short() {
286+
t.Skip("timing-sensitive; skipped under -short")
287+
}
288+
289+
measure := func(targetLen int) time.Duration {
290+
d := NewDetector(nil)
291+
text := secretsPayload(targetLen)
292+
start := time.Now()
293+
d.MaskText(text)
294+
return time.Since(start)
295+
}
296+
297+
// Warm up (first run pays for allocator/CPU-cache warmup, not algorithmic cost).
298+
measure(16 * 1024)
299+
300+
small := measure(16 * 1024)
301+
large := measure(64 * 1024) // 4x the payload
302+
303+
// The absolute floor keeps this from firing on noise when both runs are
304+
// too fast for the ratio to mean anything.
305+
if large > 10*small && large > 50*time.Millisecond {
306+
t.Fatalf("masking a payload 4x the size took %s vs %s for the baseline — looks like a complexity regression, not scheduler noise", large, small)
307+
}
308+
}
309+
310+
func BenchmarkMaskTextPayloadFullOfSecrets(b *testing.B) {
311+
d := NewDetector(nil)
312+
text := secretsPayload(64 * 1024)
313+
314+
b.SetBytes(int64(len(text)))
315+
b.ReportAllocs()
316+
b.ResetTimer()
317+
for range b.N {
318+
if _, changed := d.MaskText(text); !changed {
319+
b.Fatal("payload was not masked")
320+
}
272321
}
273-
t.Logf("masked %d bytes in %s", len(text), elapsed)
274322
}
275323

276324
// Turning detection off later must not retroactively serve the credentials in

‎internal/upstream/manager.go‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,16 @@ var dockerResolverFn = func(logger *zap.Logger) (string, error) {
4646
return shellwrap.ResolveDockerPath(logger)
4747
}
4848

49+
func runDockerInfo(ctx context.Context, dockerBin string) error {
50+
return exec.CommandContext(ctx, dockerBin, "info", "--format", "{{json .ServerVersion}}").Run()
51+
}
52+
53+
// dockerInfoRunnerFn runs the resolved docker binary's `info` command.
54+
// Overridable in tests, mirroring dockerResolverFn above so the two sibling
55+
// fakes checkDockerAvailability needs (which binary, and running it) share one
56+
// injection idiom instead of two.
57+
var dockerInfoRunnerFn = runDockerInfo
58+
4959
// Docker recovery constants - internal implementation defaults
5060
const (
5161
dockerCheckInterval = 30 * time.Second // How often to check Docker availability
@@ -3035,8 +3045,7 @@ func (m *Manager) checkDockerAvailability(ctx context.Context) error {
30353045
dockerBin = "docker"
30363046
}
30373047

3038-
cmd := exec.CommandContext(checkCtx, dockerBin, "info", "--format", "{{json .ServerVersion}}")
3039-
if err := cmd.Run(); err != nil {
3048+
if err := dockerInfoRunnerFn(checkCtx, dockerBin); err != nil {
30403049
return fmt.Errorf("docker unavailable: %w", err)
30413050
}
30423051
return nil

‎internal/upstream/manager_docker_recovery_test.go‎

Lines changed: 83 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -15,90 +15,131 @@ import (
1515
"github.com/smart-mcp-proxy/mcpproxy-go/internal/storage"
1616
)
1717

18+
// TestRunDockerInfo_ExecutesResolvedBinary exercises runDockerInfo's real
19+
// exec.CommandContext(...).Run() invocation shape end-to-end against a real
20+
// subprocess, so a regression in the argument/flag shape (wrong dockerBin
21+
// position, broken --format string) is caught even though the tests below
22+
// inject dockerInfoRunnerFn and never call this function.
23+
func TestRunDockerInfo_ExecutesResolvedBinary(t *testing.T) {
24+
if runtime.GOOS == "windows" {
25+
t.Skip("/bin/sh-style fake docker is POSIX only")
26+
}
27+
28+
dockerPath := filepath.Join(t.TempDir(), "docker")
29+
// Requires the exact argv runDockerInfo passes (not just $1 == info), so a
30+
// regression in argument count, the --format flag, or the Go template
31+
// string fails this test instead of slipping through unnoticed.
32+
script := `#!/bin/sh
33+
if [ "$#" -eq 3 ] && [ "$1" = "info" ] && [ "$2" = "--format" ] && [ "$3" = '{{json .ServerVersion}}' ]; then
34+
printf '"24.0.0"\n'
35+
exit 0
36+
fi
37+
exit 99
38+
`
39+
if err := os.WriteFile(dockerPath, []byte(script), 0o755); err != nil {
40+
t.Fatalf("write fake docker: %v", err)
41+
}
42+
43+
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
44+
defer cancel()
45+
46+
if err := runDockerInfo(ctx, dockerPath); err != nil {
47+
t.Fatalf("expected fake docker info to succeed, got: %v", err)
48+
}
49+
}
50+
51+
// TestRunDockerInfo_PropagatesNonZeroExit ensures a real non-zero process exit
52+
// (not just a synthetic Go error) surfaces as an error from runDockerInfo.
53+
func TestRunDockerInfo_PropagatesNonZeroExit(t *testing.T) {
54+
if runtime.GOOS == "windows" {
55+
t.Skip("/bin/sh-style fake docker is POSIX only")
56+
}
57+
58+
dockerPath := filepath.Join(t.TempDir(), "docker")
59+
if err := os.WriteFile(dockerPath, []byte("#!/bin/sh\nexit 7\n"), 0o755); err != nil {
60+
t.Fatalf("write fake docker: %v", err)
61+
}
62+
63+
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
64+
defer cancel()
65+
66+
if err := runDockerInfo(ctx, dockerPath); err == nil {
67+
t.Fatal("expected non-zero exit to surface as an error")
68+
}
69+
}
70+
1871
// TestCheckDockerAvailability_UsesShellwrapResolver verifies that a launchd-style
1972
// minimal PATH (the situation when mcpproxy is launched from /Applications/...app
2073
// or a LoginItem) does not break docker lookup, because checkDockerAvailability
2174
// now goes through dockerResolverFn (shellwrap-backed) instead of relying on
2275
// $PATH for the bare "docker" binary.
2376
//
24-
// We exercise the real resolver indirectly: the test installs a fake docker
25-
// shim into a temp dir, points dockerResolverFn at it, drops $PATH to the
26-
// launchd minimum, and asserts that checkDockerAvailability still succeeds.
27-
// Without the fix, exec.Command("docker") would error with `executable file
28-
// not found in $PATH` regardless of how the resolver was wired.
77+
// dockerInfoRunnerFn is faked so the test asserts the resolved executable
78+
// directly without depending on process startup or a wall-clock deadline; the
79+
// real exec.CommandContext(...).Run() shape is covered separately by
80+
// TestRunDockerInfo_ExecutesResolvedBinary above.
2981
func TestCheckDockerAvailability_UsesShellwrapResolver(t *testing.T) {
3082
if runtime.GOOS == "windows" {
3183
t.Skip("launchd PATH scenario is macOS/Linux only")
3284
}
3385

34-
tmpDir := t.TempDir()
35-
dockerPath := filepath.Join(tmpDir, "docker")
36-
37-
// Fake docker that prints a server version when invoked with `info`.
38-
// Anything else exits non-zero so we know the call shape was correct.
39-
script := `#!/bin/sh
40-
case "$1" in
41-
info) printf '"24.0.0"\n'; exit 0 ;;
42-
*) exit 99 ;;
43-
esac
44-
`
45-
if err := os.WriteFile(dockerPath, []byte(script), 0o755); err != nil {
46-
t.Fatalf("write fake docker: %v", err)
47-
}
48-
49-
// Simulate launchd's minimal PATH — fake docker is NOT on it. If the
50-
// production code resolved via os.Getenv("PATH") we would fail here.
51-
t.Setenv("PATH", "/usr/bin:/bin:/usr/sbin:/sbin")
86+
dockerPath := filepath.Join(t.TempDir(), "docker")
5287

5388
original := dockerResolverFn
5489
t.Cleanup(func() { dockerResolverFn = original })
5590
dockerResolverFn = func(*zap.Logger) (string, error) {
5691
return dockerPath, nil
5792
}
5893

94+
var resolvedBin string
95+
originalRunner := dockerInfoRunnerFn
96+
t.Cleanup(func() { dockerInfoRunnerFn = originalRunner })
97+
dockerInfoRunnerFn = func(_ context.Context, dockerBin string) error {
98+
resolvedBin = dockerBin
99+
return nil
100+
}
101+
59102
m := &Manager{logger: zap.NewNop()}
60-
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
61-
defer cancel()
62103

63-
if err := m.checkDockerAvailability(ctx); err != nil {
104+
if err := m.checkDockerAvailability(context.Background()); err != nil {
64105
t.Fatalf("expected docker to be reachable via resolver, got: %v", err)
65106
}
107+
if resolvedBin != dockerPath {
108+
t.Fatalf("docker executable = %q, want resolved path %q", resolvedBin, dockerPath)
109+
}
66110
}
67111

68112
// TestCheckDockerAvailability_FallbackOnResolverFailure ensures that even when
69-
// the resolver itself errors out we still attempt a bare-name exec — preserving
113+
// the resolver itself errors out we still select the bare executable name — preserving
70114
// the original behaviour for hosts where the shellwrap probes legitimately
71115
// cannot find docker but it IS on the parent's PATH.
72116
func TestCheckDockerAvailability_FallbackOnResolverFailure(t *testing.T) {
73117
if runtime.GOOS == "windows" {
74118
t.Skip("/bin/sh-style fake docker is POSIX only")
75119
}
76120

77-
tmpDir := t.TempDir()
78-
dockerPath := filepath.Join(tmpDir, "docker")
79-
script := `#!/bin/sh
80-
exit 0
81-
`
82-
if err := os.WriteFile(dockerPath, []byte(script), 0o755); err != nil {
83-
t.Fatalf("write fake docker: %v", err)
84-
}
85-
86-
// Resolver fails — the fallback should still exec via bare PATH.
87-
t.Setenv("PATH", tmpDir)
88-
89121
original := dockerResolverFn
90122
t.Cleanup(func() { dockerResolverFn = original })
91123
dockerResolverFn = func(*zap.Logger) (string, error) {
92124
return "", errors.New("simulated resolver failure")
93125
}
94126

127+
var resolvedBin string
128+
originalRunner := dockerInfoRunnerFn
129+
t.Cleanup(func() { dockerInfoRunnerFn = originalRunner })
130+
dockerInfoRunnerFn = func(_ context.Context, dockerBin string) error {
131+
resolvedBin = dockerBin
132+
return nil
133+
}
134+
95135
m := &Manager{logger: zap.NewNop()}
96-
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
97-
defer cancel()
98136

99-
if err := m.checkDockerAvailability(ctx); err != nil {
137+
if err := m.checkDockerAvailability(context.Background()); err != nil {
100138
t.Fatalf("expected fallback bare-name exec to succeed, got: %v", err)
101139
}
140+
if resolvedBin != "docker" {
141+
t.Fatalf("docker executable = %q, want bare-name fallback", resolvedBin)
142+
}
102143
}
103144

104145
// TestFreshenLoadedDockerRecoveryState verifies that a state loaded from the

0 commit comments

Comments
 (0)