From 973c3108b71063ea932aed6c67e16e393b5e6c41 Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Thu, 7 May 2026 00:03:30 +0300 Subject: task 81: replace test sleeps with deterministic sync --- cmd/player/main.go | 35 ++++++++++++--------- cmd/player/main_test.go | 13 ++++---- internal/model/scan_test.go | 17 +++++++--- internal/service/admin_test.go | 71 +++++++++++++++++++++--------------------- internal/service/scan.go | 12 +++++++ 5 files changed, 88 insertions(+), 60 deletions(-) diff --git a/cmd/player/main.go b/cmd/player/main.go index 66d2889..ea3de1b 100644 --- a/cmd/player/main.go +++ b/cmd/player/main.go @@ -35,20 +35,21 @@ func run(args []string) error { // appDeps bundles all wired service-layer dependencies. type appDeps struct { - store repository.Store - hasher auth.Hasher - sm *auth.SessionManager - cfg *internal.Config - clk clock.Clock - mediaSvc service.MediaService - adminSvc service.AdminService - progressSvc service.ProgressService - authSvc service.AuthService - podcastSvc service.PodcastEpisodeService - scanner scanner.Scanner - gcWorker *service.GCWorker - logger *slog.Logger - appCtx context.Context + store repository.Store + hasher auth.Hasher + sm *auth.SessionManager + cfg *internal.Config + clk clock.Clock + mediaSvc service.MediaService + adminSvc service.AdminService + progressSvc service.ProgressService + authSvc service.AuthService + podcastSvc service.PodcastEpisodeService + scanner scanner.Scanner + gcWorker *service.GCWorker + logger *slog.Logger + appCtx context.Context + workersStarted chan<- struct{} } // parseVersionFlag parses CLI flags and returns whether --version was requested. @@ -137,6 +138,12 @@ func startBackgroundWorkers(deps *appDeps) { } } }() + if deps.workersStarted != nil { + select { + case deps.workersStarted <- struct{}{}: + default: + } + } } // ensureSignalChannel returns the provided channel or creates a new one wired diff --git a/cmd/player/main_test.go b/cmd/player/main_test.go index 907e320..21b6ff0 100644 --- a/cmd/player/main_test.go +++ b/cmd/player/main_test.go @@ -106,9 +106,6 @@ func TestRunWithSignal_NormalShutdown(t *testing.T) { errCh <- runWithSignal([]string{}, sigCh) }() - // Give the server a moment to start listening. - time.Sleep(500 * time.Millisecond) - // Send a synthetic signal to trigger shutdown. sigCh <- syscall.SIGINT @@ -137,7 +134,6 @@ func TestRunWithSignal_LogLevels(t *testing.T) { go func() { errCh <- runWithSignal([]string{}, sigCh) }() - time.Sleep(200 * time.Millisecond) sigCh <- syscall.SIGTERM select { @@ -257,10 +253,15 @@ func TestStartBackgroundWorkers_StartsAndStops(t *testing.T) { defer cancel() deps := wireDeps(cfg, store, logger, ctx) + workersStarted := make(chan struct{}, 1) + deps.workersStarted = workersStarted startBackgroundWorkers(deps) - // Give the goroutines a moment to start. - time.Sleep(50 * time.Millisecond) + select { + case <-workersStarted: + case <-time.After(time.Second): + t.Fatal("timeout waiting for background workers to start") + } // Cancel the app context; workers should exit. cancel() diff --git a/internal/model/scan_test.go b/internal/model/scan_test.go index cc0ba6b..acd1dd1 100644 --- a/internal/model/scan_test.go +++ b/internal/model/scan_test.go @@ -2,8 +2,8 @@ package model import ( "errors" + "sync" "testing" - "time" ) func TestScanProgress_Start(t *testing.T) { @@ -116,8 +116,11 @@ func TestScanProgress_ConcurrentAccess(t *testing.T) { p.Start(2) p.SetFilesTotal(100) + start := make(chan struct{}) done := make(chan struct{}) + var copies sync.WaitGroup go func() { + <-start for i := 0; i < 50; i++ { p.IncrementFile() } @@ -125,9 +128,15 @@ func TestScanProgress_ConcurrentAccess(t *testing.T) { }() for i := 0; i < 50; i++ { - _ = p.Copy() - time.Sleep(time.Microsecond) - } + copies.Add(1) + go func() { + defer copies.Done() + <-start + _ = p.Copy() + }() + } + close(start) + copies.Wait() <-done cp := p.Copy() diff --git a/internal/service/admin_test.go b/internal/service/admin_test.go index 17f5373..03a177b 100644 --- a/internal/service/admin_test.go +++ b/internal/service/admin_test.go @@ -22,6 +22,15 @@ func (f *fakeScanner) Scan(ctx context.Context, root string, progress *model.Sca return nil } +func setScanDoneCh(t *testing.T, svc AdminService, doneCh chan<- struct{}) { + t.Helper() + adminSvc, ok := svc.(*adminService) + if !ok { + t.Fatalf("expected *adminService, got %T", svc) + } + adminSvc.scanService.doneCh = doneCh +} + type fakeHasher struct { fixed string err error @@ -300,35 +309,29 @@ func TestAdminService_TriggerRescan_CancelsPrevious(t *testing.T) { func TestAdminService_TriggerRescan_FreshProgressPerScan(t *testing.T) { ctx := context.Background() + started := make(chan struct{}) done := make(chan struct{}) sc := &fakeScanner{ scanFunc: func(_ context.Context, _ string, progress *model.ScanProgress) error { progress.Start(5) + close(started) <-done return nil }, } svc := NewAdminService(&repository.MockStore{}, newMockClock(), &fakeHasher{fixed: "hash"}, sc, "/media", ctx) + scanDone := make(chan struct{}, 1) + setScanDoneCh(t, svc, scanDone) if err := svc.TriggerRescan(ctx); err != nil { t.Fatalf("unexpected error: %v", err) } - // Wait for goroutine to start. - for { - p := svc.ScanProgress(ctx) - if p.Running { - break - } - time.Sleep(10 * time.Millisecond) - } + <-started close(done) - // Wait for goroutine to finish. - for { - p := svc.ScanProgress(ctx) - if !p.Running { - break - } - time.Sleep(10 * time.Millisecond) + select { + case <-scanDone: + case <-time.After(time.Second): + t.Fatal("timeout waiting for first scan to finish") } p1 := svc.ScanProgress(ctx) @@ -337,10 +340,12 @@ func TestAdminService_TriggerRescan_FreshProgressPerScan(t *testing.T) { } // Start a new scan on the same service with different progress. + started2 := make(chan struct{}) done2 := make(chan struct{}) sc2 := &fakeScanner{ scanFunc: func(_ context.Context, _ string, progress *model.ScanProgress) error { progress.Start(10) + close(started2) <-done2 return nil }, @@ -348,25 +353,17 @@ func TestAdminService_TriggerRescan_FreshProgressPerScan(t *testing.T) { // We replace the scanner field via reflection? No, easier: just create new service. // Actually, the test verifies per-service fresh progress, so new service is fine. svc2 := NewAdminService(&repository.MockStore{}, newMockClock(), &fakeHasher{fixed: "hash"}, sc2, "/media", ctx) + scanDone2 := make(chan struct{}, 1) + setScanDoneCh(t, svc2, scanDone2) if err := svc2.TriggerRescan(ctx); err != nil { t.Fatalf("unexpected error: %v", err) } - // Wait for goroutine to start. - for { - p := svc2.ScanProgress(ctx) - if p.Running { - break - } - time.Sleep(10 * time.Millisecond) - } + <-started2 close(done2) - // Wait for goroutine to finish. - for { - p := svc2.ScanProgress(ctx) - if !p.Running { - break - } - time.Sleep(10 * time.Millisecond) + select { + case <-scanDone2: + case <-time.After(time.Second): + t.Fatal("timeout waiting for second scan to finish") } p2 := svc2.ScanProgress(ctx) @@ -392,12 +389,14 @@ func TestAdminService_TriggerRescan_ConcurrentCalls(t *testing.T) { var wg sync.WaitGroup callCount := 0 var mu sync.Mutex + started := make(chan struct{}, 5) sc := &fakeScanner{ scanFunc: func(scanCtx context.Context, _ string, progress *model.ScanProgress) error { mu.Lock() callCount++ mu.Unlock() progress.Start(1) + started <- struct{}{} <-scanCtx.Done() return scanCtx.Err() }, @@ -413,13 +412,13 @@ func TestAdminService_TriggerRescan_ConcurrentCalls(t *testing.T) { } wg.Wait() - // Wait for the final surviving goroutine to start. - for { - progress := svc.ScanProgress(ctx) - if progress.Running { - break + timeout := time.After(time.Second) + for !svc.ScanProgress(ctx).Running { + select { + case <-started: + case <-timeout: + t.Fatal("timeout waiting for a running scan") } - time.Sleep(10 * time.Millisecond) } mu.Lock() diff --git a/internal/service/scan.go b/internal/service/scan.go index d019616..52bc5c5 100644 --- a/internal/service/scan.go +++ b/internal/service/scan.go @@ -22,6 +22,7 @@ type scanService struct { scanCancel context.CancelFunc progress *model.ScanProgress appCtx context.Context // application-level context used to propagate shutdown cancellation + doneCh chan<- struct{} } // NewScanService creates a ScanService. @@ -64,6 +65,7 @@ func (s *scanService) TriggerRescan(ctx context.Context) error { progress.Done(nil) s.logger.Info("rescan completed") } + s.notifyDone() }() return nil } @@ -77,3 +79,13 @@ func (s *scanService) ScanProgress(ctx context.Context) model.ScanProgress { } return progress.Copy() } + +func (s *scanService) notifyDone() { + if s.doneCh == nil { + return + } + select { + case s.doneCh <- struct{}{}: + default: + } +} -- cgit v1.2.3