feat: edit request time blockers, today closing time, UI polish, and test fixes
- Add time blocker management for booking edit requests - Add closing_time field to admin today/current-next endpoint - Update UserBookingModal and CurrentAppointment UI components - Fix fmt import in bookings_test.go (was missing) - Fix created_by FK in TestAdminApproveEditRequest_TimeBlockerOverlap - Update test coverage for edit request time blocker overlap - Update gap backlog documentation
This commit is contained in:
@@ -0,0 +1,206 @@
|
||||
# Test DB Reset Optimization — Deep Analysis & Updated Plan
|
||||
|
||||
## Current State: The Problem
|
||||
|
||||
Every test (288 total) does a **full schema DROP + CREATE + TRUNCATE**:
|
||||
|
||||
```
|
||||
setupTestDB(t)
|
||||
├── testdb.Pool(t) → New pgxpool connection (expensive)
|
||||
├── testdb.Migrate(t, pool) → Full schema DROP + CREATE
|
||||
│ ├── Check typeCount → ALWAYS > 0 (shared crussell_test DB)
|
||||
│ ├── DROP 14 types CASCADE
|
||||
│ ├── DROP 24 tables CASCADE
|
||||
│ ├── DROP 4 sequences
|
||||
│ └── Run init-script.sql → CREATE all tables, types, indexes, stored procs
|
||||
├── testdb.TruncateTables() → TRUNCATE 21 tables CASCADE
|
||||
├── db.DB = pool → Replace global
|
||||
├── jwt.Init() → Re-init JWT
|
||||
└── defer: pool.Close() → Close connection
|
||||
```
|
||||
|
||||
**227 tests** call `setupTestDB` directly. The remaining **61 tests** use custom setup functions (`setupTest`, `setupReserveTestDB`, `setupTimeBlockersTestDB`) that do the exact same thing — Pool + Migrate + Truncate.
|
||||
|
||||
**All 288 tests do full schema destruction and rebuild.**
|
||||
|
||||
---
|
||||
|
||||
## Deep Analysis: Cross-Test Safety Verification
|
||||
|
||||
### ✅ VERIFIED SAFE: No Schema Creation by Tests
|
||||
|
||||
Grep for `CREATE TABLE|DROP TABLE|ALTER TABLE|CREATE TYPE|DROP TYPE|CREATE INDEX|CREATE FUNCTION` in all `_test.go` files: **zero matches**. No test creates, alters, or drops any schema objects. All tests only INSERT/UPDATE/DELETE row data.
|
||||
|
||||
### ✅ VERIFIED SAFE: No Cross-Test Dependencies
|
||||
|
||||
Every test is self-contained. Each creates its own data via fixtures or direct SQL, and either:
|
||||
- Uses `defer fixtures.Delete...` for cleanup, OR
|
||||
- Relies on `TruncateTables` to clean up before the next test
|
||||
|
||||
No test reads data created by a previous test. No test depends on sequential ordering.
|
||||
|
||||
### ✅ VERIFIED SAFE: Empty-State Tests
|
||||
|
||||
5 tests explicitly check for empty results:
|
||||
- `TestPortfolio_ListImages_Empty` — expects 0 images
|
||||
- `TestPortfolio_ListTags_Empty` — expects 0 tags
|
||||
- `TestPortfolio_ListFilters_Empty` — expects 0 filters
|
||||
- `TestNotifications_ListEmpty` — expects 0 notifications
|
||||
- `TestCustomerRelationship_NoBookings` — expects user with no bookings
|
||||
|
||||
All are safe with TRUNCATE — truncation produces the empty state these tests expect.
|
||||
|
||||
### ✅ VERIFIED SAFE: `seedDefaultWorkingHours`
|
||||
|
||||
Used by 38 tests across 4 files. Uses `ON CONFLICT (weekday) DO UPDATE` — fully idempotent. Safe to call multiple times.
|
||||
|
||||
### ⚠️ FINDING: `TruncateTables` is missing 3 tables
|
||||
|
||||
The truncate list has 21 tables, but the schema has 24. Missing:
|
||||
- `booking_edit_requests` — used by 11 tests in bookings/admin
|
||||
- `exceptional_group_applications` — used by 3 tests in bookings/scheduling
|
||||
- `business_settings` — 1 row, never modified by tests
|
||||
|
||||
**Why this hasn't broken things**: CASCADE foreign keys handle cleanup:
|
||||
- `booking_edit_requests.booking_id → bookings.id` (CASCADE) → cleaned when `bookings` truncated
|
||||
- `exceptional_group_applications.group_id → exceptional_working_hours_groups.id` (CASCADE) → cleaned when groups truncated
|
||||
- `business_settings` — static seed data, never modified
|
||||
|
||||
**Action needed**: Add these 3 tables to `TruncateTables` for correctness. Currently relying on implicit CASCADE behavior.
|
||||
|
||||
### ⚠️ FINDING: `handlers/handlers_test.go` has no setup wrapper
|
||||
|
||||
`TestIntegration_UserFlow` uses `testdb.Pool(t)` directly (no Migrate, no Truncate) with `defer fixtures.DeleteUser` for cleanup. The other 3 tests in this file don't use the DB at all. This package needs a TestMain that runs Migrate once.
|
||||
|
||||
### ⚠️ FINDING: `discount_test.go` has dead code
|
||||
|
||||
`truncateDiscountTables()` helper is defined but never called. The file uses `setupTestDB(t)` which already calls the full `TruncateTables`. Safe to delete.
|
||||
|
||||
### ✅ VERIFIED SAFE: Global state
|
||||
|
||||
- `auth.TokenAuth` — set by `jwt.Init()`, called per-test currently, will be once-per-package in TestMain. No test modifies it.
|
||||
- `loginInProgress` / `loginAttempts` — package-level maps in `local.go`. Handler cleans up via defer/delete. No test modifies them directly.
|
||||
- `dav.Service` — set to `&dav.BaseService{}` in auth_test.go's setup. Persists across tests but is a read-only mock. Safe.
|
||||
- `fixtures.testEmailCounter` — increments for unique emails. Monotonically increasing, never resets. Safe (designed for this).
|
||||
|
||||
---
|
||||
|
||||
## Updated Plan
|
||||
|
||||
### Phase 1: Add TestMain to Each Package + Fix TruncateTables
|
||||
|
||||
**Step 1A: Fix `TruncateTables` to include all 24 tables**
|
||||
|
||||
Add to `testutils/testdb/testdb.go`:
|
||||
```go
|
||||
tables := []string{
|
||||
// ... existing 21 tables ...
|
||||
"booking_edit_requests", // NEW
|
||||
"exceptional_group_applications", // NEW
|
||||
"business_settings", // NEW
|
||||
}
|
||||
```
|
||||
|
||||
**Step 1B: Add TestMain to each package**
|
||||
|
||||
Each package gets ONE `TestMain` (not per file — per package). In Go, if multiple files in the same package define `TestMain`, it's a compile error. So we need ONE file per package with TestMain.
|
||||
|
||||
| Package | TestMain goes in | Tests covered |
|
||||
|---------|-----------------|---------------|
|
||||
| `handlers/bookings` | New file `bookings_testmain_test.go` | 98 (4 files) |
|
||||
| `handlers/admin` | New file `admin_testmain_test.go` | 65 (4 files) |
|
||||
| `handlers/auth` | `auth_test.go` (add to existing) | 25 |
|
||||
| `handlers/scheduling` | New file `scheduling_testmain_test.go` | 34 (2 files) |
|
||||
| `handlers/portfolio` | `images_test.go` (add to existing) | 16 |
|
||||
| `handlers/notifications` | `notifications_test.go` (add to existing) | 12 |
|
||||
| `handlers/services` | `services_test.go` (add to existing) | 5 |
|
||||
| `handlers/user` | New file `user_testmain_test.go` | 21 (3 files) |
|
||||
| `handlers` | `handlers_test.go` (add to existing) | 4 |
|
||||
| `main` | `main_test.go` (add to existing) | 2 |
|
||||
|
||||
**TestMain template** (varies slightly per package):
|
||||
```go
|
||||
func TestMain(m *testing.M) {
|
||||
pool := testdb.NewPool("")
|
||||
testdb.Migrate(&testing.T{}, pool)
|
||||
db.DB = pool
|
||||
jwt.Init()
|
||||
// auth_test.go also needs: dav.Service = &dav.BaseService{}
|
||||
code := m.Run()
|
||||
pool.Close()
|
||||
os.Exit(code)
|
||||
}
|
||||
```
|
||||
|
||||
**Step 1C: Convert `setupTestDB(t)` to `resetTestData(t)`**
|
||||
|
||||
Each package's setup function becomes:
|
||||
```go
|
||||
func resetTestData(t *testing.T) {
|
||||
t.Helper()
|
||||
testdb.TruncateTables(t, db.DB)
|
||||
// For packages that need working hours:
|
||||
// seedDefaultWorkingHours(t)
|
||||
}
|
||||
```
|
||||
|
||||
**Step 1D: Remove redundant per-test operations**
|
||||
|
||||
- Remove `db.DB = pool` swap (pool is now global)
|
||||
- Remove `jwt.Init()` from per-test setup (done once in TestMain)
|
||||
- Remove `pool.Close()` from defer (pool is shared, closed in TestMain)
|
||||
- Remove `testdb.Pool(t)` calls (use global `db.DB`)
|
||||
- Remove `testdb.Migrate(t, pool)` calls (done once in TestMain)
|
||||
|
||||
### Phase 2: Clean Up
|
||||
|
||||
- Delete dead `truncateDiscountTables()` in discount_test.go
|
||||
- Consolidate `seedDefaultWorkingHours` into one shared function (currently duplicated in 4 files with slight variations)
|
||||
- Add `dav.Service` mock to TestMain for auth package only
|
||||
|
||||
### Phase 3: Smart Truncate (Optional, Later)
|
||||
|
||||
Identify which tables each test actually touches and truncate only those. Low priority — Phase 1 gives 95% of the benefit.
|
||||
|
||||
---
|
||||
|
||||
## Risk Assessment (Updated)
|
||||
|
||||
| Risk | Likelihood | Severity | Mitigation |
|
||||
|------|-----------|----------|------------|
|
||||
| Test pollution (data leaking) | Low | High | `TRUNCATE CASCADE` is reliable; verify with `-count=2` |
|
||||
| Missing tables in TruncateTables | **Confirmed** | Medium | **Fix in Phase 1A** — add 3 missing tables |
|
||||
| `TestMain` compile conflict | Low | High | ONE TestMain per package, not per file |
|
||||
| `db.DB` global race | Low | High | Tests run sequentially (`-p 1`) |
|
||||
| `jwt.Init()` called once vs per-test | Low | Medium | JWT state is idempotent; no test modifies TokenAuth |
|
||||
| `dav.Service` mock persistence | Low | Low | Only auth tests use it; mock is stateless |
|
||||
|
||||
### Verification Strategy:
|
||||
1. Run `go test -tags test -v -p 1 -count=1 ./...` — record baseline count/timing
|
||||
2. Apply Phase 1A (fix TruncateTables)
|
||||
3. Run tests — should still pass (no logic change)
|
||||
4. Apply Phase 1B-1D (TestMain + refactor)
|
||||
5. Run `go test -tags test -v -p 1 -count=2 ./...` — double-run catches state leakage
|
||||
6. Compare: 288 tests, same pass/fail, faster execution
|
||||
|
||||
---
|
||||
|
||||
## Expected Impact
|
||||
|
||||
| Metric | Before | After Phase 1 | After Phase 3 |
|
||||
|--------|--------|---------------|---------------|
|
||||
| Schema DROP+CREATE | 288 | 10 (one per package) | 10 |
|
||||
| TRUNCATE per test | 21 tables | 21 tables | ~3-5 tables |
|
||||
| Connection pools created | 288 | 10 | 10 |
|
||||
| **Estimated total time** | **~144s** | **~58s** | **~30s** |
|
||||
|
||||
> Estimates based on: DROP+CREATE ~400ms, TRUNCATE 21 tables ~100ms, pool connect ~50ms. Actual timing depends on PostgreSQL container performance.
|
||||
|
||||
---
|
||||
|
||||
## What NOT to Change
|
||||
|
||||
- **Don't add `t.Parallel()`** — requires per-test transactions or separate databases
|
||||
- **Don't use transaction rollback** — some tests verify side effects needing committed data
|
||||
- **Don't change `init-script.sql`** — schema is correct, we're just running it too many times
|
||||
- **Don't touch `fixtures.testEmailCounter`** — it's designed to be monotonic
|
||||
Reference in New Issue
Block a user