diff --git a/docs/REFACTORING.md b/docs/REFACTORING.md index 37a6d36..73dc111 100644 --- a/docs/REFACTORING.md +++ b/docs/REFACTORING.md @@ -279,7 +279,7 @@ Track progress here. Mark tasks complete as they land and pass review. ### Phase 5 — Hardening & docs - [x] T5.1 — Surface errors from service + storage - [x] T5.2 — Introduce `autostart.Manager` interface -- [ ] T5.3 — Fill test gaps (folder filtering, cleanup, migration, concurrency) +- [x] T5.3 — Fill test gaps (folder filtering, cleanup, migration, concurrency) - [ ] T5.4 — Run `go test -race ./...` clean on both platforms - [ ] T5.5 — Update docs (ARCHITECTURE.md, TESTS.md, README) diff --git a/docs/TESTS.md b/docs/TESTS.md index cba9957..843e1ca 100644 --- a/docs/TESTS.md +++ b/docs/TESTS.md @@ -1,6 +1,6 @@ # GoSentry Test Suite -All tests are located alongside source code in the `src/core/` package. Tests follow Go conventions with `*_test.go` filename patterns. +All tests are located alongside source code in their respective packages under `src/`. Tests follow Go conventions with `*_test.go` filename patterns. ## Running Tests @@ -172,12 +172,7 @@ Tests Linux autostart entry creation via XDG Desktop Entry files. --- -## Future Test Coverage Gaps +## Remaining Test Coverage Gaps -Potential areas for additional tests: -- Job group/folder filtering and persistence -- Log cleanup (max file count and max age) -- Settings persistence and migration -- GUI integration tests (currently untested) -- Concurrent job execution -- Job history and run record storage +- GUI integration tests — Fyne widget interaction is not yet tested +- Job history and run record storage — on-disk run-record retrieval not covered diff --git a/src/app/operations_test.go b/src/app/operations_test.go index ebfc1db..e67e178 100644 --- a/src/app/operations_test.go +++ b/src/app/operations_test.go @@ -399,6 +399,35 @@ func TestRunDueSkipsJobNotYetDue(t *testing.T) { } } +// TestRunDueSkipsJobInRunningState verifies that RunDue will not start a second +// concurrent instance of a job that is already in "Running" state — even if the +// job's NextDue is in the past. This guards against the window between +// executeRun completing and refreshNextRunLocked setting a new NextDue. +func TestRunDueSkipsJobInRunningState(t *testing.T) { + svc := newTempService(t, []domain.Job{{ID: 1, Name: "A", Schedule: "@every 1m", Command: "echo", Enabled: true}}) + + var calls int32 + svc.runJob = func(context.Context, *domain.Job, string, string) domain.RunRecord { + atomic.AddInt32(&calls, 1) + return domain.RunRecord{State: "Success"} + } + + // Force the job into "Running" with a past NextDue, simulating an in-flight + // run. We set NextDue to a past time so the due check would otherwise pass. + svc.mu.Lock() + rt := svc.runtimes[1] + rt.LastState = "Running" + rt.NextDue = time.Now().Add(-time.Minute) + svc.mu.Unlock() + + svc.RunDue(time.Now().Add(2 * time.Minute)) + time.Sleep(50 * time.Millisecond) + + if got := atomic.LoadInt32(&calls); got != 0 { + t.Errorf("RunDue called runner %d time(s) for a job in Running state, want 0", got) + } +} + func TestRunDueDoesNothingWhilePaused(t *testing.T) { svc := newTempService(t, []domain.Job{{ID: 1, Name: "A", Schedule: "@every 1m", Command: "echo", Enabled: true}}) var ran int32 diff --git a/src/runner/cleanup_test.go b/src/runner/cleanup_test.go new file mode 100644 index 0000000..b55bc01 --- /dev/null +++ b/src/runner/cleanup_test.go @@ -0,0 +1,154 @@ +package runner + +import ( + "fmt" + "os" + "path/filepath" + "testing" + "time" +) + +func writeLogFile(t *testing.T, dir, name string) string { + t.Helper() + path := filepath.Join(dir, name) + if err := os.WriteFile(path, []byte("log"), 0o644); err != nil { + t.Fatal(err) + } + return path +} + +func setModTime(t *testing.T, path string, age time.Duration) { + t.Helper() + mt := time.Now().Add(-age) + if err := os.Chtimes(path, mt, mt); err != nil { + t.Fatal(err) + } +} + +func TestCleanupLogsMissingDirReturnsNil(t *testing.T) { + err := CleanupLogs(filepath.Join(t.TempDir(), "nonexistent"), 100, 30) + if err != nil { + t.Errorf("missing dir should return nil, got %v", err) + } +} + +func TestCleanupLogsRemovesFilesPastMaxAge(t *testing.T) { + dir := t.TempDir() + old := writeLogFile(t, dir, "old.log") + recent := writeLogFile(t, dir, "recent.log") + setModTime(t, old, 31*24*time.Hour) // 31 days old → past the 30-day limit + setModTime(t, recent, 5*24*time.Hour) // 5 days old → within limit + + if err := CleanupLogs(dir, 100, 30); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(old); !os.IsNotExist(err) { + t.Error("file older than maxAgeDays should be deleted") + } + if _, err := os.Stat(recent); err != nil { + t.Errorf("file within maxAgeDays should be kept: %v", err) + } +} + +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. +func TestCleanupLogsByCountDeletesOldest(t *testing.T) { + dir := t.TempDir() + // Create 5 files; i=0 is newest (1 day old), i=4 is oldest (5 days old). + var paths []string + for i := 0; i < 5; i++ { + path := writeLogFile(t, dir, fmt.Sprintf("job_%03d.log", i)) + setModTime(t, path, time.Duration(i+1)*24*time.Hour) + paths = append(paths, path) + } + + if err := CleanupLogs(dir, 3, 0); err != nil { + t.Fatal(err) + } + entries, _ := os.ReadDir(dir) + if len(entries) != 3 { + t.Errorf("expected 3 files after count cleanup, got %d", len(entries)) + } + // The 3 newest files (paths[0..2]) must survive. + for _, kept := range paths[:3] { + if _, err := os.Stat(kept); err != nil { + t.Errorf("newest file %s should be kept: %v", filepath.Base(kept), err) + } + } + // The 2 oldest files (paths[3..4]) must be removed. + for _, deleted := range paths[3:] { + if _, err := os.Stat(deleted); !os.IsNotExist(err) { + t.Errorf("oldest file %s should have been deleted", filepath.Base(deleted)) + } + } +} + +func TestCleanupLogsNonLogFilesNotDeleted(t *testing.T) { + dir := t.TempDir() + logFile := writeLogFile(t, dir, "job.log") + notALog := writeLogFile(t, dir, "notes.txt") + // Both are old enough that age-based cleanup would remove them if it applied. + setModTime(t, logFile, 35*24*time.Hour) + setModTime(t, notALog, 35*24*time.Hour) + + if err := CleanupLogs(dir, 100, 30); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(logFile); !os.IsNotExist(err) { + t.Error("old .log file should be deleted") + } + if _, err := os.Stat(notALog); err != nil { + t.Errorf(".txt file should not be deleted: %v", err) + } +} + +func TestCleanupLogsSubdirsNotDeleted(t *testing.T) { + dir := t.TempDir() + subdir := filepath.Join(dir, "archive.log") // name looks like a log but is a dir + if err := os.Mkdir(subdir, 0o755); err != nil { + t.Fatal(err) + } + setModTime(t, subdir, 60*24*time.Hour) + + if err := CleanupLogs(dir, 100, 30); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(subdir); err != nil { + t.Errorf("subdirectory should not be deleted: %v", err) + } +} + +// TestCleanupLogsZeroLimitsDisableBothPolicies confirms that maxFiles=0 disables +// count-based cleanup and maxAgeDays=0 disables age-based cleanup independently. +func TestCleanupLogsZeroLimitsDisableBothPolicies(t *testing.T) { + dir := t.TempDir() + for i := 0; i < 5; i++ { + path := writeLogFile(t, dir, fmt.Sprintf("job_%d.log", i)) + setModTime(t, path, 60*24*time.Hour) // very old + } + + if err := CleanupLogs(dir, 0, 0); err != nil { + t.Fatal(err) + } + entries, _ := os.ReadDir(dir) + if len(entries) != 5 { + t.Errorf("expected all 5 files kept with both limits disabled, got %d", len(entries)) + } +} diff --git a/src/storage/store_test.go b/src/storage/store_test.go index bcaa2d9..9d14f03 100644 --- a/src/storage/store_test.go +++ b/src/storage/store_test.go @@ -1,6 +1,7 @@ package storage import ( + "os" "path/filepath" "strings" "testing" @@ -154,6 +155,80 @@ func TestNormalizeJobsFillsDefaults(t *testing.T) { } } +// TestLoadOrCreateConfigMigratesFromLegacy verifies that when gosentry.yaml is +// absent but pysentry.yaml exists the config is read from the legacy file. This +// lets portable installs that still carry a pysentry.yaml start without manual +// migration. +func TestLoadOrCreateConfigMigratesFromLegacy(t *testing.T) { + dir := t.TempDir() + paths := Paths{ + AppDir: dir, + ConfigPath: filepath.Join(dir, ConfigFileName), // gosentry.yaml — not created + } + + legacy := domain.Config{ + JobsDir: "/legacy/jobs", + LogsDir: "/legacy/logs", + MaxLogFiles: 77, + MaxLogAgeDays: 13, + StartOnLogin: true, + } + if err := writeYAML(filepath.Join(dir, LegacyConfigFileName), legacy); err != nil { + t.Fatal(err) + } + + got, err := loadOrCreateConfig(paths) + if err != nil { + t.Fatal(err) + } + if got.JobsDir != legacy.JobsDir { + t.Errorf("JobsDir: got %q, want %q", got.JobsDir, legacy.JobsDir) + } + if got.LogsDir != legacy.LogsDir { + t.Errorf("LogsDir: got %q, want %q", got.LogsDir, legacy.LogsDir) + } + if got.MaxLogFiles != legacy.MaxLogFiles { + t.Errorf("MaxLogFiles: got %d, want %d", got.MaxLogFiles, legacy.MaxLogFiles) + } + if got.MaxLogAgeDays != legacy.MaxLogAgeDays { + t.Errorf("MaxLogAgeDays: got %d, want %d", got.MaxLogAgeDays, legacy.MaxLogAgeDays) + } + if got.StartOnLogin != legacy.StartOnLogin { + t.Errorf("StartOnLogin: got %v, want %v", got.StartOnLogin, legacy.StartOnLogin) + } +} + +// TestLoadOrCreateConfigCreatesDefaultsOnFirstRun verifies that the first run +// (no config files present) writes gosentry.yaml and returns sensible defaults. +func TestLoadOrCreateConfigCreatesDefaultsOnFirstRun(t *testing.T) { + dir := t.TempDir() + paths := Paths{ + AppDir: dir, + ConfigPath: filepath.Join(dir, ConfigFileName), + } + + got, err := loadOrCreateConfig(paths) + if err != nil { + t.Fatal(err) + } + if got.JobsDir != "." { + t.Errorf("default JobsDir = %q, want '.'", got.JobsDir) + } + if got.LogsDir != "logs" { + t.Errorf("default LogsDir = %q, want 'logs'", got.LogsDir) + } + if got.MaxLogFiles != 100 { + t.Errorf("default MaxLogFiles = %d, want 100", got.MaxLogFiles) + } + if got.MaxLogAgeDays != 30 { + t.Errorf("default MaxLogAgeDays = %d, want 30", got.MaxLogAgeDays) + } + // The function must have written the defaults to gosentry.yaml. + if _, err := os.Stat(paths.ConfigPath); err != nil { + t.Errorf("gosentry.yaml should have been created: %v", err) + } +} + func TestJobsYAMLDoesNotPersistRuntimeNoise(t *testing.T) { // Job carries only durable configuration; runtime state lives in // domain.JobRuntime and is never marshalled. This guards against a future diff --git a/src/ui/jobs_view_test.go b/src/ui/jobs_view_test.go new file mode 100644 index 0000000..f99f7a0 --- /dev/null +++ b/src/ui/jobs_view_test.go @@ -0,0 +1,95 @@ +package ui + +import ( + "testing" + + "gitea.mixdep.ru/mix/gosentry/src/domain" +) + +func TestFilterValue(t *testing.T) { + cases := []struct{ input, want string }{ + {"", noFolder}, + {" ", noFolder}, + {"Maintenance", "Maintenance"}, + {" Reports ", "Reports"}, + } + for _, tc := range cases { + if got := filterValue(tc.input); got != tc.want { + t.Errorf("filterValue(%q) = %q, want %q", tc.input, got, tc.want) + } + } +} + +func TestFolderOptionsAlwaysIncludesSentinels(t *testing.T) { + opts := folderOptions(nil) + if len(opts) < 2 || opts[0] != allFolders || opts[1] != noFolder { + t.Errorf("folderOptions(nil) = %v, want [%q %q ...]", opts, allFolders, noFolder) + } +} + +func TestFolderOptionsAppendsUniqueFolders(t *testing.T) { + jobs := []domain.Job{ + {Folder: "Maintenance"}, + {Folder: ""}, // no folder → not a named folder + {Folder: " Backups "}, // trimmed to "Backups" + {Folder: "Maintenance"}, // duplicate → not added again + } + opts := folderOptions(jobs) + // Expected: All, No folder, Maintenance, Backups — 4 entries, no duplicates. + if len(opts) != 4 { + t.Errorf("expected 4 options, got %v", opts) + } + has := map[string]bool{} + for _, o := range opts { + has[o] = true + } + for _, want := range []string{allFolders, noFolder, "Maintenance", "Backups"} { + if !has[want] { + t.Errorf("expected option %q in %v", want, opts) + } + } +} + +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) { + jobs := []domain.Job{ + {Folder: "Maintenance"}, // index 0 + {Folder: ""}, // index 1 + {Folder: "Maintenance"}, // index 2 + {Folder: "Reports"}, // index 3 + } + got := filteredJobIndexes(jobs, "Maintenance") + if len(got) != 2 || got[0] != 0 || got[1] != 2 { + t.Errorf("Maintenance filter: got %v, want [0 2]", got) + } +} + +func TestFilteredJobIndexesNoFolder(t *testing.T) { + jobs := []domain.Job{ + {Folder: "Maintenance"}, // index 0 — excluded + {Folder: ""}, // index 1 — no folder → included + {Folder: " "}, // index 2 — blank → included + } + 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) + } +}