diff --git a/docs/TESTS.md b/docs/TESTS.md index 8261a68..89054c0 100644 --- a/docs/TESTS.md +++ b/docs/TESTS.md @@ -102,7 +102,6 @@ Tests `Service` construction and the state-accessor contract. |------|---------| | `TestNewServiceBuildsRuntimePerJob` | Verifies that `NewService` creates a `JobRuntime` entry for every loaded job. | | `TestJobsReturnsCopy` | Verifies that `Service.Jobs` returns a defensive copy so callers cannot mutate internal state. | -| `TestStoreReturnsWiredStore` | Verifies that `Service.Store` returns the injected `storage.Store`. | --- @@ -195,7 +194,6 @@ Tests the event-emission and observer-subscription machinery. | Test | Purpose | |------|---------| | `TestEmitDeliversToAllObserversInOrder` | Verifies that all registered observers receive emitted events in registration order. | -| `TestEmitWithNoObserversIsNoop` | Verifies that emitting an event with no observers does not panic. | | `TestObserverCanReadServiceState` | Verifies that an observer called by `emit` can safely read Service state (jobs, runtimes). | --- @@ -436,10 +434,7 @@ widgets are assembled. | `TestFilterValue` | Verifies that `filterValue` returns the correct display string for the current folder filter. | | `TestFolderOptionsAlwaysIncludesSentinels` | Verifies that the folder filter list always starts with "All" and "No folder" sentinel entries. | | `TestFolderOptionsAppendsUniqueFolders` | Verifies that folder names from the job list are appended once each, in order, without duplicates. | -| `TestFilteredJobIndexesAll` | Verifies that the "All" filter returns indexes for every job. | -| `TestFilteredJobIndexesByNamedFolder` | Verifies that filtering by a named folder returns only jobs in that folder. | -| `TestFilteredJobIndexesNoFolder` | Verifies that the "No folder" filter returns only jobs with an empty folder field. | -| `TestFilteredJobIndexesEmptySlice` | Verifies that filtering an empty job slice returns an empty index list. | +| `TestFilteredJobIndexes` | Table: verifies the "All" filter returns every index, a named folder returns only its own jobs, "No folder" matches empty and blank folder fields, and an empty job list yields no indexes. | | `TestNextJobListViewFlipsBothWays` | Verifies the density toggle alternates between detailed and compact from either starting value. | | `TestViewToggleTextNamesTheAction` | Verifies the toggle button is labelled with the action it performs, not the state it is in. | | `TestJobListViewToggleShrinksRowsAndPersists` | End-to-end: one tap shrinks the row height, relabels the button, and reaches the config; tapping back undoes all three. | @@ -528,7 +523,7 @@ Tests main view construction with an injected `*app.Service`. | Test | Purpose | |------|---------| | `TestMainViewFitsTheDefaultWindowSize` | Verifies the assembled content's minimum fits the window size the app asks for, so Fyne never silently widens the window past it. The store's config path is deliberately long, since it was the path label that used to grow the Settings tab. | -| `TestMainViewBuilds` | Verifies `newMainView` assembles tabs without panic using `fyne.io/fyne/v2/test`. | +| `TestMainViewRecordStartupAddsHistoryRow` | Verifies the `recordStartup` closure `newMainView` returns appends the startup receipt to History and redraws the table, with the windowed and tray wordings `run.go` selects between. | --- diff --git a/docs/TEST_REVIEW_PLAN.md b/docs/TEST_REVIEW_PLAN.md index 4be6659..b7454bb 100644 --- a/docs/TEST_REVIEW_PLAN.md +++ b/docs/TEST_REVIEW_PLAN.md @@ -84,26 +84,34 @@ go test -coverpkg=./src/domain,./src/storage,./src/runner,./src/scheduler,./src/ 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` +- [x] `TestEmitWithNoObserversIsNoop` ([events_test.go:36](../src/app/events_test.go)) — 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` +- [x] `TestStoreReturnsWiredStore` ([service_test.go:51](../src/app/service_test.go)) — asserts that a one-line getter returns its own field. Delete. -- [ ] `TestMainViewBuilds` +- [x] `TestMainViewBuilds` ([mainwindow_test.go:72](../src/ui/mainwindow_test.go)) — 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. -- [ ] `TestFilteredJobIndexesAll` / `ByNamedFolder` / `NoFolder` / `EmptySlice` + + 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. +- [x] `TestFilteredJobIndexesAll` / `ByNamedFolder` / `NoFolder` / `EmptySlice` ([jobs_view_test.go:59-101](../src/ui/jobs_view_test.go)) — 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 (optional) +## 5. Runtime cost of the runner tests — declined, with measurements `TestRunJobLogFileAllHeaders`, `TestRunJobRecordFields`, and `TestRunJobWritesLogFile` ([runner_test.go](../src/runner/runner_test.go)) have @@ -111,9 +119,28 @@ 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. -The cost is that each spawns a real subprocess; the `runner` package takes 5.3 s. -If suite wall time becomes a concern, merge them into one `RunJob` call with -three assertion blocks. Until then, leave them alone. +- [x] 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 @@ -142,7 +169,7 @@ Recorded here so a later pass does not re-report them: 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 — only if suite wall time becomes a problem. +5. Item 5 — measured and declined; see the item. Items 1 and 4 change the test inventory, so [TESTS.md](TESTS.md) has to be updated in the same commit. No [CHANGELOG.md](CHANGELOG.md) entry is needed: diff --git a/src/app/events_test.go b/src/app/events_test.go index 1c84abf..8bcce37 100644 --- a/src/app/events_test.go +++ b/src/app/events_test.go @@ -33,12 +33,6 @@ func TestEmitDeliversToAllObserversInOrder(t *testing.T) { } } -func TestEmitWithNoObserversIsNoop(t *testing.T) { - svc := newTestService(nil) - // Must not panic with an empty observer list. - svc.emit(JobChanged{}) -} - // Observers may read Service state from within OnEvent without deadlocking, // because emit is called outside the state lock. func TestObserverCanReadServiceState(t *testing.T) { diff --git a/src/app/service_test.go b/src/app/service_test.go index b43bc55..e33c098 100644 --- a/src/app/service_test.go +++ b/src/app/service_test.go @@ -47,11 +47,3 @@ func TestJobsReturnsCopy(t *testing.T) { t.Errorf("Service state leaked through Jobs(): name = %q, want %q", again[0].Name, "Original") } } - -func TestStoreReturnsWiredStore(t *testing.T) { - store := &storage.Store{} - svc := NewService(store, nil) - if svc.Store() != store { - t.Error("Store() did not return the wired store") - } -} diff --git a/src/ui/jobs_view_test.go b/src/ui/jobs_view_test.go index 116f4b2..981ff56 100644 --- a/src/ui/jobs_view_test.go +++ b/src/ui/jobs_view_test.go @@ -56,48 +56,43 @@ func TestFolderOptionsAppendsUniqueFolders(t *testing.T) { } } -func TestFilteredJobIndexesAll(t *testing.T) { - jobs := []domain.Job{ - {Folder: "Maintenance"}, - {Folder: ""}, - {Folder: "Reports"}, - } - got := filteredJobIndexes(jobs, allFolders) - if len(got) != 3 { - t.Errorf("allFolders filter: got %d indexes, want 3", len(got)) - } -} - -func TestFilteredJobIndexesByNamedFolder(t *testing.T) { +func TestFilteredJobIndexes(t *testing.T) { jobs := []domain.Job{ {Folder: "Maintenance"}, // index 0 - {Folder: ""}, // index 1 + {Folder: ""}, // index 1 — no folder {Folder: "Maintenance"}, // index 2 {Folder: "Reports"}, // index 3 + {Folder: " "}, // index 4 — blank reads as no folder } - got := filteredJobIndexes(jobs, "Maintenance") - if len(got) != 2 || got[0] != 0 || got[1] != 2 { - t.Errorf("Maintenance filter: got %v, want [0 2]", got) + cases := []struct { + name string + jobs []domain.Job + filter string + want []int + }{ + {"all folders", jobs, allFolders, []int{0, 1, 2, 3, 4}}, + {"named folder", jobs, "Maintenance", []int{0, 2}}, + {"no folder", jobs, noFolder, []int{1, 4}}, + {"empty job list", nil, allFolders, nil}, + } + for _, tc := range cases { + got := filteredJobIndexes(tc.jobs, tc.filter) + if !sameIndexes(got, tc.want) { + t.Errorf("%s: filteredJobIndexes(_, %q) = %v, want %v", tc.name, tc.filter, got, tc.want) + } } } -func TestFilteredJobIndexesNoFolder(t *testing.T) { - jobs := []domain.Job{ - {Folder: "Maintenance"}, // index 0 — excluded - {Folder: ""}, // index 1 — no folder → included - {Folder: " "}, // index 2 — blank → included +func sameIndexes(got, want []int) bool { + if len(got) != len(want) { + return false } - got := filteredJobIndexes(jobs, noFolder) - if len(got) != 2 || got[0] != 1 || got[1] != 2 { - t.Errorf("noFolder filter: got %v, want [1 2]", got) - } -} - -func TestFilteredJobIndexesEmptySlice(t *testing.T) { - got := filteredJobIndexes(nil, allFolders) - if len(got) != 0 { - t.Errorf("empty job list should return empty indexes, got %v", got) + for i := range got { + if got[i] != want[i] { + return false + } } + return true } func TestNextJobListViewFlipsBothWays(t *testing.T) { diff --git a/src/ui/mainwindow_test.go b/src/ui/mainwindow_test.go index 9f5f567..10a894f 100644 --- a/src/ui/mainwindow_test.go +++ b/src/ui/mainwindow_test.go @@ -3,12 +3,16 @@ package ui import ( "path/filepath" "testing" + "time" "gitea.mixdep.ru/mix/gosentry/src/app" "gitea.mixdep.ru/mix/gosentry/src/domain" "gitea.mixdep.ru/mix/gosentry/src/storage" + "fyne.io/fyne/v2" + "fyne.io/fyne/v2/container" "fyne.io/fyne/v2/test" + "fyne.io/fyne/v2/widget" ) // newTestStore builds a Store rooted in a temp directory. It is separate from @@ -69,7 +73,35 @@ func TestMainViewFitsTheDefaultWindowSize(t *testing.T) { } } -func TestMainViewBuilds(t *testing.T) { +// historyTable returns the History tab's table. It is the only widget.Table the +// main view builds, so the search does not need to know the tab order. +func historyTable(t *testing.T, content fyne.CanvasObject) *widget.Table { + t.Helper() + tabs, ok := content.(*container.AppTabs) + if !ok { + t.Fatal("main view is not the expected AppTabs container") + } + for _, item := range tabs.Items { + found := findFirst(item.Content, func(o fyne.CanvasObject) bool { + _, ok := o.(*widget.Table) + return ok + }) + if found != nil { + return found.(*widget.Table) + } + } + t.Fatal("main view has no history table") + return nil +} + +// TestMainViewRecordStartupAddsHistoryRow covers the second return value of +// newMainView. run.go calls it once per launch with a different windowShown +// flag depending on whether the app started into the tray, and that call is the +// only thing that puts the startup receipt into History — so both the wording +// and the fact that the table is redrawn are worth pinning. Building the full +// tab set and setting it as the window content is a side benefit: no other test +// assembles all three tabs together. +func TestMainViewRecordStartupAddsHistoryRow(t *testing.T) { testApp := test.NewApp() defer testApp.Quit() @@ -80,9 +112,39 @@ func TestMainViewBuilds(t *testing.T) { defer svc.Stop() content, recordStartup := newMainView(w, svc) - if content == nil { - t.Fatal("newMainView returned nil content") - } w.SetContent(content) - recordStartup(0, true) + + table := historyTable(t, content) + if rows, _ := table.Length(); rows != 0 { + t.Fatalf("history rows before startup = %d, want 0", rows) + } + + recordStartup(1500*time.Millisecond, true) + recordStartup(20*time.Millisecond, false) + + rows, _ := table.Length() + if rows != 2 { + t.Fatalf("history rows after two startup records = %d, want 2", rows) + } + + // Read the rows back through the table's own cell callbacks, which is what + // the redraw does; a value only in the events slice would not prove the + // table was refreshed with it. + cell := table.CreateCell() + cellText := func(row, col int) string { + table.UpdateCell(widget.TableCellID{Row: row, Col: col}, cell) + return cell.(*widget.Label).Text + } + if got := cellText(0, 2); got != "Application" { + t.Errorf("startup row job = %q, want %q", got, "Application") + } + if got := cellText(0, 3); got != "Started" { + t.Errorf("startup row state = %q, want %q", got, "Started") + } + if got := cellText(0, 4); got != "Window shown in 1.5s" { + t.Errorf("windowed startup detail = %q, want %q", got, "Window shown in 1.5s") + } + if got := cellText(1, 4); got != "Started in tray in 20ms" { + t.Errorf("tray startup detail = %q, want %q", got, "Started in tray in 20ms") + } }