internal/notify shutdown tests: misleading failure diagnostic and an overloaded timing constant #116

New Issue

clawbot · 2026-08-09T07:34:11+02:00

clawbot commented

2026-08-09 07:34:11 +02:00

Non-blocking findings from the re-review of PR #113, collected here rather than spent on another review cycle of that PR. None of these affect whether the production code is correct — they affect whether a future failure will be diagnosable.

Applies to internal/notify/shutdown_test.go and internal/notify/shutdown.go as they exist after #113 merges.

Items

1. A failure message that reports the wrong bound. internal/notify/shutdown_test.go:496-502 compares elapsed time against idleDrainBound (500ms) but its failure message prints drainSlack (2s). So a genuine failure at, say, 600ms would print something like "exceeded 2s" while the actual violated bound was 500ms — the message contradicts the assertion, and reads as though the check passed. A test that fails with a misleading diagnostic costs more than one that fails plainly, because it sends whoever is debugging in the wrong direction.

2. drainSlack is overloaded across three distinct meanings. The same constant is doing duty for unrelated timing concepts. Split it into separately named constants, each with a comment stating what it bounds and why that magnitude was chosen. This is what made item 1 easy to write and hard to notice.

3. TestDrainWithCancelledContextDoesNotWarn asserts only an absence. It checks that no WARN was logged — which is also exactly what a trivially-passing or no-op run produces. It was empirically reliable in review (caught the removed guard 10/10 times), so this is latent rather than broken. Strengthen it to assert something positive as well: that the drain returned, and that the expected debug-level line was emitted. An absence-only assertion cannot distinguish "the guard worked" from "nothing ran at all".

Definition of done

The failure message in the idle-drain assertion names the bound it actually checked.
drainSlack is split into distinctly named constants, one per concept, each commented with what it bounds and the reasoning behind its magnitude.
TestDrainWithCancelledContextDoesNotWarn asserts a positive signal in addition to the absence of the warning.
No timing bound is loosened. These constants were tuned in #113 against observed scheduling noise, and one of them (idleDrainBound, 500ms) exists specifically because a 50ms cap flaked at 57.5ms under load. Renaming and re-commenting must not become an excuse to widen anything. If you believe a bound is genuinely wrong, say so in the PR description with evidence rather than quietly changing it.
Prove the tests still fail when they should. For each assertion you touch, temporarily break the corresponding production behaviour, confirm the test fails with a message that now names the right bound, then revert. Confirm git status is clean before committing — no mutation residue. Report what you mutated and what each mutation produced.
Run make test at least 20 consecutive times with the cache bypassed under -race and confirm all are clean. This package has produced two separate flakes already; a single green run is not evidence here.
make check green; TODO.md updated in the same commit.

The finishing commit's title must end with (closes #N) referencing this issue.

Scope

Test-quality only. Do not change the drain's structure, its locking, the sync.Once on abandon, the outstanding-count accounting, or the three-dispatcher consolidation — all of that passed adversarial review and is not in question here.

Deliberately not included, deferred from #113's first review and still deferred: refused notifications are still recorded in AlertHistory (arguably correct, arguably not — needs a decision, not a patch), and ErrDeliveryAbandoned living in shutdown.go rather than the sentinel block at notify.go:32-45. Raise them separately if you think they are worth doing.

Sequencing

Blocked until PR #113 merges — every line referenced above only exists on that branch.

Hard constraints

DNS is never mocked in this repository. Nothing here involves DNS; httptest servers standing in for webhook endpoints are HTTP and remain fine.
Do not modify .golangci.yml; do not change the golangci-lint pin.
make targets and script/ entrypoints only — no direct go test / go clean / golangci-lint invocations.

Non-blocking findings from the re-review of [PR #113](https://git.eeqj.de/sneak/dnswatcher/pulls/113), collected here rather than spent on another review cycle of that PR. None of these affect whether the production code is correct — they affect whether a future failure will be *diagnosable*. Applies to `internal/notify/shutdown_test.go` and `internal/notify/shutdown.go` as they exist after #113 merges. ## Items **1. A failure message that reports the wrong bound.** `internal/notify/shutdown_test.go:496-502` compares elapsed time against `idleDrainBound` (500ms) but its failure message prints `drainSlack` (2s). So a genuine failure at, say, 600ms would print something like "exceeded 2s" while the actual violated bound was 500ms — the message contradicts the assertion, and reads as though the check passed. A test that fails with a misleading diagnostic costs more than one that fails plainly, because it sends whoever is debugging in the wrong direction. **2. `drainSlack` is overloaded across three distinct meanings.** The same constant is doing duty for unrelated timing concepts. Split it into separately named constants, each with a comment stating what it bounds and why that magnitude was chosen. This is what made item 1 easy to write and hard to notice. **3. `TestDrainWithCancelledContextDoesNotWarn` asserts only an absence.** It checks that no WARN was logged — which is also exactly what a trivially-passing or no-op run produces. It was empirically reliable in review (caught the removed guard 10/10 times), so this is latent rather than broken. Strengthen it to assert something positive as well: that the drain returned, and that the expected debug-level line *was* emitted. An absence-only assertion cannot distinguish "the guard worked" from "nothing ran at all". ## Definition of done 1. The failure message in the idle-drain assertion names the bound it actually checked. 2. `drainSlack` is split into distinctly named constants, one per concept, each commented with what it bounds and the reasoning behind its magnitude. 3. `TestDrainWithCancelledContextDoesNotWarn` asserts a positive signal in addition to the absence of the warning. 4. **No timing bound is loosened.** These constants were tuned in #113 against observed scheduling noise, and one of them (`idleDrainBound`, 500ms) exists specifically because a 50ms cap flaked at 57.5ms under load. Renaming and re-commenting must not become an excuse to widen anything. If you believe a bound is genuinely wrong, say so in the PR description with evidence rather than quietly changing it. 5. **Prove the tests still fail when they should.** For each assertion you touch, temporarily break the corresponding production behaviour, confirm the test fails with a message that now names the right bound, then revert. Confirm `git status` is clean before committing — no mutation residue. Report what you mutated and what each mutation produced. 6. Run `make test` at least 20 consecutive times with the cache bypassed under `-race` and confirm all are clean. This package has produced two separate flakes already; a single green run is not evidence here. 7. `make check` green; `TODO.md` updated in the same commit. The finishing commit's title must end with ` (closes #N)` referencing this issue. ## Scope **Test-quality only.** Do not change the drain's structure, its locking, the `sync.Once` on `abandon`, the outstanding-count accounting, or the three-dispatcher consolidation — all of that passed adversarial review and is not in question here. Deliberately **not** included, deferred from #113's first review and still deferred: refused notifications are still recorded in `AlertHistory` (arguably correct, arguably not — needs a decision, not a patch), and `ErrDeliveryAbandoned` living in `shutdown.go` rather than the sentinel block at `notify.go:32-45`. Raise them separately if you think they are worth doing. ## Sequencing **Blocked until PR #113 merges** — every line referenced above only exists on that branch. ## Hard constraints - DNS is never mocked in this repository. Nothing here involves DNS; `httptest` servers standing in for webhook endpoints are HTTP and remain fine. - Do not modify `.golangci.yml`; do not change the golangci-lint pin. - `make` targets and `script/` entrypoints only — no direct `go test` / `go clean` / `golangci-lint` invocations.

clawbot added this to the 1.0 milestone 2026-08-09 07:34:11 +02:00

clawbot referenced this issue

2026-08-09 07:35:34 +02:00

1.0/mvp #66

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/dnswatcher#116