sneak/dnswatcher
1
0
Fork 0
Code Issues 3 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Remove DNS mocking from tests #97

Open
clawbot wants to merge 1 commits from remove-dns-mocking into main
pull from: remove-dns-mocking
merge into: sneak:main
Conversation 3 Commits 1 Files Changed 6 +461 -538
clawbot commented 2026-08-07 22:43:53 +02:00
Collaborator
Copy Link

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.

What was removed

  • mockResolver in internal/watcher/watcher_test.go: the watcher tests are now wired to the real iterative resolver and query stable public names (example.com, www.example.com).
  • timeoutClient (a fake DNSClient) in internal/resolver/resolver_test.go.
  • resolver.NewFromLoggerWithClient, the constructor whose only purpose was injecting mock DNS clients.
  • The TESTING.md carve-out that permitted DNS mocks in packages consuming the resolver; the live-DNS policy now applies to every package.

How the rewritten tests exercise live DNS

Every watcher test resolves live DNS on every run. Change detection is driven by seeding the state store with a synthetic previous observation that live DNS cannot match (reserved .invalid nameserver names, RFC 5737 documentation addresses) and then re-running against live DNS:

  • NS change: baseline NS set replaced with ns-baseline.invalid.; the next live run detects the difference.
  • Record change: each recorded nameserver answer rewritten to 203.0.113.1; the next live run observes the real records and notifies.
  • NS failure/recovery: a synthetic healthy nameserver is recorded (must be reported as disappeared) and a real live-observed one is recorded as failed (must be reported as recovered).
  • Stale-DNS regression: state seeded with a stale documentation address; after one cycle, port/TLS state must be keyed by the freshly live-resolved addresses and the stale key must be gone.
  • Startup notification tests poll for scan completion with a deadline instead of assuming a scan duration, since live scans take variable time.

The port checker, TLS checker, and notifier remain test doubles — they are not DNS — and were generalized ("all ports open/closed", "one cert for any address") so assertions hold for whatever addresses live DNS returns, including multi-IP and AAAA answers.

The resolver timeout test now queries 192.0.2.1 (RFC 5737, no nameserver can exist there) through the real UDP client and asserts a classified non-OK response with no records.

Coverage intentionally dropped

  • Exact StatusTimeout classification: the fake client guaranteed a timeout error; a real unreachable address may instead fail fast (ICMP unreachable) depending on the network path, so the live test tolerantly accepts any failure classification rather than asserting timeout specifically.

Everything else the mocked tests covered is covered live.

make check and script/cibuild (Docker, pinned toolchain) are green.

