1 Commits

Author SHA1 Message Date
a535ae864b Remove DNS mocking from tests; use live DNS everywhere
All checks were successful
check / check (push) Successful in 53s
DNS is never mocked in this repository: tests exercise live DNS,
and robustness comes from handling real-world DNS behavior with
tolerant assertions and sensible timeouts, not from mocks.

watcher: drop mockResolver and wire the real iterative resolver
into the tests, querying stable public names (example.com,
www.example.com). Change detection is exercised by seeding the
state store with a synthetic previous observation that live DNS
cannot match (reserved .invalid nameserver names and RFC 5737
documentation addresses); DNS stays live in every run. The port
checker, TLS checker, and notifier remain test doubles since they
are not DNS, keeping notification and state assertions
deterministic against whatever addresses live DNS returns.

resolver: drop the timeoutClient fake DNSClient and the
NewFromLoggerWithClient mock constructor. The timeout test is
replaced by a live query against an RFC 5737 documentation
address where no nameserver can exist, asserting a classified
non-OK response with no records.

TESTING.md: extend the live-DNS policy to every package and
remove the carve-out that permitted DNS mocks in packages that
consume the resolver.

TODO.md: update stale references to hermetic mocked-DNS work to
reflect the no-mocking policy and the current state of
feature/resolver.

Intentionally dropped coverage: the exact StatusTimeout
classification (previously forced by the fake client) is no
longer asserted, because a genuinely unreachable server may fail
fast instead of timing out depending on the network path; the
live test tolerantly accepts any failure classification.
2026-08-07 23:40:38 +00:00
7 changed files with 463 additions and 593 deletions

View File

@@ -2,8 +2,10 @@
## DNS Resolution Tests ## DNS Resolution Tests
All resolver tests **MUST** use live queries against real DNS servers. All tests that involve DNS resolution — in every package, including
No mocking of the DNS client layer is permitted. consumers of the resolver such as the watcher — **MUST** use live
queries against real DNS servers. No mocking, faking, or stubbing of
DNS at any layer is permitted.
### Rationale ### Rationale
@@ -12,6 +14,8 @@ the full delegation chain. Mocked responses cannot faithfully represent
the variety of real-world DNS behavior (truncation, referrals, glue the variety of real-world DNS behavior (truncation, referrals, glue
records, DNSSEC, varied response times, EDNS, etc.). Testing against records, DNSSEC, varied response times, EDNS, etc.). Testing against
real servers ensures the resolver works correctly in production. real servers ensures the resolver works correctly in production.
Robustness comes from handling real-world DNS behavior with tolerant
assertions and sensible timeouts, not from mocks.
### Constraints ### Constraints
@@ -24,11 +28,14 @@ real servers ensures the resolver works correctly in production.
- Flaky failures from transient network issues are acceptable and - Flaky failures from transient network issues are acceptable and
should be investigated as potential resolver bugs, not papered over should be investigated as potential resolver bugs, not papered over
with mocks or skip flags with mocks or skip flags
- Watcher change-detection tests seed a synthetic *previous state*
and compare it against fresh live lookups; the DNS side is never
faked
### What NOT to do ### What NOT to do
- **Do not mock `DNSClient`** for resolver tests (the mock constructor - **Do not mock `DNSClient`**, the watcher's `DNSResolver` interface,
exists for unit-testing other packages that consume the resolver) or any other DNS abstraction — in any package, for any reason
- **Do not add `-short` flags** to skip slow tests - **Do not add `-short` flags** to skip slow tests
- **Do not increase `-timeout`** to hide hanging queries - **Do not increase `-timeout`** to hide hanging queries
- **Do not modify linter configuration** to suppress findings - **Do not modify linter configuration** to suppress findings

31
TODO.md
View File

