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

make test is served from Go's test cache, so a repeat green proves no DNS was queried #139

New Issue
Open
opened 2026-08-10 15:26:52 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-10 15:26:52 +02:00
Collaborator
Copy Link

Found during the review of #136. Pre-existing and repo-wide, so it was deliberately kept out of that unit.

The problem

script/test does not pass -count=1, so Go serves results from its test cache when nothing has changed. Three consecutive runs on an unchanged tree returned exit 0 with internal/resolver (cached) and no DNS query was made at all.

For a normal repo that is a feature. For this one it is a false green, and a particularly bad one:

  • This repo's entire premise is that the suite exercises real DNS on every run (TESTING.md). A cached pass asserts nothing about live resolution.
  • The suite's whole reason for tolerating nondeterminism is that it hits the network. Caching removes the network, so a repeated-run green — exactly the evidence used to argue a flakiness fix works — is meaningless.
  • It silently defeats the "run it a few times to check for flakes" workflow. The second and subsequent runs are not runs.

This bit the #93 work directly: the implementer's first verification run came back green in about one second entirely from cache. He caught it and forced uncached runs, and the reviewer independently forced them with GOFLAGS=-count=1 — but nothing in the tooling prevents the next person from banking a cached green.

Definition of done

  1. script/test runs tests with caching disabled, so every invocation actually executes. -count=1 is the standard mechanism.
  2. Verify by running make test twice on an unchanged tree and confirming zero (cached) markers and a realistic duration both times.
  3. Confirm the suite still fits the 20-second target from REPO_POLICIES.md once caching is off — this makes every run pay full cost, and if it now exceeds 20s an improvement issue is owed per the owner's ruling at sneak/prompts#41 (comment) ("the hard cap is 60 for ci/green, but over 20s should be filed as an improvement bug"). Report the measured number.
  4. Preserve the conditional-verbose-rerun pattern that REPO_POLICIES.md mandates — the rerun must not reintroduce caching.
  5. make check green.

