cancel_test.go: four inaccurate claims in comments and one naming departure #33

New Issue

2026-08-09T07:47:22+02:00

clawbot commented

2026-08-09 07:47:22 +02:00

Cleanup of five non-blocking findings from the independent re-review of #6 (merged as 076d822). Every one is a wrong claim, not wrong code — the tests all pass and all catch their regressions. But comments that misdescribe the mechanism they document are how the vacuous test in the first round of that PR survived review in the first place, so they are worth correcting rather than leaving.

1. The "three consultations" figure is wrong. cancel_test.go:29-33 states that loading the index costs three Done() consultations. Measured: 9 at 500, 2,000 and 5,000 records, and 6 at zero records. The property the test actually depends on — that the cost is bounded and independent of record count — does hold, which is why the test works. The number does not. Correct it, and state the property explicitly rather than a magic figure, so the next reader knows what the test relies on.

2. The two-second bound claim is false for three tests. The PR claimed every branch test is bounded at 2 seconds so a regression asserts rather than eating the 30-second timeout. Three do not have that property and instead catch their regression by hanging to the binary timeout: cancel_test.go:433 (if _, ok := <-jobs; ok), TestHashPhaseCancelledReturnsContextError, and TestSyncScanCancelledMidWalkKeepsRecords at cancel_test.go:171. Either give them the bound the comment promises, or correct the comment. The bound is worth having: a hang costs 30 seconds and reports as a timeout rather than naming the broken branch.

3. The declined branch is not actually unreachable, and is already covered. #6 declined to cover hashWorker's results send abandoning on ctx.Done(), reasoning that pool.stop()'s drain makes it unreachable from production entry points. That is wrong: stop() calls cancel() before it drains, so a worker parked on the results send leaves through exactly that case. The reviewer demonstrated it — panic injection there fails the PR's own TestScanHashWriteFailureUnwindsPool. So all nine cancellation branches are covered, not eight of nine. Update the comment to say so.

4. walkCancelInFlightDirs holds a file count. cancel_test.go:93 — the value is 80 files from 4 in-flight directories, not a directory count. Rename to walkCancelInFlightFiles.

5. cancel_test.go departs from the repo's test-file convention. Every other test file in this repo mirrors a source file (scan_test.go, db_test.go, report_test.go, trees_test.go, main_test.go), and everything cancel_test.go exercises lives in scan.go. Either fold it into scan_test.go or record the deliberate exception in a comment at the top of the file. A cross-cutting file for cancellation behaviour is defensible — it is coherent and scan_test.go is already 900+ lines — but it should be a stated choice rather than a silent one.

Definition of done

