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

Watcher tests stub DNS with a mock resolver, against the no-DNS-mocking rule #159

New Issue
Open
opened 2026-09-28 22:15:17 +02:00 by clawbot · 1 comment
clawbot commented 2026-09-28 22:15:17 +02:00
Collaborator
Copy Link
Copy Source

internal/watcher/watcher_test.go defines mockResolver, a stand-in for the DNSResolver interface that returns canned records, and every watcher test runs against it. That is a stubbed lookup. sneak's rule (2026-08-07, recorded on #94 and in the README section "No DNS mocking. Ever.", restated on #93) is that DNS is never mocked, not in tests and not anywhere else.

Two documents still invite it: TESTING.md says "the mock constructor exists for unit-testing other packages that consume the resolver", and the comment on NewFromLoggerWithClient in internal/resolver/resolver.go calls it "useful for testing with mock DNS responses".

What to do

  • Remove mockResolver. Watcher tests that need DNS use the real resolver against live nameservers, with the robustness the resolver tests already use (retries, several nameservers, timeouts).
  • Behaviour that depends on records changing (record changes, nameserver changes, inconsistencies) is tested by preparing the saved state the watcher starts from and letting live DNS supply the current records, or by testing the comparison on record data directly with no lookup involved. Never through a stand-in resolver.
  • Stand-ins for the notifier, port checker and TLS checker may stay: they are not DNS.
  • Remove the TESTING.md exception so it matches the README. Correct the resolver.go comment; if NewFromLoggerWithClient has no use other than mocks, remove it.

Definition of done

  • No mock, fake or stub of DNS resolution anywhere in the tree, tests included.
  • Watcher tests still cover first-run baseline, record change, nameserver change, nameserver failure and recovery, port state change and TLS expiry warnings.
  • TESTING.md, the README and code comments state the same rule.
  • The suite stays within its time limit; make check green.

Model: opus-5-5

`internal/watcher/watcher_test.go` defines `mockResolver`, a stand-in for the `DNSResolver` interface that returns canned records, and every watcher test runs against it. That is a stubbed lookup. sneak's rule (2026-08-07, recorded on https://git.eeqj.de/sneak/dnswatcher/issues/94 and in the README section "No DNS mocking. Ever.", restated on https://git.eeqj.de/sneak/dnswatcher/issues/93) is that DNS is never mocked, not in tests and not anywhere else. Two documents still invite it: `TESTING.md` says "the mock constructor exists for unit-testing other packages that consume the resolver", and the comment on `NewFromLoggerWithClient` in `internal/resolver/resolver.go` calls it "useful for testing with mock DNS responses". ## What to do - Remove `mockResolver`. Watcher tests that need DNS use the real resolver against live nameservers, with the robustness the resolver tests already use (retries, several nameservers, timeouts). - Behaviour that depends on records changing (record changes, nameserver changes, inconsistencies) is tested by preparing the saved state the watcher starts from and letting live DNS supply the current records, or by testing the comparison on record data directly with no lookup involved. Never through a stand-in resolver. - Stand-ins for the notifier, port checker and TLS checker may stay: they are not DNS. - Remove the `TESTING.md` exception so it matches the README. Correct the `resolver.go` comment; if `NewFromLoggerWithClient` has no use other than mocks, remove it. ## Definition of done - No mock, fake or stub of DNS resolution anywhere in the tree, tests included. - Watcher tests still cover first-run baseline, record change, nameserver change, nameserver failure and recovery, port state change and TLS expiry warnings. - `TESTING.md`, the README and code comments state the same rule. - The suite stays within its time limit; `make check` green. Model: opus-5-5
clawbot commented 2026-09-29 01:02:06 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan. What is on next today:

  • mockResolver in internal/watcher/watcher_test.go, used by every watcher test.
  • timeoutClient in internal/resolver/resolver_test.go, a stand-in DNS client that always returns a timeout, passed in through NewFromLoggerWithClient. It is covered by this issue too.
  • The TESTING.md exception and the comment on NewFromLoggerWithClient.

Approach:

  1. Watcher tests use the real resolver (resolver.NewFromLogger) on targets in stable, well-run zones; the current records come from live DNS. Where a test needs something to have changed, it prepares the saved state the watcher starts from with values real DNS never returns (documentation addresses such as 192.0.2.1, nameserver names under .invalid), so the check sees a real difference: record change, nameserver change, nameserver disappearance. For recovery, the saved state marks a real nameserver as failed. Assert on notification titles and on the saved state, never on specific live record values.
  2. The timeout test queries a real address that never answers (192.0.2.1) through the normal client with a short deadline, and checks for the timeout status.
  3. Live-DNS robustness for the watcher tests follows what the resolver tests already do (a limit on concurrent queries, retries on transport failures). Reuse or move that code; do not build a second version of it.
  4. If NewFromLoggerWithClient has no caller left, remove it.
  5. The notifier, port checker and TLS checker stand-ins stay; they are not DNS.
  6. The suite stays within its 60-second cap (sneak's ruling on #93).

Disclosure for the PR: the watcher tests now need network access, as the resolver tests already do.

Model: opus-5-5

Plan. What is on `next` today: - `mockResolver` in `internal/watcher/watcher_test.go`, used by every watcher test. - `timeoutClient` in `internal/resolver/resolver_test.go`, a stand-in DNS client that always returns a timeout, passed in through `NewFromLoggerWithClient`. It is covered by this issue too. - The `TESTING.md` exception and the comment on `NewFromLoggerWithClient`. Approach: 1. Watcher tests use the real resolver (`resolver.NewFromLogger`) on targets in stable, well-run zones; the current records come from live DNS. Where a test needs something to have changed, it prepares the saved state the watcher starts from with values real DNS never returns (documentation addresses such as `192.0.2.1`, nameserver names under `.invalid`), so the check sees a real difference: record change, nameserver change, nameserver disappearance. For recovery, the saved state marks a real nameserver as failed. Assert on notification titles and on the saved state, never on specific live record values. 2. The timeout test queries a real address that never answers (`192.0.2.1`) through the normal client with a short deadline, and checks for the timeout status. 3. Live-DNS robustness for the watcher tests follows what the resolver tests already do (a limit on concurrent queries, retries on transport failures). Reuse or move that code; do not build a second version of it. 4. If `NewFromLoggerWithClient` has no caller left, remove it. 5. The notifier, port checker and TLS checker stand-ins stay; they are not DNS. 6. The suite stays within its 60-second cap (sneak's ruling on https://git.eeqj.de/sneak/dnswatcher/issues/93). Disclosure for the PR: the watcher tests now need network access, as the resolver tests already do. Model: opus-5-5
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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 labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/dnswatcher#159