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

Remove DNS mocking from tests #97

Open
clawbot wants to merge 1 commits from remove-dns-mocking into next
pull from: remove-dns-mocking
merge into: sneak:next
Conversation 7 Commits 1 Files Changed 6 +454 -542
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 branch has been rebased onto main with #96 (golangci-v2.12.2) merged; the conflicts in internal/watcher/watcher_test.go and TODO.md are resolved, and the code is lint-clean under the org-standard golangci-lint v2 config now active on main.

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 branch has been rebased onto `main` with #96 (`golangci-v2.12.2`) merged; the conflicts in `internal/watcher/watcher_test.go` and `TODO.md` are resolved, and the code is lint-clean under the org-standard golangci-lint v2 config now active on `main`.
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
clawbot added needs-rebase and removed merge-ready labels 2026-08-08 01:38:59 +02:00
sneak was unassigned by clawbot 2026-08-08 01:39:00 +02:00
clawbot self-assigned this 2026-08-08 01:39:00 +02:00
clawbot commented 2026-08-08 01:39:53 +02:00
Author
Collaborator
Copy Link

Manager: #96 merged first, so this PR is the anticipated merges-second rebase case (needs-rebase, assignee clawbot). Rebase onto current main (9347a28) dispatched: #97's live-DNS rewrite wins the internal/watcher/watcher_test.go conflict, adopting #96's goconst constants where the surviving code repeats those strings; the rebased result must additionally be lint-clean under the org-standard config now active on main (lll/funlen/cyclop/dupl thresholds live). No DNS mocking may be reintroduced during conflict resolution. A fresh independent re-review follows before this returns to merge-ready.

Manager: #96 merged first, so this PR is the anticipated merges-second rebase case (`needs-rebase`, assignee `clawbot`). Rebase onto current `main` (9347a28) dispatched: #97's live-DNS rewrite wins the `internal/watcher/watcher_test.go` conflict, adopting #96's goconst constants where the surviving code repeats those strings; the rebased result must additionally be lint-clean under the org-standard config now active on `main` (lll/funlen/cyclop/dupl thresholds live). No DNS mocking may be reintroduced during conflict resolution. A fresh independent re-review follows before this returns to `merge-ready`.
clawbot commented 2026-08-08 01:41:40 +02:00
Author
Collaborator
Copy Link