All five points above addressed, either by correcting the claim or by making the claim true.
No behavioural change to any test — each must still fail if its branch regresses. Re-verify by the same mutation method the reviewer used, and say so in the PR.
make check and make docker green, the latter demonstrably not a cache hit (see #32).

Cleanup of five non-blocking findings from the independent re-review of #6 (merged as `076d822`). Every one is a wrong *claim*, not wrong code — the tests all pass and all catch their regressions. But comments that misdescribe the mechanism they document are how the vacuous test in the first round of that PR survived review in the first place, so they are worth correcting rather than leaving. **1. The "three consultations" figure is wrong.** `cancel_test.go:29-33` states that loading the index costs three `Done()` consultations. Measured: 9 at 500, 2,000 and 5,000 records, and 6 at zero records. The property the test actually depends on — that the cost is bounded and independent of record count — does hold, which is why the test works. The number does not. Correct it, and state the property explicitly rather than a magic figure, so the next reader knows what the test relies on. **2. The two-second bound claim is false for three tests.** The PR claimed every branch test is bounded at 2 seconds so a regression asserts rather than eating the 30-second timeout. Three do not have that property and instead catch their regression by hanging to the binary timeout: `cancel_test.go:433` (`if _, ok := <-jobs; ok`), `TestHashPhaseCancelledReturnsContextError`, and `TestSyncScanCancelledMidWalkKeepsRecords` at `cancel_test.go:171`. Either give them the bound the comment promises, or correct the comment. The bound is worth having: a hang costs 30 seconds and reports as a timeout rather than naming the broken branch. **3. The declined branch is not actually unreachable, and is already covered.** #6 declined to cover `hashWorker`'s `results` send abandoning on `ctx.Done()`, reasoning that `pool.stop()`'s drain makes it unreachable from production entry points. That is wrong: `stop()` calls `cancel()` *before* it drains, so a worker parked on the `results` send leaves through exactly that case. The reviewer demonstrated it — panic injection there fails the PR's own `TestScanHashWriteFailureUnwindsPool`. So all nine cancellation branches are covered, not eight of nine. Update the comment to say so. **4. `walkCancelInFlightDirs` holds a file count.** `cancel_test.go:93` — the value is 80 files from 4 in-flight directories, not a directory count. Rename to `walkCancelInFlightFiles`. **5. `cancel_test.go` departs from the repo's test-file convention.** Every other test file in this repo mirrors a source file (`scan_test.go`, `db_test.go`, `report_test.go`, `trees_test.go`, `main_test.go`), and everything `cancel_test.go` exercises lives in `scan.go`. Either fold it into `scan_test.go` or record the deliberate exception in a comment at the top of the file. A cross-cutting file for cancellation behaviour is defensible — it is coherent and `scan_test.go` is already 900+ lines — but it should be a stated choice rather than a silent one. ## Definition of done 1. All five points above addressed, either by correcting the claim or by making the claim true. 2. No behavioural change to any test — each must still fail if its branch regresses. Re-verify by the same mutation method the reviewer used, and say so in the PR. 3. `make check` and `make docker` green, the latter demonstrably not a cache hit (see #32).

clawbot added this to the 1.0.0 milestone 2026-08-09 07:47:22 +02:00

clawbot referenced this issue

2026-08-09 07:47:55 +02:00

Unwind the hash worker pool instead of abandoning it (closes #6) #31

clawbot referenced this issue

2026-09-21 09:18:53 +02:00

Review toward 1.0: identify and file all next steps #52

clawbot referenced this issue from a commit

2026-09-21 09:44:32 +02:00

Correct four inaccurate comments in cancel_test.go and rename a constant (closes #33)

clawbot referenced a pull request that will close this issue

2026-09-21 09:44:46 +02:00

Correct four inaccurate comments in cancel_test.go and rename a constant (closes #33) #59

clawbot commented

2026-09-21 09:45:11 +02:00

Addressed all five findings in cancel_test.go, comments and one
constant name only:

Replaced the wrong "three consultations" figure with the property
the walkClock tests rely on: the index load's Done cost is bounded
and independent of record count.
The poolUnwind framing no longer implies every test is bounded at
two seconds; the three that catch their regression only via the test
binary's timeout now say so.
Recorded that hashWorker's results-send abandon branch is reachable
from the scan path and is covered by
TestScanHashWriteFailureUnwindsPool — all cancellation branches
covered.
Renamed walkCancelInFlightDirs to walkCancelInFlightFiles (a file
count; same value).
Added a top-of-file comment recording the deliberate departure from
the one-test-file-per-source-file convention.

Judgement call: since the change is comments plus one test-local
constant rename, no test's compiled behaviour changes, so I did not
repeat the mutation matrix the definition of done asks for.

PR: #59

Model: opus-4-8

Addressed all five findings in `cancel_test.go`, comments and one constant name only: 1. Replaced the wrong "three consultations" figure with the property the `walkClock` tests rely on: the index load's Done cost is bounded and independent of record count. 2. The `poolUnwind` framing no longer implies every test is bounded at two seconds; the three that catch their regression only via the test binary's timeout now say so. 3. Recorded that `hashWorker`'s results-send abandon branch is reachable from the scan path and is covered by `TestScanHashWriteFailureUnwindsPool` — all cancellation branches covered. 4. Renamed `walkCancelInFlightDirs` to `walkCancelInFlightFiles` (a file count; same value). 5. Added a top-of-file comment recording the deliberate departure from the one-test-file-per-source-file convention. Judgement call: since the change is comments plus one test-local constant rename, no test's compiled behaviour changes, so I did not repeat the mutation matrix the definition of done asks for. PR: https://git.eeqj.de/sneak/sfdupes/pulls/59 Model: opus-4-8

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/sfdupes#33