@@ -14,7 +14,10 @@ pre-1.0. No git tags. Core resolver work in flight on feature/resolver
(dirty: internal/resolver/resolver_test.go). Local checkout has diverged (dirty: internal/resolver/resolver_test.go). Local checkout has diverged
from origin: origin/main is 8 commits ahead (watcher orchestrator, from origin: origin/main is 8 commits ahead (watcher orchestrator,
unified TARGETS) and origin/feature/resolver already contains the full unified TARGETS) and origin/feature/resolver already contains the full
iterative resolver implementation with hermetic mocked tests. iterative resolver implementation. DNS mocking is banned in this repo
(see `TESTING.md`): all tests use live DNS only. The hermetic mocked
tests previously noted on `feature/resolver` are gone from its current
tip, which carries a live-DNS suite against `*.dns.sneak.cloud`.
# Next Step # Next Step
@@ -25,13 +28,10 @@ confirm make check still passes.
# Completed Steps # Completed Steps
- 2026-08-09: `script/bootstrap` now installs the pinned `golangci-lint` - 2026-08-07: DNS mocking removed from the entire test suite; watcher
and `goimports` unconditionally instead of only when the binary is tests now drive the real iterative resolver against live DNS and
absent from `PATH`, so the commit pins actually take effect on `TESTING.md` bans DNS mocks in every package (`remove-dns-mocking`
already-provisioned machines; it also warns when `PATH` resolves branch)
either tool to a copy outside the directory `go install` writes to.
The `missing` presence check is retained for `git`, `make`, and `go`
(#117)
- 2026-08-07: golangci-lint bumped to v2.12.2 (commit-pinned installs - 2026-08-07: golangci-lint bumped to v2.12.2 (commit-pinned installs
in `Dockerfile` and `script/bootstrap`); `.golangci.yml` set to the in `Dockerfile` and `script/bootstrap`); `.golangci.yml` set to the
org-standard v2-schema config used across the org's repos org-standard v2-schema config used across the org's repos
@@ -43,7 +43,8 @@ confirm make check still passes.
- 2026-07-07 Adopted scripts-to-rule-them-all: `script/` entrypoints, - 2026-07-07 Adopted scripts-to-rule-them-all: `script/` entrypoints,
Makefile shims, README Entrypoints section Makefile shims, README Entrypoints section
- 2026-02-20: iterative DNS resolver implemented; tests made hermetic - 2026-02-20: iterative DNS resolver implemented; tests made hermetic
with mocked DNS (origin/feature/resolver, unmerged) with mocked DNS (origin/feature/resolver, unmerged; superseded — DNS
mocking is banned, see `TESTING.md`)
- 2026-02-20: CI actions and go install refs pinned to commit SHAs; - 2026-02-20: CI actions and go install refs pinned to commit SHAs;
Gitea Actions workflow for make check (origin/ci/make-check, unmerged) Gitea Actions workflow for make check (origin/ci/make-check, unmerged)
- 2026-02-20: watcher monitoring orchestrator merged to main (#8) - 2026-02-20: watcher monitoring orchestrator merged to main (#8)
@@ -70,8 +71,9 @@ Branch reconciliation:
- Sync local checkout with origin: local main is 8 commits behind - Sync local checkout with origin: local main is 8 commits behind
origin/main; local feature/resolver has diverged from origin/main; local feature/resolver has diverged from
origin/feature/resolver, which already implements the resolver origin/feature/resolver, which already implements the resolver
- Merge in-flight branches to main once green: feature/resolver, - Merge in-flight branches to main once green: feature/resolver
ci/make-check, feature/portcheck-implementation, (confirm its tests remain live-DNS — DNS mocking is banned, see
`TESTING.md`), ci/make-check, feature/portcheck-implementation,
feature/tlscheck-implementation feature/tlscheck-implementation
Resolver (plan from untracked TODO.md; largely implemented on Resolver (plan from untracked TODO.md; largely implemented on
@@ -155,6 +157,7 @@ Infrastructure notes (from untracked TODO.md):
- Module path sneak.berlin/go/dnswatcher differs from the git.eeqj.de - Module path sneak.berlin/go/dnswatcher differs from the git.eeqj.de
remote intentionally; do not "fix" it remote intentionally; do not "fix" it
- Dependencies: github.com/miekg/dns, golang.org/x/net/publicsuffix - Dependencies: github.com/miekg/dns, golang.org/x/net/publicsuffix
- Resolver tests originally used live DNS against *.dns.sneak.cloud - Resolver tests originally used live DNS against `*.dns.sneak.cloud`
(required records documented in the test file header); origin now has (required records documented in the test file header); `main` now
mocked hermetic tests, keep them hermetic tests against live public DNS. DNS mocking is banned (see
`TESTING.md`); never reintroduce hermetic mocked DNS tests

View File

@@ -7,8 +7,8 @@ import (
"github.com/miekg/dns" "github.com/miekg/dns"
) )
// DNSClient abstracts DNS wire-protocol exchanges so the resolver // DNSClient abstracts DNS wire-protocol exchanges over a single
// can be tested without hitting real nameservers. // transport, letting the resolver switch between UDP and TCP.
type DNSClient interface { type DNSClient interface {
ExchangeContext( ExchangeContext(
ctx context.Context, ctx context.Context,

View File

@@ -67,17 +67,4 @@ func NewFromLogger(log *slog.Logger) *Resolver {
} }
} }
// NewFromLoggerWithClient creates a Resolver with a custom DNS
// client, useful for testing with mock DNS responses.
func NewFromLoggerWithClient(
log *slog.Logger,
client DNSClient,
) *Resolver {
return &Resolver{
log: log,
client: client,
tcp: client,
}
}
// Method implementations are in iterative.go. // Method implementations are in iterative.go.

View File

@@ -10,7 +10,6 @@ import (
"testing" "testing"
"time" "time"
"github.com/miekg/dns"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
@@ -624,58 +623,41 @@ func TestQueryAllNameservers_ContextCanceled(t *testing.T) {
} }
// ---------------------------------------------------------------- // ----------------------------------------------------------------
// Timeout tests // Unreachable nameserver tests
// ---------------------------------------------------------------- // ----------------------------------------------------------------
func TestQueryNameserverIP_Timeout(t *testing.T) { func TestQueryNameserverIP_UnreachableServer(t *testing.T) {
t.Parallel() t.Parallel()
log := slog.New(slog.NewTextHandler( r := newTestResolver(t)
os.Stderr,
&slog.HandlerOptions{Level: slog.LevelDebug},
))
r := resolver.NewFromLoggerWithClient(
log, &timeoutClient{},
)
ctx, cancel := context.WithTimeout( ctx, cancel := context.WithTimeout(
context.Background(), 10*time.Second, context.Background(), 10*time.Second,
) )
t.Cleanup(cancel) t.Cleanup(cancel)
// Query any IP — the client always returns a timeout error. // 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( resp, err := r.QueryNameserverIP(
ctx, "unreachable.test.", "192.0.2.1", ctx, "unreachable.test.", "192.0.2.1",
"example.com", "example.com",
) )
require.NoError(t, err) require.NoError(t, err)
assert.Equal(t, resolver.StatusTimeout, resp.Status) assert.NotEqual(t, resolver.StatusOK, resp.Status)
assert.NotEmpty(t, resp.Error)
}
// timeoutClient simulates DNS timeout errors for testing. totalRecords := 0
type timeoutClient struct{} for _, values := range resp.Records {
totalRecords += len(values)
func (c *timeoutClient) ExchangeContext(
_ context.Context,
_ *dns.Msg,
_ string,
) (*dns.Msg, time.Duration, error) {
return nil, 0, &net.OpError{
Op: "read",
Net: "udp",
Err: &timeoutError{},
} }
assert.Zero(t, totalRecords)
} }
type timeoutError struct{}
func (e *timeoutError) Error() string { return "i/o timeout" }
func (e *timeoutError) Timeout() bool { return true }
func (e *timeoutError) Temporary() bool { return true }
func TestResolveIPAddresses_ContextCanceled(t *testing.T) { func TestResolveIPAddresses_ContextCanceled(t *testing.T) {
t.Parallel() t.Parallel()

File diff suppressed because it is too large Load Diff

View File

@@ -1,13 +1,10 @@
#!/bin/sh #!/bin/sh
# script/bootstrap: install all dependencies needed to build and develop # script/bootstrap: install all dependencies needed to build and develop
# this repo. Base tooling (git, make, go) comes from nix, apt, brew, or # this repo. Idempotent: every install is guarded by a check so already
# apk (detected in that order) and is installed only when absent; # installed tools are skipped. Base tooling comes from nix, apt, brew,
# assumes nothing is present. golangci-lint and goimports are always # or apk (detected in that order); assumes nothing is present.
# (re)installed via `go install` at the same pinned commits the # golangci-lint and goimports are installed via `go install` at the same
# Dockerfile uses (never "latest") -- a presence check cannot tell the # pinned commits the Dockerfile uses (never "latest").
# pinned build from an arbitrary one already on PATH, so guarding them
# would make the pins inert. Idempotent either way: running this twice
# succeeds both times and leaves the same result.
set -eu set -eu
ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"
@@ -65,31 +62,6 @@ missing() {
! command -v "$1" >/dev/null 2>&1 ! command -v "$1" >/dev/null 2>&1
} }
# go_bin_dir: directory `go install` writes binaries to.
go_bin_dir() {
gobin="$(go env GOBIN)"
if [ -n "$gobin" ]; then
echo "$gobin"
else
echo "$(go env GOPATH)/bin"
fi
}
# warn_if_shadowed <tool> <dir>: the pinned build was just installed
# into <dir>. If PATH resolves <tool> anywhere else, that other copy is
# what `make lint` and `make fmt` will actually run, and it is not the
# pinned version. Warn loudly rather than failing, since the fix is the
# user's PATH and not anything this script can do.
warn_if_shadowed() {
resolved="$(command -v "$1" 2>/dev/null || true)"
if [ "$resolved" != "$2/$1" ]; then
echo "bootstrap: WARNING: installed pinned $1 to $2/$1, but PATH" >&2
echo "bootstrap: WARNING: resolves $1 to ${resolved:-(not on PATH)};" >&2
echo "bootstrap: WARNING: put $2 first on PATH or lint results will" >&2
echo "bootstrap: WARNING: not match CI." >&2
fi
}
main() { main() {
cd "$ROOT" cd "$ROOT"
@@ -97,17 +69,10 @@ main() {
if missing make; then pkg_install gnumake make make make; fi if missing make; then pkg_install gnumake make make make; fi
if missing go; then pkg_install go golang go go; fi if missing go; then pkg_install go golang go go; fi
# Lint/format tools, pinned via go install. These are installed # Lint/format tools, pinned via go install (installs into
# unconditionally: `command -v` only proves *some* build is on PATH, # "$(go env GOPATH)/bin"; ensure that is on your PATH).
# and a wrong golangci-lint either cannot parse our v2-schema if missing golangci-lint; then go install "$GOLANGCI_LINT_REF"; fi
# .golangci.yml at all or silently disagrees with CI. Installing at if missing goimports; then go install "$GOIMPORTS_REF"; fi
# a fixed commit ref is idempotent and cheap with a warm module
# cache, so there is nothing to save by skipping it.
GOBIN_DIR="$(go_bin_dir)"
go install "$GOLANGCI_LINT_REF"
go install "$GOIMPORTS_REF"
warn_if_shadowed golangci-lint "$GOBIN_DIR"
warn_if_shadowed goimports "$GOBIN_DIR"
go mod download go mod download