gosentry/docs/TEST_REVIEW_PLAN.md

Test-suite review — action plan

Working document for the findings of the 2026-08-04 review of the test suite. It is not part of the permanent doc set: delete it once every item below is either done or moved to ROADMAP.md.

The rules the findings were judged against live in STANDARDS.md; the suite itself is described in TESTS.md.

Baseline the review started from

• 172 tests, 4693 lines of test code.
• 84.4% statement coverage across domain, storage, runner, scheduler, and app measured together with -coverpkg (per-package figures understate it, because e.g. domain.NewRuntime is exercised from the app tests).
• TESTS.md documents 171 of the 172 tests.

Overall finding: the suite is not padded. Every test but one carries a real assertion, and most record the property they pin. The items below are the exceptions.

Method note: redundancy was not judged by reading. Each suspected pair was run in isolation with -coverprofile and the profiles compared. "Identical coverage" below means the two profiles were byte-identical after sorting. Identical coverage alone is not grounds for deletion — several kept tests hit the same statements while asserting genuinely different properties. Deletion requires identical coverage and assertions that are a subset.

1. Delete the measured duplicates

Each of these has a byte-identical coverage profile with an existing test whose assertions are a superset. Roughly 30 lines total.

• TestCleanupLogsKeepsFilesWithinAgeLimit (cleanup_test.go:53) — delete. TestCleanupLogsRemovesFilesPastMaxAge already asserts that the file inside the age limit survives.
• TestRunDueEmptyOverlapInheritsGlobal (run_test.go:457) — delete. It builds the same service as TestRunDueQueueRerunsAfterFinish (parallel mode, global queue, a job with an empty OverlapPolicy) and asserts strictly less. Before deleting, move its one unique line — the setup guard svc.jobs[0].OverlapPolicy != "" — into TestRunDueQueueRerunsAfterFinish, so that test still states out loud that it is exercising the inherited policy rather than an explicit one.
• TestSameWindowsPathHandlesSpaces (autostart_windows_test.go:20) — delete. It is the same case as TestSameWindowsPathIgnoresCaseAndQuotes (quoted path, mixed case); sameWindowsPath does not split on spaces, so the space in the fixture reaches no new code. If the spaces case is worth naming, fold the path into the surviving test's fixture instead.

After the deletions, re-run the affected packages and confirm coverage is unchanged:

go test -coverpkg=./src/domain,./src/storage,./src/runner,./src/scheduler,./src/app ./src/domain ./src/storage ./src/runner ./src/scheduler ./src/app

2. Fix the documentation drift

• TESTS.md claims TestLoadOrCreateConfigCreatesDefaultsOnFirstRun verifies that a missing config file is created "with sane defaults and a sample job". The test never touches jobs, and storage.defaultJobs sits at 0% coverage. Decide which half is wrong: either drop the claim from the table, or add the assertion that the seeded jobs.json contains the sample jobs. Adding the assertion is the better outcome — defaultJobs is the only accidental coverage gap the review found.
• TESTS.md does not list TestCancelRowOverlapAddsBackOneInnerPadding (layout_test.go:35). Add it to the src/ui/layout_test.go table.

3. Replace the hand-rolled helper in test code

• seed_test.go:34 defines itoa: 18 lines of digit-by-digit conversion with a fresh allocation per digit, in a file that already imports strconv. Replace the calls with strconv.FormatInt and delete the helper. Untested logic inside a test file is exactly what produces a test result nobody can trust.

4. Thin tests — decide, then act

None of these is wrong; each is close enough to worthless that it should be either justified or removed. Grouped because they want one decision, not four.

• TestEmitWithNoObserversIsNoop (events_test.go:36) — the only test in the suite with no assertion at all. Ranging over a nil slice cannot panic in Go, so it pins nothing. Delete.

• TestStoreReturnsWiredStore (service_test.go:51) — asserts that a one-line getter returns its own field. Delete.

• TestMainViewBuilds (mainwindow_test.go:72) — a smoke test; TestMainViewFitsTheDefaultWindowSize builds the same view. Its only unique coverage is w.SetContent(content) and recordStartup(0, true). Either fold those two calls into the sizing test and delete this one, or keep it and say in its comment that recordStartup is what it is for.

  Done as neither: folding an assertionless `recordStartup` call into the
  sizing test would have put unrelated work inside an F1/F3 regression
  guard. It became `TestMainViewRecordStartupAddsHistoryRow`, which calls
  the closure for both wordings `run.go` selects between and asserts the
  two rows arrive in the History table, read back through the table's own
  cell callbacks so the refresh is proved too. Same unique coverage, plus
  the previously uncovered `!windowShown` branch.
  

• TestFilteredJobIndexesAll / ByNamedFolder / NoFolder / EmptySlice (jobs_view_test.go:59-101) — four tests over one small pure function. Collapse into one table-driven test in the style of TestFilterValue directly above them; the EmptySlice case becomes one row rather than a function.

5. Runtime cost of the runner tests — declined, with measurements

