Skip to content

Bug-prone pure functions have zero test coverage: EscapeTag, statsd wire format, UrlString redaction, LoadConfig, HTTPS #47

@dolph

Description

@dolph

A handful of pure or nearly-pure functions have no tests at all, even though several of them are the exact site of currently-open bugs. These are the highest-leverage tests to add because they require no network, no routing table, and no goroutines — they are immediately hermetic.

The gap

Function File Why it needs tests
EscapeTag statsd.go #14 — escapes :, |, ,, @ but not \n / \r, enabling wire-protocol injection. A test pinning the current set + a failing test for newline/CR is one PR.
Increment / Count / Timer / Gauge statsd.go Wire-format strings (metric:value|c|#tag1,tag2). No assertion that the format matches dogstatsd. A regression here is silent — metrics just stop being parsed by the collector. The queue channel is package-global so tests can read directly from it.
UrlString destinations.go Password redaction (user:[...]@host). #12 depends on this being right. Easy table-driven test: with/without user, with/without password, with port -1 (icmp), with scheme != protocol.
LoadConfig config.go #6 — loader unmarshals into map[string]string, silently dropping typed config keys. A test using t.TempDir() + a fixture YAML would have caught this on day one. Also covers default-value logic (StatsdHost, StatsdPort, StatsdProtocol fallbacks).
HTTPS https.go #7, #15 — follows up to 10 redirects, accepts non-2xx, leaks body, no timeout. A httptest.Server makes all of this trivially testable without real network.
ParseDestinations connectivity.go #5 — drops index 0 unconditionally. A 4-line test would have caught it. Currently the function os.Exit(2)s on error which makes it un-testable as-is; this is its own design defect worth surfacing.
Destination.tags() destinations.go Tags carry user-controlled strings (Label, Host) through EscapeTag; verify the per-destination tag set is well-formed.

Every item above has a known or latent bug. Adding these tests would:

  1. Lock in correct behavior before fixing the bugs (TDD per AGENTS.md).
  2. Remove ~80% of the "untested modules" surface that Tests aren't hermetic and major modules are untested; production code is hard to test #24 enumerates, hermetically.
  3. Cost nothing in CI time — these are pure-function tests.

Suggested approach

  • One PR per function (single-purpose, per AGENTS.md scope discipline).
  • For each, write the test first, watch it fail on the current (broken) behavior, then the fix lands in a separate PR with Fixes #N.
  • Use table-driven cases under t.Run.
  • For LoadConfig, use t.TempDir() + os.WriteFile — no need to mock the filesystem.
  • For HTTPS, use httptest.NewServer / httptest.NewTLSServer — covers status codes, redirect chains, timeouts via a time.Sleep-handler.
  • For statsd format functions, drain queue in the test (it's already a buffered channel) and assert on the string.

Refs #24 (umbrella). This proposes the specific, ordered list to discharge most of "major modules are untested," prioritized by current-bug-overlap.

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions