diff --git a/cmd/server/tests/TEST_CLEANUP_PATTERN.md b/cmd/server/tests/TEST_CLEANUP_PATTERN.md new file mode 100644 index 0000000..2af6491 --- /dev/null +++ b/cmd/server/tests/TEST_CLEANUP_PATTERN.md @@ -0,0 +1,283 @@ +# Test Resource Cleanup Pattern + +## Overview + +This document describes the **TestServerSetup** pattern used for automatic resource cleanup in integration tests, which prevents database connection leaks and goroutine leaks. + +## Problem + +Prior to this pattern, integration tests had resource leaks: + +```go +// OLD PATTERN (BROKEN) +func TestExample(t *testing.T) { + ts, db, _ := setupTestServer(t) + defer ts.Close() // ❌ Only closes HTTP server + + // ... test code ... + // ❌ dbPool never closed + // ❌ connManager.StartCleanupTask() goroutine never stopped + // ❌ queueProcessor.Start() goroutine never stopped +} +``` + +**Impact:** +- Each test leaked ~4 database connections (pgxpool default max_conns) +- Leaked 2+ goroutines per test (cleanup task, queue processor) +- ~160 tests = potential 640+ leaked connections +- PostgreSQL max_connections = 100 → exhaustion after ~25 tests + +## Solution + +### TestServerSetup Struct + +Location: `/cmd/server/tests/test_helpers.go` + +```go +// TestServerSetup manages the lifecycle of a test server with proper resource cleanup +type TestServerSetup struct { + Server *httptest.Server + DB *database.Queries + DBPool *pgxpool.Pool + Config *config.Config + ConnManager *wsync.ConnectionManager + QueueProcessor *wsync.SyncQueueProcessor + CleanupCancel context.CancelFunc // For connManager cleanup task + QueueCtx context.Context // For queueProcessor + QueueCancel context.CancelFunc // For queueProcessor + mu sync.Mutex + closed bool +} + +// Close cleans up all resources in the correct order +func (s *TestServerSetup) Close() error { + s.mu.Lock() + defer s.mu.Unlock() + + if s.closed { + return nil + } + + // 1. Stop queue processor goroutine + if s.QueueCancel != nil { + s.QueueCancel() + s.QueueCancel = nil + } + + // 2. Stop connection manager cleanup task + if s.CleanupCancel != nil { + s.CleanupCancel() + s.CleanupCancel = nil + } + + // 3. Close HTTP server + if s.Server != nil { + s.Server.Close() + s.Server = nil + } + + // 4. Close database pool (waits for all connections to release) + if s.DBPool != nil { + s.DBPool.Close() + s.DBPool = nil + } + + s.closed = true + return nil +} +``` + +### setupTestServer Function + +```go +func setupTestServer(t *testing.T) *TestServerSetup { + cfg := config.LoadConfig() + // ... config setup ... + + // Create database pool + dbPool, err := pgxpool.New(context.Background(), cfg.DatabaseURL()) + require.NoError(t, err) + + // Create connManager and capture cleanup cancel function + connManager := wsync.NewConnectionManager() + cleanupCancel := connManager.StartCleanupTask() // ← Returns CancelFunc! + + // Create queue processor with cancellable context + queueProcessor := wsync.NewSyncQueueProcessor(queries) + queueCtx, queueCancel := context.WithCancel(context.Background()) + go queueProcessor.Start(queueCtx) // ← Now cancellable! + + // ... create handlers, router, etc ... + + ts := httptest.NewServer(e) + + setup := &TestServerSetup{ + Server: ts, + DB: queries, + DBPool: dbPool, + Config: cfg, + ConnManager: connManager, + QueueProcessor: queueProcessor, + CleanupCancel: cleanupCancel, // ← Saved for cleanup + QueueCtx: queueCtx, + QueueCancel: queueCancel, // ← Saved for cleanup + } + + // AUTOMATIC CLEANUP via t.Cleanup() + t.Cleanup(func() { + if err := setup.Close(); err != nil { + t.Errorf("Failed to cleanup test server: %v", err) + } + }) + + return setup +} +``` + +## Usage + +### NEW PATTERN (Correct) + +```go +func TestExample(t *testing.T) { + setup := setupTestServer(t) + // No defer needed! t.Cleanup handles it automatically + + // Access resources through setup + token := loginTestUser(t, setup.Server, setup.DB) + mediaID := createTestMediaItemID(t, setup.Server, token) + + req, _ := http.NewRequest("GET", setup.Server.URL+"/api/test", nil) + // ... test code ... + + // When test completes (pass or fail), setup.Close() is called automatically +} +``` + +### Nested Tests + +```go +func TestWithSubtests(t *testing.T) { + setup := setupTestServer(t) + // setup is available in outer scope + + t.Run("subtest 1", func(t *testing.T) { + // setup is available here too + req, _ := http.NewRequest("POST", setup.Server.URL+"/api/test", nil) + // ... + }) + + t.Run("subtest 2", func(t *testing.T) { + // Each subtest shares the same setup + // Cleanup happens when outer test completes + }) +} +``` + +### Helper Functions + +**IMPORTANT:** Helper functions that take `ts *httptest.Server` as parameter: + +```go +// CORRECT: Helper uses ts parameter +func createTestLibrary(t *testing.T, ts *httptest.Server, token string) string { + req, _ := http.NewRequest("POST", ts.URL+"/api/libraries", ...) + // ... +} + +// CORRECT: Call helper with setup.Server +func TestSomething(t *testing.T) { + setup := setupTestServer(t) + libID := createTestLibrary(t, setup.Server, token, "test-lib") +} +``` + +## Resource Cleanup Order + +When `setup.Close()` is called (automatically via `t.Cleanup()`): + +1. **Stop Queue Processor** (`QueueCancel()`) + - Stops goroutine processing sync queue + - Releases queue resources + +2. **Stop Connection Manager** (`CleanupCancel()`) + - Stops goroutine cleaning stale WebSocket connections + - Releases WebSocket resources + +3. **Close HTTP Server** (`Server.Close()`) + - Stops accepting new connections + - Shuts down HTTP server gracefully + +4. **Close Database Pool** (`DBPool.Close()`) + - Waits for all connections to be released + - Returns connections to pool + - Closes all database connections + +## Benefits + +✅ **No manual cleanup needed** - `t.Cleanup()` handles it automatically +✅ **Works even if test panics** - Go runtime calls cleanup +✅ **Thread-safe** - Mutex prevents double-close issues +✅ **Idempotent** - Can call `Close()` multiple times safely +✅ **Catches test failures** - Cleanup happens even on test failure + +## Migration Guide + +To migrate an existing test: + +**Before:** +```go +func TestOld(t *testing.T) { + ts, db, _ := setupTestServer(t) + defer ts.Close() + + token := loginTestUser(t, ts, db) + req, _ := http.NewRequest("GET", ts.URL+"/api/test", nil) +} +``` + +**After:** +```go +func TestNew(t *testing.T) { + setup := setupTestServer(t) + // No defer needed + + token := loginTestUser(t, setup.Server, setup.DB) + req, _ := http.NewRequest("GET", setup.Server.URL+"/api/test", nil) +} +``` + +## Verification + +Check that cleanup is working: + +```bash +# Before tests +podman exec bookhoard_db psql -U postgres -d bookhoard -c \ + "SELECT count(*) FROM pg_stat_activity WHERE datname = 'bookhoard';" +# Should be: 3 (app + 2 idle) + +# Run tests +go test -v ./cmd/server/tests/ + +# After tests +podman exec bookhoard_db psql -U postgres -d bookhoard -c \ + "SELECT count(*) FROM pg_stat_activity WHERE datname = 'bookhoard';" +# Should still be: 3 (not 3 + number of tests × 4) +``` + +## Implementation History + +- **Created**: 2026-02-10 +- **Commits**: + - `f3141f1` - Create TestServerSetup struct + - `6c61046` - Update all test files + - `5b32b59` - Fix edge cases + - `f15bf21` - Fix t.Run block issues + - `bb2ba14` - Final compilation fixes + +## Related Files + +- `/cmd/server/tests/test_helpers.go` - TestServerSetup implementation +- `/cmd/server/tests/*.go` - All test files using the pattern +- `PROJECT_GUIDELINES.md` - Project coding standards