docs(tests): Add TestServerSetup cleanup pattern documentation

2026-02-10 13:09:09 -05:00
parent bb2ba14976
commit 509291b584
1 changed files with 283 additions and 0 deletions
@@ -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