diff --git a/TESTING.md b/TESTING.md index f52c54a..573d156 100644 --- a/TESTING.md +++ b/TESTING.md @@ -31,6 +31,13 @@ assertions and sensible timeouts, not from mocks. - Watcher change-detection tests seed a synthetic *previous state* and compare it against fresh live lookups; the DNS side is never faked +- Live query concurrency is bounded per package (`liveGate` in + `internal/resolver`, `liveWatcherGate` in `internal/watcher`) so + parallel tests do not burst at the root servers +- Those gates are package-scoped and therefore per test binary, so + `script/test` also passes `-p 1`: with both live-DNS packages + running at once the gates sum instead of holding, and the resolver's + per-attempt deadlines start expiring ### What NOT to do @@ -41,4 +48,8 @@ assertions and sensible timeouts, not from mocks. - **Do not remove `-count=1` from `script/test`** — Go's test cache replays a previous run's output without querying anything, so a cached pass is not evidence that live resolution works +- **Do not remove `-p 1` from `script/test`** — running the live-DNS + packages in parallel oversubscribes live DNS past what their gates + bound, which surfaces as unrelated resolver tests failing on + expired deadlines - **Do not modify linter configuration** to suppress findings diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go index b69ce6a..1e6d4b0 100644 --- a/internal/resolver/resolver_test.go +++ b/internal/resolver/resolver_test.go @@ -8,7 +8,6 @@ import ( "sort" "strings" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -518,41 +517,15 @@ func TestQueryAllNameservers_ContextCanceled(t *testing.T) { assert.Error(t, err) } -// ---------------------------------------------------------------- -// Unreachable nameserver tests -// ---------------------------------------------------------------- - -func TestQueryNameserverIP_UnreachableServer(t *testing.T) { - t.Parallel() - - r := newTestResolver(t) - - ctx, cancel := context.WithTimeout( - context.Background(), 10*time.Second, - ) - t.Cleanup(cancel) - - // 192.0.2.1 is an RFC 5737 documentation address: no - // nameserver can exist there. Depending on the network - // path the queries either time out (silent drop) or fail - // fast (ICMP unreachable), so accept any non-OK status; - // the resolver must return a classified response with no - // records rather than an error or a hang. - resp, err := r.QueryNameserverIP( - ctx, "unreachable.test.", "192.0.2.1", - "example.com", - ) - require.NoError(t, err) - - assert.NotEqual(t, resolver.StatusOK, resp.Status) - - totalRecords := 0 - for _, values := range resp.Records { - totalRecords += len(values) - } - - assert.Zero(t, totalRecords) -} +// The resolver's transport-failure classification (StatusTimeout / +// StatusError for a nameserver that does not answer) is deliberately +// not covered here. Forcing it required the mock DNSClient this +// change removes, and a live substitute is not available: the build +// environment transparently intercepts all UDP/53 traffic and answers +// it locally, so a query to a black-holed address such as an RFC 5737 +// documentation address comes back StatusOK with real records. See +// TESTING.md; restoring this coverage needs a mechanism that is +// neither a mock nor dependent on the sandbox's network behaviour. func TestResolveIPAddresses_ContextCanceled(t *testing.T) { t.Parallel() diff --git a/internal/watcher/watcher_test.go b/internal/watcher/watcher_test.go index ff4d998..3907fb6 100644 --- a/internal/watcher/watcher_test.go +++ b/internal/watcher/watcher_test.go @@ -191,12 +191,40 @@ type testDeps struct { config *config.Config } +// liveWatcherConcurrency caps how many of this package's tests may +// be driving live DNS at once. Every test here runs a full iterative +// resolution, and the package is entirely parallel, so without a +// bound all of them burst against the same root servers in the same +// instant and get rate-limited — the fan-out pathology that +// internal/resolver/livedns_test.go exists to prevent. That gate is +// package-scoped and so cannot reach across into this package's test +// binary; this is its counterpart here. +const liveWatcherConcurrency = 3 + +// liveWatcherGate bounds concurrent live-DNS watcher tests. It has to +// be package scoped: the point is that every parallel test shares it. +// +//nolint:gochecknoglobals // package-wide live query rate limit +var liveWatcherGate = make(chan struct{}, liveWatcherConcurrency) + +// acquireLiveSlot blocks until this test may run live DNS, releasing +// the slot when the test ends. +func acquireLiveSlot(t *testing.T) { + t.Helper() + + liveWatcherGate <- struct{}{} + + t.Cleanup(func() { <-liveWatcherGate }) +} + func newTestWatcher( t *testing.T, cfg *config.Config, ) (*watcher.Watcher, *testDeps) { t.Helper() + acquireLiveSlot(t) + deps := &testDeps{ portChecker: &mockPortChecker{}, tlsChecker: &mockTLSChecker{}, diff --git a/script/test b/script/test index b4ef492..68f5e54 100755 --- a/script/test +++ b/script/test @@ -19,15 +19,26 @@ # # -timeout 90s is a deliberate backstop above the 60s hard cap on # suite duration. Do not lower it. +# +# -p 1 runs one test package at a time, and is load-bearing. Live DNS +# is a resource outside the process: the concurrency gates that keep +# this suite from bursting at the root servers +# (internal/resolver/livedns_test.go, internal/watcher/watcher_test.go) +# are package-scoped, so each one only bounds its own test binary. Go +# runs package binaries in parallel by default, so with both live-DNS +# packages in flight at once their gates sum instead of holding, the +# root and TLD servers rate-limit the excess, and the resolver +# package's per-attempt deadlines expire. Serialising packages is what +# makes each gate authoritative while its package runs. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" main() { cd "$ROOT" - go test -count=1 -race -timeout 90s -cover ./... || { + go test -count=1 -p 1 -race -timeout 90s -cover ./... || { echo "--- Rerunning with -v for details ---" >&2 - go test -count=1 -race -timeout 90s -v ./... || true + go test -count=1 -p 1 -race -timeout 90s -v ./... || true exit 1 } }