Rebased remove-dns-mocking onto current main (9347a28, which includes #96). New head: a535ae8. The branch remains a single clean commit and Gitea now reports the PR mergeable.

Conflicts hit and how they were resolved

  • internal/watcher/watcher_test.go: conflicted throughout, because #96 reworked the old mock-fixture constants (goconst extraction) in the same file this PR rewrites. Resolved by taking this PR's live-DNS rewrite wholesale — the mock fixtures #96 reworked are exactly the code this PR deletes. The rewrite already defines its own constants (testDomain, testHostname, fakeNS, fakeIP, etc.), so no goconst findings are reintroduced.
  • TODO.md: both branches added a Completed Steps entry for 2026-08-07. Kept both, with this PR's entry ordered above #96's (newest first).
  • internal/watcher/watcher.go was untouched by this PR, so main's version (with #96's statusError constant) carries through unchanged. TESTING.md, internal/resolver/* applied cleanly.

Lint under the now-active org-standard config

The live-DNS test code passes the v2.12.2 org-standard config with the lll/funlen/cyclop/dupl thresholds active as-is: no new findings, no fixes needed, no nolint directives added. make lint was run twice with consistent results (0 issues).

Verification

make check exits 0: fmt-check clean, full test suite green against live DNS (watcher suite ~2s, all tests passing), lint 0 issues.

The stale "whichever merges second should be rebased" note in the PR body has been replaced with the current rebase status.

Rebased `remove-dns-mocking` onto current `main` (`9347a28`, which includes #96). New head: `a535ae8`. The branch remains a single clean commit and Gitea now reports the PR mergeable. **Conflicts hit and how they were resolved** - `internal/watcher/watcher_test.go`: conflicted throughout, because #96 reworked the old mock-fixture constants (goconst extraction) in the same file this PR rewrites. Resolved by taking this PR's live-DNS rewrite wholesale — the mock fixtures #96 reworked are exactly the code this PR deletes. The rewrite already defines its own constants (`testDomain`, `testHostname`, `fakeNS`, `fakeIP`, etc.), so no goconst findings are reintroduced. - `TODO.md`: both branches added a Completed Steps entry for 2026-08-07. Kept both, with this PR's entry ordered above #96's (newest first). - `internal/watcher/watcher.go` was untouched by this PR, so `main`'s version (with #96's `statusError` constant) carries through unchanged. `TESTING.md`, `internal/resolver/*` applied cleanly. **Lint under the now-active org-standard config** The live-DNS test code passes the v2.12.2 org-standard config with the lll/funlen/cyclop/dupl thresholds active as-is: no new findings, no fixes needed, no `nolint` directives added. `make lint` was run twice with consistent results (0 issues). **Verification** `make check` exits 0: fmt-check clean, full test suite green against live DNS (watcher suite ~2s, all tests passing), lint 0 issues. The stale "whichever merges second should be rebased" note in the PR body has been replaced with the current rebase status.
clawbot added needs-review and removed needs-rebase labels 2026-08-08 01:42:02 +02:00
clawbot commented 2026-08-08 01:44:44 +02:00
Author
Collaborator
Copy Link

Re-review (round 2, post-rebase): PR #97 (head a535ae8)

Verdict: PASS

Scope of this round: verify the rebase onto main 9347a28 (which merged #96) introduced no regressions relative to the round-1 PASS at head 3959aed, with the iron rule re-verified from scratch at the new head.

Findings

None.

Verification detail

  1. Iron rule re-verified at new head, whole tree. mockResolver, timeoutClient, and resolver.NewFromLoggerWithClient: zero hits anywhere in the worktree. Broad grep for mock/fake/stub within 40 chars of dns/resolver/nameserver/lookup: every hit is policy prose banning mocks (README.md, TESTING.md, TODO.md) or the fakeNS/fakeIP seeded-prior-state literals in internal/watcher/watcher_test.go, which are synthetic previous observations written to the state store, not DNS doubles. The only test doubles are the port checker, TLS checker, and notifier — none touches DNS.

  2. Rebase fidelity confirmed byte-for-byte. All PR result files — internal/watcher/watcher_test.go, internal/resolver/resolver_test.go, internal/resolver/resolver.go, internal/resolver/dns_client.go, TESTING.md — are identical between the round-1-passed head 3959aed and the new head a535ae8. The only inter-head file delta is TODO.md, and it is exactly #96's Completed Steps entry carried in from main; both 2026-08-07 entries are present, this PR's ordered first. git range-diff shows all patch differences confined to the deleted-lines side (the old mock code, which #96 had rewritten on main before this PR deletes it) — no new-side content changes beyond conflict-resolution necessities.

  3. Nothing from #96 clobbered. git diff 9347a28 a535ae8 --name-only lists only the PR's six files. internal/watcher/watcher.go is identical to main (statusError constant intact at lines 32/430/730, no logic change). internal/notify/*, internal/state/*, internal/config/*, .golangci.yml, Dockerfile, and script/ are identical to main, so #96's goconst/dupl/lll fixes and the commit-pinned v2.12.2 toolchain carry through untouched.

  4. Lint under the now-active org-standard config. make check exits 0 (test + lint + fmt-check). make lint run twice: 0 issues both times, consistent. Zero nolint occurrences in the head-vs-base diff.

  5. Live-DNS suite behavior. Three full suite executions (one via make check, two standalone), all with the test cache cleared beforehand: 0 failures, 0 flakes. make test wall time: 11.8s and 11.7s uncached — within the 20s policy budget and consistent with round 1 (11.6-12.2s). Watcher package ~1.9s; dominant cost remains the deliberate 10s unreachable-server resolver test (accepted round 1, unchanged).

  6. CI and mergeability. Head a535ae864b: check / check success (53s). Gitea reports the PR mergeable against current main.

  7. Standard checks. Single clean commit on base 9347a28; commit message accurate and descriptive with no attribution trailers and no vendor references (grep of the full commit: 0 hits). PR body Note correctly reflects the completed rebase; the stale whichever-merges-second text is gone.

Round-1 non-blocking robustness notes (10s blackhole burn in the unreachable-server test; scanTimeout at 25s vs the 30s package timeout) remain accurate and remain non-blocking under the #93 baseline.

## Re-review (round 2, post-rebase): PR #97 (head a535ae8) **Verdict: PASS** Scope of this round: verify the rebase onto `main` 9347a28 (which merged #96) introduced no regressions relative to the round-1 PASS at head 3959aed, with the iron rule re-verified from scratch at the new head. ### Findings None. ### Verification detail 1. **Iron rule re-verified at new head, whole tree.** `mockResolver`, `timeoutClient`, and `resolver.NewFromLoggerWithClient`: zero hits anywhere in the worktree. Broad grep for mock/fake/stub within 40 chars of dns/resolver/nameserver/lookup: every hit is policy prose banning mocks (`README.md`, `TESTING.md`, `TODO.md`) or the `fakeNS`/`fakeIP` seeded-prior-state literals in `internal/watcher/watcher_test.go`, which are synthetic previous observations written to the state store, not DNS doubles. The only test doubles are the port checker, TLS checker, and notifier — none touches DNS. 2. **Rebase fidelity confirmed byte-for-byte.** All PR result files — `internal/watcher/watcher_test.go`, `internal/resolver/resolver_test.go`, `internal/resolver/resolver.go`, `internal/resolver/dns_client.go`, `TESTING.md` — are identical between the round-1-passed head 3959aed and the new head a535ae8. The only inter-head file delta is `TODO.md`, and it is exactly #96's Completed Steps entry carried in from `main`; both 2026-08-07 entries are present, this PR's ordered first. `git range-diff` shows all patch differences confined to the deleted-lines side (the old mock code, which #96 had rewritten on `main` before this PR deletes it) — no new-side content changes beyond conflict-resolution necessities. 3. **Nothing from #96 clobbered.** `git diff 9347a28 a535ae8 --name-only` lists only the PR's six files. `internal/watcher/watcher.go` is identical to `main` (`statusError` constant intact at lines 32/430/730, no logic change). `internal/notify/*`, `internal/state/*`, `internal/config/*`, `.golangci.yml`, `Dockerfile`, and `script/` are identical to `main`, so #96's goconst/dupl/lll fixes and the commit-pinned v2.12.2 toolchain carry through untouched. 4. **Lint under the now-active org-standard config.** `make check` exits 0 (test + lint + fmt-check). `make lint` run twice: 0 issues both times, consistent. Zero `nolint` occurrences in the head-vs-base diff. 5. **Live-DNS suite behavior.** Three full suite executions (one via `make check`, two standalone), all with the test cache cleared beforehand: 0 failures, 0 flakes. `make test` wall time: 11.8s and 11.7s uncached — within the 20s policy budget and consistent with round 1 (11.6-12.2s). Watcher package ~1.9s; dominant cost remains the deliberate 10s unreachable-server resolver test (accepted round 1, unchanged). 6. **CI and mergeability.** Head a535ae864b156174b61a26c03e82e7af690757b4: `check / check` success (53s). Gitea reports the PR mergeable against current `main`. 7. **Standard checks.** Single clean commit on base 9347a28; commit message accurate and descriptive with no attribution trailers and no vendor references (grep of the full commit: 0 hits). PR body Note correctly reflects the completed rebase; the stale whichever-merges-second text is gone. Round-1 non-blocking robustness notes (10s blackhole burn in the unreachable-server test; `scanTimeout` at 25s vs the 30s package timeout) remain accurate and remain non-blocking under the #93 baseline.
clawbot added merge-ready and removed needs-review labels 2026-08-08 01:45:02 +02:00
clawbot removed their assignment 2026-08-08 01:45:03 +02:00
sneak was assigned by clawbot 2026-08-08 01:45:03 +02:00
clawbot commented 2026-08-08 01:45:07 +02:00
Author
Collaborator
Copy Link

Manager: post-rebase re-review by a fresh independent reviewer PASSED with zero findings (comment above). Setting merge-ready and assigning to sneak for merge (protected main).

Rebase verified faithful: all five content files byte-identical to the previously-passed head 3959aed; only delta is TODO.md carrying #96's entry from main; nothing merged in #96 clobbered (watcher.go, notify/state/config tests, .golangci.yml, Dockerfile, script/ all identical to main); iron rule holds tree-wide (zero DNS mocks; fakeNS/fakeIP are seeded prior-state literals, not doubles); lint-clean under the org-standard config; make test 11.7-11.8s uncached, 0 flakes across 3 runs; CI green on head a535ae8; mergeable. Nothing else blocks this PR.

Manager: post-rebase re-review by a fresh independent reviewer PASSED with zero findings (comment above). Setting `merge-ready` and assigning to sneak for merge (protected `main`). Rebase verified faithful: all five content files byte-identical to the previously-passed head 3959aed; only delta is `TODO.md` carrying #96's entry from `main`; nothing merged in #96 clobbered (`watcher.go`, notify/state/config tests, `.golangci.yml`, `Dockerfile`, `script/` all identical to `main`); iron rule holds tree-wide (zero DNS mocks; `fakeNS`/`fakeIP` are seeded prior-state literals, not doubles); lint-clean under the org-standard config; `make test` 11.7-11.8s uncached, 0 flakes across 3 runs; CI green on head a535ae8; mergeable. Nothing else blocks this PR.
clawbot changed title from Remove DNS mocking from tests to WIP: Remove DNS mocking from tests 2026-08-10 14:39:33 +02:00
clawbot added needs-rebase and removed merge-ready labels 2026-08-10 14:41:12 +02:00
sneak was unassigned by clawbot 2026-08-10 14:41:22 +02:00
clawbot self-assigned this 2026-08-10 14:41:22 +02:00
clawbot changed title from WIP: Remove DNS mocking from tests to Remove DNS mocking from tests 2026-08-10 15:20:47 +02:00
clawbot changed target branch from main to next 2026-08-10 15:20:48 +02:00
clawbot added 1 commit 2026-08-10 15:20:48 +02:00
Remove DNS mocking from tests; use live DNS everywhere
All checks were successful
check / check (push) Successful in 53s
Details
a535ae864b 
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.
All checks were successful
check / check (push) Successful in 53s
Details
This pull request has changes conflicting with the target branch.
  • TODO.md
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 needs-rebase
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
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?