diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index b086257..c840fd7 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -104,6 +104,24 @@ dragged.** split in one pass during the next whole-project review, since six separate passes would settle the same seam question six ways. +**Tests:** + +- Three tests with byte-identical coverage to an existing test and no unique + assertion are gone: `TestCleanupLogsKeepsFilesWithinAgeLimit`, + `TestRunDueEmptyOverlapInheritsGlobal` (its one unique setup guard moved into + `TestRunDueQueueRerunsAfterFinish`), and `TestSameWindowsPathHandlesSpaces` + (its spaces case folded into `TestSameWindowsPathIgnoresCaseAndQuotes`'s + fixture). +- `storage.defaultJobs` — the one accidental 0%-coverage gap the review + found — is now exercised by + `TestLoadOrCreateJobsSeedsSampleJobsOnFirstRun`, which also corrects + `docs/TESTS.md`: `TestLoadOrCreateConfigCreatesDefaultsOnFirstRun` never + touched jobs, so the "and a sample job" half of its old description was + wrong. +- `src/runner/seed_test.go`'s hand-rolled `itoa` — 18 lines of digit-by-digit + conversion in a file that already imports `strconv` — is replaced with + `strconv.FormatInt`. + ## 0.15.0 - 2026-07-26 **Settings points at the jobs file itself, not the folder holding it.** diff --git a/docs/TESTS.md b/docs/TESTS.md index 56159e1..8261a68 100644 --- a/docs/TESTS.md +++ b/docs/TESTS.md @@ -175,11 +175,10 @@ and scheduler edge cases using injected `runJob` and `primeDue`. | `TestRunDueParallelStartsAllDueJobs` | Parallel mode: both due jobs enter the runner before either completes. | | `TestRunDueSequentialSerializes` | Sequential mode: job 2 waits until job 1 finishes. | | `TestRunDueSkipDropsOverlap` | Global skip: no second concurrent run, `PendingRuns` stays 0. | -| `TestRunDueQueueRerunsAfterFinish` | Queue: one deferred run after an in-flight finish. | +| `TestRunDueQueueRerunsAfterFinish` | Queue: one deferred run after an in-flight finish; also covers an empty per-job policy inheriting the global default. | | `TestRunDueQueueDrainsMultipleOverlaps` | Queue: multiple missed ticks drain as separate runs. | | `TestRunDuePerJobQueueOverridesGlobalSkip` | Per-job `queue` beats global `skip`. | | `TestRunDuePerJobSkipOverridesGlobalQueue` | Per-job `skip` beats global `queue`. | -| `TestRunDueEmptyOverlapInheritsGlobal` | Empty per-job policy inherits the global default. | | `TestRunNowSequentialGuard` | Manual run refused while another job runs in sequential mode. | | `TestStartRunLockedRollbackOnSaveFailure` | Regression: run does not start when `SaveJobs` fails. | | `TestRunDueQueueDrainSkippedWhenPaused` | Queued overlaps are not drained while the scheduler is paused. | @@ -234,7 +233,8 @@ Tests JSON round-tripping, default generation, and backward compatibility. | `TestJobsRoundTrip` | Verifies that jobs saved to JSON are reloaded with identical field values. | | `TestConfigRoundTrip` | Verifies that settings saved to JSON are reloaded with identical field values. | | `TestNormalizeJobsFillsDefaults` | Verifies that `normalizeJobs` assigns sequential IDs and sets default name, schedule, and command for jobs missing those fields. | -| `TestLoadOrCreateConfigCreatesDefaultsOnFirstRun` | Verifies that a missing config file is created with sane defaults and a sample job. | +| `TestLoadOrCreateConfigCreatesDefaultsOnFirstRun` | Verifies that a missing config file is created with sane defaults. | +| `TestLoadOrCreateJobsSeedsSampleJobsOnFirstRun` | Verifies that a missing jobs file is created with the sample jobs from `defaultJobs`. | | `TestLoadOrCreateConfigKeepsZeroTimeoutOnReload` | Verifies that `default_timeout_seconds: 0` survives a reload rather than being normalized away — 0 is a value, not a missing field. | | `TestLoadOrCreateConfigMigratesJobsDir` | Verifies that a pre-0.15 `jobs_dir` becomes `jobs_file` pointing at the same `jobs.json`, and that the retired key is not written back. | | `TestLoadOrCreateConfigMigratesLegacyThemeDefault` | Verifies that a config storing the retired `"default"` theme value is normalized to `system` on load. | @@ -355,8 +355,7 @@ Tests log-file cleanup by age and by count. | Test | Purpose | |------|---------| | `TestCleanupLogsMissingDirReturnsNil` | Verifies that cleanup returns nil (not an error) when the logs directory does not exist. | -| `TestCleanupLogsRemovesFilesPastMaxAge` | Verifies that `.log` files older than `MaxLogAgeDays` are deleted. | -| `TestCleanupLogsKeepsFilesWithinAgeLimit` | Verifies that `.log` files within the age limit are retained. | +| `TestCleanupLogsRemovesFilesPastMaxAge` | Verifies that `.log` files older than `MaxLogAgeDays` are deleted and files within the limit are retained. | | `TestCleanupLogsByCountDeletesOldest` | Verifies that when file count exceeds `MaxLogFiles`, the oldest files are removed first. | | `TestCleanupLogsNonLogFilesNotDeleted` | Verifies that non-`.log` files in the logs directory are never deleted by cleanup. | | `TestCleanupLogsSubdirsNotDeleted` | Verifies that subdirectories inside the logs directory are not deleted by cleanup. | @@ -373,8 +372,7 @@ Tests Windows autostart via shortcuts in the Startup folder. | Test | Purpose | |------|---------| -| `TestSameWindowsPathIgnoresCaseAndQuotes` | Verifies that Windows path comparison is case-insensitive and handles quote marks correctly. | -| `TestSameWindowsPathHandlesSpaces` | Verifies that Windows path comparison matches paths with and without surrounding quotes. | +| `TestSameWindowsPathIgnoresCaseAndQuotes` | Verifies that Windows path comparison is case-insensitive, handles quote marks, and matches paths containing spaces. | | `TestSameWindowsPathStripsExtendedLengthPrefix` | Verifies that `\\?\`-prefixed paths are compared correctly after stripping the prefix. | | `TestSameWindowsPathMatchesShortNameViaFilesystem` | Verifies that 8.3 short names are resolved to long names for comparison. | | `TestStartupShortcutPathUsesUserStartupFolder` | Verifies that the shortcut path resolves into `%APPDATA%\Microsoft\Windows\Start Menu\Programs\Startup`. | @@ -501,6 +499,7 @@ Tests the theme-derived sizing helpers in `layout.go`. | Test | Purpose | |------|---------| | `TestRowOverlapMatchesInnerPadding` | Pins `rowOverlap` to `-theme.InnerPadding()` under two themes, the property that lets it follow a theme instead of drifting from a hand-tuned literal. | +| `TestCancelRowOverlapAddsBackOneInnerPadding` | Verifies that `cancelRowOverlap` adds back exactly one inner padding on the top edge only, leaving width and the row below unaffected. | | `TestCaptionColumnWidth` | Covers no captions, one, and several of varying length, at two text sizes. | --- diff --git a/docs/TEST_REVIEW_PLAN.md b/docs/TEST_REVIEW_PLAN.md index 85e21f6..4be6659 100644 --- a/docs/TEST_REVIEW_PLAN.md +++ b/docs/TEST_REVIEW_PLAN.md @@ -31,11 +31,11 @@ requires identical coverage **and** assertions that are a subset. Each of these has a byte-identical coverage profile with an existing test whose assertions are a superset. Roughly 30 lines total. -- [ ] `TestCleanupLogsKeepsFilesWithinAgeLimit` +- [x] `TestCleanupLogsKeepsFilesWithinAgeLimit` ([cleanup_test.go:53](../src/runner/cleanup_test.go)) — delete. `TestCleanupLogsRemovesFilesPastMaxAge` already asserts that the file inside the age limit survives. -- [ ] `TestRunDueEmptyOverlapInheritsGlobal` +- [x] `TestRunDueEmptyOverlapInheritsGlobal` ([run_test.go:457](../src/app/run_test.go)) — delete. It builds the same service as `TestRunDueQueueRerunsAfterFinish` (parallel mode, global `queue`, a job with an empty `OverlapPolicy`) and asserts strictly less. @@ -43,7 +43,7 @@ assertions are a superset. Roughly 30 lines total. `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` +- [x] `TestSameWindowsPathHandlesSpaces` ([autostart_windows_test.go:20](../src/platform/autostart/autostart_windows_test.go)) — delete. It is the same case as `TestSameWindowsPathIgnoresCaseAndQuotes` (quoted path, mixed case); `sameWindowsPath` does not split on spaces, so @@ -59,21 +59,21 @@ go test -coverpkg=./src/domain,./src/storage,./src/runner,./src/scheduler,./src/ ## 2. Fix the documentation drift -- [ ] [TESTS.md](TESTS.md) claims `TestLoadOrCreateConfigCreatesDefaultsOnFirstRun` +- [x] [TESTS.md](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](TESTS.md) does not list +- [x] [TESTS.md](TESTS.md) does not list `TestCancelRowOverlapAddsBackOneInnerPadding` ([layout_test.go:35](../src/ui/layout_test.go)). Add it to the `src/ui/layout_test.go` table. ## 3. Replace the hand-rolled helper in test code -- [ ] [seed_test.go:34](../src/runner/seed_test.go) defines `itoa`: 18 lines of +- [x] [seed_test.go:34](../src/runner/seed_test.go) 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 diff --git a/src/app/run_test.go b/src/app/run_test.go index bbc2d51..a0c064b 100644 --- a/src/app/run_test.go +++ b/src/app/run_test.go @@ -255,6 +255,10 @@ func TestRunDueQueueRerunsAfterFinish(t *testing.T) { svc := newQueueService(t, domain.ExecutionModeParallel, domain.OverlapPolicyQueue, []domain.Job{ {ID: 1, Name: "A", Schedule: "@every 1h", Command: "echo", Enabled: true}, }) + // Empty per-job OverlapPolicy inherits the global queue policy. + if svc.jobs[0].OverlapPolicy != "" { + t.Fatalf("test setup: job OverlapPolicy = %q, want empty (inherit)", svc.jobs[0].OverlapPolicy) + } entered := make(chan int, 2) release := make(chan struct{}) @@ -451,51 +455,6 @@ func TestRunDuePerJobSkipOverridesGlobalQueue(t *testing.T) { } } -// TestRunDueEmptyOverlapInheritsGlobal verifies that a job with no own policy -// inherits the global default: with global "queue" and an empty Job.OverlapPolicy -// the job queues a re-run. -func TestRunDueEmptyOverlapInheritsGlobal(t *testing.T) { - svc := newQueueService(t, domain.ExecutionModeParallel, domain.OverlapPolicyQueue, []domain.Job{ - {ID: 1, Name: "A", Schedule: "@every 1h", Command: "echo", Enabled: true}, - }) - if svc.jobs[0].OverlapPolicy != "" { - t.Fatalf("test setup: job OverlapPolicy = %q, want empty (inherit)", svc.jobs[0].OverlapPolicy) - } - - entered := make(chan int, 2) - release := make(chan struct{}) - svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string, _ time.Duration) (domain.RunRecord, error) { - entered <- job.ID - <-release - return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"}, nil - } - done := completions(svc) - - primeDue(t, svc, 1) - svc.RunDue(time.Now()) - if id := <-entered; id != 1 { - t.Fatalf("started job = %d, want 1", id) - } - - primeDue(t, svc, 1) - svc.RunDue(time.Now()) - expectNoEntry(t, entered) - - svc.mu.Lock() - pending := svc.runtimes[1].PendingRuns - svc.mu.Unlock() - if pending != 1 { - t.Fatalf("empty per-job policy must inherit global queue, PendingRuns = %d", pending) - } - - close(release) - waitRecord(t, done) - if id := <-entered; id != 1 { - t.Fatalf("inherited-queue re-run job = %d, want 1", id) - } - waitRecord(t, done) -} - // TestRunNowSequentialGuard verifies the sequential-mode guard in RunNow: a manual // run is refused while another job is running, and allowed once nothing is. func TestRunNowSequentialGuard(t *testing.T) { diff --git a/src/platform/autostart/autostart_windows_test.go b/src/platform/autostart/autostart_windows_test.go index 4bfd23c..5992bd6 100644 --- a/src/platform/autostart/autostart_windows_test.go +++ b/src/platform/autostart/autostart_windows_test.go @@ -12,14 +12,8 @@ import ( ) func TestSameWindowsPathIgnoresCaseAndQuotes(t *testing.T) { - if !sameWindowsPath(`"D:\Apps\GoSentry\gosentry.exe"`, `d:\apps\gosentry\gosentry.exe`) { - t.Fatal("expected paths to match") - } -} - -func TestSameWindowsPathHandlesSpaces(t *testing.T) { if !sameWindowsPath(`"D:\Local Git\GoSentry\gosentry.exe"`, `d:\local git\gosentry\gosentry.exe`) { - t.Fatal("expected paths with spaces to match") + t.Fatal("expected paths to match") } } diff --git a/src/runner/cleanup_test.go b/src/runner/cleanup_test.go index b55bc01..58b3add 100644 --- a/src/runner/cleanup_test.go +++ b/src/runner/cleanup_test.go @@ -50,22 +50,6 @@ func TestCleanupLogsRemovesFilesPastMaxAge(t *testing.T) { } } -func TestCleanupLogsKeepsFilesWithinAgeLimit(t *testing.T) { - dir := t.TempDir() - for i := 1; i <= 3; i++ { - path := writeLogFile(t, dir, fmt.Sprintf("job_%d.log", i)) - setModTime(t, path, time.Duration(i)*24*time.Hour) - } - - if err := CleanupLogs(dir, 100, 30); err != nil { - t.Fatal(err) - } - entries, _ := os.ReadDir(dir) - if len(entries) != 3 { - t.Errorf("expected 3 files kept within age limit, got %d", len(entries)) - } -} - // TestCleanupLogsByCountDeletesOldest verifies the count-based policy: when more // than maxFiles log files exist the oldest (by modification time) are removed. // maxAgeDays=0 disables age-based cleanup so the test exercises count only. diff --git a/src/runner/seed_test.go b/src/runner/seed_test.go index 0ff2123..7d92919 100644 --- a/src/runner/seed_test.go +++ b/src/runner/seed_test.go @@ -22,7 +22,7 @@ func writeTestLog(t *testing.T, dir, filename, state string, durationMS int64, j content.WriteString("\n") } if durationMS >= 0 { - content.WriteString("state: " + state + "\nduration: " + itoa(durationMS) + "\n\n") + content.WriteString("state: " + state + "\nduration: " + strconv.FormatInt(durationMS, 10) + "\n\n") } else { content.WriteString("state: " + state + "\n\n") } @@ -31,25 +31,6 @@ func writeTestLog(t *testing.T, dir, filename, state string, durationMS int64, j } } -func itoa(n int64) string { - if n == 0 { - return "0" - } - neg := n < 0 - if neg { - n = -n - } - buf := make([]byte, 0, 20) - for n > 0 { - buf = append([]byte{byte('0' + n%10)}, buf...) - n /= 10 - } - if neg { - buf = append([]byte{'-'}, buf...) - } - return string(buf) -} - func TestSeedStatsBasic(t *testing.T) { dir := t.TempDir() job := domain.Job{ID: 1, Name: "Build"} diff --git a/src/storage/store_test.go b/src/storage/store_test.go index a07b1f4..4ad188b 100644 --- a/src/storage/store_test.go +++ b/src/storage/store_test.go @@ -183,6 +183,41 @@ func TestLoadOrCreateConfigCreatesDefaultsOnFirstRun(t *testing.T) { } } +// TestLoadOrCreateJobsSeedsSampleJobsOnFirstRun verifies that a missing +// jobs.json is created with the sample jobs from defaultJobs, so a new user +// sees scheduled and manual execution without inventing a command. +func TestLoadOrCreateJobsSeedsSampleJobsOnFirstRun(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "jobs.json") + + got, err := loadOrCreateJobs(path) + if err != nil { + t.Fatal(err) + } + want := defaultJobs() + if len(got) != len(want) { + t.Fatalf("got %d jobs, want %d", len(got), len(want)) + } + for i := range want { + if got[i].Name != want[i].Name || got[i].Schedule != want[i].Schedule || got[i].Command != want[i].Command || got[i].Enabled != want[i].Enabled { + t.Errorf("job %d = %+v, want %+v", i, got[i], want[i]) + } + } + + // The function must have written the seeded jobs to jobs.json. + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("jobs.json should have been created: %v", err) + } + var file domain.JobsFile + if err := json.Unmarshal(data, &file); err != nil { + t.Fatal(err) + } + if len(file.Jobs) != len(want) { + t.Errorf("jobs.json has %d jobs, want %d", len(file.Jobs), len(want)) + } +} + // TestLoadOrCreateConfigMigratesLegacyThemeDefault covers a gosentry.json that // still stores the retired "default" theme value: load normalizes it to system. func TestLoadOrCreateConfigMigratesLegacyThemeDefault(t *testing.T) {