T5.3: Fill test gaps — cleanup, folder filtering, migration, concurrent RunDue

- src/runner/cleanup_test.go (new): 7 tests for CleanupLogs covering
  age-based deletion, count-based trimming (oldest removed, newest kept),
  non-.log files and subdirs are never touched, and zero-limit behaviour.
- src/ui/jobs_view_test.go (new): 6 tests for filteredJobIndexes,
  folderOptions, and filterValue — the folder filter logic is now verified
  without Fyne widget interaction.
- src/storage/store_test.go: two new tests — legacy pysentry.yaml migration
  and first-run default creation.
- src/app/operations_test.go: TestRunDueSkipsJobInRunningState confirms
  RunDue honours the LastState == "Running" guard and refuses a second
  concurrent run even when NextDue is in the past.
- docs/REFACTORING.md: mark T5.3 complete.
- docs/TESTS.md: retire the covered gaps; update package path.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

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)
 

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:
+- GUI integration tests — Fyne widget interaction is not yet tested
-- Job group/folder filtering and persistence
+- Job history and run record storage — on-disk run-record retrieval not covered
-- 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

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

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))
+	}
+}

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

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)
+	}
+}