add hermetic tests for EscapeTag, statsd, LoadConfig, HTTPS, UrlString#52
Merged
Merged
Conversation
Adds table-driven tests for pure functions that previously had no coverage, all hermetic per AGENTS.md (no real network beyond loopback httptest, no filesystem outside t.TempDir). - statsd_test.go: EscapeTag for :, |, ,, @ plus a refs-#14 case pinning the missing newline/CR handling. Count/Timer/Gauge wire-format assertions with queue draining to keep package state clean between cases. - config_test.go: LoadConfig with t.TempDir fixtures (empty path, minimal YAML, multi-URL), a refs-#6 case encoding the map[string]string loader bug, and a subprocess-based check pinning the log.Fatalf-on-missing-file boundary. - https_test.go: HTTPS against httptest.NewServer covering 2xx success, refs-#7 (5xx still reports success), refs-#15 (silently follows redirects), and a dial-failure case that's already correct. - destinations_test.go: TestUrlString table-driven cases pinning password redaction as "[...]" (refs #12), userinfo variants, custom paths, the icmp port=-1 omission, and the scheme!=protocol parens annotation. Tests for known bugs (#6, #7, #12, #14, #15) document current behavior so future fixes have a failing target to flip; each is marked "Refs #N -- flip when fixed". Fixes #47 https://claude.ai/code/session_01WjHPSobuzrRkjwUgjAJWMk
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
2 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
Adds table-driven, hermetic tests for the bug-prone pure functions identified in #47. Test-only change; no production code is modified.
statsd_test.go—EscapeTag(the four currently-escaped specials plus a refs-StatsdEscapeTagis incomplete: newline/CR enables wire-protocol injection #14 case pinning the missing newline/CR handling), and wire-format assertions forCount/Increment/Timer/Gaugethat drain the package-levelqueuechannel.config_test.go—LoadConfigwitht.TempDirfixtures (empty path, minimal YAML, multi-URL YAML), a refs-Statsd YAML config is silently ignored and corrupts URL parsing #6 case encoding the currentmap[string]stringloader bug (typed keys become URLs and defaults silently kick in), and a subprocess re-exec that pins thelog.Fatalf-on-missing-file boundary. Also coversFindConfigreturning an error when no config exists.https_test.go—HTTPSagainsthttptest.NewServerfor 2xx success, refs-HTTP/HTTPS check accepts non-2xx responses, leaks response body, and has no timeout #7 (5xx still reports success), refs-HTTPS check follows arbitrary redirects — silent SSRF-style misvalidation #15 (silently follows redirects to a second test server), and a transport-failure case that's already correct.destinations_test.go—TestUrlStringtable covering password redaction as[...](refs URL passwords leak into logs viahttp.Geterror messages #12), userinfo variants, custom paths, theicmpPort == -1omission, and thescheme != protocolparens annotation. Plus a belt-and-suspendersTestUrlString_RedactedDoesNotContainPassword.Each bug-documenting case is commented
// Refs #N -- flip when fixedso a future fix has a green target to flip rather than a silent passing test.Test plan
go vet ./...cleangofmt -l .prints nothinggo build ./...succeedsgo test -race ./...— all 19 new tests pass; only the pre-existingTestRouteToLoopback{1,2,3}failures from sandbox routing (Tests aren't hermetic and major modules are untested; production code is hard to test #24) remain, which also fail onorigin/mainhttptest, no filesystem outsidet.TempDir(), no routing-table readsTestLoadConfig_MissingFileFatalsuses an env-gated helper branch to safely cover thelog.Fatalfpath without killing the parent test processFixes #47
https://claude.ai/code/session_01WjHPSobuzrRkjwUgjAJWMk
Generated by Claude Code