diff --git a/README.md b/README.md index 063dfd3..020ee8b 100644 --- a/README.md +++ b/README.md @@ -74,7 +74,7 @@ Default logins (password: `password`): ```bash cd backend && go build -o bin/backend ./main.go cd frontend && npm ci && npm run build -cd backend && go test -tags "test,dev" -p 1 ./... # 803/806 passing, 3 skipped +cd backend && go test -tags "test,dev" ./... # 953/957 passing, 8 skipped ``` ## Full Documentation diff --git a/local-dev-2.sh b/local-dev-2.sh index f13e66d..ef87524 100755 --- a/local-dev-2.sh +++ b/local-dev-2.sh @@ -70,33 +70,18 @@ log_step "Allowing PostgreSQL to fully initialize..." sleep 5 # --- 3b. Create Test Database --- -log_step "Setting up test database (crussell_test)..." -docker exec postgres psql -U myuser -d mydb -c "CREATE DATABASE crussell_test;" 2>&1 || log_info "Test database may already exist" -sleep 2 - -# --- 3c. Seed Test Database Schema --- -log_step "Seeding test database schema..." -docker exec -i postgres psql -U myuser -d crussell_test < init-scripts/init-script.sql 2>&1 | head -5 || true -log_success "Test database schema seeded" - -# --- 3d. Verify test database is ready --- -log_step "Verifying test database readiness..." -MAX_ATTEMPTS=10 -ATTEMPT=0 -while [ $ATTEMPT -lt $MAX_ATTEMPTS ]; do - if docker exec postgres psql -U myuser -d crussell_test -c "SELECT 1;" > /dev/null 2>&1; then - log_success "Test database ready" - break - fi - ATTEMPT=$((ATTEMPT+1)) - sleep 1 +log_step "Cleaning up stale test databases..." +docker exec postgres psql -U myuser -d mydb -t -c " + SELECT datname FROM pg_database WHERE datname LIKE 'crussell_test%'; +" 2>&1 | grep crussell_test | while read -r dbname; do + docker exec postgres psql -U myuser -d mydb -c " + SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE datname = '$dbname' AND pid != pg_backend_pid(); + DROP DATABASE IF EXISTS \"$dbname\"; + " 2>&1 done -if [ $ATTEMPT -eq $MAX_ATTEMPTS ]; then - log_error "Test database failed to initialize after $MAX_ATTEMPTS attempts" - exit 1 -fi +log_success "Test databases cleaned" -# --- 3e. Rustfs (wipe data for fresh start) --- +# --- 3c. Rustfs (wipe data for fresh start) --- log_step "Wiping Rustfs (S3 storage)..." docker compose stop rustfs > /dev/null 2>&1 || true docker compose rm -f rustfs > /dev/null 2>&1 || true @@ -1426,7 +1411,26 @@ fi echo "${C_GREEN}โœ… Created $edit_req_count Edit Requests${C_RESET}" - +# =========================================================================== +# 7c. NAME HISTORY (for testing "formerly" display) +# =========================================================================== +echo -e "\n${C_BLUE}๐Ÿ“ Creating name history entry for test user...${C_RESET}" +USER_TOKEN=$(login "user@example.com" "password") +if [[ -n "$USER_TOKEN" ]]; then + name_resp=$(curl -s -w "\n%{http_code}" -X PUT \ + -H 'Content-Type: application/json' \ + -H "Authorization: Bearer $USER_TOKEN" \ + -d '{"firstName":"Regular","lastName":"Joe","phone":"+447000000001"}' \ + "$BASE_URL/user/profile") + name_code=$(echo "$name_resp" | tail -n1) + if [[ "$name_code" =~ ^2 ]]; then + echo "${C_GREEN}โœ… Name history entry created (Regular User โ†’ Joe shows as 'formerly')${C_RESET}" + else + echo "${C_YELLOW}โš ๏ธ Failed to update user name (HTTP $name_code)${C_RESET}" + fi +else + echo "${C_YELLOW}โš ๏ธ Could not log in as test user for name change${C_RESET}" +fi # =========================================================================== # SUMMARY @@ -1471,7 +1475,7 @@ cd /home/popertots/Crussell/backend export POSTGRES_USER POSTGRES_PASSWORD POSTGRES_HOST POSTGRES_DB GO_TESTING=1 TEST_OUTPUT_FILE=$(mktemp) START_TIME=$(date +%s) -go test -tags "test,dev" -v -p 1 -count=1 ./... 2>&1 | tee "$TEST_OUTPUT_FILE" || true + go test -tags "test,dev" -v -count=1 ./... 2>&1 | tee "$TEST_OUTPUT_FILE" || true END_TIME=$(date +%s) DURATION=$((END_TIME - START_TIME)) MINUTES=$((DURATION / 60)) diff --git a/obsidian/Crussell/Future Work - Gap Backlog.md b/obsidian/Crussell/Future Work - Gap Backlog.md index 10bc9c9..1e7f0e5 100644 --- a/obsidian/Crussell/Future Work - Gap Backlog.md +++ b/obsidian/Crussell/Future Work - Gap Backlog.md @@ -14,7 +14,7 @@ These are blockers: missing functionality that prevents daily operations, legal | # | Gap | Effort | Area | Notes | |---|---|---|---|---| | 1 | **CurrentAppointment action stubs** | M (1d) | Frontend | `Extend` and `Cancel` buttons on Today page are dead. Staff cannot cancel or extend an in-progress appointment from the Today page. Edit, Take Payment, and Reschedule are already wired. | -| 2 | **Reservation/anonymization background cron** | S (2-3h) | Backend | `CleanupOldReservations()`, `AnonymizeStaleGuestAccounts()`, `CleanupExpiredGiftCards()`, `CleanupIdleAccounts()`, `CleanupExpiredFinancialRecords()` all run on `GET /api/availability`. If no one fetches availability for days, expired reservations persist and stale guest data isn't anonymized. Should be a background ticker in `main.go` (or a lightweight cron job). | +| 2 | **Reservation/anonymization background cron** | S (2-3h) | Backend | `CleanupOldReservations()`, `AnonymizeStaleGuestAccounts()`, `CleanupExpiredGiftCards()`, `CleanupIdleAccounts()`, `CleanupExpiredFinancialRecords()`, `CleanupOldNameHistory()` all run on `GET /api/availability`. If no one fetches availability for days, expired reservations persist and stale guest data isn't anonymized. Should be a background ticker in `main.go` (or a lightweight cron job). | | 3 | **VAT/Tax export endpoints** | M (1-2d) | Backend | `get_vat_return_data()` and `export_sales_transactions()` SQL functions exist. No admin API to trigger them. Needed for HMRC Making Tax Digital compliance. | | 4 | **Password reset flow** | S (2-3h) | Frontend | Backend has `/api/verify/generate` and `/api/verify/check`. Login page has no "forgot password" link or form. Customers who forget their password must call the salon. | | 5 | **Email verification flow** | S (2-3h) | Frontend | Users register with `unverified_email` role. No UI to enter verification code or resend. `+layout.svelte` has an alert-based prototype that needs to be wired properly. | diff --git a/obsidian/Crussell/Overview.md b/obsidian/Crussell/Overview.md index 4cbdd9f..e0e0a36 100644 --- a/obsidian/Crussell/Overview.md +++ b/obsidian/Crussell/Overview.md @@ -204,15 +204,15 @@ npm run dev # Dev server with HMR ```bash cd backend -go test -tags "test,dev" -p 1 -count=1 ./... # 782/785 passing, 3 skipped +go test -tags "test,dev" ./... # 953/957 passing, 8 skipped go test -tags "test,dev" -v -run TestName ./... # Single test ``` Test infrastructure notes: -- Shared test database (`crussell_test`), sequential execution (`-p 1`) +- **Per-package databases:** Each package gets its own `crussell_test_*` database, created in `TestMain` via `CreateTestDatabase()`. Enables parallel execution (`-p` defaults to `GOMAXPROCS`). +- **โš ๏ธ Build tag:** Always use `-tags "test,dev"`. The `dev` tag is required by Square mock (`internal/square/square_dev.go`) and rate limiter (`mw/ratelimit_dev.go`). Without it, handlers/payments and handlers/bookings tests are silently skipped. - `TestMain` per package โ€” schema migration runs once per package -- `TruncateTables()` between tests โ€” `TRUNCATE TABLE ... CASCADE` (~60% faster than DROP+CREATE) -- Advisory locks: `pg_advisory_lock(1337)` protects migration DDL, `pg_advisory_lock(1338)` prevents CASCADE deadlocks +- `TruncateTables()` between tests โ€” single `TRUNCATE TABLE ... CASCADE` statement (down from 44 separate truncates), no advisory locks - Statement-by-statement SQL parser (`splitSQLStatements()`) respects dollar-quoted PL/pgSQL blocks - Build tag: all test files use `//go:build test` - Test JWT secret: `test-secret-key-for-testing-only` diff --git a/obsidian/Crussell/Technical Manual.md b/obsidian/Crussell/Technical Manual.md index a38491a..16a7de9 100644 --- a/obsidian/Crussell/Technical Manual.md +++ b/obsidian/Crussell/Technical Manual.md @@ -692,7 +692,7 @@ pending/confirmed โ”€โ”€(deadline passed)โ”€โ”€โ†’ pending_release โ”€โ”€(slot c 2. Subtracting existing bookings (with gap logic) 3. Subtracting time blockers (including reservations) 4. **Late night lock**: After 22:00, blocks next morning 00:00-11:00 for non-admin users -5. Triggers `CleanupOldReservations()`, `AnonymizeStaleGuestAccounts()`, `CleanupExpiredLoyaltyRedemptions()`, `CleanupExpiredFinancialRecords()`, `CleanupExpiredGiftCards()`, `CleanupIdleAccounts()` +5. Triggers `CleanupOldReservations()`, `AnonymizeStaleGuestAccounts()`, `CleanupExpiredLoyaltyRedemptions()`, `CleanupExpiredFinancialRecords()`, `CleanupExpiredGiftCards()`, `CleanupIdleAccounts()`, `CleanupOldNameHistory()` **Time Blockers:** Can be one-off (no cron) or recurring (cron expression). Cron expansion via `robfig/cron/v3` parser. @@ -1124,15 +1124,16 @@ Files with this pattern: `bookings.go` (4 handlers), `custom_services.go`, `user ### Test Configuration -- **Shared test database:** `crussell_test` -- **Sequential execution:** `go test -p 1` +- **Per-package databases:** Each package gets its own `crussell_test_*` database, created in `TestMain` via `CreateTestDatabase()` +- **Parallel execution:** `-p` defaults to `GOMAXPROCS` โ€” packages run concurrently against their own databases +- **โš ๏ธ Build tag:** Always `-tags "test,dev"`. Without `dev`, `square_dev.go` (mock client) and `ratelimit_dev.go` are excluded, causing `handlers/payments` and `handlers/bookings` tests to be silently skipped. - **Build tag:** `//go:build test` - **Test secret:** `test-secret-key-for-testing-only` - **Fixtures:** Auto-generate unique emails using `fmt.Sprintf("test-%d-%d@example.com", time.Now().UnixNano(), rand.Int())` ### Test Coverage -**633/636 tests passing** (3 skipped) across 14+ packages. +**953/957 tests passing** (8 skipped) across 18 packages. | Package | Coverage Area | |---------|--------------| diff --git a/obsidian/Crussell/Testing Architecture & DB Management.md b/obsidian/Crussell/Testing Architecture & DB Management.md index b2868e9..4997337 100644 --- a/obsidian/Crussell/Testing Architecture & DB Management.md +++ b/obsidian/Crussell/Testing Architecture & DB Management.md @@ -1,123 +1,201 @@ # Testing Architecture & DB Management -**Last Updated:** June 2026 +**Last Updated:** June 2026 (major revision โ€” per-package databases, parallel execution) --- -## 1. Running Tests +## 1. Architecture Overview + +Every Go package with integration tests gets **its own isolated PostgreSQL database**. This eliminates all cross-package test contamination and enables parallel test execution. + +``` +โ”Œโ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ” โ”Œโ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ” +โ”‚ handlers/bookings โ”‚ โ”€โ”€โ”€ โ”‚ crussell_test_bookings โ”‚ +โ”œโ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”ค โ”œโ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”ค +โ”‚ handlers/auth โ”‚ โ”€โ”€โ”€ โ”‚ crussell_test_handlers_auth โ”‚ +โ”œโ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”ค โ”œโ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”ค +โ”‚ handlers/payments โ”‚ โ”€โ”€โ”€ โ”‚ crussell_test_handlers_payments โ”‚ +โ”œโ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”ค โ”œโ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”ค +โ”‚ ... โ”‚ โ”‚ ... โ”‚ +โ””โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”˜ โ””โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”˜ +``` + +Each database is created on demand by `testdb.CreateTestDatabase(dbName)` in the package's `TestMain`, migrated with the full schema (`init-scripts/init-script.sql`), and dropped by `testdb.DestroyTestDatabase(pool, dbName)` when tests finish. + +--- + +## 2. Running Tests ### Environment Variables -| Var | Purpose | Example | -|-----|---------|---------| -| `TEST_DB_DSN` | Connection string for the test database | `postgres://myuser:mypassword@localhost:5432/crussell_test?sslmode=disable` | -| `JWT_SECRET_KEY` | Signs test JWT tokens | `my_test_secret_key` | -| `GO_TESTING` | Triggers mock clients (Square, etc.); suppresses artificial delays in Square mock | `1` | +| Var | Purpose | Required? | +|-----|---------|-----------| +| `GO_TESTING` | Suppresses artificial delays in Square mock; disables zxcvbn password checks in RegisterHandler | **Yes** for dev server & tests | +| `JWT_SECRET_KEY` | Used by production `main.go` init() | Only for `go run ./main.go` (not needed for tests since the `-test.` flag guard skips it) | + +`POSTGRES_USER`, `POSTGRES_PASSWORD`, `POSTGRES_HOST`, `POSTGRES_DB` are set by `local-dev-2.sh` but NOT consumed by the test infrastructure directly โ€” the test DSN is hardcoded in `testutils/testdb/testdb.go`. ### Commands ```bash -# Full suite (sequential โ€” required to avoid deadlocks) -go test -tags "test,dev" -p 1 -count=1 ./... +# Full suite (parallel โ€” packages run concurrently against their own databases) +go test -tags "test,dev" -count=1 ./... # Single package go test -tags "test,dev" -v -count=1 ./handlers/payments/ # Single test -go test -tags "test,dev" -v -run TestAcquirePaymentLock_Confirmed ./handlers/payments/ +go test -tags "test,dev" -v -count=1 -run TestMyTest ./handlers/payments/ + +# Compile-check only (zero-test run) +go test -tags "test,dev" -count=1 -run ^$ ./path/to/package ``` -`JWT_SECRET_KEY=test-secret-key` is required for the root package tests (main.go init() reads JWT_SECRET_KEY). +Packages **can safely run in parallel** (`-p` defaults to `GOMAXPROCS`). Each package has its own database, so there's no advisory lock contention. The `-p 1` flag from the old architecture is **no longer required or recommended**. -The `-p 1` flag serialises packages so their `TestMain` migrations don't clash. **Never omit it.** +### What Happens When You Run Tests + +1. `go test` compiles each package's test binary +2. Each binary starts โ†’ runs `TestMain` โ†’ calls `CreateTestDatabase("crussell_test_")` +3. `CreateTestDatabase`: + - Connects to `mydb` (admin DB) + - Drops the package's old test database if it exists + - Creates a fresh database + - Runs `init-scripts/init-script.sql` (full schema migration) + - Returns a pool connected to the new database +4. Tests run, each starting with `resetTestData(t)` which does a **single `TRUNCATE ... CASCADE`** across all tables +5. `TestMain` calls `DestroyTestDatabase` which closes the pool and drops the database + +**Duration:** Full suite ~40s (parallel), ~90s (serial `-p 1`) --- -## 2. Database Setup +## 3. Package Setup โ€” `TestMain` Pattern -### Schema Lifecycle - -Every package that runs integration tests has its own `TestMain` that: -1. Creates a connection pool via `testdb.NewPool("")` -2. Calls `testdb.Migrate(t, pool)` โ€” drops everything (tables + enums) in CASCADE order, then re-runs `init-scripts/init-script.sql` statement-by-statement -3. Assigns `db.DB = pool` for the handlers - -Each individual test calls `resetTestData(t)` which **truncates** all tables (CASCADE), leaving the schema intact. - -### What You Need To Do When Adding a New Table or Enum - -| Change | File to Edit | What to Add | -|--------|-------------|-------------| -| New table | `backend/testutils/testdb/testdb.go` โ€” append table name to **beginning** of `dropOrder` array AND to `tables` list in `TruncateTables` | The table's Go identifier | -| New enum type | `backend/testutils/testdb/testdb.go` โ€” add to `typeDrops` array | `"DROP TYPE IF EXISTS new_type CASCADE"` | -| New `booking_status` value | `init-scripts/init-script.sql` โ€” update `CREATE TYPE booking_status AS ENUM (...)` | The new value in the list | - -If you forget these, `testdb.Migrate` will fail with `relation "X" already exists` or `type "X" already exists` because the old objects survive between runs. - ---- - -## 3. Writing Tests โ€” Patterns That Work - -### Basic Structure +Every package with integration tests follows this exact pattern: ```go //go:build test -func TestMyHandler(t *testing.T) { - resetTestData(t) // only when crossing packages or testing schema-level changes - // seed fixtures, build request, call handler, assert +package mypackage + +import ( + "os" + "testing" + + "crussell/db" + "crussell/testutils/jwt" + "crussell/testutils/testdb" +) + +func TestMain(m *testing.M) { + pool := testdb.CreateTestDatabase("crussell_test_mypackage") + db.DB = pool + jwt.Init() + // Any additional global setup (Square client, DAV service, etc.) + code := m.Run() + testdb.DestroyTestDatabase(pool, "crussell_test_mypackage") + os.Exit(code) +} + +func resetTestData(t *testing.T) { + t.Helper() + testdb.TruncateTables(t, db.DB) } ``` -**Don't reset the DB if you don't need to.** Full truncation (`resetTestData`) is expensive โ€” it acquires advisory locks, cascades through every table, and re-seeds working hours. Prefer creating fresh entities per test: +### Database naming convention -```go -// Fast: create a new user + booking for each test, rely on unique IDs -userID, err := fixtures.CreateTestUser(db.DB) -bookingID, err := fixtures.CreateTestBookingAtTime(db.DB, userID, serviceID, someTime) +`crussell_test_` + +| Package path | Database name | +|---|---| +| `.` (root) | `crussell_test` | +| `auth` | `crussell_test_auth` | +| `handlers/admin` | `crussell_test_handlers_admin` | +| `handlers/bookings` | `crussell_test_handlers_bookings` | +| `handlers/user` | `crussell_test_handlers_user` | +| `handlers/today` | `crussell_test_handlers_today` | + +### Exceptions + +- **`auth/jwt_test.go`**: Uses the standard `CreateTestDatabase` + `DestroyTestDatabase` pattern. JTI revocation tests require the database; non-DB tests (token format, uniqueness) work without it. +- **`db/db_test.go`**: Manages its own connection pool via `db.Connect()`. Uses `CreateTestDatabase` to ensure the database exists, but the pool is managed separately. + +--- + +## 4. Adding a New Test Package + +1. **Create the test file(s)** in the package directory with `//go:build test` constraint +2. **Add TestMain** (see ยง3 pattern). Use the naming convention for the database name. +3. **Add the database name** to the cleanup list in `local-dev-2.sh` if it doesn't match `crussell_test*`: + +```bash +# In local-dev-2.sh โ€” the cleanup regex catches crussell_test* by default +docker exec postgres psql -U myuser -d mydb -t -c " + SELECT datname FROM pg_database WHERE datname LIKE 'crussell_test%'; +" | grep crussell_test | while read -r dbname; do ... done ``` -This is safe as long as tests in the same package don't assert on global counts (total users, total bookings) and don't mutate shared data in conflicting ways. Each test's entities get their own IDs, so they never collide. +### What It Creates Automatically -**`resetTestData(t)` is required when:** -- Testing schema changes or enum value drops -- Testing global aggregate queries (total bookings, revenue sums) -- A previous test may have left state that would cause a false positive/negative (ambiguous โ€” use your judgement) -- Switching packages (each `TestMain` calls `testdb.Migrate` which drops everything) +- Each package's first test run: `CreateTestDatabase` drops old DB โ†’ creates new DB โ†’ runs `init-script.sql` migration (~1s) +- Subsequent runs: same flow but creates fresh database each time -**`resetTestData(t)` is wasteful when:** -- Every test creates its own user + booking via fixtures -- Tests only read or update their own rows -- The only shared table is `working_hours` (seed once in the first test, or use `ON CONFLICT DO UPDATE`) +--- + +## 5. Adding a New Table or Type to the Schema + +When you modify `init-scripts/init-script.sql` to add tables or types, you must update the teardown lists in `backend/testutils/testdb/testdb.go`: + +| Change | What to Do | +|--------|-----------| +| **New table** | Add to `dropOrder` array AND to the `TRUNCATE TABLE ... CASCADE` statement in `TruncateTables` | +| **New enum type** | Add to `typeDrops` array in `Migrate` | +| **New sequence** | Add to `seqDrops` | +| **Removed table/type** | Remove from the corresponding lists | + +**If you forget:** The test will either: +- Hang on the second run when `Migrate` tries to `DROP TYPE ... CASCADE` and fails because a table that depends on the type isn't in the drop chain +- Fail with `relation "X" already exists` when `init-script.sql` tries to CREATE something that survived the DROP phase + +Both are caught immediately by running the test suite twice. + +--- + +## 6. Writing Tests + +### Basic Structure + +```go +func TestMyHandler(t *testing.T) { + resetTestData(t) // clean slate + // create fixtures, make request, assert +} +``` + +Every test should call `resetTestData(t)` at the start. It's cheap now โ€” a single `TRUNCATE TABLE ... CASCADE` statement, no advisory lock. + +### Fixtures + +Use helpers from `crussell/testutils/fixtures`: + +```go +userID, err := fixtures.CreateTestUser(db.DB) +serviceID, err := fixtures.CreateTestService(db.DB) +bookingID, err := fixtures.CreateTestBookingAtTime(db.DB, userID, serviceID, time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) +``` ### Test Helpers per Package -Each test package defines its own helpers. Look at the existing test file before writing new ones: +| Package | Key Helpers | +|---------|------------| +| `bookings` | `makeRequest`, `makeAuthRequest`, `makeAdminRequest`, `serveChiHandler`, `serveAdminHandler` | +| `payments` | `makePaymentRequest`, `setupPaymentStatusTest(status)`, `setupTestDataPast(t)`, `setupDepositBookingPast(t)` | +| `admin` | `makeAdminRequest`, `makeUserRequest` | +| `scheduling` | Package-specific helpers in test files | -| Package | Helper File | Key Functions | -|---------|------------|---------------| -| `bookings` | `bookings_test.go` | `makeRequest`, `makeAuthRequest`, `makeAdminRequest`, `serveChiHandler`, `serveAdminHandler` | -| `payments` | `payments_test.go` | `makePaymentRequest`, `setupPaymentStatusTest(status)`, `setupTestDataPast(t)`, `setupDepositBookingPast(t)` | -| `admin` | `test_helpers.go` | `makeAdminRequest`, `makeUserRequest`, `resetTestData` | -| `scheduling` | (in test files) | Package-specific helpers | - -Chi URL params need manual wiring. Use `serveChiHandler` or `serveAdminHandler` for routes with path params like `{id}`. - -### Avoiding Payment-Split Headaches - -`CreateBookingPayment` splits future-dated payments into deposit + non-deposit records. This causes payment-count assertions to fail if you aren't expecting it. - -```go -// Booking in the past โ†’ no split (correct for idempotency / count tests) -userID, bookingID, _ := setupTestDataPast(t) - -// Future booking โ†’ split fires (correct for verifying 2-record output) -userID, bookingID := setupDepositBooking(t) -``` - -Helpers live in `backend/handlers/payments/payments_test.go`. - -### Seeding Working Hours +### Working Hours Weekday mapping: `0=Monday, 6=Sunday` (converted from Go's `time.Weekday`). @@ -130,178 +208,194 @@ if todayWeekday == 0 { } ``` -### Seeding Exceptional Hours (Holidays) +### Cursor Pagination in Tests + +Cursor values contain timestamps with `+` timezone offsets. Always URL-encode them: ```go -var groupID int -db.DB.QueryRow(ctx, `INSERT INTO exceptional_working_hours_groups (name) VALUES ('Test') RETURNING id`).Scan(&groupID) -db.DB.Exec(ctx, `INSERT INTO exceptional_working_hours (group_id, weekday, start_time, end_time, is_open) VALUES ($1, $2, '00:00', '00:00', false)`, groupID, todayWeekday) -db.DB.Exec(ctx, `INSERT INTO exceptional_group_applications (group_id, week_start) VALUES ($1, $2::date)`, groupID, mondayStr) +nextCursor := resp.NextCursor +req := httptest.NewRequest("GET", "/api/admin/bookings?cursor="+url.QueryEscape(nextCursor), nil) ``` -The column is `week_start`, **not** `monday_week_start`. +### Parallelism Within a Package -### Auth & JWT Tests +Tests within a package run **sequentially** (no `t.Parallel()`). Each test calls `resetTestData` which does a global `TRUNCATE`, so parallel tests would corrupt each other's data. Making intra-package parallelism safe would require wrapping each test in its own transaction โ€” a significant refactor. -The `crussell/auth` package now connects to the test database for JTI revocation tests. Its `TestMain` calls `testdb.NewPool("")` and `testdb.Migrate()`: +--- +## 7. What Can Go Wrong & How to Fix It + +### Test suite hangs on startup (no output) + +**Likely cause:** Stale `crussell_test_*` database from a killed test run has active connections, preventing `CreateTestDatabase` from dropping it. + +**Fix:** +```bash +docker exec postgres psql -U myuser -d mydb -t -c " + SELECT pg_terminate_backend(pid) + FROM pg_stat_activity + WHERE datname LIKE 'crussell_test%' AND pid != pg_backend_pid(); +" +``` + +Or drop and recreate the admin DB connection: +```bash +docker exec postgres psql -U myuser -d mydb -c " + SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE datname LIKE 'crussell_test%'; + DROP DATABASE IF EXISTS crussell_test_handlers_bookings; +" +``` + +### `database "crussell_test_*" is being accessed by other users` + +`DestroyTestDatabase` failed to drop the database because connections lingered after `pool.Close()`. The retry loop in `DestroyTestDatabase` handles this, but if it exhausts 3 attempts, it logs a warning and moves on. The stale database is cleaned up by the next test run's `CreateTestDatabase`. + +If you see this repeatedly, increase the retry count in `DestroyTestDatabase`: ```go -func TestMain(m *testing.M) { - InitJWT("test-secret-key-for-jwt-test") - pool, err := testdb.NewPool("") - if err != nil { - fmt.Fprintf(os.Stderr, "WARN: No test DB: %v\n", err) - } else { - testdb.Migrate(&testing.T{}, pool) - db.DB = pool - } - code := m.Run() - if pool != nil { pool.Close() } - os.Exit(code) -} +for attempt := 0; attempt < 5; attempt++ { ... } ``` -Tests that need a database call `requiresDB(t)` which skips if `db.DB` is nil. +### Tests fail after schema changes -**Important:** `GO_TESTING=1` env var skips the server-side zxcvbn password strength check in `RegisterHandler`. Without it, seeded passwords like `"password"` are rejected (zxcvbn score 0). The dev server and CI should always set this. +**Old schema objects survive in the test database.** `CreateTestDatabase` creates a fresh database each time, so this shouldn't happen. But if you changed `init-script.sql` while a test binary was running, or if Docker volumes persist stale data: -### Testing Pagination Caps - -```go -// per_page=500 accepted -w := makeAdminRequest(handler, "GET", "/api/admin/bookings?per_page=500", nil) -// per_page=600 โ†’ capped to default 10 -``` - ---- - -## 4. Current Status - -- **All 16 packages pass**, 0 failed -- Skipped: `TestAdminNotifications_List` / `TestAdminNotifications_Acknowledge` โ€” WIP, no handler yet (also `mw/ratelimit_test.go` skipped by `!dev` build tag) -- All packages pass clean: 16 tested packages (plus 10 with no test files) -- `GO_TESTING=1` suppresses the 1s/3s artificial delays in Square mock (`internal/square/square_dev.go`) -- New test files: - - `handlers/payments/loyalty_test.go` โ€” 10 tests (apply-redemption + campaign auto-apply) - - `handlers/payments/refund_exclude_test.go` โ€” 2 tests (discount/OTH exclusion) - - `handlers/bookings/dedup_test.go` โ€” 8 tests (dedup guards + no-show) - - `handlers/payments/discount_preview_test.go` โ€” 13 tests (discount preview endpoint) - - `handlers/payments/payment_status_test.go` โ€” 32 tests (status guards, payment lock, release lock) - - `handlers/payments/refunds_test.go` โ€” 20 tests (cancellation refund calc + processing) - - `handlers/bookings/deposit_test.go` โ€” 30 tests (deposit fields, eviction, admin cancel) - - `handlers/webhooks/webhooks_test.go` โ€” 13 tests (Square webhook signature + dispatch) - - `handlers/admin/discount_campaigns_test.go` โ€” 11 tests (campaign CRUD + stats) - - `handlers/user/patch_tests_test.go` โ€” 7 tests (user patch test list/delete) -- Extended tests: `handlers/scheduling/time_blockers_test.go` (CleanupExpiredLoyaltyRedemptions), `handlers/notifications/notifications_test.go` (AcknowledgePendingBookingNotification), `handlers/payments/payment_status_test.go` (ReleasePaymentLock) -- Schema changes: `patch_tests.id` now uses `generate_patch_test_id()` (was `generate_service_id()`); `user_patch_tests.id` changed from `BIGSERIAL` to `CHAR(12)` with `generate_user_patch_test_id()` - ---- - -## 5. Debugging โ€” Issues & Resolutions - -### Test suite hangs on startup (0% CPU, no output) - -**Why:** `testdb.TruncateTables` or `testdb.Migrate` blocks on a lock held by another session. - -**Previously resolved by:** -- Killing the Go dev server (`go run ./main.go`) โ€” it holds open connections -- Fixing leaked rows/transactions โ€” `.Query()` without `rows.Close()` or `.Begin()` without commit/rollback leaves the connection in a bad state -- Enforcing `pool.Acquire()` instead of `pool.Exec()` for advisory locks โ€” lock and work **must** use the same connection (see `testdb.go` TruncateTables for the pattern) - -**What to do:** ```bash -docker exec postgres psql -U myuser -d crussell_test -c "SELECT pid, query, state FROM pg_stat_activity WHERE state != 'idle';" -``` -Kill stale connections: -```sql -SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE datname = 'crussell_test' AND pid != pg_backend_pid(); +# Nuclear option โ€” drop ALL test databases +docker exec postgres psql -U myuser -d mydb -t -c " + SELECT datname FROM pg_database WHERE datname LIKE 'crussell_test%'; +" | grep crussell_test | while read db; do + docker exec postgres psql -U myuser -d mydb -c " + SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE datname = '$db'; + DROP DATABASE \"$db\"; + " +done ``` ---- +### `relation "X" already exists` or `type "X" already exists` -### Tests fail with `relation "X" does not exist` during migration +You added a new table/type to `init-script.sql` but forgot to add it to the drop lists in `testdb.go`. See ยง5. -**Why:** Table A references Table B but A is created before B in `init-script.sql`. +### Deadlock detected -**Previously resolved by:** Moving inline FK constraints to `ALTER TABLE ADD CONSTRAINT` statements at the bottom of `init-script.sql`. +Should not happen with per-package databases. If it does, the cause is likely two concurrent `TruncateTables` calls from the same test binary (within a package). This would require `t.Parallel()` which we don't use. If you're adding parallel tests, make sure they don't share database access. -**What to do:** Declare the column without a REFERENCES clause, then add the constraint at the end of the file. +### OptionalAuth: invalid token: ... illegal base64 data + +This is **expected** โ€” the test `TestOptionalAuth_InvalidToken` sends a deliberately malformed token and the middleware logs the error. The **test passes.** This is just noisy output, not a failure. + +### `Warning: Failed to delete CardDAV contact` + +This appears in `TestAccount_DeleteGuest` and `TestLoyalty_Get`. The `dav.Service` is initialized as `&dav.BaseService{}` with a nil database connection. The CardDAV cleanup is a best-effort background goroutine. The **test passes.** This is harmless noise. --- -### Tests fail with `relation "X" already exists` or `type "X" already exists` +## 8. Performance -**Why:** A new table or enum was added to the schema but not to the test teardown lists. +### Current Baseline -**Previously resolved by:** Adding the table to `dropOrder` + `tables` in `TruncateTables`, or the type to `typeDrops`, in `backend/testutils/testdb/testdb.go`. +| Metric | Value | +|--------|-------| +| Serial (`-p 1`) | ~90s | +| **Parallel** | **~40s** | +| Packages | 18 tested, 0 failures | +| Tests | 949 passing, 8 skipped, 0 failing | -**What to do:** Follow the table in ยง2. If you see this for an existing object, check whether the test database has stale objects from a previous schema version โ€” truncate + recreate manually. +### What Drives Test Time + +| Component | Time | +|-----------|------| +| `CREATE DATABASE` + migration per package | ~1s ร— 15 packages = ~4s (parallelized) | +| Test execution (heaviest: `bookings` 245 tests) | ~25s | +| Test execution (admin 156 tests) | ~16s | +| Test execution (payments: 120+ tests) | ~13s | + +### Bottleneck + +The `CREATE DATABASE` operation serializes at the PostgreSQL catalog level. With 15+ packages creating databases concurrently, they queue on catalog locks. This adds ~4-8s of overhead in parallel mode. + +### How to Make It Faster + +| Approach | Gain | Effort | +|----------|------|--------| +| **Shard slow packages** (split `bookings` test files โ†’ parallel sub-packages) | 25s โ†’ ~13s (halve bookings) | Moderate โ€” 2-3 hours refactoring | +| **Template databases** (create once, clone via `CREATE DATABASE ... TEMPLATE`) | ~4-8s saved on database creation | Low โ€” was attempted, needs careful parallel-safety work | +| **`t.Parallel()` within packages** with transaction-per-test | ~25s โ†’ ~15s (bookings halved again) | High โ€” major test refactor | +| **Reduce polling in Square mock** | Already done (0.012s, was 8s) | โœ… Complete | --- -### Deadlock detected during `TRUNCATE` +## 9. FAQ -**Why:** Two concurrent `TruncateTables` calls each hold one end of a circular FK lock chain. +### Q: Can I run `go test ./pkg1/ & go test ./pkg2/ &` in separate terminals? -**Previously resolved by:** -- Adding `pg_advisory_lock(1338)` to serialise all truncation (lock ID 1338; migrate uses 1337 โ€” must not collide) -- Enforcing `pool.Acquire()` so lock, work, and unlock share one connection -- Always using `-p 1` to serialise packages +**Yes.** Each package gets its own database. No shared state, no lock contention. This is the entire point of the per-package database architecture. -**What to do:** -1. Kill duplicate `go test` processes -2. Kill stale connections (see first FAQ) -3. Verify `TruncateTables` uses `pool.Acquire()` with lock 1338 -4. Run with `-p 1` +### Q: Why don't we use `t.Parallel()`? ---- +Because `resetTestData(t)` does a global `TRUNCATE ... CASCADE`. Two parallel tests would truncate each other's data mid-flight. Fixing this requires either: +- Per-test transactions with rollback (wrap every DB operation in a test-scoped `Tx`, roll back at the end) +- Separate PostgreSQL schemas per test -### Tests get 0 results from queries using enum values +Both are significant refactors. For now, intra-package parallelism isn't worth the complexity. -**Why:** You removed a value from a PG enum (e.g. `no_deposit` from `booking_status`) but a `WHERE status NOT IN (...)` query still references it. PG returns an error for enum literal checks against removed values โ€” if the error is silently ignored (e.g. `_ = db.DB.QueryRow(...).Scan(&x)`), the target variable stays at Go zero value. +### Q: What happens if `go test` is killed mid-run? -**Previously resolved by:** Replacing all `'no_deposit'` references with the replacement status (`'deposit_lapsed'`) across: -- `backend/handlers/today/today.go` (2 occurrences) -- `backend/handlers/bookings/bookings.go` (multiple) -- `backend/handlers/bookings/manage.go` -- `backend/handlers/scheduling/default-hours.go` -- Frontend `*.svelte` files referencing the old status name +Stale `crussell_test_*` databases are left behind. The next full test run cleans them up in `CreateTestDatabase` (which drops the old database before creating a new one). The cleanup script in `local-dev-2.sh` also drops all `crussell_test*` databases at startup. + +If a database can't be dropped because of stale connections, `DestroyTestDatabase` tries `pg_terminate_backend` + retry loop. If that fails after 3 attempts, it logs a warning and moves on. The database is cleaned up on the next run. + +### Q: Why is `testdb.Migrate` called only once per package run? + +It's called inside `CreateTestDatabase`, which is called in `TestMain`. Since each package gets a fresh database per test binary run, migration only happens once. Individual tests don't call `Migrate` โ€” they call `TruncateTables` via `resetTestData`, which is now a single `TRUNCATE TABLE ... CASCADE`. + +### Q: Do I need to worry about advisory locks? + +**No.** Advisory locks (`pg_advisory_lock`) have been removed from both `Migrate` and `TruncateTables`. With per-package databases, there's no cross-package contention. The old lock IDs (1337, 1338) are no longer used. + +### Q: What's the `GO_TESTING` env var for? + +It's set by `local-dev-2.sh` and consumed in several places: +- `internal/square/square_dev.go`: `mockSleep(d)` skips the sleep when `GO_TESTING=1` (this is how the 4s checkout polling delay is bypassed) +- `handlers/auth/local.go`: zxcvbn password strength check is skipped (seeded passwords like "password" would otherwise be rejected) +- `main.go` init(): checks for `-test.` flags in os.Args (more precise than GO_TESTING) to skip JWT initialization during test runs + +### Q: Why does `main.go` skip JWT init during tests? + +The root `main.go` package's `init()` function reads `JWT_SECRET_KEY` and calls `auth.InitJWT()`. When the root package's tests run, `JWT_SECRET_KEY` may not be set. The `init()` function checks `os.Args` for `-test.` flags โ€” if present, it's a test binary, and JWT init is handled by `testutils/jwt` which each test package imports. + +This uses `strings.HasPrefix(arg, "-test.")` rather than the env var `GO_TESTING` because the env var can persist in the shell environment and leak into non-test contexts (like the seeding script that starts the actual server). + +### Q: I added a new test package and it shows `[no test files]` + +Make sure your test file has the `//go:build test` constraint and that you're passing the `test` build tag: -**What to do:** Grep the full codebase for the removed enum value before dropping it from the schema: ```bash -grep -rn "'old_value'" backend/ frontend/ | grep -v "_test.go" | grep -v ".md" +go test -tags "test,dev" ./path/to/package/ ``` ---- +The `dev` tag is required by some packages (Square mock, rate limiter). Always use `-tags "test,dev"`. -### Panic: `interface conversion: interface {} is nil, not string` during auth +### Q: Tests pass in isolation but fail in the full suite -**Why:** Test request is missing auth context โ€” token not set, or context key not wired. +This was a common issue in the old architecture (shared `crussell_test` database). With per-package databases, **this should no longer happen.** Each package is fully isolated. If you see it, check: +1. Are two tests in the **same package** interfering? (Shared state, global variables) +2. Is the test environment different? (Different env vars, Docker state) -**Previously resolved by:** Ensuring `jwt.Init()` runs in `TestMain` and the test uses `makeAuthRequest` (or manually injects `mw.UserIDKey`/`mw.UserRoleKey` into the request context). +### Q: How do I debug a hanging test? -**What to do:** Look at how `makePaymentAuthRequest` in `payments_test.go` or `makeRequestWithContext` in `admin/test_helpers.go` sets up auth context. Copy that pattern. +The hang is almost certainly in `CreateTestDatabase` or the first `resetTestData` call. Run with verbose output and a timeout: ---- - -### Cursor-paginated tests return 0 items on the second page - -**Why:** The cursor contains a `+` timezone offset (e.g. `2026-06-17T12:49:32+01:00`) which, when passed unescaped in a URL query string, is decoded as a space. The timestamp parser then fails because it expects `Z07:00` format. - -**Previously resolved by:** Using `url.QueryEscape(cursor)` when building the request URL, or โ€” for tests that only need to verify basic listing โ€” skipping the second-page assertion entirely. - -**What to do:** If you need to test second-page results, URL-encode the cursor value. If the test only needs to verify the listing contract (total, per_page, presence of next_cursor), the first-page assertions are sufficient. - ---- - -### Pre-existing test files don't compile - -**Why:** Tests with `//go:build test` constraint are only compiled when the `test` build tag is active. `go build` without test tags skips them entirely, so compilation errors in test files are silent until you run `go test`. - -**Previously resolved by:** Running `go test -tags "test,dev" -count=1 -run ^$ ./path/` โ€” a zero-test run that still compiles all test files โ€” as part of every PR verification. - -**What to do:** After changing test files, always run a compile check: ```bash -go test -tags "test,dev" -count=1 -run ^$ ./package/... +timeout 30 go test -tags "test,dev" -v -count=1 ./mypackage/ 2>&1 | head -20 ``` -This catches test-only compilation errors without executing any tests. + +If you see no output at all for 30s, the hang is in `CreateTestDatabase`. Check for stale database connections (see ยง7). + +### Q: I see `WARN: No test DB available` from `auth/jwt_test.go` + +This shouldn't appear anymore โ€” the auth package's TestMain was updated to use `CreateTestDatabase` like all other packages. If you see it, the auth package TestMain wasn't updated. Fix: replace `testdb.NewPool("")` with `testdb.CreateTestDatabase("crussell_test_auth")`. + +### Q: What's the total test count? + +Current: ~957 tests (18 packages, 0 failures, 8 skipped). +Tracked by counting `^=== RUN` lines in the output (excluding sub-tests).