From fefe24ba7f46785f1f11c6370be6129b85ca6ac6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 16 May 2026 13:49:23 +0000 Subject: [PATCH] Fix test helpers to print the field under test, not the container The shared assertion helpers in destinations_test.go printed the whole *Destination via %s instead of the int/string field they claimed to be checking, so when an assertion fired the failure message hid the value that mismatched. assertError accepted any error, which is the checklist theater AGENTS.md warns against. This change rewrites each helper to take the field directly (matching AGENTS.md's "test helpers take only the field under test" guidance), adds t.Helper() so failure lines point at the caller, and adds assertErrorContains for tests that should pin which error came back. Meta-tests using a capturing fake testing.T lock in the new failure message format. Call sites in resolver_test.go are updated to pass a context name instead of *Destination. Fixes #46 https://claude.ai/code/session_01WjHPSobuzrRkjwUgjAJWMk --- destinations_test.go | 253 ++++++++++++++++++++++++++++++++++--------- resolver_test.go | 16 +-- 2 files changed, 212 insertions(+), 57 deletions(-) diff --git a/destinations_test.go b/destinations_test.go index 6e4d7f7..db9f4aa 100644 --- a/destinations_test.go +++ b/destinations_test.go @@ -1,109 +1,264 @@ package main import ( + "fmt" + "strings" "testing" ) -func assertSchemeEquals(t *testing.T, got *Destination, want string) { - if got.Scheme != want { - t.Errorf("dest.Scheme = %s; want %s", got, want) +// testingT is the subset of *testing.T used by the assertion helpers in this +// file. Defining it lets meta-tests substitute a capturing fake so we can +// assert on the failure messages the helpers emit (see TestAssert*). +type testingT interface { + Errorf(format string, args ...interface{}) + Helper() +} + +// fakeT captures a single failure message so meta-tests can assert that +// helpers print a useful diagnostic when they fire. Only the subset of +// *testing.T surface used by the assertion helpers is implemented. +type fakeT struct { + failed bool + msg string +} + +func (f *fakeT) Errorf(format string, args ...interface{}) { + f.failed = true + f.msg = fmt.Sprintf(format, args...) +} + +func (f *fakeT) Helper() {} + +// assertPortEquals takes the int field under test directly so the failure +// message can format it with %d and refer to "Port" by name. +func assertPortEquals(t testingT, got int, want int) { + t.Helper() + if got != want { + t.Errorf("Port = %d; want %d", got, want) } } -func assertHostEquals(t *testing.T, got *Destination, want string) { - if got.Host != want { - t.Errorf("dest.Host = %s; want %s", got, want) +// assertSchemeEquals takes the string field under test directly so the failure +// message can print the actual scheme that mismatched. +func assertSchemeEquals(t testingT, got string, want string) { + t.Helper() + if got != want { + t.Errorf("Scheme = %q; want %q", got, want) } } -func assertPortEquals(t *testing.T, got *Destination, want int) { - if got.Port != want { - t.Errorf("dest.Port = %s; want %d", got, want) +// assertHostEquals takes the string field under test directly so the failure +// message can print the actual host that mismatched. +func assertHostEquals(t testingT, got string, want string) { + t.Helper() + if got != want { + t.Errorf("Host = %q; want %q", got, want) } } -func assertNoError(t *testing.T, got *Destination, err error) { +// assertNoError fails the test if err is non-nil. The name parameter labels +// the call site so failures in table-driven tests point at the right row. +func assertNoError(t testingT, name string, err error) { + t.Helper() if err != nil { - t.Errorf("got = %s with %v; want no err", got, err) + t.Errorf("%s: unexpected error: %v", name, err) + } +} + +// assertError fails the test if err is nil. Prefer assertErrorContains when +// the caller knows which error to expect; this helper exists for the cases +// where the test only cares that an error was returned at all. +func assertError(t testingT, name string, err error) { + t.Helper() + if err == nil { + t.Errorf("%s: expected an error; got nil", name) } } -func assertError(t *testing.T, got *Destination, err error) { +// assertErrorContains fails the test if err is nil or if err.Error() does not +// contain substr. Use this instead of assertError to lock in which error a +// failing path is expected to return (avoiding "checklist theater" — see #46). +func assertErrorContains(t testingT, err error, substr string) { + t.Helper() if err == nil { - t.Errorf("got = %s; want err", got) + t.Errorf("expected an error containing %q; got nil", substr) + return + } + if !strings.Contains(err.Error(), substr) { + t.Errorf("error = %q; want substring %q", err.Error(), substr) + } +} + +// errString is a tiny error type used to build errors with known messages for +// the helper meta-tests, without depending on production code paths. +type errString string + +func (e errString) Error() string { return string(e) } + +func TestAssertPortEquals_FailureMessageNamesPort(t *testing.T) { + ft := &fakeT{} + assertPortEquals(ft, 80, 443) + if !ft.failed { + t.Fatalf("assertPortEquals(80, 443) did not fail; want failure") + } + if !strings.Contains(ft.msg, "Port") { + t.Errorf("message = %q; want it to mention %q", ft.msg, "Port") + } + if !strings.Contains(ft.msg, "80") || !strings.Contains(ft.msg, "443") { + t.Errorf("message = %q; want both got (80) and want (443) to appear", ft.msg) + } +} + +func TestAssertPortEquals_NoFailureWhenEqual(t *testing.T) { + ft := &fakeT{} + assertPortEquals(ft, 443, 443) + if ft.failed { + t.Errorf("assertPortEquals(443, 443) reported failure %q; want no failure", ft.msg) + } +} + +func TestAssertSchemeEquals_FailureMessageNamesScheme(t *testing.T) { + ft := &fakeT{} + assertSchemeEquals(ft, "http", "https") + if !ft.failed { + t.Fatalf("assertSchemeEquals(http, https) did not fail; want failure") + } + if !strings.Contains(ft.msg, "Scheme") { + t.Errorf("message = %q; want it to mention %q", ft.msg, "Scheme") + } + if !strings.Contains(ft.msg, "http") || !strings.Contains(ft.msg, "https") { + t.Errorf("message = %q; want both got (http) and want (https) to appear", ft.msg) + } +} + +func TestAssertHostEquals_FailureMessageNamesHost(t *testing.T) { + ft := &fakeT{} + assertHostEquals(ft, "example.com", "example.org") + if !ft.failed { + t.Fatalf("assertHostEquals failed-case did not fail; want failure") + } + if !strings.Contains(ft.msg, "Host") { + t.Errorf("message = %q; want it to mention %q", ft.msg, "Host") + } + if !strings.Contains(ft.msg, "example.com") || !strings.Contains(ft.msg, "example.org") { + t.Errorf("message = %q; want both got and want hosts to appear", ft.msg) + } +} + +func TestAssertErrorContains_PassesWhenSubstringMatches(t *testing.T) { + ft := &fakeT{} + assertErrorContains(ft, errString("port required"), "port required") + if ft.failed { + t.Errorf("assertErrorContains with matching substring failed; got message %q", ft.msg) + } +} + +func TestAssertErrorContains_FailsWhenSubstringMissing(t *testing.T) { + ft := &fakeT{} + assertErrorContains(ft, errString("out of memory"), "port required") + if !ft.failed { + t.Fatalf("assertErrorContains with non-matching substring did not fail; want failure") + } + if !strings.Contains(ft.msg, "port required") { + t.Errorf("message = %q; want it to mention the expected substring %q", ft.msg, "port required") + } + if !strings.Contains(ft.msg, "out of memory") { + t.Errorf("message = %q; want it to include the actual error %q", ft.msg, "out of memory") + } +} + +func TestAssertErrorContains_FailsOnNilError(t *testing.T) { + ft := &fakeT{} + assertErrorContains(ft, nil, "port required") + if !ft.failed { + t.Errorf("assertErrorContains with nil error did not fail; want failure") + } +} + +func TestAssertNoError_FailureMessageIncludesName(t *testing.T) { + ft := &fakeT{} + assertNoError(ft, "tcp://host:123", errString("boom")) + if !ft.failed { + t.Fatalf("assertNoError with non-nil error did not fail; want failure") + } + if !strings.Contains(ft.msg, "tcp://host:123") { + t.Errorf("message = %q; want it to mention the call-site name", ft.msg) + } + if !strings.Contains(ft.msg, "boom") { + t.Errorf("message = %q; want it to include the underlying error", ft.msg) } } func TestMinimalHttpUrl(t *testing.T) { got, err := NewDestination(Url{Label: "host", Url: "http://host"}) - assertNoError(t, got, err) - assertSchemeEquals(t, got, "http") - assertHostEquals(t, got, "host") - assertPortEquals(t, got, 80) + assertNoError(t, "http://host", err) + assertSchemeEquals(t, got.Scheme, "http") + assertHostEquals(t, got.Host, "host") + assertPortEquals(t, got.Port, 80) } func TestMinimalHttpsUrl(t *testing.T) { got, err := NewDestination(Url{Label: "host", Url: "https://host"}) - assertNoError(t, got, err) - assertSchemeEquals(t, got, "https") - assertHostEquals(t, got, "host") - assertPortEquals(t, got, 443) + assertNoError(t, "https://host", err) + assertSchemeEquals(t, got.Scheme, "https") + assertHostEquals(t, got.Host, "host") + assertPortEquals(t, got.Port, 443) } func TestMinimalMysqlUrl(t *testing.T) { got, err := NewDestination(Url{Label: "mysql_host", Url: "mysql://host"}) - assertNoError(t, got, err) - assertSchemeEquals(t, got, "mysql") - assertHostEquals(t, got, "host") - assertPortEquals(t, got, 3306) + assertNoError(t, "mysql://host", err) + assertSchemeEquals(t, got.Scheme, "mysql") + assertHostEquals(t, got.Host, "host") + assertPortEquals(t, got.Port, 3306) } func TestMinimalPostgresUrl(t *testing.T) { got, err := NewDestination(Url{Label: "postgres_host", Url: "postgres://host"}) - assertNoError(t, got, err) - assertSchemeEquals(t, got, "postgres") - assertHostEquals(t, got, "host") - assertPortEquals(t, got, 5432) + assertNoError(t, "postgres://host", err) + assertSchemeEquals(t, got.Scheme, "postgres") + assertHostEquals(t, got.Host, "host") + assertPortEquals(t, got.Port, 5432) } func TestMinimalNatsUrl(t *testing.T) { got, err := NewDestination(Url{Label: "nats_host", Url: "nats://host"}) - assertNoError(t, got, err) - assertSchemeEquals(t, got, "nats") - assertHostEquals(t, got, "host") - assertPortEquals(t, got, 4222) + assertNoError(t, "nats://host", err) + assertSchemeEquals(t, got.Scheme, "nats") + assertHostEquals(t, got.Host, "host") + assertPortEquals(t, got.Port, 4222) } func TestSchemeNormalization(t *testing.T) { got, err := NewDestination(Url{Label: "schemy_host", Url: "HTtP://host"}) - assertNoError(t, got, err) - assertSchemeEquals(t, got, "http") - assertHostEquals(t, got, "host") - assertPortEquals(t, got, 80) + assertNoError(t, "HTtP://host", err) + assertSchemeEquals(t, got.Scheme, "http") + assertHostEquals(t, got.Host, "host") + assertPortEquals(t, got.Port, 80) } func TestTcpUrlWithoutPort(t *testing.T) { - got, err := NewDestination(Url{Label: "tcp_host", Url: "tcp://host"}) - assertError(t, got, err) + _, err := NewDestination(Url{Label: "tcp_host", Url: "tcp://host"}) + assertErrorContains(t, err, "Unsupported scheme") } func TestTcpUrlWithPort(t *testing.T) { got, err := NewDestination(Url{Label: "tcp_host", Url: "tcp://host:123"}) - assertNoError(t, got, err) - assertSchemeEquals(t, got, "tcp") - assertHostEquals(t, got, "host") - assertPortEquals(t, got, 123) + assertNoError(t, "tcp://host:123", err) + assertSchemeEquals(t, got.Scheme, "tcp") + assertHostEquals(t, got.Host, "host") + assertPortEquals(t, got.Port, 123) } func TestUdpUrlWithoutPort(t *testing.T) { - got, err := NewDestination(Url{Label: "udp_host", Url: "udp://host"}) - assertError(t, got, err) + _, err := NewDestination(Url{Label: "udp_host", Url: "udp://host"}) + assertErrorContains(t, err, "Unsupported scheme") } func TestUdpUrlWithPort(t *testing.T) { got, err := NewDestination(Url{Label: "udp_host", Url: "udp://host:123"}) - assertNoError(t, got, err) - assertSchemeEquals(t, got, "udp") - assertHostEquals(t, got, "host") - assertPortEquals(t, got, 123) + assertNoError(t, "udp://host:123", err) + assertSchemeEquals(t, got.Scheme, "udp") + assertHostEquals(t, got.Host, "host") + assertPortEquals(t, got.Port, 123) } diff --git a/resolver_test.go b/resolver_test.go index e1d4fc0..61919b5 100644 --- a/resolver_test.go +++ b/resolver_test.go @@ -36,32 +36,32 @@ func assertLenOfResultsInRange(t *testing.T, got []net.IP, wantMin int, wantMax func TestLookupLoopbackIp(t *testing.T) { dest, err := NewDestination(Url{Label: "localhost", Url: "http://127.0.0.1"}) - assertNoError(t, dest, err) + assertNoError(t, "NewDestination(http://127.0.0.1)", err) got, err := Lookup(dest) - assertNoError(t, dest, err) + assertNoError(t, "Lookup(127.0.0.1)", err) assertLookupEquals(t, got, []net.IP{net.ParseIP("127.0.0.1")}) } func TestLookupPublicIp(t *testing.T) { dest, err := NewDestination(Url{Label: "google_dns", Url: "http://8.8.8.8"}) - assertNoError(t, dest, err) + assertNoError(t, "NewDestination(http://8.8.8.8)", err) got, err := Lookup(dest) - assertNoError(t, dest, err) + assertNoError(t, "Lookup(8.8.8.8)", err) assertLookupEquals(t, got, []net.IP{net.ParseIP("8.8.8.8")}) } func TestLookupExample(t *testing.T) { dest, err := NewDestination(Url{Label: "example", Url: "https://example.com"}) - assertNoError(t, dest, err) + assertNoError(t, "NewDestination(https://example.com)", err) got, err := Lookup(dest) - assertNoError(t, dest, err) + assertNoError(t, "Lookup(example.com)", err) assertLenOfResultsInRange(t, got, 1, 10) } func TestLookupInvalidHostname(t *testing.T) { dest, err := NewDestination(Url{Label: "invalid", Url: "https://a.b.c"}) - assertNoError(t, dest, err) + assertNoError(t, "NewDestination(https://a.b.c)", err) got, err := Lookup(dest) - assertError(t, dest, err) + assertError(t, "Lookup(a.b.c)", err) assertLenOfResultsInRange(t, got, 0, 0) }