test: rework live-DNS quorum unit — tolerate silence, never a wrong answer
All checks were successful
check / check (push) Successful in 1m23s
All checks were successful
check / check (push) Successful in 1m23s
Rework of the unit at #93 (commit9cb2c2b), against the review at #136 (comment). Review of9cb2c2bfound the quorum assertions could not fail on a class of wrong answer. Each test banned exactly one bad status — _AllReturnOK banned only nxdomain, _NXDomainFromAllNS banned only ok — so resolver.StatusNoData passed both. nodata is a wrong answer, not silence, and answeredCount counted it as answered, so it did not even trigger a retry; with a quorum of 3 of 4 a single wrong nameserver slid through undetected. That is assertion-loosening beyond what the quorum change requires. Tolerance is now a closed allowlist rather than a blocklist of one status. unsanctionedStatuses() reports every per-nameserver result whose status the caller did not explicitly sanction: ok/timeout/error for the all-OK test, nxdomain/timeout/error for the NXDOMAIN test. Silence (timeout, error) is the only thing quorum exists to tolerate; any other status, including one added to the resolver later, fails by name. answeredCount is likewise an allowlist of ok/nxdomain/nodata, so an unknown status counts as silence and can only cause a retry and then a loud failure, never a quiet pass. Two harness tests cover the regression directly: three OK plus one nodata (quorum satisfied, no nxdomain present — the input that used to pass) is now reported as unsanctioned, and an unknown status is neither counted as answered nor tolerated. Verified by re-running the reviewer's probe: queryEachNS patched to force one of google.com's four nameservers to return StatusNoData turns both tests red, naming the offending nameserver and status — --- FAIL: TestQueryAllNameservers_AllReturnOK (1.12s) Should be empty, but was [ns1.google.com.=nodata] every nameserver must answer OK or not answer at all: ns1.google.com.=nodata ns2.google.com.=ok ns3.google.com.=ok ns4.google.com.=ok --- FAIL: TestQueryAllNameservers_NXDomainFromAllNS (1.34s) Should be empty, but was [ns1.google.com.=nodata] every nameserver must report NXDOMAIN or not answer at all: ns1.google.com.=nodata ns2.google.com.=nxdomain ns3.google.com.=nxdomain ns4.google.com.=nxdomain — and green with the probe reverted. Also fixes the review's nit: the per-attempt deadline assertion had no lower bound, so it passed for a deadline far shorter than intended. No production code changed; DNS is still never mocked.
This commit is contained in:
@@ -324,12 +324,21 @@ func TestQueryAllNameservers_AllReturnOK(t *testing.T) {
|
||||
describeStatuses(results),
|
||||
)
|
||||
|
||||
// Any nameserver claiming google.com does not exist is a
|
||||
// real failure and is never tolerated.
|
||||
assert.Zero(
|
||||
// Quorum tolerates SILENCE only. Every individual result must
|
||||
// be either the expected answer or a non-answer: ok, timeout
|
||||
// or error, and nothing else. Stated as a closed allowlist so
|
||||
// that a wrong answer no one thought to ban — nxdomain and
|
||||
// nodata today, any status added later — fails here rather
|
||||
// than sliding through under the quorum.
|
||||
assert.Empty(
|
||||
t,
|
||||
countStatus(results, resolver.StatusNXDomain),
|
||||
"no nameserver should report NXDOMAIN: %s",
|
||||
unsanctionedStatuses(
|
||||
results,
|
||||
resolver.StatusOK,
|
||||
resolver.StatusTimeout,
|
||||
resolver.StatusError,
|
||||
),
|
||||
"every nameserver must answer OK or not answer at all: %s",
|
||||
describeStatuses(results),
|
||||
)
|
||||
}
|
||||
@@ -352,12 +361,20 @@ func TestQueryAllNameservers_NXDomainFromAllNS(
|
||||
describeStatuses(results),
|
||||
)
|
||||
|
||||
// Silence is tolerated; a positive answer for a name that
|
||||
// does not exist is not.
|
||||
assert.Zero(
|
||||
// Silence is tolerated; any actual answer other than NXDOMAIN
|
||||
// is not. Closed allowlist for the same reason as above: a
|
||||
// server answering `ok` or `nodata` for a name that must not
|
||||
// exist is a wrong answer, not a slow one.
|
||||
assert.Empty(
|
||||
t,
|
||||
countStatus(results, resolver.StatusOK),
|
||||
"no nameserver should answer OK: %s",
|
||||
unsanctionedStatuses(
|
||||
results,
|
||||
resolver.StatusNXDomain,
|
||||
resolver.StatusTimeout,
|
||||
resolver.StatusError,
|
||||
),
|
||||
"every nameserver must report NXDOMAIN or not answer "+
|
||||
"at all: %s",
|
||||
describeStatuses(results),
|
||||
)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user