Commit title ends with (closes #N) for this issue.

Found during the review of https://git.eeqj.de/sneak/dnswatcher/pulls/136. Pre-existing and repo-wide, so it was deliberately kept out of that unit. ## The problem `script/test` does not pass `-count=1`, so Go serves results from its test cache when nothing has changed. Three consecutive runs on an unchanged tree returned exit 0 with `internal/resolver (cached)` and **no DNS query was made at all**. For a normal repo that is a feature. For this one it is a false green, and a particularly bad one: - This repo's entire premise is that the suite exercises **real DNS on every run** (`TESTING.md`). A cached pass asserts nothing about live resolution. - The suite's whole reason for tolerating nondeterminism is that it hits the network. Caching removes the network, so a repeated-run green — exactly the evidence used to argue a flakiness fix works — is meaningless. - It silently defeats the "run it a few times to check for flakes" workflow. The second and subsequent runs are not runs. This bit the https://git.eeqj.de/sneak/dnswatcher/issues/93 work directly: the implementer's first verification run came back green in about one second entirely from cache. He caught it and forced uncached runs, and the reviewer independently forced them with `GOFLAGS=-count=1` — but nothing in the tooling prevents the next person from banking a cached green. ## Definition of done 1. `script/test` runs tests with caching disabled, so every invocation actually executes. `-count=1` is the standard mechanism. 2. Verify by running `make test` twice on an unchanged tree and confirming **zero** `(cached)` markers and a realistic duration both times. 3. Confirm the suite still fits the 20-second target from `REPO_POLICIES.md` once caching is off — this makes every run pay full cost, and if it now exceeds 20s an improvement issue is owed per the owner's ruling at https://git.eeqj.de/sneak/prompts/issues/41#issuecomment-53166 ("the hard cap is 60 for ci/green, but over 20s should be filed as an improvement bug"). Report the measured number. 4. Preserve the conditional-verbose-rerun pattern that `REPO_POLICIES.md` mandates — the rerun must not reintroduce caching. 5. `make check` green. Commit title ends with ` (closes #N)` for this issue.
clawbot commented 2026-08-10 15:49:38 +02:00
Author
Collaborator
Copy Link

Landed on next as 6f6bf3a, in #136.

script/test now passes -count=1. The conditional verbose rerun REPO_POLICIES.md mandates was missing at the same spot (the primary run was unconditionally -v) and is added: quiet first, -v only on failure, -count=1 on both, exit forced to 1 regardless of the rerun's result. -timeout 90s untouched. No carve-out for the Docker build, whose test cache is empty anyway.

Verification, all from a warm cache. Before: 8 of 8 packages (cached), real 0m0.203s, no DNS queried. After, three back-to-back runs on an unchanged tree, zero (cached) in each:

run 1  real 0m4.174s     run 2  real 0m4.015s     run 3  real 0m4.519s
ok  .../internal/resolver  3.005s / 2.846s   (no (cached) markers)

Uncached wall time 4.0-4.5s — inside the 20s target, so no improvement bug is owed. -race and -cover still compose with -count=1; coverage percentages unchanged.

Failure path exercised rather than assumed, against a flaky test that fails once then passes:

exit code: 1
--- FAIL: TestFlaky
--- Rerunning with -v for details ---
--- PASS: TestFlaky
ok      flakeproof  1.014s

The rerun genuinely re-executed (it passed, so it did not replay the cached FAIL) and the build still failed.

make check green, lint stage executed not cached (0 issues., DONE 26.2s).

Landed on `next` as `6f6bf3a`, in https://git.eeqj.de/sneak/dnswatcher/pulls/136. `script/test` now passes `-count=1`. The conditional verbose rerun `REPO_POLICIES.md` mandates was missing at the same spot (the primary run was unconditionally `-v`) and is added: quiet first, `-v` only on failure, `-count=1` on both, exit forced to 1 regardless of the rerun's result. `-timeout 90s` untouched. No carve-out for the Docker build, whose test cache is empty anyway. Verification, all from a **warm** cache. Before: 8 of 8 packages `(cached)`, `real 0m0.203s`, no DNS queried. After, three back-to-back runs on an unchanged tree, zero `(cached)` in each: ``` run 1 real 0m4.174s run 2 real 0m4.015s run 3 real 0m4.519s ok .../internal/resolver 3.005s / 2.846s (no (cached) markers) ``` **Uncached wall time 4.0-4.5s** — inside the 20s target, so no improvement bug is owed. `-race` and `-cover` still compose with `-count=1`; coverage percentages unchanged. Failure path exercised rather than assumed, against a flaky test that fails once then passes: ``` exit code: 1 --- FAIL: TestFlaky --- Rerunning with -v for details --- --- PASS: TestFlaky ok flakeproof 1.014s ``` The rerun genuinely re-executed (it passed, so it did not replay the cached `FAIL`) and the build still failed. `make check` green, lint stage executed not cached (`0 issues.`, `DONE 26.2s`).
clawbot commented 2026-08-10 15:59:55 +02:00
Author
Collaborator
Copy Link

[manager] Landed on next as 6f6bf3a (#136). Review passed. make test uncached 3.7–6.2s, inside the 20s target.

Scope note: the commit also implements the conditional-verbose-rerun pattern, which turned out to be missing entirely rather than at risk. That satisfies #103 — see the note there.

Worth keeping: || true before exit 1 is required, not sloppy. Under set -eu the brace group is the last command of the AND-OR list, so without it the script exits with the rerun's status and never reaches exit 1 — a flake passing on retry would green the build. The REPO_POLICIES.md snippet is a make recipe and does not port to a set -e script unchanged.

**[manager]** Landed on `next` as `6f6bf3a` (https://git.eeqj.de/sneak/dnswatcher/pulls/136). Review passed. `make test` uncached 3.7–6.2s, inside the 20s target. Scope note: the commit also implements the conditional-verbose-rerun pattern, which turned out to be missing entirely rather than at risk. That satisfies https://git.eeqj.de/sneak/dnswatcher/issues/103 — see the note there. Worth keeping: `|| true` before `exit 1` is required, not sloppy. Under `set -eu` the brace group is the last command of the AND-OR list, so without it the script exits with the *rerun's* status and never reaches `exit 1` — a flake passing on retry would green the build. The `REPO_POLICIES.md` snippet is a `make` recipe and does not port to a `set -e` script unchanged.
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 Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/dnswatcher#139
Delete Branch "%!s()"

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?