docs: update README, obsidian docs, dev scripts, and SQL init
Update documentation and configuration: - README: reflect new test patterns and architecture - Obsidian docs: update Technical Manual, Overview, Testing Architecture - init-script.sql: schema updates - local-dev-2.sh: dev script adjustments Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
This commit is contained in:
@@ -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.
|
**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.
|
**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
|
```bash
|
||||||
cd backend && go build -o bin/backend ./main.go
|
cd backend && go build -o bin/backend ./main.go
|
||||||
cd frontend && npm ci && npm run build
|
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
|
## Full Documentation
|
||||||
|
|||||||
@@ -1154,15 +1154,6 @@ BEGIN
|
|||||||
) ORDER BY ap.created_at DESC), '[]'::json)
|
) ORDER BY ap.created_at DESC), '[]'::json)
|
||||||
FROM affiliate_payouts ap WHERE ap.affiliate_id = target_user_id
|
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', (
|
'forgiven_no_shows', (
|
||||||
SELECT COALESCE(json_agg(json_build_object(
|
SELECT COALESCE(json_agg(json_build_object(
|
||||||
'id', fns.id,
|
'id', fns.id,
|
||||||
@@ -1195,6 +1186,25 @@ BEGIN
|
|||||||
FROM gift_card_transactions gct
|
FROM gift_card_transactions gct
|
||||||
WHERE gct.user_id = target_user_id
|
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', (
|
'login_audit', (
|
||||||
SELECT COALESCE(json_agg(json_build_object(
|
SELECT COALESCE(json_agg(json_build_object(
|
||||||
'attempt_type', la.attempt_type,
|
'attempt_type', la.attempt_type,
|
||||||
|
|||||||
+1
-1
@@ -1475,7 +1475,7 @@ cd /home/popertots/Crussell/backend
|
|||||||
export POSTGRES_USER POSTGRES_PASSWORD POSTGRES_HOST POSTGRES_DB GO_TESTING=1
|
export POSTGRES_USER POSTGRES_PASSWORD POSTGRES_HOST POSTGRES_DB GO_TESTING=1
|
||||||
TEST_OUTPUT_FILE=$(mktemp)
|
TEST_OUTPUT_FILE=$(mktemp)
|
||||||
START_TIME=$(date +%s)
|
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)
|
END_TIME=$(date +%s)
|
||||||
DURATION=$((END_TIME - START_TIME))
|
DURATION=$((END_TIME - START_TIME))
|
||||||
MINUTES=$((DURATION / 60))
|
MINUTES=$((DURATION / 60))
|
||||||
|
|||||||
@@ -83,7 +83,7 @@ Campaign lifecycle: `draft → active → completed` (or any → `cancelled`, `a
|
|||||||
|
|
||||||
### Compliance
|
### 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.
|
**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
|
```bash
|
||||||
cd backend
|
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
|
go test -tags "test,dev" -v -run TestName ./... # Single test
|
||||||
```
|
```
|
||||||
|
|
||||||
Test infrastructure notes:
|
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`).
|
- **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.
|
- **⚠️ 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
|
- **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)`.
|
||||||
- `TruncateTables()` between tests — single `TRUNCATE TABLE ... CASCADE` statement (down from 44 separate truncates), no advisory locks
|
- **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
|
- Statement-by-statement SQL parser (`splitSQLStatements()`) respects dollar-quoted PL/pgSQL blocks
|
||||||
- Build tag: all test files use `//go:build test`
|
- Build tag: all test files use `//go:build test`
|
||||||
- Test JWT secret: `test-secret-key-for-testing-only`
|
- Test JWT secret: `test-secret-key-for-testing-only`
|
||||||
|
|||||||
@@ -50,7 +50,7 @@ Backend (:8080)
|
|||||||
| `handlers/auth` | local.go, social.go | Registration (with referral code validation), login, refresh, email verification |
|
| `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/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/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/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/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) |
|
| `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**: `postgres://USER:PASSWORD@HOST:5432/DB`
|
||||||
- **Connection pooling**: Built-in via pgxpool
|
- **Connection pooling**: Built-in via pgxpool
|
||||||
- **Build tags**: `db_dev.go` (dev, localhost) vs `db.go` (prod, env var)
|
- **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
|
### Internal Packages
|
||||||
|
|
||||||
@@ -206,6 +218,8 @@ src/lib/components/
|
|||||||
|
|
||||||
Added in the June 2026 security pass:
|
Added in the June 2026 security pass:
|
||||||
|
|
||||||
|
### Security Headers
|
||||||
|
|
||||||
| Header | Value | Location |
|
| Header | Value | Location |
|
||||||
|--------|-------|----------|
|
|--------|-------|----------|
|
||||||
| `Content-Security-Policy` | `default-src 'none'; frame-ancestors 'none'` | Global middleware (`main.go:139`) |
|
| `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-Methods` | `GET, POST, PUT, PATCH, DELETE, OPTIONS` | Global middleware |
|
||||||
| `Access-Control-Allow-Headers` | `Authorization, Content-Type, Idempotency-Key` | 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.
|
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 |
|
| `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 |
|
| `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 |
|
| `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 |
|
| `get_vat_return_data(start, end)` | VAT return summary for MTD |
|
||||||
| `export_sales_transactions(start, end, include_vat)` | Tax-compatible transaction export |
|
| `export_sales_transactions(start, end, include_vat)` | Tax-compatible transaction export |
|
||||||
| `get_monthly_business_summary(start, end)` | Monthly revenue breakdown |
|
| `get_monthly_business_summary(start, end)` | Monthly revenue breakdown |
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
# Testing Architecture & DB Management
|
# 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
|
```bash
|
||||||
# Full suite (parallel — packages run concurrently against their own databases)
|
# 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
|
# Single package
|
||||||
go test -tags "test,dev" -v -count=1 ./handlers/payments/
|
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
|
- Creates a fresh database
|
||||||
- Runs `init-scripts/init-script.sql` (full schema migration)
|
- Runs `init-scripts/init-script.sql` (full schema migration)
|
||||||
- Returns a pool connected to the new database
|
- 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
|
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. `TestMain` calls `DestroyTestDatabase` which closes the pool and drops the database
|
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"
|
"testing"
|
||||||
|
|
||||||
"crussell/db"
|
"crussell/db"
|
||||||
|
"crussell/testutils"
|
||||||
"crussell/testutils/jwt"
|
"crussell/testutils/jwt"
|
||||||
"crussell/testutils/testdb"
|
"crussell/testutils/testdb"
|
||||||
)
|
)
|
||||||
|
|
||||||
func TestMain(m *testing.M) {
|
func TestMain(m *testing.M) {
|
||||||
pool := testdb.CreateTestDatabase("crussell_test_mypackage")
|
pool := testdb.CreateTestDatabase("crussell_test_mypackage")
|
||||||
db.DB = pool
|
db.Conn = db.NewPoolProxy(pool)
|
||||||
|
testdb.SeedBaseline(pool) // working hours, business settings
|
||||||
jwt.Init()
|
jwt.Init()
|
||||||
// Any additional global setup (Square client, DAV service, etc.)
|
|
||||||
code := m.Run()
|
code := m.Run()
|
||||||
|
db.Conn.Pool().Close()
|
||||||
testdb.DestroyTestDatabase(pool, "crussell_test_mypackage")
|
testdb.DestroyTestDatabase(pool, "crussell_test_mypackage")
|
||||||
os.Exit(code)
|
os.Exit(code)
|
||||||
}
|
}
|
||||||
|
```
|
||||||
|
|
||||||
func resetTestData(t *testing.T) {
|
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.
|
||||||
t.Helper()
|
|
||||||
testdb.TruncateTables(t, db.DB)
|
**`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
|
### Database naming convention
|
||||||
|
|
||||||
`crussell_test_<package_path_with_underscores>`
|
`crussell_test_<package_path_with_underscores>`
|
||||||
@@ -127,8 +179,10 @@ func resetTestData(t *testing.T) {
|
|||||||
## 4. Adding a New Test Package
|
## 4. Adding a New Test Package
|
||||||
|
|
||||||
1. **Create the test file(s)** in the package directory with `//go:build test` constraint
|
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.
|
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. **Add the database name** to the cleanup list in `local-dev-2.sh` if it doesn't match `crussell_test*`:
|
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
|
```bash
|
||||||
# In local-dev-2.sh — the cleanup regex catches crussell_test* by default
|
# 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)
|
- 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
|
- 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
|
## 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 |
|
| 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 enum type** | Add to `typeDrops` array in `Migrate` |
|
||||||
| **New sequence** | Add to `seqDrops` |
|
| **New sequence** | Add to `seqDrops` |
|
||||||
| **Removed table/type** | Remove from the corresponding lists |
|
| **Removed table/type** | Remove from the corresponding lists |
|
||||||
|
|
||||||
**If you forget:** The test will either:
|
**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.
|
||||||
- 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.
|
**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
|
```go
|
||||||
func TestMyHandler(t *testing.T) {
|
func TestMyHandler(t *testing.T) {
|
||||||
resetTestData(t) // clean slate
|
t.Parallel()
|
||||||
// create fixtures, make request, assert
|
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
|
### 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
|
```go
|
||||||
userID, err := fixtures.CreateTestUser(db.DB)
|
userID, err := fixtures.CreateTestUser(tx) // inside a test tx
|
||||||
serviceID, err := fixtures.CreateTestService(db.DB)
|
serviceID, err := fixtures.CreateTestService(tx)
|
||||||
bookingID, err := fixtures.CreateTestBookingAtTime(db.DB, userID, serviceID, time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC))
|
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
|
### Test Helpers per Package
|
||||||
@@ -192,22 +294,19 @@ bookingID, err := fixtures.CreateTestBookingAtTime(db.DB, userID, serviceID, tim
|
|||||||
|---------|------------|
|
|---------|------------|
|
||||||
| `bookings` | `makeRequest`, `makeAuthRequest`, `makeAdminRequest`, `serveChiHandler`, `serveAdminHandler` |
|
| `bookings` | `makeRequest`, `makeAuthRequest`, `makeAdminRequest`, `serveChiHandler`, `serveAdminHandler` |
|
||||||
| `payments` | `makePaymentRequest`, `setupPaymentStatusTest(status)`, `setupTestDataPast(t)`, `setupDepositBookingPast(t)` |
|
| `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 |
|
| `scheduling` | Package-specific helpers in test files |
|
||||||
|
|
||||||
### Working Hours
|
### 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
|
```go
|
||||||
todayWeekday := int(time.Now().Weekday())
|
dbWeekday := int((localStart.Weekday() + 6) % 7) // Go Sunday=0 → DB 6
|
||||||
if todayWeekday == 0 {
|
|
||||||
todayWeekday = 6
|
|
||||||
} else {
|
|
||||||
todayWeekday -= 1
|
|
||||||
}
|
|
||||||
```
|
```
|
||||||
|
|
||||||
|
`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 Pagination in Tests
|
||||||
|
|
||||||
Cursor values contain timestamps with `+` timezone offsets. Always URL-encode them:
|
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
|
### 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.
|
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
|
### 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
|
### OptionalAuth: invalid token: ... illegal base64 data
|
||||||
|
|
||||||
@@ -295,32 +479,41 @@ This appears in `TestAccount_DeleteGuest` and `TestLoyalty_Get`. The `dav.Servic
|
|||||||
|
|
||||||
| Metric | Value |
|
| Metric | Value |
|
||||||
|--------|-------|
|
|--------|-------|
|
||||||
| Serial (`-p 1`) | ~90s |
|
| Quick check (`-count=1`) | **~9s** |
|
||||||
| **Parallel** | **~40s** |
|
| Thorough (`-count=10`) | **~39s** |
|
||||||
| Packages | 18 tested, 0 failures |
|
| Strict serial (`-p 1`) | ~90s |
|
||||||
| Tests | 949 passing, 8 skipped, 0 failing |
|
| 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
|
### What Drives Test Time
|
||||||
|
|
||||||
| Component | Time |
|
| Component | Time |
|
||||||
|-----------|------|
|
|-----------|------|
|
||||||
| `CREATE DATABASE` + migration per package | ~1s × 15 packages = ~4s (parallelized) |
|
| `CREATE DATABASE` + migration per package | ~1s × 18 packages = ~4s (parallelized) |
|
||||||
| Test execution (heaviest: `bookings` 245 tests) | ~25s |
|
| Test execution (heaviest: `bookings` ~200 tests) | ~25s |
|
||||||
| Test execution (admin 156 tests) | ~16s |
|
| Test execution (admin ~180 tests) | ~17s |
|
||||||
| Test execution (payments: 120+ tests) | ~13s |
|
| 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
|
### 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 |
|
| Approach | Gain | Effort |
|
||||||
|----------|------|--------|
|
|----------|------|--------|
|
||||||
| **Shard slow packages** (split `bookings` test files → parallel sub-packages) | 25s → ~13s (halve bookings) | Moderate — 2-3 hours refactoring |
|
| **Template databases** (clone via `CREATE DATABASE ... TEMPLATE`) | ~4-8s saved on database creation | Low — was attempted, needs care |
|
||||||
| **Template databases** (create once, clone via `CREATE DATABASE ... TEMPLATE`) | ~4-8s saved on database creation | Low — was attempted, needs careful parallel-safety work |
|
| **Shard heaviest package** (split bookings into sub-packages) | ~10-12s saved | High — significant refactor |
|
||||||
| **`t.Parallel()` within packages** with transaction-per-test | ~25s → ~15s (bookings halved again) | High — major test refactor |
|
| **Eliminate deferred `fixtures.Delete*` calls** (they waste tx operations before rollback) | ~1-2s | Low — sed removal |
|
||||||
| **Reduce polling in Square mock** | Already done (0.012s, was 8s) | ✅ Complete |
|
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
@@ -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.
|
**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:
|
Most test files using `SetupTestTx` now also use `t.Parallel()`. The exceptions that intentionally lack `t.Parallel()`:
|
||||||
- Per-test transactions with rollback (wrap every DB operation in a test-scoped `Tx`, roll back at the end)
|
|
||||||
- Separate PostgreSQL schemas per test
|
|
||||||
|
|
||||||
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?
|
### 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?
|
### 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?
|
### 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?
|
### 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
|
### 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:
|
This can still happen within a single package when tests run in parallel. Common causes:
|
||||||
1. Are two tests in the **same package** interfering? (Shared state, global variables)
|
1. **Package-level globals** (e.g., `testAdminID` set by one test, read by another)
|
||||||
2. Is the test environment different? (Different env vars, Docker state)
|
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?
|
### 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?
|
### Q: What's the total test count?
|
||||||
|
|
||||||
Current: ~957 tests (18 packages, 0 failures, 8 skipped).
|
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.
|
||||||
Tracked by counting `^=== RUN` lines in the output (excluding sub-tests).
|
|
||||||
|
### 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.
|
||||||
|
|||||||
Reference in New Issue
Block a user