Note: this will conflict slightly with #96 (golangci-v2.12.2) in internal/watcher/watcher_test.go (goconst fixture constants); whichever merges second should be rebased — this branch intentionally stays on main's current linter config.

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. ## What was removed - `mockResolver` in `internal/watcher/watcher_test.go`: the watcher tests are now wired to the real iterative resolver and query stable public names (`example.com`, `www.example.com`). - `timeoutClient` (a fake `DNSClient`) in `internal/resolver/resolver_test.go`. - `resolver.NewFromLoggerWithClient`, the constructor whose only purpose was injecting mock DNS clients. - The `TESTING.md` carve-out that permitted DNS mocks in packages consuming the resolver; the live-DNS policy now applies to every package. ## How the rewritten tests exercise live DNS Every watcher test resolves live DNS on every run. Change detection is driven by seeding the state store with a synthetic *previous observation* that live DNS cannot match (reserved `.invalid` nameserver names, RFC 5737 documentation addresses) and then re-running against live DNS: - NS change: baseline NS set replaced with `ns-baseline.invalid.`; the next live run detects the difference. - Record change: each recorded nameserver answer rewritten to `203.0.113.1`; the next live run observes the real records and notifies. - NS failure/recovery: a synthetic healthy nameserver is recorded (must be reported as disappeared) and a real live-observed one is recorded as failed (must be reported as recovered). - Stale-DNS regression: state seeded with a stale documentation address; after one cycle, port/TLS state must be keyed by the freshly live-resolved addresses and the stale key must be gone. - Startup notification tests poll for scan completion with a deadline instead of assuming a scan duration, since live scans take variable time. The port checker, TLS checker, and notifier remain test doubles — they are not DNS — and were generalized ("all ports open/closed", "one cert for any address") so assertions hold for whatever addresses live DNS returns, including multi-IP and AAAA answers. The resolver timeout test now queries `192.0.2.1` (RFC 5737, no nameserver can exist there) through the real UDP client and asserts a classified non-OK response with no records. ## Coverage intentionally dropped - Exact `StatusTimeout` classification: the fake client guaranteed a timeout error; a real unreachable address may instead fail fast (ICMP unreachable) depending on the network path, so the live test tolerantly accepts any failure classification rather than asserting `timeout` specifically. Everything else the mocked tests covered is covered live. `make check` and `script/cibuild` (Docker, pinned toolchain) are green. Note: this will conflict slightly with #96 (`golangci-v2.12.2`) in `internal/watcher/watcher_test.go` (goconst fixture constants); whichever merges second should be rebased — this branch intentionally stays on `main`'s current linter config.
clawbot added 1 commit 2026-08-07 22:43:53 +02:00
Remove DNS mocking from tests; use live DNS everywhere
All checks were successful
check / check (push) Successful in 50s
Details
3959aedb6a 
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.
clawbot added the needs-review label 2026-08-07 22:55:12 +02:00
clawbot self-assigned this 2026-08-07 22:55:13 +02:00
clawbot commented 2026-08-07 23:00:59 +02:00
Author
Collaborator
Copy Link

Review: PR #97 (head 3959aed)

Verdict: PASS

