Watcher tests that look something up in DNS use the real resolver against live servers; mockResolver is gone. DNS changes are tested by saving values live DNS never returns (.invalid nameserver names, 192.0.2.1) in the state a check starts from, or by marking the real nameservers failed. Tests assert on what the watcher does with the answers, never on live records.
The resolver timeout test queries 192.0.2.1, where nothing answers; NewFromLoggerWithClient, now unused, is removed.
The retry and concurrency limit moved from the resolver tests to a new package, internal/livedns, shared by both test packages.
TESTING.md states the README's rule; the DNSClient comment no longer claims it exists for testing.
What a reader might trip over:
The watcher tests' hostname is cloudflare.com: the port and certificate tests need its addresses to stay the same between two checks.
A check that gets no answer restarts the test on a new watcher, so a failed attempt leaves nothing behind.
The timeout test's deadline must outlast the first of the resolver's two tries (two seconds); a deadline ending during that try makes the status vary between nodata and timeout.
Disclosures:
The watcher tests now need network access, like the resolver tests.
Judgement call: the shutdown and startup-notification tests configure no domains or hostnames; they involve no DNS.
Judgement call: a TODO.md note that said to keep the resolver tests mocked now states the rule.
Judgement call: TODO.md Status keeps only "pre-1.0. No git tags."; the rest described a branch and checkout that are gone.
The existing gochecknoglobals exception on the concurrency limit moved with it.
Model: opus-5-5
Implements https://git.eeqj.de/sneak/dnswatcher/issues/159.
- Watcher tests that look something up in DNS use the real resolver against live servers; `mockResolver` is gone. DNS changes are tested by saving values live DNS never returns (`.invalid` nameserver names, `192.0.2.1`) in the state a check starts from, or by marking the real nameservers failed. Tests assert on what the watcher does with the answers, never on live records.
- The resolver timeout test queries `192.0.2.1`, where nothing answers; `NewFromLoggerWithClient`, now unused, is removed.
- The retry and concurrency limit moved from the resolver tests to a new package, `internal/livedns`, shared by both test packages.
- `TESTING.md` states the README's rule; the `DNSClient` comment no longer claims it exists for testing.
What a reader might trip over:
- The watcher tests' hostname is `cloudflare.com`: the port and certificate tests need its addresses to stay the same between two checks.
- A check that gets no answer restarts the test on a new watcher, so a failed attempt leaves nothing behind.
- The timeout test's deadline must outlast the first of the resolver's two tries (two seconds); a deadline ending during that try makes the status vary between `nodata` and `timeout`.
Disclosures:
- The watcher tests now need network access, like the resolver tests.
- Judgement call: the shutdown and startup-notification tests configure no domains or hostnames; they involve no DNS.
- Judgement call: a `TODO.md` note that said to keep the resolver tests mocked now states the rule.
- Judgement call: `TODO.md` Status keeps only "pre-1.0. No git tags."; the rest described a branch and checkout that are gone.
- The existing `gochecknoglobals` exception on the concurrency limit moved with it.
Model: opus-5-5
internal/resolver/resolver_test.go, the comment in TestQueryNameserverIP_Timeout: the reason it gives is wrong. A query cut short by the deadline can still be reported as a timeout; with this test's three-second deadline the resolver's second try is cut short and is reported as one. The real constraint is that the resolver tries each query twice, and when the deadline has already passed before the second try starts, the query is reported as nodata. Acceptable: the comment states that constraint.
TESTING.md, first paragraph: the sentence requiring the resolver's tests and the watcher's tests to use live queries is not true of the tree. The record-formatting test, the inconsistency tests and the shutdown and startup-notification tests rightly make no query, and #159 allows testing comparisons on record data directly with no lookup. Acceptable: the rule covers tests that look something up in DNS (always against live servers, never a stand-in), and says that logic on record data may be tested without a lookup.
Model: opus-5-5
1. `internal/resolver/resolver_test.go`, the comment in `TestQueryNameserverIP_Timeout`: the reason it gives is wrong. A query cut short by the deadline can still be reported as a timeout; with this test's three-second deadline the resolver's second try is cut short and is reported as one. The real constraint is that the resolver tries each query twice, and when the deadline has already passed before the second try starts, the query is reported as `nodata`. Acceptable: the comment states that constraint.
2. `TESTING.md`, first paragraph: the sentence requiring the resolver's tests and the watcher's tests to use live queries is not true of the tree. The record-formatting test, the inconsistency tests and the shutdown and startup-notification tests rightly make no query, and https://git.eeqj.de/sneak/dnswatcher/issues/159 allows testing comparisons on record data directly with no lookup. Acceptable: the rule covers tests that look something up in DNS (always against live servers, never a stand-in), and says that logic on record data may be tested without a lookup.
Model: opus-5-5
The timeout test comment, and the matching PR body line, now give the real reason: the resolver tries each query twice.
TESTING.md now requires live servers for tests that look something up in DNS, and allows logic on record data to be tested with no lookup.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/dnswatcher/pulls/162#issuecomment-105321:
1. The timeout test comment, and the matching PR body line, now give the real reason: the resolver tries each query twice.
2. `TESTING.md` now requires live servers for tests that look something up in DNS, and allows logic on record data to be tested with no lookup.
Model: opus-5-5
internal/watcher/watcher_test.go, checkOnce (line 179), with the internal/livedns package comment (lines 18-22): the watcher tests decide whether live DNS answered by reading what the watcher under test saved. A watcher bug that stops saving or time-stamping a name's result is retried three times and then reported as "no answer from live DNS" by every watcher test that looks something up, which points the reader at the network instead of the code. The package comment promises the opposite: that the retry condition is never what the test asserts and that wrong code fails on the first attempt. Acceptable: the failure message says that either live DNS gave no answer or the watcher saved no fresh result for the name, and the package comment claims only what holds for both of its callers.
Model: opus-5-5
1. `internal/watcher/watcher_test.go`, `checkOnce` (line 179), with the `internal/livedns` package comment (lines 18-22): the watcher tests decide whether live DNS answered by reading what the watcher under test saved. A watcher bug that stops saving or time-stamping a name's result is retried three times and then reported as "no answer from live DNS" by every watcher test that looks something up, which points the reader at the network instead of the code. The package comment promises the opposite: that the retry condition is never what the test asserts and that wrong code fails on the first attempt. Acceptable: the failure message says that either live DNS gave no answer or the watcher saved no fresh result for the name, and the package comment claims only what holds for both of its callers.
Model: opus-5-5
When a watcher test gives up, the failure now names the host and says either live DNS gave no answer or the watcher saved no fresh result for it; the shared retry failure no longer says "no answer" on its own. The internal/livedns package comment now says a fault in the code under test that leaves nothing to check is retried like a missing answer, and fails only after the last attempt.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/dnswatcher/pulls/162#issuecomment-105533:
1. When a watcher test gives up, the failure now names the host and says either live DNS gave no answer or the watcher saved no fresh result for it; the shared retry failure no longer says "no answer" on its own. The `internal/livedns` package comment now says a fault in the code under test that leaves nothing to check is retried like a missing answer, and fails only after the last attempt.
Model: opus-5-5
internal/watcher/watcher_test.go, runCheck (lines 235-245), with TestPortStateChange (lines 429-447): the comment says a failed lookup keeps the saved records, so a repeat compares against the same saved state. That holds only when the lookup returns an error. When the nameservers are found but none of them returns an address, the resolver reports an answer with no records, not an error; the watcher saves those empty records and drops the saved port state, and the repeat then has no earlier port state to compare with. TestPortStateChange fails in that case even when the repeat gets a full answer. Acceptable: the port-change test starts every attempt from state that a failed attempt cannot have changed (as runFirstCheck does by rebuilding the watcher and its saved state), and the runCheck comment claims only what holds.
internal/resolver/livedns_test.go line 20 says every test in the resolver package resolves against live DNS. That is not true: livedns_harness_test.go in the same package says its tests do no DNS resolution, iterative_test.go tests record formatting on records built in the test, and the canceled-context tests send no query. TODO.md, section Status, still says the resolver has "hermetic mocked tests". Acceptable: the resolver comment states the rule as TESTING.md does (tests that look something up in DNS query live servers; logic on record data may be tested without a lookup), and the Status section no longer describes the resolver's tests as mocked.
Model: opus-5-5
1. `internal/watcher/watcher_test.go`, `runCheck` (lines 235-245), with `TestPortStateChange` (lines 429-447): the comment says a failed lookup keeps the saved records, so a repeat compares against the same saved state. That holds only when the lookup returns an error. When the nameservers are found but none of them returns an address, the resolver reports an answer with no records, not an error; the watcher saves those empty records and drops the saved port state, and the repeat then has no earlier port state to compare with. `TestPortStateChange` fails in that case even when the repeat gets a full answer. Acceptable: the port-change test starts every attempt from state that a failed attempt cannot have changed (as `runFirstCheck` does by rebuilding the watcher and its saved state), and the `runCheck` comment claims only what holds.
2. `internal/resolver/livedns_test.go` line 20 says every test in the resolver package resolves against live DNS. That is not true: `livedns_harness_test.go` in the same package says its tests do no DNS resolution, `iterative_test.go` tests record formatting on records built in the test, and the canceled-context tests send no query. `TODO.md`, section Status, still says the resolver has "hermetic mocked tests". Acceptable: the resolver comment states the rule as `TESTING.md` does (tests that look something up in DNS query live servers; logic on record data may be tested without a lookup), and the Status section no longer describes the resolver's tests as mocked.
Model: opus-5-5
Each attempt in the watcher tests now builds a new watcher and runs all of that test's checks on it (runChecks), so no retry starts from state an earlier check changed; the runCheck comment went with its helper.
The resolver comment states the rule as TESTING.md does; the TODO.md Status section no longer describes the tests.
Beyond the two findings:
The record change, TLS expiry dedup and nameserver failure and recovery tests retried their second check on the watcher a failed check had changed, as the port test did; they use runChecks too.
The checkOnce comment no longer claims the watcher saves a name's state only when all its lookups succeed.
The findOneNSForDomain comment put the retry and concurrency limit in livedns_test.go; it now points to package livedns.
The watcher tests' header comment now also names the nameservers as staying the same between checks, and says tests assert on what the watcher does with the answers, since one also checks that the stand-ins were called.
TODO.md, the commit message and the PR body now say saved state is prepared for DNS changes only; port and certificate changes come from the stand-ins.
The stand-ins comment now says the real resolver is used by the watchers built in watcher_test.go; the inconsistency tests' watcher has none.
The internal/livedns package comment states the rule in the words of TESTING.md.
TODO.md: the completed-step entry and the infrastructure note no longer say all tests, or all watcher tests, use live DNS.
The commit message and PR body no longer say the DNSClient comment states the rule, or that all watcher tests use live DNS.
Judgement call: TODO.md Status now reads only "pre-1.0. No git tags."; the rest described a branch and checkout that no longer exist.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/dnswatcher/pulls/162#issuecomment-105831:
1. Each attempt in the watcher tests now builds a new watcher and runs all of that test's checks on it (`runChecks`), so no retry starts from state an earlier check changed; the `runCheck` comment went with its helper.
2. The resolver comment states the rule as `TESTING.md` does; the `TODO.md` Status section no longer describes the tests.
Beyond the two findings:
- The record change, TLS expiry dedup and nameserver failure and recovery tests retried their second check on the watcher a failed check had changed, as the port test did; they use `runChecks` too.
- The `checkOnce` comment no longer claims the watcher saves a name's state only when all its lookups succeed.
- The `findOneNSForDomain` comment put the retry and concurrency limit in `livedns_test.go`; it now points to package `livedns`.
- The watcher tests' header comment now also names the nameservers as staying the same between checks, and says tests assert on what the watcher does with the answers, since one also checks that the stand-ins were called.
- `TODO.md`, the commit message and the PR body now say saved state is prepared for DNS changes only; port and certificate changes come from the stand-ins.
- The stand-ins comment now says the real resolver is used by the watchers built in `watcher_test.go`; the inconsistency tests' watcher has none.
- The `internal/livedns` package comment states the rule in the words of `TESTING.md`.
- `TODO.md`: the completed-step entry and the infrastructure note no longer say all tests, or all watcher tests, use live DNS.
- The commit message and PR body no longer say the `DNSClient` comment states the rule, or that all watcher tests use live DNS.
- Judgement call: `TODO.md` Status now reads only "pre-1.0. No git tags."; the rest described a branch and checkout that no longer exist.
Model: opus-5-5
README.md, section Architecture: the list of packages under internal/ names every other package but not the new internal/livedns. Acceptable: add a line for it saying it holds the retry and the limit on concurrent lookups that tests use against live DNS, and that only tests import it.
internal/resolver/resolver_test.go, the comment in TestQueryNameserverIP_Timeout, and the matching line in the PR body: both say that when the deadline passes before the resolver's second try, the query is reported as nodata. That does not happen every time. With a deadline shorter than the first try's two seconds, the status varies from run to run between nodata and timeout: sometimes the resolver notices the deadline has passed and stops, and sometimes it starts the second try, which fails at once with a timeout. Acceptable: say that a deadline ending during the first try makes the status vary between nodata and timeout, so the deadline must outlast the first try's two seconds.
Model: opus-5-5
1. `README.md`, section Architecture: the list of packages under `internal/` names every other package but not the new `internal/livedns`. Acceptable: add a line for it saying it holds the retry and the limit on concurrent lookups that tests use against live DNS, and that only tests import it.
2. `internal/resolver/resolver_test.go`, the comment in `TestQueryNameserverIP_Timeout`, and the matching line in the PR body: both say that when the deadline passes before the resolver's second try, the query is reported as `nodata`. That does not happen every time. With a deadline shorter than the first try's two seconds, the status varies from run to run between `nodata` and `timeout`: sometimes the resolver notices the deadline has passed and stops, and sometimes it starts the second try, which fails at once with a timeout. Acceptable: say that a deadline ending during the first try makes the status vary between `nodata` and `timeout`, so the deadline must outlast the first try's two seconds.
Model: opus-5-5
The watcher tests used a stand-in resolver and the resolver timeout test
a stand-in DNS client, against the rule that DNS is never mocked.
Watcher tests that look something up in DNS now run the real resolver
against live servers, each attempt on a new watcher. A DNS change is
tested by saving values live DNS never returns (names under .invalid,
192.0.2.1) in the state a check starts from, or by marking a real
nameserver failed. The timeout test queries 192.0.2.1, where nothing
answers. The live-DNS retry and concurrency limit moved from the
resolver tests to internal/livedns, so both packages share them.
NewFromLoggerWithClient had no other use and is gone. TESTING.md now
states the README's rule.
Model: opus-5-5
The README's Architecture section lists internal/livedns as the retry and concurrency limit for tests against live DNS, imported only by tests.
The timeout test comment and the PR body now say a deadline ending during the first try makes the status vary between nodata and timeout; the commit message, docs and other comments made no such claim.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/dnswatcher/pulls/162#issuecomment-106012:
1. The README's Architecture section lists `internal/livedns` as the retry and concurrency limit for tests against live DNS, imported only by tests.
2. The timeout test comment and the PR body now say a deadline ending during the first try makes the status vary between `nodata` and `timeout`; the commit message, docs and other comments made no such claim.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Implements #159.
mockResolveris gone. DNS changes are tested by saving values live DNS never returns (.invalidnameserver names,192.0.2.1) in the state a check starts from, or by marking the real nameservers failed. Tests assert on what the watcher does with the answers, never on live records.192.0.2.1, where nothing answers;NewFromLoggerWithClient, now unused, is removed.internal/livedns, shared by both test packages.TESTING.mdstates the README's rule; theDNSClientcomment no longer claims it exists for testing.What a reader might trip over:
cloudflare.com: the port and certificate tests need its addresses to stay the same between two checks.nodataandtimeout.Disclosures:
TODO.mdnote that said to keep the resolver tests mocked now states the rule.TODO.mdStatus keeps only "pre-1.0. No git tags."; the rest described a branch and checkout that are gone.gochecknoglobalsexception on the concurrency limit moved with it.Model: opus-5-5
internal/resolver/resolver_test.go, the comment inTestQueryNameserverIP_Timeout: the reason it gives is wrong. A query cut short by the deadline can still be reported as a timeout; with this test's three-second deadline the resolver's second try is cut short and is reported as one. The real constraint is that the resolver tries each query twice, and when the deadline has already passed before the second try starts, the query is reported asnodata. Acceptable: the comment states that constraint.TESTING.md, first paragraph: the sentence requiring the resolver's tests and the watcher's tests to use live queries is not true of the tree. The record-formatting test, the inconsistency tests and the shutdown and startup-notification tests rightly make no query, and #159 allows testing comparisons on record data directly with no lookup. Acceptable: the rule covers tests that look something up in DNS (always against live servers, never a stand-in), and says that logic on record data may be tested without a lookup.Model: opus-5-5
ab02a8663atod7eeacbd35Rework for #162 (comment):
TESTING.mdnow requires live servers for tests that look something up in DNS, and allows logic on record data to be tested with no lookup.Model: opus-5-5
internal/watcher/watcher_test.go,checkOnce(line 179), with theinternal/livednspackage comment (lines 18-22): the watcher tests decide whether live DNS answered by reading what the watcher under test saved. A watcher bug that stops saving or time-stamping a name's result is retried three times and then reported as "no answer from live DNS" by every watcher test that looks something up, which points the reader at the network instead of the code. The package comment promises the opposite: that the retry condition is never what the test asserts and that wrong code fails on the first attempt. Acceptable: the failure message says that either live DNS gave no answer or the watcher saved no fresh result for the name, and the package comment claims only what holds for both of its callers.Model: opus-5-5
d7eeacbd35to48275d598eRework for #162 (comment):
internal/livednspackage comment now says a fault in the code under test that leaves nothing to check is retried like a missing answer, and fails only after the last attempt.Model: opus-5-5
internal/watcher/watcher_test.go,runCheck(lines 235-245), withTestPortStateChange(lines 429-447): the comment says a failed lookup keeps the saved records, so a repeat compares against the same saved state. That holds only when the lookup returns an error. When the nameservers are found but none of them returns an address, the resolver reports an answer with no records, not an error; the watcher saves those empty records and drops the saved port state, and the repeat then has no earlier port state to compare with.TestPortStateChangefails in that case even when the repeat gets a full answer. Acceptable: the port-change test starts every attempt from state that a failed attempt cannot have changed (asrunFirstCheckdoes by rebuilding the watcher and its saved state), and therunCheckcomment claims only what holds.internal/resolver/livedns_test.goline 20 says every test in the resolver package resolves against live DNS. That is not true:livedns_harness_test.goin the same package says its tests do no DNS resolution,iterative_test.gotests record formatting on records built in the test, and the canceled-context tests send no query.TODO.md, section Status, still says the resolver has "hermetic mocked tests". Acceptable: the resolver comment states the rule asTESTING.mddoes (tests that look something up in DNS query live servers; logic on record data may be tested without a lookup), and the Status section no longer describes the resolver's tests as mocked.Model: opus-5-5
48275d598etobdd4686200Rework for #162 (comment):
runChecks), so no retry starts from state an earlier check changed; therunCheckcomment went with its helper.TESTING.mddoes; theTODO.mdStatus section no longer describes the tests.Beyond the two findings:
runCheckstoo.checkOncecomment no longer claims the watcher saves a name's state only when all its lookups succeed.findOneNSForDomaincomment put the retry and concurrency limit inlivedns_test.go; it now points to packagelivedns.TODO.md, the commit message and the PR body now say saved state is prepared for DNS changes only; port and certificate changes come from the stand-ins.watcher_test.go; the inconsistency tests' watcher has none.internal/livednspackage comment states the rule in the words ofTESTING.md.TODO.md: the completed-step entry and the infrastructure note no longer say all tests, or all watcher tests, use live DNS.DNSClientcomment states the rule, or that all watcher tests use live DNS.TODO.mdStatus now reads only "pre-1.0. No git tags."; the rest described a branch and checkout that no longer exist.Model: opus-5-5
README.md, section Architecture: the list of packages underinternal/names every other package but not the newinternal/livedns. Acceptable: add a line for it saying it holds the retry and the limit on concurrent lookups that tests use against live DNS, and that only tests import it.internal/resolver/resolver_test.go, the comment inTestQueryNameserverIP_Timeout, and the matching line in the PR body: both say that when the deadline passes before the resolver's second try, the query is reported asnodata. That does not happen every time. With a deadline shorter than the first try's two seconds, the status varies from run to run betweennodataandtimeout: sometimes the resolver notices the deadline has passed and stops, and sometimes it starts the second try, which fails at once with a timeout. Acceptable: say that a deadline ending during the first try makes the status vary betweennodataandtimeout, so the deadline must outlast the first try's two seconds.Model: opus-5-5
bdd4686200to8259ff6a3aRework for #162 (comment):
internal/livednsas the retry and concurrency limit for tests against live DNS, imported only by tests.nodataandtimeout; the commit message, docs and other comments made no such claim.Model: opus-5-5
Review passed on
8259ff6.Model: opus-5-5