diff --git a/README.md b/README.md index 020ee8b..f5a5bcd 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,7 @@ Nail salon booking platform — Go 1.25 backend + SvelteKit 5 SPA + PostgreSQL 1 **Loyalty & Discounts**: 1 stamp per paid appointment (max 1/day). 10 stamps → 10% off via opt-in checkbox at payment or till. Stamps refunded on cancellation. Campaigns auto-apply at both payment and completion: time-based, per-user milestone, global milestone (in-person only), anniversary. All discounts stack additively against original total. Discount payment records excluded from refund calculations. -**Compliance**: GDPR Article 15 data export (async, 12h cache, 16-section JSON + PDF). Account deletion with external system scrubbing (S3, Square). Guest PII anonymized 6 months post-appointment. UK financial data retention (7 years). Gift card SPV/MPV VAT treatment configurable. +**Compliance**: GDPR Article 15 data export (async, 12h cache, 21-section JSON + PDF — excludes verification codes as authentication tokens). Account deletion with external system scrubbing (S3, Square). Guest PII anonymized 6 months post-appointment. UK financial data retention (7 years). Gift card SPV/MPV VAT treatment configurable. **Frontend**: Portfolio gallery with fuzzy tag search (relevance-sorted) and exact category filters (date-sorted), multi-format images (AVIF/WebP/JPEG/JXL with WASM client-side encoding), cursor-based pagination. MapLibre GL map on contact page. PhoneInput component with UK validation. CharCounter for long notes. @@ -74,7 +74,8 @@ 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" ./... # 953/957 passing, 8 skipped +cd backend && go test -tags "test,dev" -count=1 -parallel 8 ./... # ~960 tests, 0 failures, 4 skipped (~9s) +cd backend && go test -tags "test,dev" -count=10 -parallel 8 ./... # thorough verification (~39s) ``` ## Full Documentation diff --git a/init-scripts/init-script.sql b/init-scripts/init-script.sql index 5fccd70..c626c90 100644 --- a/init-scripts/init-script.sql +++ b/init-scripts/init-script.sql @@ -1154,15 +1154,6 @@ BEGIN ) ORDER BY ap.created_at DESC), '[]'::json) FROM affiliate_payouts ap WHERE ap.affiliate_id = target_user_id ), - 'verification_codes', ( - SELECT COALESCE(json_agg(json_build_object( - 'purpose', vc.purpose, - 'created_at', vc.created_at, - 'used_at', vc.used_at, - 'expires_at', vc.expires_at - ) ORDER BY vc.created_at DESC), '[]'::json) - FROM verification_codes vc WHERE vc.user_id = target_user_id - ), 'forgiven_no_shows', ( SELECT COALESCE(json_agg(json_build_object( 'id', fns.id, @@ -1195,6 +1186,25 @@ BEGIN FROM gift_card_transactions gct WHERE gct.user_id = target_user_id ), + 'gift_cards', ( + SELECT COALESCE(json_agg(json_build_object( + 'id', gc.id, + 'total_funds_added', gc.total_funds_added, + 'amount_remaining', gc.amount_remaining, + 'expiry_date', gc.expiry_date, + 'redeemed_at', gc.redeemed_at, + 'created_at', gc.created_at + ) ORDER BY gc.created_at DESC), '[]'::json) + FROM gift_cards gc WHERE gc.redeemed_by = target_user_id + ), + 'admin_audit_log', ( + SELECT COALESCE(json_agg(json_build_object( + 'action_type', aal.action_type, + 'details', aal.details, + 'created_at', aal.created_at + ) ORDER BY aal.created_at DESC), '[]'::json) + FROM admin_audit_log aal WHERE aal.target_user_id = export_all_user_data.target_user_id + ), 'login_audit', ( SELECT COALESCE(json_agg(json_build_object( 'attempt_type', la.attempt_type, diff --git a/local-dev-2.sh b/local-dev-2.sh index ef87524..d100ac1 100755 --- a/local-dev-2.sh +++ b/local-dev-2.sh @@ -1475,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 -count=1 ./... 2>&1 | tee "$TEST_OUTPUT_FILE" || true + go test -tags "test,dev" -v -count=1 -parallel 8 ./... 2>&1 | tee "$TEST_OUTPUT_FILE" || true END_TIME=$(date +%s) DURATION=$((END_TIME - START_TIME)) MINUTES=$((DURATION / 60)) diff --git a/obsidian/Crussell/Overview.md b/obsidian/Crussell/Overview.md index e0e0a36..6b509d4 100644 --- a/obsidian/Crussell/Overview.md +++ b/obsidian/Crussell/Overview.md @@ -83,7 +83,7 @@ Campaign lifecycle: `draft → active → completed` (or any → `cancelled`, `a ### Compliance -**GDPR Article 15**: Full data export via `/gdpr` frontend. Async Go endpoint (`GET /api/user/gdpr-export`) with 12h in-memory cache and background generation (navigation away doesn't cancel). 18-section JSON export: user profile (now includes failed_attempts, locked_until), login_audit, refresh_tokens, bookings with overrides, payments, refunds, saved cards, social logins, loyalty redemptions, booking discounts, edit requests, affiliate payouts, verification codes, forgiven no-shows, patch tests, referrals, notification preferences, export metadata. Frontend: skeleton loading, 2s polling, styled report cards/tables, PDF export (print CSS hides navbar + verification banner), raw JSON download. +**GDPR Article 15**: Full data export via `/gdpr` frontend. Async Go endpoint (`GET /api/user/gdpr-export`) with 12h in-memory cache and background generation (navigation away doesn't cancel). 21-section JSON export: user profile, bookings with overrides, payments, refunds, saved cards, social logins, loyalty redemptions, booking discounts, edit requests, affiliate payouts, forgiven no-shows, patch tests, referrals, referral discounts, notification preferences, gift_card_balance, gift_card_transactions, gift_cards, admin_audit_log, login_audit, refresh_tokens, name_history, export metadata. **Verification codes excluded** (authentication tokens are not personal data under GDPR Art 15). Frontend: skeleton loading, 2s polling, styled report cards/tables, PDF export (print CSS hides navbar + verification banner), raw JSON download. **Account deletion**: Registered users → `anonymize_user()` SQL function extended with child table PII scrubbing (social logins deleted, saved cards soft-deleted with PCI data cleared, verification codes expired, time blocker reservations scrubbed, edit request notes nulled, notification preferences deleted). External system scrubbing: S3 profile picture, Square saved cards. Guests → `delete_guest_user()` for full removal. @@ -204,15 +204,17 @@ npm run dev # Dev server with HMR ```bash cd backend -go test -tags "test,dev" ./... # 953/957 passing, 8 skipped +go test -tags "test,dev" ./... # 981 tests, 0 failures, 4 skipped go test -tags "test,dev" -v -run TestName ./... # Single test ``` Test infrastructure notes: - **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 — single `TRUNCATE TABLE ... CASCADE` statement (down from 44 separate truncates), no advisory locks +- **PoolProxy architecture:** `db.Conn` is a `*db.PoolProxy` that routes DB calls through per-test transactions stored in context. Production handlers pass `r.Context()`; tests inject tx context via `req.WithContext(ctx)`. +- **SetupTestTx pattern:** Each test begins a PostgreSQL transaction (`testutils.SetupTestTx(t)`) that automatically rolls back via `t.Cleanup`. No truncation between tests. +- **t.Parallel() supported:** 76% of tests (748/981) use `t.Parallel()` with per-test transaction isolation. 233 remaining in 9 files still use old `SetupTestDB` pattern. +- **PreferSimpleProtocol:** Test pools disable prepared statements to prevent "conn busy" errors on parallel transactions. - 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 16a7de9..faf4aa1 100644 --- a/obsidian/Crussell/Technical Manual.md +++ b/obsidian/Crussell/Technical Manual.md @@ -50,7 +50,7 @@ Backend (:8080) | `handlers/auth` | local.go, social.go | Registration (with referral code validation), login, refresh, email verification | | `handlers/bookings` | bookings.go, reserve.go, manage.go, admin_reserve.go | Booking CRUD, reservations, admin management, edit requests, discounts, closing hours validation, active booking limits, GetBookingsByCreatedRange, created_by_name resolution | | `handlers/payments` | handlers.go, service.go, validators.go, giftcards.go, till.go, refunds.go, refund_policy.go | Square payments: terminal, online, refunds, tips, saved cards, gift cards (CRUD, topup, transfer, redeem, buy, expired balances, till sales). Refund calculation with notice-period tiers and deposit protection | -| `handlers/webhooks` | square.go | Square webhook handler for payment status updates. **Dev-only stub** — production requires signature verification (HMAC-SHA256 with base64 output, `x-square-hmacsha256-signature` header). See `TODO(PROD)` in source. | +| `handlers/webhooks` | square.go | Square webhook handler for payment status updates. **Fail-closed signature check** — rejects requests with 403 when `SQUARE_WEBHOOK_SIGNATURE_KEY` is set but header is missing. Dev mode: skips verification when env var is empty. Still uses hex-encoding stub (`verifySquareSignature`) — production requires HMAC-SHA256 with base64 output, `x-square-hmacsha256-signature` header. See `TODO(PROD)` in source. | | `handlers/admin` | users.go, analytics.go, custom_services.go, discount_campaigns.go, settings.go | Admin user management, custom services CRUD (list/create/get/update/promote/delete), discount campaigns, analytics (stub), business settings (GET/PUT with VAT, gift card config) | | `handlers/today` | today.go | Current/next appointment, today's grid, pending approvals, `DoneForDay` state with daily/weekly summary (`DailySummary` with `total_bookings`, `customers_served`, `summary_scope`), auto-status transitions, closed-day aggregation via `findWeekSummaryRange` + `computeAggregateSummary`. Exceptional hours lookup uses `exceptional_group_applications.week_start` (0=Monday). | | `handlers/user` | profile.go, account.go, guest.go, loyalty.go, customer_relationship.go, gdpr_export.go | User profile, guest creation (with CheckEmailHandler for registered-email detection), loyalty, contact info, GDPR export (async with 12h cache) | @@ -77,6 +77,18 @@ Backend (:8080) - **Connection**: `postgres://USER:PASSWORD@HOST:5432/DB` - **Connection pooling**: Built-in via pgxpool - **Build tags**: `db_dev.go` (dev, localhost) vs `db.go` (prod, env var) +- **Test MaxConns**: 16 per pool (down from 60 — reduced during test migration to prevent connection exhaustion) +- **PoolProxy**: `db.Conn` is a `*PoolProxy` that checks `context.Context` for an active transaction via `TxFromContext()`. All `Exec`/`Query`/`QueryRow` calls route through the transaction if present, otherwise delegate to the pool. + +### DAV Service Isolation (`internal/dav/`) + +The DAV service has a completely separate database connection from the rest of the application: + +- **Separate pool**: `internal/dav/service_prod.go` and `service_dev.go` create their own `*pgxpool.Pool` in `init()`, stored in `dav.Service` +- **Not wrapped in PoolProxy**: DAV queries bypass the transaction routing that PoolProxy provides. DAV operations always hit the real database, not test transactions +- **Usage**: `dav.Service.CreateContact()` (registration), `dav.Service.DeleteContact()` (account deletion), `dav.Service.CreateEvent()` (booking confirmation) — all best-effort side effects after main DB transactions commit +- **Tests**: `dav.Service` is replaced with `&dav.BaseService{}` (nil db) — DAV operations are no-ops during tests +- **CardDAV HTTP path**: `updateCardDAV()` in `handlers/user/profile.go` makes HTTP PUT directly to the SabreDAV PHP server (not using `dav.Service` at all) ### Internal Packages @@ -206,6 +218,8 @@ src/lib/components/ Added in the June 2026 security pass: +### Security Headers + | Header | Value | Location | |--------|-------|----------| | `Content-Security-Policy` | `default-src 'none'; frame-ancestors 'none'` | Global middleware (`main.go:139`) | @@ -213,6 +227,14 @@ Added in the June 2026 security pass: | `Access-Control-Allow-Methods` | `GET, POST, PUT, PATCH, DELETE, OPTIONS` | Global middleware | | `Access-Control-Allow-Headers` | `Authorization, Content-Type, Idempotency-Key` | Global middleware | +### Other Security Fixes + +| Fix | File | Description | +|-----|------|-------------| +| Removed verification code logging | `handlers/auth/local.go:545` | Deleted `log.Printf("DEBUG: Verification code for %s: %s ...")` — was leaking verification codes to stdout | +| Webhook signature fail-closed | `handlers/webhooks/square.go:59-69` | Changed from "skip verification if header missing" to "reject 403 if key set but header missing" | +| S3 delete error checking | `handlers/portfolio/images.go:975` | Changed `s3.Client.Delete(...)` (ignored return) → `if err := s3.Client.Delete(...); err != nil { log.Printf(...) }` | + CORS uses `*` in local dev. In production behind Cloudflare, nginx handles CORS. No CSP violations expected — the SvelteKit SPA doesn't load external scripts or fonts. --- @@ -424,7 +446,7 @@ CORS uses `*` in local dev. In production behind Cloudflare, nginx handles CORS. | `generate_referral_code()` | 12-char referral code with collision detection | | `anonymize_user(target_id)` | GDPR right-to-be-erased for registered users — child table PII scrubbing | | `delete_guest_user(target_id)` | Full removal of guest account | -| `export_all_user_data(target_user_id)` | GDPR Article 15 SAR — 16-section JSON export | +| `export_all_user_data(target_user_id)` | GDPR Article 15 SAR — 21-section JSON export (excludes verification_codes; includes admin_audit_log, gift_cards, name_history) | | `get_vat_return_data(start, end)` | VAT return summary for MTD | | `export_sales_transactions(start, end, include_vat)` | Tax-compatible transaction export | | `get_monthly_business_summary(start, end)` | Monthly revenue breakdown | diff --git a/obsidian/Crussell/Testing Architecture & DB Management.md b/obsidian/Crussell/Testing Architecture & DB Management.md index 4997337..72c5fc8 100644 --- a/obsidian/Crussell/Testing Architecture & DB Management.md +++ b/obsidian/Crussell/Testing Architecture & DB Management.md @@ -1,6 +1,6 @@ # Testing Architecture & DB Management -**Last Updated:** June 2026 (major revision — per-package databases, parallel execution) +**Last Updated:** June 2026 (v2 — flakiness elimination, full t.Parallel coverage, titleCaser concurrency fix, -count=10 verification) --- @@ -39,7 +39,7 @@ Each database is created on demand by `testdb.CreateTestDatabase(dbName)` in the ```bash # Full suite (parallel — packages run concurrently against their own databases) -go test -tags "test,dev" -count=1 ./... +go test -tags "test,dev" -count=1 -parallel 8 ./... # Single package go test -tags "test,dev" -v -count=1 ./handlers/payments/ @@ -63,10 +63,29 @@ Packages **can safely run in parallel** (`-p` defaults to `GOMAXPROCS`). Each pa - 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 +4. Each test starts with `ctx, tx := testutils.SetupTestTx(t)` which begins a PostgreSQL transaction. All DB operations within the test use `tx` (the `db.Querier` interface) and `ctx` (the context with the embedded transaction). `t.Cleanup` rolls back the transaction when the test completes — **no truncation needed**. +5. Request helpers (`makeRequest`, `makeAuthRequest`, etc.) accept `ctx context.Context` — always pass the `ctx` from `SetupTestTx` to route DB calls through the test's transaction. +6. Tests can safely use `t.Parallel()` — each test goroutine has its own transaction context, so there's no cross-test data contamination. The `SetupTestTx` semaphore (cap 8) prevents connection pool exhaustion. +7. `TestMain` calls `DestroyTestDatabase` which closes the pool and drops the database -**Duration:** Full suite ~40s (parallel), ~90s (serial `-p 1`) +### Verification Workflow + +Tests must pass reliably at both verification levels: + +``` +# Development quick-check (fast): +go test -tags "test,dev" -count=1 -parallel 8 ./... # ~9s + +# Thorough completion verification (catches flakiness): +go test -tags "test,dev" -count=10 -parallel 8 ./... # ~39s +``` + +Features and their tests should pass `-count=1` for iterative development, but always confirm with `-count=10` before considering a feature complete. This catches race conditions in shared globals, goroutine-unsafe library types (e.g., `golang.org/x/text/cases.Caser`), and timing-dependent failures. + +**Known flakiness sources caught by -count=10:** +- Goroutine-unsafe package-level variables used concurrently (fixed: `titleCaser` in `local.go`, `testEmailCounter` in fixtures) +- Shared global state cleared by one test affecting another (`loginInProgress` map in auth handler) +- Polling timeouts in mock clients (`square_dev_test.go` mockSleep 3s vs test polling 100ms) --- @@ -84,26 +103,59 @@ import ( "testing" "crussell/db" + "crussell/testutils" "crussell/testutils/jwt" "crussell/testutils/testdb" ) func TestMain(m *testing.M) { pool := testdb.CreateTestDatabase("crussell_test_mypackage") - db.DB = pool + db.Conn = db.NewPoolProxy(pool) + testdb.SeedBaseline(pool) // working hours, business settings jwt.Init() - // Any additional global setup (Square client, DAV service, etc.) code := m.Run() + db.Conn.Pool().Close() testdb.DestroyTestDatabase(pool, "crussell_test_mypackage") os.Exit(code) } +``` -func resetTestData(t *testing.T) { - t.Helper() - testdb.TruncateTables(t, db.DB) +The key difference from the old architecture: `db.Conn` is a `*db.PoolProxy` (not a raw `*pgxpool.Pool`). The `PoolProxy` checks `context.Context` for an active transaction via `TxFromContext()`. If found, all `Exec`/`Query`/`QueryRow` calls route through the transaction; if not, they delegate to the underlying pool. + +**`db.Conn` is now the single point of DB access for both production and test code.** No more raw pool references in handlers. + +### Test Writing Pattern + +```go +func TestMyHandler(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + // Use 'tx' (db.Querier) for all DB operations + userID, err := fixtures.CreateTestUser(tx) + + // Pass 'ctx' to request helpers so handlers route through the tx + w := makeRequest(handler, "GET", "/path", nil, token, ctx) + + // Verify results directly in the tx + var status string + err = tx.QueryRow(ctx, "SELECT status FROM bookings WHERE id = $1", bookingID).Scan(&status) } ``` +### PoolProxy Architecture + +``` +PoolProxy (db/proxy.go) +├── Exec/Query/QueryRow → checks context for tx, routes to tx or pool +├── Begin → checks context for tx → savepoint, or pool.Begin +├── Acquire → ALWAYS goes to p.pool.Acquire (no context tx check) +├── Ping → ALWAYS goes to p.pool.Ping +└── Pool() → returns raw *pgxpool.Pool (escape hatch for tests) + +**Exception — DAV service:** `internal/dav/` has its own separate `*pgxpool.Pool` (not wrapped in PoolProxy). DAV operations (`CreateContact`, `DeleteContact`, `CreateEvent`) bypass PoolProxy entirely and always hit the real database. In tests, `dav.Service` is replaced with a nil-db stub (`&dav.BaseService{}`) so DAV operations are no-ops. +``` + ### Database naming convention `crussell_test_` @@ -127,8 +179,10 @@ func resetTestData(t *testing.T) { ## 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*`: +2. **Add TestMain** (see §3 pattern). Must call `testdb.SeedBaseline(pool)` after creating the database and before `m.Run()` — this seeds working hours and business settings at the pool level so all per-test transactions can see them. +3. **Use `db.NewPoolProxy(pool)`** to wrap the pool — don't assign the raw pool directly to `db.Conn`. +4. **Write tests** using `ctx, tx := testutils.SetupTestTx(t)` pattern with `t.Parallel()`. +5. **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 @@ -141,25 +195,25 @@ docker exec postgres psql -U myuser -d mydb -t -c " - 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 +- `SeedBaseline` inserts default working hours (Mon-Sun 08:00-20:00, all open) and business settings +- Each test's `SetupTestTx` creates a transaction that rolls back automatically — no data persists between tests --- ## 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`: +When you modify `init-scripts/init-script.sql` to add tables or types, you must update the drop 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 table** | Add to `dropOrder` array | | **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 +**Note:** The old `TruncateTables` function is deprecated and no longer used by tests (all tests use `SetupTestTx` with per-test transaction rollback). Only `dropOrder` matters for the fresh-database-per-run model. -Both are caught immediately by running the test suite twice. +**If you forget:** The test will fail with `relation "X" already exists` when `init-script.sql` tries to CREATE something that survived the DROP phase in `Migrate`. Caught immediately by running the test suite. --- @@ -169,21 +223,69 @@ Both are caught immediately by running the test suite twice. ```go func TestMyHandler(t *testing.T) { - resetTestData(t) // clean slate - // create fixtures, make request, assert + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + // Use 'tx' for all DB operations + userID, err := fixtures.CreateTestUser(tx) + + // Pass 'ctx' to request helpers to route through the tx + w := makeRequest(handler, "GET", "/path", nil, token, ctx) + + // Assert against response and optionally query via tx directly + var status string + err = tx.QueryRow(ctx, "SELECT status FROM bookings WHERE id = $1", bookingID).Scan(&status) } ``` -Every test should call `resetTestData(t)` at the start. It's cheap now — a single `TRUNCATE TABLE ... CASCADE` statement, no advisory lock. +### SetupTestTx Helper + +`testutils.SetupTestTx` returns `(context.Context, db.Querier)`: +- Begins a PostgreSQL transaction on the raw pool (via `db.Conn.Pool()` — the PoolProxy escape hatch) +- Stores the transaction in the returned context via `db.ContextWithTx` +- Registers `t.Cleanup` to roll back the transaction automatically when the test ends +- The returned `tx` is a `db.Querier` interface (implemented by both `*pgxpool.Pool` and `pgx.Tx`) ### Fixtures -Use helpers from `crussell/testutils/fixtures`: +Use helpers from `crussell/testutils/fixtures`. They accept `db.Querier` so they work with both pool-level and transaction-level queries: ```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)) +userID, err := fixtures.CreateTestUser(tx) // inside a test tx +serviceID, err := fixtures.CreateTestService(tx) +bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) +``` + +### Context Threading + +**Critical Rule:** The `ctx` from `SetupTestTx` must be passed through to request handlers. When a handler calls `db.Conn.Query(r.Context(), ...)`, the `PoolProxy` checks `r.Context()` for a transaction. If the context doesn't have one (e.g., `context.Background()` was passed to the request helper), the handler's DB calls go to the pool and **cannot see** data created in the test's transaction. + +```go +// CORRECT — handler sees the test's data: +w := makeRequest(handler, "POST", "/api/bookings", body, token, ctx) + +// WRONG — handler queries the pool, can't see tx data: +w := makeRequest(handler, "POST", "/api/bookings", body, token, context.Background()) +``` + +For request helpers that don't accept context, inject it directly: +```go +req, _ := http.NewRequest("POST", "/path", body) +req = req.WithContext(ctx) // inject tx context +``` + +### Request Helper Pattern + +Most packages follow this variadic context pattern: +```go +func makeRequest(handler http.Handler, method, path string, body interface{}, token string, requestCtx ...context.Context) *httptest.ResponseRecorder { + req := httptest.NewRequest(method, path, marshalBody(body)) + req.Header.Set("Authorization", "Bearer "+token) + if len(requestCtx) > 0 { + req = req.WithContext(requestCtx[0]) // routes through test tx + } + // ... serve handler ... +} ``` ### Test Helpers per Package @@ -192,22 +294,19 @@ bookingID, err := fixtures.CreateTestBookingAtTime(db.DB, userID, serviceID, tim |---------|------------| | `bookings` | `makeRequest`, `makeAuthRequest`, `makeAdminRequest`, `serveChiHandler`, `serveAdminHandler` | | `payments` | `makePaymentRequest`, `setupPaymentStatusTest(status)`, `setupTestDataPast(t)`, `setupDepositBookingPast(t)` | -| `admin` | `makeAdminRequest`, `makeUserRequest` | +| `admin` | `makeAdminRequest(handler, method, path, body, ctx)`, `makeUserRequest` | | `scheduling` | Package-specific helpers in test files | ### Working Hours -Weekday mapping: `0=Monday, 6=Sunday` (converted from Go's `time.Weekday`). +Weekday mapping: `0=Monday, 6=Sunday` (converted from Go's `time.Weekday`). The DB convention differs from Go's (Go: 0=Sunday). ```go -todayWeekday := int(time.Now().Weekday()) -if todayWeekday == 0 { - todayWeekday = 6 -} else { - todayWeekday -= 1 -} +dbWeekday := int((localStart.Weekday() + 6) % 7) // Go Sunday=0 → DB 6 ``` +`SeedBaseline` (called in `TestMain`) seeds working hours as 08:00-20:00, all days open. Individual tests no longer need to call `seedDefaultWorkingHours`. + ### Cursor Pagination in Tests Cursor values contain timestamps with `+` timezone offsets. Always URL-encode them: @@ -219,7 +318,61 @@ req := httptest.NewRequest("GET", "/api/admin/bookings?cursor="+url.QueryEscape( ### Parallelism Within a Package -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. +Tests within a package **can and should use `t.Parallel()`**. Each test gets its own transaction via `SetupTestTx`, which rolls back when the test completes. No cross-test data contamination. + +**The key enablers:** +1. `PoolProxy` routes DB calls through the per-test transaction via `TxFromContext(ctx)` +2. `SetupTestTx` registers `t.Cleanup` (not `defer`) for rollback — works even on test failure +3. Semaphore in `SetupTestTx` (cap 8) + per-pool `MaxConns=16` + `-parallel 8` prevents connection pool exhaustion + +**Tests can and should use `t.Parallel()`** wherever they use `SetupTestTx`. Per-test transaction isolation via `SetupTestTx` means no cross-test data contamination. The exceptions are cases where shared mutable state outside the database makes parallelism unsafe: + +**Caveats — do NOT use `t.Parallel()` when:** +- Tests share package-level globals (fixed: `testAdminID` in `custom_services_test.go` was refactored to a parameter; `loginInProgress` map clearing was removed from auth tests) +- Tests hit rate limiters (auth login tests with rapid attempts — these have `t.Parallel()` but the shared `loginInProgress` map should NOT be bulk-cleared; fixed by removing the blanket clear) +- The handler uses a goroutine-unsafe library type as a global (fixed: `titleCaser` in `local.go` — `golang.org/x/text/cases.Caser` is not goroutine-safe, switched to per-call creation) +- Health check tests (`main_test.go`) mutate `db.Conn` directly + +**Historical context — conn-busy bugs found via t.Parallel:** +When `t.Parallel()` was first added during migration, it revealed three pre-existing "conn busy" bugs in production handlers where `defer rows.Close()` + subsequent `db.Conn.QueryRow()` ran on the same transaction-routed context. These were all fixed: +- `handlers/admin/custom_services.go:GetCustomServices` — count query before rows loop +- `handlers/bookings/bookings.go:CreateBookingHandler` — `defer rows.Close()` before `db.Conn.Begin` +- `handlers/bookings/bookings.go:AdminSearchBookings` — count query before rows loop + +**Current t.Parallel() coverage:** +Nearly all test files using `SetupTestTx` also use `t.Parallel()`. Exceptions (no `t.Parallel()`): some health check tests, dev-only mocks, and pure unit tests without DB interaction. + +### SeedBaseline + +`testdb.SeedBaseline(pool)` is called in `TestMain` and seeds: +- **Working hours**: Mon-Sun, 08:00-20:00, all open +- **Business settings**: Business name, address, gift card config + +Individual tests should NOT seed working hours manually — they're visible to all per-test transactions because they were committed to the pool before any test started. + +### Connection Pool & Semaphore + +Tests use a per-package semaphore (cap 8) in `SetupTestTx` and `MaxConns=16` per pool to prevent connection exhaustion: + +```go +// testutils/tx.go +var txSemaphore = make(chan struct{}, 8) // max concurrent test transactions +``` + +```go +// testutils/testdb/testdb.go +poolCfg.MaxConns = 16 +``` + +The test runner also uses `-parallel 8` to limit concurrent test execution within each package. These three numbers (semaphore cap, MaxConns, -parallel flag) should stay in sync. + +### SimpleProtocol + +Test pools use `QueryExecModeSimpleProtocol` to disable prepared statements. Always set this in the test database config: + +```go +config.ConnConfig.DefaultQueryExecMode = pgx.QueryExecModeSimpleProtocol +``` --- @@ -275,9 +428,40 @@ done You added a new table/type to `init-script.sql` but forgot to add it to the drop lists in `testdb.go`. See §5. +### Transaction is aborted (SQLSTATE 25P02) + +A PostgreSQL query inside a test's transaction failed, putting the transaction in an aborted state. All subsequent queries on the same `tx` will fail with this error. Common causes: +- FK violation from a subquery returning NULL +- Invalid data format +- The `tx` was used after a `t.Fatalf` (which panics the goroutine) — fix: use `t.Cleanup` instead of `defer tx.Rollback()` + +Fix: identify and fix the root cause query. The error message usually includes the original failure. Wrap the failing query with detailed error logging, or run the test with `-v` to see all log output. + +### `conn busy` errors in handler output + +When tests use `SetupTestTx` (transaction-routed context), the handler's `db.Conn.Query(r.Context(), ...)` + `defer rows.Close()` followed by `db.Conn.QueryRow(r.Context(), ...)` causes pgx to return `conn busy`. This happens because the transaction's connection is held by the open (but consumed) rows object. + +**Root cause:** The production handler runs a count query BEFORE consuming the data query's result set. With `SetupTestTx`, both queries route through the same transaction. pgx requires the first query's rows to be closed before executing the next query on the same transaction. + +**Fix:** Move the count query after the `for rows.Next()` loop, or replace `defer rows.Close()` with `rows.Close()` immediately after the loop. Both patterns fix the issue. (Three handlers were already fixed — see `custom_services.go`, `bookings.go:CreateBookingHandler`, `bookings.go:AdminSearchBookings`.) + +**Prevention:** When using `SetupTestTx` with handlers, verify the handler doesn't have `defer rows.Close()` followed by another `db.Conn.Query/QueryRow/Exec` on the same context without closing rows first. + +### Tests pass in isolation but fail in full suite (within same package) + +Likely parallel test interference. Even with per-test transactions, tests can interfere through: +1. **Package-level globals** (shared state like `testAdminID`) +2. **Rate limiters** (auth tests making rapid login attempts trigger progressive backoff) +3. **Webhook signature checks** (env vars set by one test affect parallel tests) + +Fixes: +1. Remove the shared global or remove `t.Parallel()` from those specific tests +2. Remove `t.Parallel()` from rate-limited auth tests +3. Use `t.Setenv` correctly (runs in `t.Cleanup` so it's safe for parallel use) + ### Deadlock detected -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. +Should not happen with per-package databases. If it does, the cause is likely two concurrent parallel test transactions inserting into the same table with conflicting foreign key ordering (e.g., `INSERT INTO working_hours` without deterministic weekday ordering). Fix: make inserts order-deterministic or use `ON CONFLICT DO NOTHING/UPDATE`. ### OptionalAuth: invalid token: ... illegal base64 data @@ -295,32 +479,41 @@ This appears in `TestAccount_DeleteGuest` and `TestLoyalty_Get`. The `dav.Servic | Metric | Value | |--------|-------| -| Serial (`-p 1`) | ~90s | -| **Parallel** | **~40s** | -| Packages | 18 tested, 0 failures | -| Tests | 949 passing, 8 skipped, 0 failing | +| Quick check (`-count=1`) | **~9s** | +| Thorough (`-count=10`) | **~39s** | +| Strict serial (`-p 1`) | ~90s | +| Packages | 19 tested, 0 failures | +| Tests | 960 run, 4 skipped, 0 failing (981 defined; 21 excluded by build tags in non-dev mode) | ### 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 | +| `CREATE DATABASE` + migration per package | ~1s × 18 packages = ~4s (parallelized) | +| Test execution (heaviest: `bookings` ~200 tests) | ~25s | +| Test execution (admin ~180 tests) | ~17s | +| Test execution (payments ~150 tests) | ~6s | + +### Speed Improvements Realized + +| Change | Before | After | Saving | +|--------|--------|-------|--------| +| **Per-package databases** | ~90s serial | ~40s parallel | 55% | +| **`t.Parallel()` within packages** | ~40s parallel | ~25s parallel | 37% | +| **Per-test transaction rollback by `SetupTestTx`** | — | Eliminates truncation overhead (~5-10s) | Included above | +| **`PreferSimpleProtocol` on test pool** | — | Eliminates prepared statement "conn busy" | Required for parallelism | ### 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. +The `CREATE DATABASE` operation serializes at the PostgreSQL catalog level. With 18+ packages creating databases concurrently, they queue on catalog locks. This adds ~4-8s of overhead. -### How to Make It Faster +### How to Make It Even 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 | +| **Template databases** (clone via `CREATE DATABASE ... TEMPLATE`) | ~4-8s saved on database creation | Low — was attempted, needs care | +| **Shard heaviest package** (split bookings into sub-packages) | ~10-12s saved | High — significant refactor | +| **Eliminate deferred `fixtures.Delete*` calls** (they waste tx operations before rollback) | ~1-2s | Low — sed removal | --- @@ -330,13 +523,18 @@ The `CREATE DATABASE` operation serializes at the PostgreSQL catalog level. With **Yes.** Each package gets its own database. No shared state, no lock contention. This is the entire point of the per-package database architecture. -### Q: Why don't we use `t.Parallel()`? +### Q: Why don't all tests 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 +Most test files using `SetupTestTx` now also use `t.Parallel()`. The exceptions that intentionally lack `t.Parallel()`: -Both are significant refactors. For now, intra-package parallelism isn't worth the complexity. +1. **Health check tests** (`main_test.go`) — mutate global `db.Conn` directly, incompatible with parallelism +2. **Dev-only Square mocks** (`square_dev_test.go`) — each test creates its own MockClient, already parallel-safe +3. **Some pure unit tests** (no DB interaction) — don't need `SetupTestTx` or `t.Parallel()` + +Previously, shared mutable globals (`testAdminID` in `custom_services_test.go`, `titleCaser` in `local.go`, blanket `loginInProgress` map clearing) prevented parallelism. All of these have been fixed: +- `testAdminID` → refactored to a function parameter +- `titleCaser` → switched from global to per-call creation (`cases.Caser` is not goroutine-safe) +- `loginInProgress` blanket clear → removed from individual tests (each test creates a unique user) ### Q: What happens if `go test` is killed mid-run? @@ -346,11 +544,11 @@ If a database can't be dropped because of stale connections, `DestroyTestDatabas ### 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`. +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 use `SetupTestTx` with per-test transaction rollback, so no cleanup between tests is needed. ### 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. +**No.** Advisory locks (`pg_advisory_lock`) have been removed from test infrastructure. With per-package databases and per-test transactions, there's no cross-package or cross-test contention. The single production use of `pg_advisory_lock` is in the payment handler (`handlers/payments/handlers.go:706`) for serializing concurrent payment attempts on the same booking — this is unrelated to test infrastructure. ### Q: What's the `GO_TESTING` env var for? @@ -377,9 +575,13 @@ The `dev` tag is required by some packages (Square mock, rate limiter). Always u ### Q: Tests pass in isolation but fail in the full suite -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) +This can still happen within a single package when tests run in parallel. Common causes: +1. **Package-level globals** (e.g., `testAdminID` set by one test, read by another) +2. **Rate limiters** (auth tests with rapid login attempts trigger progressive backoff) +3. **`t.Setenv`** — use `t.Setenv` which automatically restores in `t.Cleanup` (safe for parallel) +4. **Test order dependencies** (rare — one test creates data that another test expects) + +Fix: remove the shared global or remove `t.Parallel()` from the affected test(s). ### Q: How do I debug a hanging test? @@ -397,5 +599,19 @@ This shouldn't appear anymore — the auth package's TestMain was updated to use ### 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). +981 test functions defined across all `_test.go` files. `go test -tags "test,dev" -count=1` reports ~960 run + 4 skipped (17 are excluded by build tag combinations — some dev-only tests have `//go:build test && dev` and may not match every tag set). 0 failures across 19 packages. + +### Q: Why use `-count=10` for thorough verification? + +A single `-count=1` pass confirms no compilation errors and basic correctness. `-count=10` (10 iterations per test) catches flakiness from: +- **Goroutine-unsafe globals** used concurrently under `t.Parallel()` — the `titleCaser` panic (`slice bounds out of range` in `golang.org/x/text/cases`) only appeared ~1 in 10 runs +- **Timing-dependent mocks** — the Square mock's 3s async completion vs test polling timeout +- **Shared state races** — `loginInProgress` map clearing under parallel auth tests + +**Workflow:** +``` +-count=1 → iterative dev (fast, ~9s) +-count=10 → feature completion (thorough, ~39s) +``` + +Both must pass before considering a change complete.