Mandate verification

  1. No DNS mocking remains anywhere in the tree at this head. Full-tree grep for mock/fake/stub near DNS/resolver/nameserver terms: zero hits. timeoutClient, timeoutError, and mockResolver are gone; the miekg/dns import is gone from internal/resolver/resolver_test.go; no net.DefaultResolver overrides, dial hooks, or hosts-file tricks anywhere. The only remaining test doubles are mockPortChecker, mockTLSChecker, and mockNotifier in internal/watcher/watcher_test.go — none touches DNS. Their generalization does not hollow out the assertions: TestPortStateChange still flips open-to-closed and requires a Port Change: notification; TestTLSExpiryWarningDedup requires at least one warning and then a stable count; TestDomainPortAndTLSChecks still asserts nonzero call counts and populated state.

  2. resolver.NewFromLoggerWithClient removal is clean. Zero remaining references in the tree; internal package, nothing external can depend on it; everything compiles and passes.

  3. Dropped coverage is exactly as documented, plus one trivial sibling. Old vs new test enumeration: internal/watcher/watcher_test.go has 13 test functions before and after — every old scenario (baseline, NS change, record change, port change, TLS expiry + dedup, graceful shutdown, DNS-before-port/TLS ordering, startup notification x3, NS failure/recovery) survives; the NS failure/recovery test now covers both directions in one cycle. In the resolver test, TestQueryNameserverIP_Timeout became TestQueryNameserverIP_UnreachableServer; the exact StatusTimeout assertion is dropped as documented, and with it the (undocumented but subsumed) assert.NotEmpty(resp.Error) — minor, acceptable: on a live 192.0.2.1 path the failure mode genuinely varies, and the new test still requires a classified non-OK status with zero records and no error/hang. The old mockResolver error-injection fields (lookupNSErr etc.) were never exercised by any removed test, so nothing else was lost.

  4. Seeded-state design is sound. fakeNS uses the RFC 2606 .invalid TLD, which can never appear in live NS answers; fakeIP (203.0.113.1) and 192.0.2.1 are RFC 5737 documentation addresses that IANA-operated example.com/www.example.com can never legitimately resolve to. No hard assertions on example.com record contents, no specific IP-count or ordering assumptions (liveIPs deduplicates and sorts), and no IPv6-connectivity assumption (AAAA answers are recorded from DNS; connections go only through the mocked port/TLS checkers, which accept any address). Startup-notification tests poll with a deadline instead of sleeping a fixed scan duration. Residual brittleness, noted as within the accepted live-DNS baseline (#93), not blockers: (a) TestQueryNameserverIP_UnreachableServer burns its full 10s context when packets to 192.0.2.1 drop silently (observed 10.01s locally), one third of the resolver package's 30s budget; (b) scanTimeout of 25s in the watcher tests sits close to the 30s go test timeout, so a badly degraded network would surface as a package timeout panic rather than a clean single-test failure. Observed watcher package time is ~2-3s, so headroom is real today.

  5. make test timing: within policy. Two fully uncached runs (test cache cleared between): 12.2s and 11.6s wall, against the 20s policy budget and 30s Makefile timeout. Dominant cost is the deliberate 10s unreachable-server test running parallel inside the resolver package (11.0s package time); watcher package 2.0-2.8s.

  6. make check: exit 0 (test + lint + fmt-check), lint reports 0 issues, fmt-check clean. Zero test failures and zero flakes across 3 total live-DNS suite executions (2 uncached). CI on head 3959aed: check / check success (50s).

  7. #96 interplay claim confirmed. origin/golangci-v2.12.2 rewrites the mocked fixture literals in internal/watcher/watcher_test.go into constants (108+/97-) — the very code this PR deletes. The hunks overlap; whichever merges second needs a rebase (and if this merges first, most of #96's watcher_test.go delta becomes moot since this PR already introduces testDomain/testHostname constants). This branch staying on main's current lint config is coherent: local lint passes 0 issues against the current .golangci.yml.

  8. Standard checks. No new nolint (all 15 in-tree hits pre-date this PR). No attribution trailers or vendor references anywhere. TESTING.md changes correctly extend the live-DNS rule to every package and remove the mock carve-out, consistent with the iron rule and with the #93 discussion (which this PR neither closes nor pre-empts — the default-suite gating decision there remains open and untouched). TODO.md updates are in scope (stale mock references). Commit message is accurate and descriptive; no (closes #N) because no issue defines this work — the only related open issue (#93) is intentionally not closed by it. Mergeable against current main (f79cd98): confirmed; main's one commit past the branch point touches only README.md, which this PR does not modify.

Verification detail

  • Worktree at detached head 3959aedb6a.
  • make test (uncached run 1): 12.2s, all packages ok. (uncached run 2 after go clean -testcache): 11.6s, all packages ok. Coverage: watcher 83.7%, resolver 77.4%.
  • make check: exit 0, lint 0 issues.
  • Flake count: 0 failures in 3 suite executions.
  • Greps: NewFromLoggerWithClient 0 hits; DNS-adjacent mock/fake/stub 0 hits; timeoutClient/timeoutError 0 hits.
## Review: PR #97 (head 3959aed) **Verdict: PASS** ### Mandate verification 1. **No DNS mocking remains anywhere in the tree at this head.** Full-tree grep for mock/fake/stub near DNS/resolver/nameserver terms: zero hits. `timeoutClient`, `timeoutError`, and `mockResolver` are gone; the `miekg/dns` import is gone from `internal/resolver/resolver_test.go`; no `net.DefaultResolver` overrides, dial hooks, or hosts-file tricks anywhere. The only remaining test doubles are `mockPortChecker`, `mockTLSChecker`, and `mockNotifier` in `internal/watcher/watcher_test.go` — none touches DNS. Their generalization does not hollow out the assertions: `TestPortStateChange` still flips open-to-closed and requires a `Port Change: ` notification; `TestTLSExpiryWarningDedup` requires at least one warning and then a stable count; `TestDomainPortAndTLSChecks` still asserts nonzero call counts and populated state. 2. **`resolver.NewFromLoggerWithClient` removal is clean.** Zero remaining references in the tree; internal package, nothing external can depend on it; everything compiles and passes. 3. **Dropped coverage is exactly as documented, plus one trivial sibling.** Old vs new test enumeration: `internal/watcher/watcher_test.go` has 13 test functions before and after — every old scenario (baseline, NS change, record change, port change, TLS expiry + dedup, graceful shutdown, DNS-before-port/TLS ordering, startup notification x3, NS failure/recovery) survives; the NS failure/recovery test now covers both directions in one cycle. In the resolver test, `TestQueryNameserverIP_Timeout` became `TestQueryNameserverIP_UnreachableServer`; the exact `StatusTimeout` assertion is dropped as documented, and with it the (undocumented but subsumed) `assert.NotEmpty(resp.Error)` — minor, acceptable: on a live 192.0.2.1 path the failure mode genuinely varies, and the new test still requires a classified non-OK status with zero records and no error/hang. The old `mockResolver` error-injection fields (`lookupNSErr` etc.) were never exercised by any removed test, so nothing else was lost. 4. **Seeded-state design is sound.** `fakeNS` uses the RFC 2606 `.invalid` TLD, which can never appear in live NS answers; `fakeIP` (203.0.113.1) and 192.0.2.1 are RFC 5737 documentation addresses that IANA-operated `example.com`/`www.example.com` can never legitimately resolve to. No hard assertions on example.com record contents, no specific IP-count or ordering assumptions (`liveIPs` deduplicates and sorts), and no IPv6-connectivity assumption (AAAA answers are recorded from DNS; connections go only through the mocked port/TLS checkers, which accept any address). Startup-notification tests poll with a deadline instead of sleeping a fixed scan duration. Residual brittleness, noted as within the accepted live-DNS baseline (#93), not blockers: (a) `TestQueryNameserverIP_UnreachableServer` burns its full 10s context when packets to 192.0.2.1 drop silently (observed 10.01s locally), one third of the resolver package's 30s budget; (b) `scanTimeout` of 25s in the watcher tests sits close to the 30s `go test` timeout, so a badly degraded network would surface as a package timeout panic rather than a clean single-test failure. Observed watcher package time is ~2-3s, so headroom is real today. 5. **`make test` timing: within policy.** Two fully uncached runs (test cache cleared between): 12.2s and 11.6s wall, against the 20s policy budget and 30s Makefile timeout. Dominant cost is the deliberate 10s unreachable-server test running parallel inside the resolver package (11.0s package time); watcher package 2.0-2.8s. 6. **`make check`: exit 0** (test + lint + fmt-check), lint reports 0 issues, fmt-check clean. Zero test failures and zero flakes across 3 total live-DNS suite executions (2 uncached). CI on head 3959aed: `check / check` success (50s). 7. **#96 interplay claim confirmed.** `origin/golangci-v2.12.2` rewrites the mocked fixture literals in `internal/watcher/watcher_test.go` into constants (108+/97-) — the very code this PR deletes. The hunks overlap; whichever merges second needs a rebase (and if this merges first, most of #96's watcher_test.go delta becomes moot since this PR already introduces `testDomain`/`testHostname` constants). This branch staying on main's current lint config is coherent: local lint passes 0 issues against the current `.golangci.yml`. 8. **Standard checks.** No new `nolint` (all 15 in-tree hits pre-date this PR). No attribution trailers or vendor references anywhere. `TESTING.md` changes correctly extend the live-DNS rule to every package and remove the mock carve-out, consistent with the iron rule and with the #93 discussion (which this PR neither closes nor pre-empts — the default-suite gating decision there remains open and untouched). `TODO.md` updates are in scope (stale mock references). Commit message is accurate and descriptive; no `(closes #N)` because no issue defines this work — the only related open issue (#93) is intentionally not closed by it. Mergeable against current `main` (f79cd98): confirmed; main's one commit past the branch point touches only `README.md`, which this PR does not modify. ### Verification detail - Worktree at detached head 3959aedb6a4302a7ef833d419c1dbe5ab71c5787. - `make test` (uncached run 1): 12.2s, all packages ok. (uncached run 2 after `go clean -testcache`): 11.6s, all packages ok. Coverage: watcher 83.7%, resolver 77.4%. - `make check`: exit 0, lint 0 issues. - Flake count: 0 failures in 3 suite executions. - Greps: `NewFromLoggerWithClient` 0 hits; DNS-adjacent mock/fake/stub 0 hits; `timeoutClient`/`timeoutError` 0 hits.
clawbot added merge-ready and removed needs-review labels 2026-08-07 23:01:18 +02:00
clawbot removed their assignment 2026-08-07 23:01:20 +02:00
sneak was assigned by clawbot 2026-08-07 23:01:20 +02:00
clawbot commented 2026-08-07 23:01:27 +02:00
Author
Collaborator
Copy Link

Manager: independent adversarial review PASSED (comment above). Setting merge-ready and assigning to sneak for merge (protected main).

Review highlights: no DNS mock/fake/stub remains anywhere in the tree (iron rule fully implemented); NewFromLoggerWithClient removal is clean with zero remaining references; 13 watcher test functions before and after (nothing silently dropped); the one documented coverage drop (exact StatusTimeout classification) plus its minor sibling (assert.NotEmpty(resp.Error)) judged acceptable — the live test still requires a classified non-OK response with zero records; make test measured at 11.6-12.2s uncached (inside the 20s policy budget), 0 flakes across 3 suite executions; CI green on head 3959aed.

Non-blocking robustness notes for the record (within the accepted #93 baseline, candidates for future hardening, not this PR): TestQueryNameserverIP_UnreachableServer burns its full 10s context when 192.0.2.1 blackholes; scanTimeout = 25s in internal/watcher/watcher_test.go sits close to the 30s go test timeout, so a badly degraded network would present as a package-timeout panic rather than a clean single-test failure.

Merge-order note: #96 (also merge-ready) rewrites the same internal/watcher/watcher_test.go region this PR deletes — whichever merges second needs a small rebase; ping this repo's manager (or leave a comment) and the rebase will be run through the loop.

Manager: independent adversarial review PASSED (comment above). Setting `merge-ready` and assigning to sneak for merge (protected `main`). Review highlights: no DNS mock/fake/stub remains anywhere in the tree (iron rule fully implemented); `NewFromLoggerWithClient` removal is clean with zero remaining references; 13 watcher test functions before and after (nothing silently dropped); the one documented coverage drop (exact `StatusTimeout` classification) plus its minor sibling (`assert.NotEmpty(resp.Error)`) judged acceptable — the live test still requires a classified non-OK response with zero records; `make test` measured at 11.6-12.2s uncached (inside the 20s policy budget), 0 flakes across 3 suite executions; CI green on head 3959aed. Non-blocking robustness notes for the record (within the accepted #93 baseline, candidates for future hardening, not this PR): `TestQueryNameserverIP_UnreachableServer` burns its full 10s context when 192.0.2.1 blackholes; `scanTimeout = 25s` in `internal/watcher/watcher_test.go` sits close to the 30s `go test` timeout, so a badly degraded network would present as a package-timeout panic rather than a clean single-test failure. Merge-order note: #96 (also `merge-ready`) rewrites the same `internal/watcher/watcher_test.go` region this PR deletes — whichever merges second needs a small rebase; ping this repo's manager (or leave a comment) and the rebase will be run through the loop.
sneak commented 2026-08-07 23:16:15 +02:00
Owner
Copy Link

@clawbot merged the other one first, fix pls

@clawbot merged the other one first, fix pls
All checks were successful
check / check (push) Successful in 50s
Required
Details
This pull request has changes conflicting with the target branch.
  • TODO.md
  • internal/watcher/watcher_test.go
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin remove-dns-mocking:remove-dns-mocking
git checkout remove-dns-mocking
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No Label merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees sneak
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/dnswatcher#97
Delete Branch "remove-dns-mocking"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?