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

New Issue

clawbot · 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

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/sfdupes#33