TestRunJobLogFileAllHeaders, TestRunJobRecordFields, and TestRunJobWritesLogFile (runner_test.go) have identical coverage profiles but assert three genuinely different things — log headers, RunRecord field values, and the log file's name and directory. They are not duplicates and should not be deleted on that basis.

• Decided: do not merge them. The premise was wrong. Per-test timings from go test -count=1 -v ./src/runner:

  | Test | Time |
  |---|---|
  | `TestRunJobTimesOut` | 2.10 s |
  | `TestRunJobZeroTimeoutMeansNoTimeout` | 1.05 s |
  | `TestRunJobWritesLogFile` | 0.05 s |
  | `TestRunJobLogFileAllHeaders` | 0.05 s |
  | `TestRunJobRecordFields` | 0.04 s |
  
  The three candidates cost 0.14 s combined, so the merge buys back about
  90 ms. The package's runtime is the two deliberate waits in the timeout
  tests plus build time — subprocess spawn is not what makes `runner` slow.
  Against that, the three fixtures differ in ways the assertions read:
  `TestRunJobWritesLogFile` runs the `Manual` trigger, the other two run
  `Schedule`, and each uses its own job ID and name. Merging forces one
  fixture and drops the `Manual` path from the log-header assertions — the
  exact silent loss this item warned about, for 90 ms.
  
  If `runner` wall time ever does become a problem, the two timeout tests
  are where the seconds are.
  

Explicitly not changing

Recorded here so a later pass does not re-report them:

• TestSetGlobalPausePersistsToConfigFile has the same coverage as TestSetGlobalPauseUpdatesRuntimesAndEmits but asserts a different property — that the flag reaches gosentry.json, which is what makes the pause survive a restart.
• TestRunDueQueueDrainsMultipleOverlaps has the same coverage as TestRunDueQueueRerunsAfterFinish, but drains three queued occurrences rather than one. It is the test that would catch a drain loop that fires once.
• TestCreateStartupShortcutHandlesCyrillicPath and ...HandlesSpaces cover the same statements, but non-ASCII paths and paths with spaces are different real-world failure modes for the WScript.Shell COM call.
• The 0% functions in the non-UI packages are deliberate: the real Clock (a fake is injected everywhere), OpenStore / ResolvePaths / Service.Start / Service.Open and the autostart and desktop-icon wrappers (process entry points and OS integration), and ShouldNotifyOnFailure (a getter under the mutex). storage.defaultJobs is the exception — see item 2.

Suggested order

1. Item 2 (docs) — smallest, and the defaultJobs assertion is the only one that adds coverage.
2. Item 3 (itoa) — independent of everything else.
3. Item 1 (deletions) — one commit, with the coverage re-run as evidence.
4. Item 4 (thin tests) — needs a judgment call per test.
5. Item 5 — measured and declined; see the item.

Items 1 and 4 change the test inventory, so TESTS.md has to be updated in the same commit. No CHANGELOG.md entry is needed: none of this changes shipped behavior.

Which model to use

For running these items in Claude Code. The deciding factor here is not task size — it is that the feedback loop is slow: the ui package needs the MSYS2 UCRT64 toolchain with CGO on, and a cold go test ./src/ui/... took 258 s during the review. A model that gets an edit right on the first pass is worth more than a faster one that needs a second build to find out.

Item	Model	Why
3 — itoa → strconv.FormatInt	Haiku 4.5 (claude-haiku-4-5)	A mechanical substitution in one file, in the runner package, which needs no CGO and runs in ~5 s. Nothing to weigh.
2 — docs, and the defaultJobs assertion	Sonnet 5 (claude-sonnet-5)	Two doc edits plus one new assertion in storage. Reading loadOrCreateJobs to write the assertion is real work, but the answer is not in doubt. No CGO.
1 — the three deletions	Sonnet 5	Deleting is easy; the judgment is narrow and already made in this document (which line to carry over from TestRunDueEmptyOverlapInheritsGlobal, and that identical coverage must be re-verified afterwards). One of the three is in platform/autostart, which is Windows-gated but CGO-free.
4 — the four thin tests	Opus 5 (claude-opus-5)	This is the only item that is genuinely a judgment call rather than an execution task: whether each test should exist at all, and — for TestMainViewBuilds — whether to fold two calls into the sizing test or keep it with a better comment. Two of the four are in ui, so a wrong call costs a 4-minute rebuild to discover.
5 — merging the runner tests	Opus 5, if attempted	It requires holding three distinct sets of assertions and confirming none is silently dropped in the merge. It is also the item most likely to be not worth doing — a model that will say so is the point.

Two notes on this table:

• Sonnet 5 is the reasonable single choice if you would rather not switch models per item. It is near-Opus on coding and agentic work, and only item 4 really rewards the step up. The introductory pricing through 2026-08-31 ($2/$10 per MTok vs $3/$15) makes it cheaper than usual relative to Opus 5's $5/$25.
• Fast mode is available on Opus 5 (toggle with /fast). It is the same model with higher output throughput, not a downgrade — but it bills at $10/$50, so it only pays for itself when you are waiting on the output. Given that the actual wait here is the Fyne build rather than token generation, it is unlikely to help on this plan.