john-okeefe
289284522b
- Add TEST_RELIABILITY_PLAN.md documenting test strategy - Add devices_test.go with device handler tests - Add device_auth_test.go with device authentication middleware tests
3389 lines
112 KiB
Markdown
3389 lines
112 KiB
Markdown
# Test Reliability Improvement Plan
|
|
## Bookhoard Pre-Production Test Overhaul
|
|
|
|
**Status**: Planning Phase - DO NOT IMPLEMENT YET
|
|
**Date**: 2025-02-13
|
|
**Scope**: Comprehensive test refactoring for production-grade reliability
|
|
**Goal**: Transform tests from brittle map-based assertions to robust struct-based validation with full database verification and edge case coverage
|
|
|
|
---
|
|
|
|
## 🎯 Executive Summary
|
|
|
|
### Current State
|
|
- **507+ instances** of `map[string]interface{}` instead of structs
|
|
- **Zero concurrency tests** for race conditions
|
|
- **Minimal database verification** after mutations
|
|
- **Happy-path only** for WebSocket, sync, OPDS, and book matching
|
|
- **No null/empty/missing** field edge case coverage
|
|
|
|
### Target State
|
|
- ✅ **100% struct-based assertions** for API responses (compile-time safety)
|
|
- ✅ **100% database verification** for all mutation operations
|
|
- ✅ **Comprehensive edge cases**: null/empty/missing, boundaries, Unicode
|
|
- ✅ **Concurrency coverage** for all critical sync paths
|
|
- ✅ **Error path coverage** for WebSocket, sync, OPDS, matching
|
|
|
|
### Impact
|
|
- **Silent API contract changes** → Compile-time detection
|
|
- **Data corruption bugs** → Pre-deployment prevention
|
|
- **Race conditions** → Caught in testing
|
|
- **Edge case crashes** → Eliminated before production
|
|
|
|
---
|
|
|
|
## 📋 Table of Contents
|
|
|
|
1. [Phase 1: Stop Silent Failures](#phase-1-stop-silent-failures) - **HIGHEST ROI**
|
|
2. [Phase 2: Concurrency Protection](#phase-2-concurrency-protection)
|
|
3. [Phase 3: Hardened Error Handling](#phase-3-hardened-error-handling)
|
|
4. [Phase 4: Load and Security](#phase-4-load-and-security)
|
|
5. [Implementation Order](#implementation-order)
|
|
6. [Verification Checklist](#verification-checklist)
|
|
|
|
---
|
|
|
|
## Phase 1: Stop Silent Failures
|
|
|
|
**Priority**: 🔴 CRITICAL - **Do this first**
|
|
**Time Estimate**: 20-25 files
|
|
**Risk Level**: Medium (compilation errors will guide fixes)
|
|
**ROI**: Highest - catches API changes and data corruption before production
|
|
|
|
### Objectives
|
|
1. Replace `map[string]interface{}` with actual structs in all assertions
|
|
2. Add database state verification after all mutation operations
|
|
3. Add null/empty/missing field edge cases for critical paths
|
|
|
|
---
|
|
|
|
### 1.1 Replace Map-Based Assertions with Structs
|
|
|
|
#### Problem Areas (507+ instances)
|
|
|
|
| File | Lines | Issue | Impact |
|
|
|-------|--------|--------|--------|
|
|
| `cmd/server/tests/registration_test.go` | 18-305 | Registration requests/responses use maps | Silent field renames |
|
|
| `cmd/server/tests/user_test.go` | 14-488 | Profile updates use maps | API contract drift |
|
|
| `cmd/server/tests/device_test.go` | 18-529 | Device CRUD uses maps | Missing field validation |
|
|
| `cmd/server/tests/collections_bulk_test.go` | 1-200 | Bulk operations use maps | Data corruption undetected |
|
|
| `cmd/server/tests/media_bulk_test.go` | 14-100 | Bulk updates use maps | Schema changes missed |
|
|
| `cmd/server/tests/conflicts_bulk_test.go` | 1-150 | Conflict resolution uses maps | Resolution failures silent |
|
|
| `cmd/server/tests/analytics_test.go` | 1-250 | Analytics queries use maps | Wrong field types undetected |
|
|
| `cmd/server/tests/search_test.go` | 21-244 | Search responses use maps | API changes break users |
|
|
| `cmd/server/tests/filtering_test.go` | 20-262 | Filter results use maps | Null handling untested |
|
|
| `cmd/server/tests/sorting_test.go` | 1-150 | Sort responses use maps | Type errors silent |
|
|
| `cmd/server/tests/library_test.go` | 15-378 | Library CRUD uses maps | Missing fields in tests |
|
|
| `cmd/server/tests/auth_test.go` | 19-300 | Auth responses use maps | Security issues undetected |
|
|
|
|
#### Conversion Pattern
|
|
|
|
**BEFORE** (brittle):
|
|
```go
|
|
// device_test.go:138-141
|
|
var response map[string]interface{}
|
|
json.Unmarshal(rec.Body.Bytes(), &response)
|
|
devices := response["devices"].([]interface{})
|
|
firstDevice := devices[0].(map[string]interface{})
|
|
deviceName, ok := firstDevice["device_name"].(string)
|
|
assert.True(t, ok, "Should have device_name")
|
|
assert.Equal(t, "Test Device", deviceName)
|
|
// ❌ If "device_name" → "device_name_display", test passes with empty string
|
|
```
|
|
|
|
**AFTER** (safe):
|
|
```go
|
|
// Import handler types
|
|
import "bookhoard/internal/handlers"
|
|
|
|
// device_test.go:138-141 (NEW)
|
|
var response handlers.DeviceListResponse
|
|
err := json.Unmarshal(rec.Body.Bytes(), &response)
|
|
require.NoError(t, err, "Response should match DeviceListResponse schema")
|
|
require.Greater(t, len(response.Devices), 0, "Should have at least one device")
|
|
assert.Equal(t, "Test Device", response.Devices[0].DeviceName)
|
|
// ✅ If DeviceName field renamed, COMPILATION FAILS
|
|
```
|
|
|
|
#### File-by-File Plan
|
|
|
|
##### File: `cmd/server/tests/device_test.go`
|
|
|
|
**Lines to modify**:
|
|
- 18-529: Full file conversion
|
|
|
|
**Specific changes**:
|
|
|
|
| Line Range | Current | New |
|
|
|------------|---------|-----|
|
|
| 36-38 | `var regResponse map[string]interface{}` | `var response handlers.DeviceRegistrationResponse` |
|
|
| 60-61 | `var statusResponse map[string]interface{}` | `var response handlers.DeviceAuthStatusResponse` |
|
|
| 82-85 | `var loginResponse map[string]interface{}` | `var response handlers.LoginResponse` |
|
|
| 131-135 | `var response map[string]interface{}` | `var response handlers.DeviceListResponse` |
|
|
| 167-173 | `var response map[string]interface{}` | `var response handlers.DeviceUpdateResponse` |
|
|
| 342-345 | `var response map[string]interface{}` | `var response handlers.DeviceTokenResponse` |
|
|
|
|
**Required imports** (add to top):
|
|
```go
|
|
import (
|
|
"bookhoard/internal/handlers" // NEW
|
|
// ... existing imports
|
|
)
|
|
```
|
|
|
|
**Action**: Convert all response parsing to use handler structs. Update assertions to access struct fields directly.
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/user_test.go`
|
|
|
|
**Lines to modify**:
|
|
- 14-488: Full file conversion
|
|
|
|
**Specific changes**:
|
|
|
|
| Line Range | Current | New |
|
|
|------------|---------|-----|
|
|
| 43-50 | `profile := map[string]interface{}` | Use `handlers.UserInfo` struct |
|
|
| 258-260 | `var result map[string]interface{}` | `var response handlers.UpdateResponse` |
|
|
| 472-481 | `users := []map[string]interface{}` | `var response handlers.UsersListResponse` |
|
|
|
|
**Action**: Import `handlers` package. Convert all user profile and list responses to structs.
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/registration_test.go`
|
|
|
|
**Lines to modify**:
|
|
- 18-305: Full file conversion
|
|
|
|
**Specific changes**:
|
|
|
|
| Test Function | Lines | Change |
|
|
|--------------|--------|--------|
|
|
| `TestRegistration_Flow` | 18-85 | Use `handlers.DeviceRegistrationResponse` |
|
|
| `TestRegistration_StatusCheck` | 87-120 | Use `handlers.DeviceAuthStatusResponse` |
|
|
| `TestRegistration_Approval` | 122-160 | Use `handlers.ApprovalResponse` |
|
|
| `TestRegistration_Rejection` | 162-200 | Use `handlers.RejectionResponse` |
|
|
| `TestRegistration_QRCode` | 202-240 | Use `handlers.QRCodeResponse` |
|
|
| `TestRegistration_Expiry` | 242-280 | Use `handlers.ExpiryResponse` |
|
|
| `TestRegistration_Concurrent` | 282-305 | Use all above structs |
|
|
|
|
**Action**: Import `handlers` package. Replace all map-based response parsing with struct-based unmarshaling.
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/collections_bulk_test.go`
|
|
|
|
**Lines to modify**:
|
|
- 1-200: Full file conversion
|
|
|
|
**Specific changes**:
|
|
|
|
| Test | Lines | Current | New |
|
|
|-------|--------|---------|-----|
|
|
| `TestCollectionsBulk_Add` | 14-50 | `map[string]interface{}` | `handlers.BulkAddResponse` |
|
|
| `TestCollectionsBulk_Remove` | 52-100 | `map[string]interface{}` | `handlers.BulkRemoveResponse` |
|
|
| `TestCollectionsBulk_Move` | 102-150 | `map[string]interface{}` | `handlers.BulkMoveResponse` |
|
|
| `TestCollectionsBulk_Validation` | 152-200 | `map[string]interface{}` | `handlers.ValidationErrorResponse` |
|
|
|
|
**Action**:
|
|
1. Import `handlers` package
|
|
2. Create response structs in `handlers/collections.go` if they don't exist
|
|
3. Update all bulk operation tests to use structs
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/media_bulk_test.go`
|
|
|
|
**Lines to modify**:
|
|
- 14-100: Full file conversion
|
|
|
|
**Specific changes**:
|
|
|
|
| Test | Lines | Current | New |
|
|
|-------|--------|---------|-----|
|
|
| `TestMediaBulk_Delete` | 14-50 | `map[string]interface{}` | `handlers.BulkDeleteResponse` |
|
|
| `TestMediaBulk_Update` | 52-100 | `map[string]interface{}` | `handlers.BulkUpdateResponse` |
|
|
|
|
**Action**:
|
|
1. Import `handlers` package
|
|
2. Ensure `handlers.MediaHandler` has response structs for bulk operations
|
|
3. Update all bulk media tests to use structs
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/conflicts_bulk_test.go`
|
|
|
|
**Lines to modify**:
|
|
- 1-150: Full file conversion
|
|
|
|
**Specific changes**:
|
|
|
|
| Test | Lines | Current | New |
|
|
|-------|--------|---------|-----|
|
|
| `TestConflictsBulk_Resolve` | 14-50 | `map[string]interface{}` | `handlers.BulkResolveResponse` |
|
|
| `TestConflictsBulk_Dismiss` | 52-100 | `map[string]interface{}` | `handlers.BulkDismissResponse` |
|
|
| `TestConflictsBulk_Escalate` | 102-150 | `map[string]interface{}` | `handlers.BulkEscalateResponse` |
|
|
|
|
**Action**:
|
|
1. Import `handlers` package
|
|
2. Ensure conflict response structs exist
|
|
3. Update all bulk conflict tests to use structs
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/analytics_test.go`
|
|
|
|
**Lines to modify**:
|
|
- 1-250: Full file conversion
|
|
|
|
**Specific changes**:
|
|
|
|
| Test | Lines | Current | New |
|
|
|-------|--------|---------|-----|
|
|
| `TestAnalytics_ReadingStats` | 14-50 | `map[string]interface{}` | `handlers.ReadingStatsResponse` |
|
|
| `TestAnalytics_PopularBooks` | 52-100 | `map[string]interface{}` | `handlers.PopularBooksResponse` |
|
|
| `TestAnalytics_DeviceUsage` | 102-150 | `map[string]interface{}` | `handlers.DeviceUsageResponse` |
|
|
| `TestAnalytics_UserActivity` | 152-200 | `map[string]interface{}` | `handlers.UserActivityResponse` |
|
|
|
|
**Action**:
|
|
1. Import `handlers` package
|
|
2. Ensure analytics response structs exist
|
|
3. Update all analytics tests to use structs
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/search_test.go`
|
|
|
|
**Lines to modify**:
|
|
- 21-244: Full file conversion
|
|
|
|
**Specific changes**:
|
|
|
|
| Test | Lines | Current | New |
|
|
|-------|--------|---------|-----|
|
|
| `TestSearch_Books` | 21-80 | `map[string]interface{}` | `handlers.SearchBooksResponse` |
|
|
| `TestSearch_Authors` | 82-140 | `map[string]interface{}` | `handlers.SearchAuthorsResponse` |
|
|
| `TestSearch_Series` | 142-200 | `map[string]interface{}` | `handlers.SearchSeriesResponse` |
|
|
| `TestSearch_Advanced` | 202-244 | `map[string]interface{}` | `handlers.AdvancedSearchResponse` |
|
|
|
|
**Action**:
|
|
1. Import `handlers` package
|
|
2. Ensure search response structs exist
|
|
3. Update all search tests to use structs
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/filtering_test.go`
|
|
|
|
**Lines to modify**:
|
|
- 20-262: Full file conversion
|
|
|
|
**Specific changes**:
|
|
|
|
| Test | Lines | Current | New |
|
|
|-------|--------|---------|-----|
|
|
| `TestFiltering_ByStatus` | 20-60 | `map[string]interface{}` | `handlers.FilteredBooksResponse` |
|
|
| `TestFiltering_ByCollection` | 62-120 | `map[string]interface{}` | `handlers.CollectionBooksResponse` |
|
|
| `TestFiltering_ByDateRange` | 122-180 | `map[string]interface{}` | `handlers.DateRangeResponse` |
|
|
| `TestFiltering_Advanced` | 182-262 | `map[string]interface{}` | `handlers.AdvancedFilterResponse` |
|
|
|
|
**Action**:
|
|
1. Import `handlers` package
|
|
2. Ensure filtering response structs exist
|
|
3. Update all filtering tests to use structs
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/sorting_test.go`
|
|
|
|
**Lines to modify**:
|
|
- 1-150: Full file conversion
|
|
|
|
**Specific changes**:
|
|
|
|
| Test | Lines | Current | New |
|
|
|-------|--------|---------|-----|
|
|
| `TestSorting_ByTitle` | 14-40 | `map[string]interface{}` | `handlers.SortedBooksResponse` |
|
|
| `TestSorting_ByAuthor` | 42-80 | `map[string]interface{}` | `handlers.SortedBooksResponse` |
|
|
| `TestSorting_ByDateAdded` | 82-120 | `map[string]interface{}` | `handlers.SortedBooksResponse` |
|
|
| `TestSorting_ByRating` | 122-150 | `map[string]interface{}` | `handlers.SortedBooksResponse` |
|
|
|
|
**Action**:
|
|
1. Import `handlers` package
|
|
2. Ensure sorting response struct exists
|
|
3. Update all sorting tests to use structs
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/library_test.go`
|
|
|
|
**Lines to modify**:
|
|
- 15-378: Full file conversion
|
|
|
|
**Specific changes**:
|
|
|
|
| Test | Lines | Current | New |
|
|
|-------|--------|---------|-----|
|
|
| `TestLibraryCreation` | 91-161 | `map[string]interface{}` | `handlers.LibraryResponse` |
|
|
| `TestLibraryTypes` | 164-205 | `map[string]interface{}` | `handlers.LibraryTypesResponse` |
|
|
| `TestUserVisibleLibraries` | 208-238 | `map[string]interface{}` | `handlers.VisibleLibrariesResponse` |
|
|
| `TestMediaItemsList` | 241-269 | `map[string]interface{}` | `handlers.MediaItemsResponse` |
|
|
| `TestJSONValidation` | 272-333 | `map[string]interface{}` | `handlers.ValidationResponse` |
|
|
| `TestErrorHandling` | 336-377 | `map[string]interface{}` | `handlers.ErrorResponse` |
|
|
|
|
**Action**:
|
|
1. Import `handlers` package
|
|
2. Ensure library response structs exist
|
|
3. Update all library tests to use structs
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/auth_test.go`
|
|
|
|
**Lines to modify**:
|
|
- 19-300: Full file conversion
|
|
|
|
**Specific changes**:
|
|
|
|
| Test | Lines | Current | New |
|
|
|-------|--------|---------|-----|
|
|
| `TestAuth_Login` | 19-80 | `map[string]interface{}` | `handlers.LoginResponse` |
|
|
| `TestAuth_Register` | 82-140 | `map[string]interface{}` | `handlers.RegisterResponse` |
|
|
| `TestAuth_Refresh` | 142-200 | `map[string]interface{}` | `handlers.RefreshResponse` |
|
|
| `TestAuth_Logout` | 202-250 | `map[string]interface{}` | `handlers.LogoutResponse` |
|
|
| `TestAuth_PasswordReset` | 252-300 | `map[string]interface{}` | `handlers.PasswordResetResponse` |
|
|
|
|
**Action**:
|
|
1. Import `handlers` package
|
|
2. Ensure auth response structs exist
|
|
3. Update all auth tests to use structs
|
|
|
|
---
|
|
|
|
### 1.2 Add Database State Verification
|
|
|
|
#### Problem Areas
|
|
|
|
| File | Tests Missing DB Verification | Risk |
|
|
|------|---------------------------|-------|
|
|
| `device_test.go` | `TestUpdateDevice`, `TestDeleteDevice` | API says success but DB unchanged |
|
|
| `user_test.go` | All profile update tests | Silent data loss |
|
|
| `collections_bulk_test.go` | All bulk operations | Orphaned collection_books records |
|
|
| `media_bulk_test.go` | All bulk deletes | Files deleted but DB records remain |
|
|
| `conflicts_bulk_test.go` | All conflict resolutions | Conflicts marked resolved but aren't |
|
|
| `library_test.go` | Create, update, delete libraries | Schema violations undetected |
|
|
| `registration_test.go` | Device approval flow | Device created but not linked to user |
|
|
| `sync_integration_test.go` | Sync success but no DB verification | Progress lost |
|
|
|
|
#### Verification Pattern
|
|
|
|
**BEFORE** (insufficient):
|
|
```go
|
|
// device_test.go:191
|
|
assert.Equal(t, http.StatusNoContent, rec.Code, "Should delete device")
|
|
// ❌ No DB query - device might still exist
|
|
```
|
|
|
|
**AFTER** (complete):
|
|
```go
|
|
// device_test.go:191 (NEW)
|
|
assert.Equal(t, http.StatusNoContent, rec.Code, "Should delete device")
|
|
|
|
// NEW: Verify device actually deleted from database
|
|
pgDeviceID := pgtype.UUID{Bytes: [16]byte(device.ID), Valid: true}
|
|
_, err := setup.DB.GetDevice(context.Background(), pgDeviceID)
|
|
assert.Error(t, err, "Device should be deleted from database")
|
|
// ✅ Now we know delete actually worked
|
|
```
|
|
|
|
#### File-by-File Plan
|
|
|
|
##### File: `cmd/server/tests/device_test.go`
|
|
|
|
**Tests needing DB verification**:
|
|
|
|
| Test Function | Lines | Current State | Required Addition |
|
|
|--------------|--------|---------------|-------------------|
|
|
| `TestUpdateDevice` | 144-176 | Checks HTTP 200 only | Query DB to verify name, sync_enabled, sync_frequency actually updated |
|
|
| `TestDeleteDevice` | 178-197 | Already has DB check | ✅ Already good - keep as example |
|
|
| `TestRegenerateDeviceToken_Success` | 326-362 | Checks HTTP 200 only | Query DB to verify auth_token actually changed |
|
|
| `TestRegenerateDeviceToken_OldTokenInvalidated` | 364-407 | Checks token works only | Query DB to verify only one valid token exists |
|
|
| `TestDeviceRegistrationFlow` | 18-114 | Checks response codes | Query DB after step 4 to verify device exists and is linked to user |
|
|
|
|
**Action**:
|
|
After each successful mutation, add DB query to verify state change:
|
|
|
|
```go
|
|
// Example for TestUpdateDevice (after line 175)
|
|
// NEW: Verify database state
|
|
updatedDevice, err := setup.DB.GetDevice(context.Background(), pgtype.UUID{Bytes: [16]byte(device.ID), Valid: true})
|
|
require.NoError(t, err, "Should retrieve updated device")
|
|
assert.Equal(t, "Updated Device Name", updatedDevice.DeviceName)
|
|
assert.Equal(t, false, updatedDevice.SyncEnabled.Bool)
|
|
assert.Equal(t, int32(10), updatedDevice.SyncFrequencyMinutes.Int32)
|
|
```
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/user_test.go`
|
|
|
|
**Tests needing DB verification**:
|
|
|
|
| Test Function | Lines | Current State | Required Addition |
|
|
|--------------|--------|---------------|-------------------|
|
|
| `TestUserProfileEndpoints` | 15-77 | Checks HTTP 200 only | Query DB after PUT to verify first_name, last_name updated |
|
|
| `TestUserUpdateEndpoints` | 80-336 | All checks HTTP codes only | Query DB after each successful update (email, username, password, theme) |
|
|
| `TestAccountDeletion` | 340-440 | Checks HTTP codes only | Query DB after delete to verify user record removed |
|
|
|
|
**Action**:
|
|
Add DB verification queries after successful updates:
|
|
|
|
```go
|
|
// Example for email update (after line 112)
|
|
// NEW: Verify database state
|
|
updatedUser, err := setup.DB.GetUserByEmail(context.Background(), "newemail@example.com")
|
|
require.NoError(t, err, "Should find user with new email")
|
|
assert.Equal(t, "newemail@example.com", updatedUser.Email)
|
|
```
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/collections_bulk_test.go`
|
|
|
|
**Tests needing DB verification**:
|
|
|
|
| Test Function | Lines | Current State | Required Addition |
|
|
|--------------|--------|---------------|-------------------|
|
|
| `TestCollectionsBulk_Add` | All | Checks response only | Query collection_items table to verify all book IDs added |
|
|
| `TestCollectionsBulk_Remove` | All | Checks response only | Query collection_items table to verify all book IDs removed |
|
|
| `TestCollectionsBulk_Move` | All | Checks response only | Query collection_items table to verify books moved to target collection |
|
|
| `TestCollectionsBulk_DuplicateHandling` | All | Checks response only | Query collection_items to verify no duplicate entries created |
|
|
|
|
**Action**:
|
|
After bulk operations, query junction table:
|
|
|
|
```go
|
|
// Example for TestCollectionsBulk_Add (after assertion)
|
|
// NEW: Verify database state
|
|
collectionItems, err := setup.DB.GetCollectionItems(context.Background(), collectionID)
|
|
require.NoError(t, err, "Should retrieve collection items")
|
|
require.Equal(t, len(bookIDs), len(collectionItems), "All books should be in collection")
|
|
|
|
// Verify each book ID exists
|
|
actualIDs := make([]uuid.UUID, len(collectionItems))
|
|
for i, item := range collectionItems {
|
|
actualIDs[i] = item.MediaItemID
|
|
}
|
|
assert.ElementsMatch(t, bookIDs, actualIDs, "All book IDs should match")
|
|
```
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/media_bulk_test.go`
|
|
|
|
**Tests needing DB verification**:
|
|
|
|
| Test Function | Lines | Current State | Required Addition |
|
|
|--------------|--------|---------------|-------------------|
|
|
| `TestMediaBulk_Delete` | 80-100 | Checks response only | Query media_items table to verify records deleted (not just file_deleted flag) |
|
|
| `TestMediaBulk_Update` | All (if exists) | Checks response only | Query media_items table to verify all fields actually updated |
|
|
|
|
**Action**:
|
|
Add DB verification after bulk deletes:
|
|
|
|
```go
|
|
// Example for TestMediaBulk_Delete (after line 99)
|
|
// NEW: Verify database state
|
|
for _, mediaID := range mediaIDs {
|
|
pgID := pgtype.UUID{Bytes: [16]byte(mediaID), Valid: true}
|
|
_, err := setup.DB.GetMediaItem(context.Background(), pgID)
|
|
assert.Error(t, err, "Media item should be deleted from database")
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/conflicts_bulk_test.go`
|
|
|
|
**Tests needing DB verification**:
|
|
|
|
| Test Function | Lines | Current State | Required Addition |
|
|
|--------------|--------|---------------|-------------------|
|
|
| `TestConflictsBulk_Resolve` | All | Checks response only | Query sync_conflicts table to verify resolution_status updated |
|
|
| `TestConflictsBulk_Dismiss` | All | Checks response only | Query sync_conflicts table to verify dismissed status |
|
|
| `TestConflictsBulk_Escalate` | All | Checks response only | Query sync_conflicts table to verify escalated to admin |
|
|
|
|
**Action**:
|
|
Add DB verification after conflict operations:
|
|
|
|
```go
|
|
// Example for TestConflictsBulk_Resolve (after assertion)
|
|
// NEW: Verify database state
|
|
for _, conflictID := range conflictIDs {
|
|
pgID := pgtype.UUID{Bytes: [16]byte(conflictID), Valid: true}
|
|
conflict, err := setup.DB.GetSyncConflict(context.Background(), pgID)
|
|
require.NoError(t, err, "Should retrieve conflict")
|
|
assert.Equal(t, "resolved", conflict.ResolutionStatus.String)
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/sync_integration_test.go`
|
|
|
|
**Tests needing DB verification**:
|
|
|
|
| Test Function | Lines | Current State | Required Addition |
|
|
|--------------|--------|---------------|-------------------|
|
|
| `TestSyncFull_Initial` | All | Checks response only | Query reading_progress, media_notes, media_highlights tables to verify sync data saved |
|
|
| `TestSyncFull_Incremental` | All | Checks response only | Query DB to verify only changed fields updated |
|
|
| `TestSyncConflict_Resolution` | All | Checks response only | Query sync_conflicts table to verify conflict created and resolved |
|
|
|
|
**Action**:
|
|
Add comprehensive DB verification after sync:
|
|
|
|
```go
|
|
// Example for TestSyncFull_Initial (after success assertion)
|
|
// NEW: Verify database state
|
|
progress, err := setup.DB.GetReadingProgress(context.Background(), database.GetReadingProgressParams{
|
|
MediaItemID: pgtype.UUID{Bytes: [16]byte(mediaItemID), Valid: true},
|
|
UserID: pgtype.UUID{Bytes: [16]byte(userID), Valid: true},
|
|
})
|
|
require.NoError(t, err, "Should retrieve reading progress")
|
|
assert.Equal(t, testProgress.Percentage, progress.Percentage.Float64)
|
|
```
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/registration_test.go`
|
|
|
|
**Tests needing DB verification**:
|
|
|
|
| Test Function | Lines | Current State | Required Addition |
|
|
|--------------|--------|---------------|-------------------|
|
|
| `TestRegistrationFlow` | 18-114 | Checks responses only | After approval, query devices table to verify device exists and linked to user |
|
|
| `TestRegistrationApproval` | All | Checks response only | Query devices table to verify auth_token set and device active |
|
|
|
|
**Action**:
|
|
Add DB verification after registration approval:
|
|
|
|
```go
|
|
// Example for TestRegistrationFlow (after line 93)
|
|
// NEW: Verify database state
|
|
pgDeviceID := pgtype.UUID{Bytes: [16]byte(deviceID), Valid: true}
|
|
device, err := setup.DB.GetDevice(context.Background(), pgDeviceID)
|
|
require.NoError(t, err, "Device should exist in database")
|
|
assert.NotEmpty(t, device.AuthToken, "Device should have auth token after approval")
|
|
```
|
|
|
|
---
|
|
|
|
### 1.3 Add Null/Empty/Missing Edge Cases
|
|
|
|
#### Problem Areas
|
|
|
|
| Field Type | Missing Tests | Production Risk |
|
|
|-----------|---------------|-----------------|
|
|
| **Text fields** (`first_name`, `last_name`, `description`) | `null` vs `""` vs missing | Constraint violations, different behavior |
|
|
| **Optional arrays** (`tags`, `contributors`) | `null` vs `[]` vs missing | Search failures |
|
|
| **Optional dates** (`date_published`) | `null` vs invalid vs missing | Display corruption |
|
|
| **Optional IDs** (`asin`, `isbn`) | `null` vs `""` vs missing | External API failures |
|
|
| **Optional enums** (`reading_status`) | `null` vs invalid vs missing | Filter breaks |
|
|
|
|
#### Test Patterns to Add
|
|
|
|
##### Pattern 1: Null vs Empty String vs Missing
|
|
|
|
```go
|
|
// NEW TEST: Add to user_test.go
|
|
func TestUserProfile_NullVsEmptyVsMissing(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
|
|
testCases := []struct {
|
|
name string
|
|
payload map[string]interface{}
|
|
expectOK bool
|
|
}{
|
|
{
|
|
name: "Null first_name",
|
|
payload: map[string]interface{}{
|
|
"first_name": nil,
|
|
},
|
|
expectOK: true, // null should be allowed (optional field)
|
|
},
|
|
{
|
|
name: "Empty string first_name",
|
|
payload: map[string]interface{}{
|
|
"first_name": "",
|
|
},
|
|
expectOK: true, // empty string should be allowed
|
|
},
|
|
{
|
|
name: "Missing first_name",
|
|
payload: map[string]interface{}{
|
|
// first_name not included
|
|
},
|
|
expectOK: true, // missing should be allowed (optional)
|
|
},
|
|
{
|
|
name: "Valid first_name",
|
|
payload: map[string]interface{}{
|
|
"first_name": "John",
|
|
},
|
|
expectOK: true,
|
|
},
|
|
}
|
|
|
|
for _, tc := range testCases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
jsonData, _ := json.Marshal(tc.payload)
|
|
req := httptest.NewRequest("PUT", "/api/auth/profile", bytes.NewBuffer(jsonData))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
rec := httptest.NewRecorder()
|
|
setup.Server.Config.Handler.ServeHTTP(rec, req)
|
|
|
|
if tc.expectOK {
|
|
assert.Equal(t, http.StatusOK, rec.Code)
|
|
} else {
|
|
assert.NotEqual(t, http.StatusOK, rec.Code)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
```
|
|
|
|
##### Pattern 2: Null vs Empty Array vs Missing
|
|
|
|
```go
|
|
// NEW TEST: Add to media_bulk_test.go
|
|
func TestMediaTags_NullVsEmptyVsMissing(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
mediaID := createTestMediaItemID(t, setup.Server, token)
|
|
|
|
testCases := []struct {
|
|
name string
|
|
tags interface{}
|
|
expectOK bool
|
|
}{
|
|
{
|
|
name: "Null tags",
|
|
tags: nil,
|
|
expectOK: true, // null should clear tags
|
|
},
|
|
{
|
|
name: "Empty array tags",
|
|
tags: []string{},
|
|
expectOK: true, // empty array should clear tags
|
|
},
|
|
{
|
|
name: "Missing tags",
|
|
// tags field not included
|
|
expectOK: true, // missing should not change tags
|
|
},
|
|
{
|
|
name: "Valid tags",
|
|
tags: []string{"fiction", "science-fiction"},
|
|
expectOK: true,
|
|
},
|
|
}
|
|
|
|
for _, tc := range testCases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
payload := map[string]interface{}{"tags": tc.tags}
|
|
jsonData, _ := json.Marshal(payload)
|
|
|
|
req := httptest.NewRequest("PUT", fmt.Sprintf("/api/media-items/%s", mediaID), bytes.NewBuffer(jsonData))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
rec := httptest.NewRecorder()
|
|
setup.Server.Config.Handler.ServeHTTP(rec, req)
|
|
|
|
if tc.expectOK {
|
|
assert.Equal(t, http.StatusOK, rec.Code)
|
|
|
|
// NEW: Verify database state
|
|
pgMediaID := pgtype.UUID{Bytes: [16]byte(mediaID), Valid: true}
|
|
media, err := setup.DB.GetMediaItem(context.Background(), pgMediaID)
|
|
require.NoError(t, err)
|
|
|
|
if tc.tags == nil {
|
|
assert.Empty(t, media.Tags, "Tags should be null/empty")
|
|
} else if tags, ok := tc.tags.([]string); ok && len(tags) == 0 {
|
|
assert.Empty(t, media.Tags, "Tags should be empty")
|
|
} else if tags, ok := tc.tags.([]string); ok {
|
|
assert.ElementsMatch(t, tags, media.Tags, "Tags should match")
|
|
}
|
|
}
|
|
})
|
|
}
|
|
}
|
|
```
|
|
|
|
#### File-by-File Plan
|
|
|
|
##### File: `cmd/server/tests/user_test.go`
|
|
|
|
**Add new test function after line 336**:
|
|
|
|
```go
|
|
// NEW TEST FUNCTION
|
|
func TestUserProfile_NullVsEmptyVsMissing(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
|
|
testCases := []struct {
|
|
name string
|
|
field string
|
|
value interface{}
|
|
expectOK bool
|
|
}{
|
|
// First name tests
|
|
{"Null first_name", "first_name", nil, true},
|
|
{"Empty first_name", "first_name", "", true},
|
|
{"Missing first_name", "first_name", nil, true}, // Not included in payload
|
|
{"Valid first_name", "first_name", "Jane", true},
|
|
|
|
// Last name tests
|
|
{"Null last_name", "last_name", nil, true},
|
|
{"Empty last_name", "last_name", "", true},
|
|
{"Valid last_name", "last_name", "Doe", true},
|
|
|
|
// Email tests (required field, different expectations)
|
|
{"Null email", "email", nil, false}, // Email required, null should fail
|
|
{"Empty email", "email", "", false}, // Email required, empty should fail
|
|
{"Valid email", "email", "new@example.com", true},
|
|
}
|
|
|
|
for _, tc := range testCases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
payload := map[string]interface{}{tc.field: tc.value}
|
|
jsonData, _ := json.Marshal(payload)
|
|
|
|
req := httptest.NewRequest("PUT", "/api/auth/profile", bytes.NewBuffer(jsonData))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
rec := httptest.NewRecorder()
|
|
setup.Server.Config.Handler.ServeHTTP(rec, req)
|
|
|
|
if tc.expectOK {
|
|
assert.Equal(t, http.StatusOK, rec.Code, tc.name)
|
|
} else {
|
|
assert.NotEqual(t, http.StatusOK, rec.Code, tc.name)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
```
|
|
|
|
**Action**: Add new test file or append to existing `user_test.go` after line 336.
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/media_bulk_test.go`
|
|
|
|
**Add new test function**:
|
|
|
|
```go
|
|
// NEW TEST FUNCTION
|
|
func TestMediaItem_NullVsEmptyVsMissing(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
mediaID := createTestMediaItemID(t, setup.Server, token)
|
|
|
|
testCases := []struct {
|
|
name string
|
|
field string
|
|
value interface{}
|
|
expectOK bool
|
|
verifyDB bool
|
|
}{
|
|
// Tags tests (array field)
|
|
{"Null tags", "tags", nil, true, true},
|
|
{"Empty array tags", "tags", []string{}, true, true},
|
|
{"Valid tags", "tags", []string{"fiction", "sci-fi"}, true, true},
|
|
|
|
// ISBN tests (optional text)
|
|
{"Null ISBN", "isbn", nil, true, true},
|
|
{"Empty ISBN", "isbn", "", true, true},
|
|
{"Valid ISBN", "isbn", "978-0-123456-78-9", true, true},
|
|
|
|
// ASIN tests (optional text)
|
|
{"Null ASIN", "asin", nil, true, true},
|
|
{"Empty ASIN", "asin", "", true, true},
|
|
{"Valid ASIN", "asin", "B08XXXXXXX", true, true},
|
|
|
|
// Date published tests (optional date)
|
|
{"Null date_published", "date_published", nil, true, true},
|
|
{"Invalid date_published", "date_published", "not-a-date", false, false},
|
|
{"Valid date_published", "date_published", "2024-01-15", true, true},
|
|
}
|
|
|
|
for _, tc := range testCases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
payload := map[string]interface{}{tc.field: tc.value}
|
|
jsonData, _ := json.Marshal(payload)
|
|
|
|
req := httptest.NewRequest("PUT", fmt.Sprintf("/api/media-items/%s", mediaID), bytes.NewBuffer(jsonData))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
rec := httptest.NewRecorder()
|
|
setup.Server.Config.Handler.ServeHTTP(rec, req)
|
|
|
|
if tc.expectOK {
|
|
assert.Equal(t, http.StatusOK, rec.Code, tc.name)
|
|
|
|
// NEW: Verify database state
|
|
if tc.verifyDB {
|
|
pgMediaID := pgtype.UUID{Bytes: [16]byte(mediaID), Valid: true}
|
|
media, err := setup.DB.GetMediaItem(context.Background(), pgMediaID)
|
|
require.NoError(t, err, "Should retrieve media item")
|
|
|
|
// Verify field value matches expected
|
|
switch tc.field {
|
|
case "tags":
|
|
if tc.value == nil {
|
|
assert.Empty(t, media.Tags, "Tags should be null/empty")
|
|
} else if tags, ok := tc.value.([]string); ok {
|
|
assert.ElementsMatch(t, tags, media.Tags, "Tags should match")
|
|
}
|
|
case "isbn":
|
|
if tc.value == nil || tc.value == "" {
|
|
assert.False(t, media.Isbn.Valid, "ISBN should be null")
|
|
} else {
|
|
assert.True(t, media.Isbn.Valid, "ISBN should be set")
|
|
assert.Equal(t, tc.value.(string), media.Isbn.String)
|
|
}
|
|
case "asin":
|
|
if tc.value == nil || tc.value == "" {
|
|
assert.False(t, media.Asin.Valid, "ASIN should be null")
|
|
} else {
|
|
assert.True(t, media.Asin.Valid, "ASIN should be set")
|
|
assert.Equal(t, tc.value.(string), media.Asin.String)
|
|
}
|
|
}
|
|
}
|
|
} else {
|
|
assert.NotEqual(t, http.StatusOK, rec.Code, tc.name)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
```
|
|
|
|
**Action**: Add new test function to `media_bulk_test.go`.
|
|
|
|
---
|
|
|
|
##### File: `cmd/server/tests/library_test.go`
|
|
|
|
**Add new test function**:
|
|
|
|
```go
|
|
// NEW TEST FUNCTION
|
|
func TestLibrary_NullVsEmptyVsMissing(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
userID := getTestUserID(t, setup.DB)
|
|
|
|
// Create library first
|
|
libReq := map[string]interface{}{
|
|
"name": "Test Library",
|
|
"type": "ebooks",
|
|
}
|
|
libBody, _ := json.Marshal(libReq)
|
|
req := httptest.NewRequest("POST", setup.Server.URL+"/api/libraries", bytes.NewBuffer(libBody))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
|
|
client := &http.Client{}
|
|
resp, err := client.Do(req)
|
|
require.NoError(t, err)
|
|
defer resp.Body.Close()
|
|
require.Equal(t, http.StatusCreated, resp.StatusCode)
|
|
|
|
var libResponse map[string]interface{}
|
|
json.NewDecoder(resp.Body).Decode(&libResponse)
|
|
libraryID := libResponse["id"].(string)
|
|
|
|
testCases := []struct {
|
|
name string
|
|
field string
|
|
value interface{}
|
|
expectOK bool
|
|
}{
|
|
{"Null description", "description", nil, true},
|
|
{"Empty description", "description", "", true},
|
|
{"Valid description", "description", "A test library", true},
|
|
|
|
{"Null name", "name", nil, false}, // Name required
|
|
{"Empty name", "name", "", false}, // Name required
|
|
{"Valid name", "name", "Updated Library", true},
|
|
}
|
|
|
|
for _, tc := range testCases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
payload := map[string]interface{}{tc.field: tc.value}
|
|
jsonData, _ := json.Marshal(payload)
|
|
|
|
req := httptest.NewRequest("PUT", fmt.Sprintf("%s/api/libraries/%s", setup.Server.URL, libraryID), bytes.NewBuffer(jsonData))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
|
|
resp, err := client.Do(req)
|
|
require.NoError(t, err)
|
|
defer resp.Body.Close()
|
|
|
|
if tc.expectOK {
|
|
assert.Equal(t, http.StatusOK, resp.StatusCode, tc.name)
|
|
} else {
|
|
assert.NotEqual(t, http.StatusOK, resp.StatusCode, tc.name)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
```
|
|
|
|
**Action**: Add new test function to `library_test.go`.
|
|
|
|
---
|
|
|
|
### Phase 1 Summary
|
|
|
|
**Files Modified**: 12+ test files
|
|
**Lines Changed**: ~2000+ lines
|
|
**New Test Functions**: 15+
|
|
**Compilation Checks**: 100% (struct-based assertions)
|
|
**Database Verification**: 100% of mutations
|
|
|
|
**Verification Steps**:
|
|
1. ✅ All tests compile with struct-based assertions
|
|
2. ✅ All mutation operations verify database state
|
|
3. ✅ Null/empty/missing edge cases covered for critical fields
|
|
|
|
---
|
|
|
|
## Phase 2: Concurrency Protection
|
|
|
|
**Priority**: 🟠 HIGH - **Do after Phase 1**
|
|
**Time Estimate**: 8-10 new test functions
|
|
**Risk Level**: Low (new tests only)
|
|
**ROI**: High - prevents lost updates and deadlocks
|
|
|
|
### Objectives
|
|
1. Add race condition tests for all sync operations
|
|
2. Add concurrent bulk operation tests
|
|
3. Add concurrent profile + device update tests
|
|
|
|
---
|
|
|
|
### 2.1 Sync Operation Race Conditions
|
|
|
|
#### Problem Areas
|
|
|
|
| Operation | Missing Tests | Risk |
|
|
|-----------|---------------|-------|
|
|
| **Reading progress sync** | Multiple devices updating same book simultaneously | Lost updates, last-write-wins |
|
|
| **Conflict resolution** | Two users resolving same conflict | Duplicate resolution records |
|
|
| **Bulk operations** | Bulk delete + individual update simultaneously | Orphaned records |
|
|
| **Device registration** | Multiple devices registering simultaneously | Duplicate device IDs |
|
|
| **Token regeneration** | Regenerate token while old token in use | Auth failures |
|
|
|
|
#### Test Pattern
|
|
|
|
```go
|
|
// NEW TEST TEMPLATE: Concurrent sync
|
|
func TestSyncConcurrent_ProgressUpdates(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
device := setup.CreateDevice(t, "Test Device", "koreader", "test-concurrent")
|
|
mediaID := createTestMediaItemID(t, setup.Server, token)
|
|
|
|
// Start with initial progress
|
|
initialProgress := map[string]interface{}{
|
|
"media_item_id": mediaID,
|
|
"percentage": 25.0,
|
|
"current_page": 50,
|
|
"total_pages": 200,
|
|
}
|
|
|
|
// Simulate 3 concurrent updates from different "sources"
|
|
var wg sync.WaitGroup
|
|
errors := make(chan error, 3)
|
|
|
|
for i := 0; i < 3; i++ {
|
|
wg.Add(1)
|
|
go func(updateNum int) {
|
|
defer wg.Done()
|
|
|
|
progressData := initialProgress
|
|
progressData["percentage"] = 25.0 + float64(updateNum * 10)
|
|
progressData["current_page"] = 50 + (updateNum * 20)
|
|
|
|
jsonData, _ := json.Marshal(progressData)
|
|
req := httptest.NewRequest("POST", setup.Server.URL+"/api/sync/koreader/progress", bytes.NewBuffer(jsonData))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+device.AuthToken)
|
|
|
|
client := &http.Client{Timeout: 5 * time.Second}
|
|
resp, err := client.Do(req)
|
|
if err != nil {
|
|
errors <- err
|
|
return
|
|
}
|
|
defer resp.Body.Close()
|
|
|
|
if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusConflict {
|
|
errors <- fmt.Errorf("unexpected status: %d", resp.StatusCode)
|
|
}
|
|
}(i)
|
|
}
|
|
|
|
wg.Wait()
|
|
close(errors)
|
|
|
|
// Check for errors (conflicts are OK, but not failures)
|
|
for err := range errors {
|
|
t.Logf("Concurrent update error: %v", err)
|
|
// We expect either success or conflict, not failures
|
|
}
|
|
|
|
// NEW: Verify final state is consistent
|
|
pgMediaID := pgtype.UUID{Bytes: [16]byte(mediaID), Valid: true}
|
|
pgUserID := pgtype.UUID{Bytes: [16]byte(setup.User.ID), Valid: true}
|
|
progress, err := setup.DB.GetReadingProgress(context.Background(), database.GetReadingProgressParams{
|
|
MediaItemID: pgMediaID,
|
|
UserID: pgUserID,
|
|
})
|
|
require.NoError(t, err, "Should retrieve final progress")
|
|
|
|
// Verify one of the updates won (no corruption)
|
|
assert.GreaterOrEqual(t, progress.Percentage.Float64, 25.0)
|
|
assert.LessOrEqual(t, progress.Percentage.Float64, 55.0)
|
|
assert.True(t, progress.Percentage.Float64 == 25.0 ||
|
|
progress.Percentage.Float64 == 35.0 ||
|
|
progress.Percentage.Float64 == 45.0,
|
|
"Final progress should be one of the concurrent updates")
|
|
}
|
|
```
|
|
|
|
#### File-by-File Plan
|
|
|
|
##### File: `cmd/server/tests/sync_integration_test.go`
|
|
|
|
**Add new test functions**:
|
|
|
|
1. **TestSyncConcurrent_ProgressUpdates** (after existing sync tests)
|
|
- Lines: Add ~80 lines
|
|
- Purpose: Multiple devices updating same book's progress
|
|
- Verification: Final progress is one of the updates (no corruption)
|
|
|
|
2. **TestSyncConcurrent_NotesUpdates** (after progress test)
|
|
- Lines: Add ~80 lines
|
|
- Purpose: Multiple devices adding notes to same book
|
|
- Verification: All notes saved, no duplicates
|
|
|
|
3. **TestSyncConcurrent_HighlightsUpdates** (after notes test)
|
|
- Lines: Add ~80 lines
|
|
- Purpose: Multiple devices adding highlights to same book
|
|
- Verification: All highlights saved, no corruption
|
|
|
|
4. **TestSyncConcurrent_ProgressPlusNote** (after highlights test)
|
|
- Lines: Add ~80 lines
|
|
- Purpose: Progress update + note creation simultaneously
|
|
- Verification: Both operations succeed
|
|
|
|
**Action**: Add 4 new test functions to `sync_integration_test.go`.
|
|
|
|
---
|
|
|
|
### 2.2 Bulk Operation Race Conditions
|
|
|
|
#### Test Pattern
|
|
|
|
```go
|
|
// NEW TEST: Concurrent bulk + individual operations
|
|
func TestBulkConcurrent_DeleteAndUpdate(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
|
|
// Create multiple media items
|
|
var mediaIDs []string
|
|
for i := 0; i < 5; i++ {
|
|
id := createTestMediaItemID(t, setup.Server, token)
|
|
mediaIDs = append(mediaIDs, id)
|
|
}
|
|
|
|
var wg sync.WaitGroup
|
|
errors := make(chan error, 2)
|
|
|
|
// Concurrent operation 1: Bulk delete
|
|
wg.Add(1)
|
|
go func() {
|
|
defer wg.Done()
|
|
|
|
payload := map[string]interface{}{
|
|
"media_item_ids": mediaIDs[0:3], // Delete first 3
|
|
}
|
|
jsonData, _ := json.Marshal(payload)
|
|
|
|
req := httptest.NewRequest("POST", setup.Server.URL+"/api/media-items/bulk-delete", bytes.NewBuffer(jsonData))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
|
|
client := &http.Client{Timeout: 5 * time.Second}
|
|
resp, err := client.Do(req)
|
|
if err != nil {
|
|
errors <- err
|
|
return
|
|
}
|
|
defer resp.Body.Close()
|
|
|
|
if resp.StatusCode != http.StatusOK {
|
|
errors <- fmt.Errorf("bulk delete failed: %d", resp.StatusCode)
|
|
}
|
|
}()
|
|
|
|
// Concurrent operation 2: Individual update on same items
|
|
wg.Add(1)
|
|
go func() {
|
|
defer wg.Done()
|
|
|
|
// Try to update item that might be deleted
|
|
payload := map[string]interface{}{
|
|
"title": "Updated Title",
|
|
}
|
|
jsonData, _ := json.Marshal(payload)
|
|
|
|
req := httptest.NewRequest("PUT", fmt.Sprintf("%s/api/media-items/%s", setup.Server.URL, mediaIDs[1]), bytes.NewBuffer(jsonData))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
|
|
client := &http.Client{Timeout: 5 * time.Second}
|
|
resp, err := client.Do(req)
|
|
if err != nil {
|
|
errors <- err
|
|
return
|
|
}
|
|
defer resp.Body.Close()
|
|
|
|
// Either succeeds (delete hadn't run yet) or fails (item deleted)
|
|
if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusNotFound {
|
|
errors <- fmt.Errorf("update failed with unexpected status: %d", resp.StatusCode)
|
|
}
|
|
}()
|
|
|
|
wg.Wait()
|
|
close(errors)
|
|
|
|
// Check for unexpected errors
|
|
for err := range errors {
|
|
t.Logf("Concurrent operation error: %v", err)
|
|
}
|
|
|
|
// NEW: Verify final database state
|
|
// Items 0,1,2 should be deleted (bulk delete won or ran first)
|
|
// Item 1 might be updated (update ran first)
|
|
// Items 3,4 should exist untouched
|
|
|
|
for i, mediaID := range mediaIDs {
|
|
pgID := pgtype.UUID{Bytes: [16]byte(mediaID), Valid: true}
|
|
_, err := setup.DB.GetMediaItem(context.Background(), pgID)
|
|
|
|
if i < 3 {
|
|
// Should be deleted
|
|
assert.Error(t, err, "Media item %d should be deleted", i)
|
|
} else {
|
|
// Should exist
|
|
assert.NoError(t, err, "Media item %d should exist", i)
|
|
}
|
|
}
|
|
}
|
|
```
|
|
|
|
#### File-by-File Plan
|
|
|
|
##### File: `cmd/server/tests/media_bulk_test.go`
|
|
|
|
**Add new test function**:
|
|
|
|
1. **TestBulkConcurrent_DeleteAndUpdate** (after existing bulk tests)
|
|
- Lines: Add ~80 lines
|
|
- Purpose: Bulk delete + individual update on same items
|
|
- Verification: Consistent final state, no orphaned records
|
|
|
|
##### File: `cmd/server/tests/collections_bulk_test.go`
|
|
|
|
**Add new test function**:
|
|
|
|
1. **TestBulkConcurrent_AddAndRemove** (after existing bulk tests)
|
|
- Lines: Add ~80 lines
|
|
- Purpose: Add items to collection while removing from same collection
|
|
- Verification: Collection items consistent
|
|
|
|
##### File: `cmd/server/tests/conflicts_bulk_test.go`
|
|
|
|
**Add new test function**:
|
|
|
|
1. **TestBulkConcurrent_ResolveAndEscalate** (after existing bulk tests)
|
|
- Lines: Add ~80 lines
|
|
- Purpose: Resolve conflicts while escalating others to admin
|
|
- Verification: No conflicts in ambiguous state
|
|
|
|
**Action**: Add 3 new test functions to respective bulk test files.
|
|
|
|
---
|
|
|
|
### 2.3 Profile + Device Concurrent Updates
|
|
|
|
#### Test Pattern
|
|
|
|
```go
|
|
// NEW TEST: Concurrent profile + device updates
|
|
func TestUserConcurrent_ProfileAndDeviceUpdates(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
device := setup.CreateDevice(t, "Test Device", "koreader", "test-profile-device")
|
|
|
|
var wg sync.WaitGroup
|
|
errors := make(chan error, 2)
|
|
|
|
// Concurrent operation 1: Update user profile
|
|
wg.Add(1)
|
|
go func() {
|
|
defer wg.Done()
|
|
|
|
payload := map[string]interface{}{
|
|
"first_name": "UpdatedFirstName",
|
|
}
|
|
jsonData, _ := json.Marshal(payload)
|
|
|
|
req := httptest.NewRequest("PUT", setup.Server.URL+"/api/auth/profile", bytes.NewBuffer(jsonData))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
|
|
client := &http.Client{Timeout: 5 * time.Second}
|
|
resp, err := client.Do(req)
|
|
if err != nil {
|
|
errors <- err
|
|
return
|
|
}
|
|
defer resp.Body.Close()
|
|
|
|
if resp.StatusCode != http.StatusOK {
|
|
errors <- fmt.Errorf("profile update failed: %d", resp.StatusCode)
|
|
}
|
|
}()
|
|
|
|
// Concurrent operation 2: Update device sync settings (affects user)
|
|
wg.Add(1)
|
|
go func() {
|
|
defer wg.Done()
|
|
|
|
payload := map[string]interface{}{
|
|
"sync_enabled": false,
|
|
}
|
|
jsonData, _ := json.Marshal(payload)
|
|
|
|
req := httptest.NewRequest("PUT", fmt.Sprintf("%s/api/devices/%s", setup.Server.URL, device.ID.String()), bytes.NewBuffer(jsonData))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
|
|
client := &http.Client{Timeout: 5 * time.Second}
|
|
resp, err := client.Do(req)
|
|
if err != nil {
|
|
errors <- err
|
|
return
|
|
}
|
|
defer resp.Body.Close()
|
|
|
|
if resp.StatusCode != http.StatusOK {
|
|
errors <- fmt.Errorf("device update failed: %d", resp.StatusCode)
|
|
}
|
|
}()
|
|
|
|
wg.Wait()
|
|
close(errors)
|
|
|
|
// Check for unexpected errors
|
|
for err := range errors {
|
|
t.Logf("Concurrent update error: %v", err)
|
|
}
|
|
|
|
// NEW: Verify final database state
|
|
// Both updates should succeed
|
|
pgUserID := pgtype.UUID{Bytes: [16]byte(setup.User.ID), Valid: true}
|
|
user, err := setup.DB.GetUser(context.Background(), pgUserID)
|
|
require.NoError(t, err, "Should retrieve user")
|
|
assert.Equal(t, "UpdatedFirstName", user.FirstName.String)
|
|
|
|
pgDeviceID := pgtype.UUID{Bytes: [16]byte(device.ID), Valid: true}
|
|
updatedDevice, err := setup.DB.GetDevice(context.Background(), pgDeviceID)
|
|
require.NoError(t, err, "Should retrieve device")
|
|
assert.False(t, updatedDevice.SyncEnabled.Bool)
|
|
}
|
|
```
|
|
|
|
#### File-by-File Plan
|
|
|
|
##### File: `cmd/server/tests/user_test.go`
|
|
|
|
**Add new test function**:
|
|
|
|
1. **TestUserConcurrent_ProfileAndDevice** (after existing user tests)
|
|
- Lines: Add ~70 lines
|
|
- Purpose: Profile update + device update simultaneously
|
|
- Verification: Both updates succeed
|
|
|
|
##### File: `cmd/server/tests/device_test.go`
|
|
|
|
**Add new test function**:
|
|
|
|
1. **TestDeviceConcurrent_TokenRegenAndSync** (after existing device tests)
|
|
- Lines: Add ~80 lines
|
|
- Purpose: Regenerate token while sync is in progress
|
|
- Verification: Old token fails current sync, new token works for next
|
|
|
|
**Action**: Add 2 new test functions.
|
|
|
|
---
|
|
|
|
### Phase 2 Summary
|
|
|
|
**New Test Functions**: 10+
|
|
**Lines Added**: ~800 lines
|
|
**Race Coverage**: Sync, bulk, profile + device
|
|
**Database Verification**: 100% of final states
|
|
|
|
**Verification Steps**:
|
|
1. ✅ All sync operations have concurrent tests
|
|
2. ✅ All bulk operations have concurrent tests
|
|
3. ✅ Profile + device interactions have concurrent tests
|
|
|
|
---
|
|
|
|
## Phase 3: Hardened Error Handling
|
|
|
|
**Priority**: 🟡 MEDIUM - **Do after Phase 2**
|
|
**Time Estimate**: 15-20 new test functions
|
|
**Risk Level**: Low (new tests only)
|
|
**ROI**: Medium - prevents edge case crashes
|
|
|
|
### Objectives
|
|
1. Add WebSocket error path tests
|
|
2. Add sync failure scenario tests
|
|
3. Add OPDS edge case tests
|
|
4. Add book matching edge case tests
|
|
|
|
---
|
|
|
|
### 3.1 WebSocket Error Paths
|
|
|
|
#### Problem Areas
|
|
|
|
| Test File | Missing Tests | Risk |
|
|
|-----------|---------------|-------|
|
|
| `websocket_test.go` | Disconnection mid-message | Orphaned connections |
|
|
| | Malformed WebSocket frames | Parser crashes |
|
|
| | Connection timeout | Resource leaks |
|
|
| | Connection hijacking | Security issues |
|
|
|
|
#### Test Patterns
|
|
|
|
##### Pattern 1: Disconnection Mid-Message
|
|
|
|
```go
|
|
// NEW TEST: WebSocket disconnect during message
|
|
func TestWebSocket_DisconnectMidMessage(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
device := setup.CreateDevice(t, "Test Device", "koreader", "test-ws-disconnect")
|
|
|
|
// Connect WebSocket
|
|
wsURL := strings.Replace(setup.Server.URL, "http", "ws", 1) + "/ws/device/" + device.AuthToken
|
|
ws, _, err := websocket.DefaultDialer.Dial(wsURL, nil)
|
|
require.NoError(t, err, "Should connect WebSocket")
|
|
defer ws.Close()
|
|
|
|
// Send partial message
|
|
partialData := map[string]interface{}{
|
|
"media_item_id": uuid.New(),
|
|
"percentage": 25.5,
|
|
// Missing required fields for incomplete message
|
|
}
|
|
jsonData, _ := json.Marshal(partialData)
|
|
|
|
// Send message
|
|
err = ws.WriteMessage(websocket.TextMessage, jsonData)
|
|
require.NoError(t, err, "Should send message")
|
|
|
|
// Immediately close connection (simulating disconnect)
|
|
ws.Close()
|
|
|
|
// Wait a bit for server to process
|
|
time.Sleep(100 * time.Millisecond)
|
|
|
|
// NEW: Verify no orphaned progress records in database
|
|
// If message was processed, it should be complete
|
|
// If not, it should be rolled back
|
|
pgUserID := pgtype.UUID{Bytes: [16]byte(setup.User.ID), Valid: true}
|
|
progress, err := setup.DB.ListReadingProgress(context.Background(), pgUserID)
|
|
|
|
// Either message processed (with default values) or rolled back
|
|
// No partial/corrupted records should exist
|
|
for _, p := range progress {
|
|
if p.Percentage.Float64 == 25.5 {
|
|
// If partial percentage saved, other fields should have defaults
|
|
assert.NotZero(t, p.TotalPages.Int32, "Should have default total_pages")
|
|
}
|
|
}
|
|
}
|
|
```
|
|
|
|
##### Pattern 2: Malformed Frames
|
|
|
|
```go
|
|
// NEW TEST: Malformed WebSocket frames
|
|
func TestWebSocket_MalformedFrames(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
device := setup.CreateDevice(t, "Test Device", "koreader", "test-ws-malformed")
|
|
|
|
// Connect WebSocket
|
|
wsURL := strings.Replace(setup.Server.URL, "http", "ws", 1) + "/ws/device/" + device.AuthToken
|
|
ws, _, err := websocket.DefaultDialer.Dial(wsURL, nil)
|
|
require.NoError(t, err, "Should connect WebSocket")
|
|
defer ws.Close()
|
|
|
|
// Send invalid JSON
|
|
invalidJSON := []byte("{invalid json}")
|
|
err = ws.WriteMessage(websocket.TextMessage, invalidJSON)
|
|
require.NoError(t, err, "Should send message")
|
|
|
|
// Send binary data (unexpected)
|
|
binaryData := []byte{0x00, 0x01, 0x02}
|
|
err = ws.WriteMessage(websocket.BinaryMessage, binaryData)
|
|
require.NoError(t, err, "Should send binary")
|
|
|
|
// Send extremely large message
|
|
largeData := make([]byte, 1024*1024) // 1MB
|
|
err = ws.WriteMessage(websocket.TextMessage, largeData)
|
|
if err != nil {
|
|
t.Logf("Large message rejected (expected): %v", err)
|
|
}
|
|
|
|
// Wait for server to process
|
|
time.Sleep(100 * time.Millisecond)
|
|
|
|
// NEW: Verify WebSocket still open (not crashed)
|
|
err = ws.WriteMessage(websocket.PingMessage, nil)
|
|
// If server crashed, this will fail
|
|
if err != nil {
|
|
t.Logf("WebSocket connection closed after malformed frames: %v", err)
|
|
}
|
|
}
|
|
```
|
|
|
|
#### File-by-File Plan
|
|
|
|
##### File: `cmd/server/tests/websocket_test.go`
|
|
|
|
**Add new test functions**:
|
|
|
|
1. **TestWebSocket_DisconnectMidMessage** (after existing tests)
|
|
- Lines: Add ~50 lines
|
|
- Purpose: Connection closes during message processing
|
|
- Verification: No partial/corrupted data in DB
|
|
|
|
2. **TestWebSocket_MalformedFrames** (after previous test)
|
|
- Lines: Add ~50 lines
|
|
- Purpose: Invalid JSON, binary data, oversized messages
|
|
- Verification: Server handles gracefully, no crashes
|
|
|
|
3. **TestWebSocket_ConnectionTimeout** (after previous test)
|
|
- Lines: Add ~40 lines
|
|
- Purpose: Connection times out during inactivity
|
|
- Verification: Resources cleaned up
|
|
|
|
4. **TestWebSocket_ConcurrentMessages** (after previous test)
|
|
- Lines: Add ~60 lines
|
|
- Purpose: Multiple messages sent simultaneously
|
|
- Verification: All processed correctly
|
|
|
|
**Action**: Add 4 new test functions to `websocket_test.go`.
|
|
|
|
---
|
|
|
|
### 3.2 Sync Failure Scenarios
|
|
|
|
#### Problem Areas
|
|
|
|
| Scenario | Missing Tests | Risk |
|
|
|-----------|---------------|-------|
|
|
| **Offline device** | Device syncs after being offline | Stale data overwrites fresh |
|
|
| **Sync queue overflow** | Too many pending sync operations | Queue exhaustion |
|
|
| **Stale sync data** | Old sync data arrives after newer | Incorrect progress |
|
|
| **Sync conflicts** | Two devices with conflicting data | Data inconsistency |
|
|
|
|
#### Test Patterns
|
|
|
|
##### Pattern 1: Offline Device Sync
|
|
|
|
```go
|
|
// NEW TEST: Device syncs after extended offline period
|
|
func TestSync_OfflineDevice(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
device := setup.CreateDevice(t, "Test Device", "koreader", "test-offline")
|
|
mediaID := createTestMediaItemID(t, setup.Server, token)
|
|
|
|
// Device sets progress to 50% while offline
|
|
offlineProgress := map[string]interface{}{
|
|
"media_item_id": mediaID,
|
|
"percentage": 50.0,
|
|
"current_page": 100,
|
|
"total_pages": 200,
|
|
"timestamp": time.Now().Add(-24 * time.Hour).Unix(), // 24 hours ago
|
|
}
|
|
|
|
// Simulate offline sync (old timestamp)
|
|
jsonData, _ := json.Marshal(offlineProgress)
|
|
req := httptest.NewRequest("POST", setup.Server.URL+"/api/sync/koreader/progress", bytes.NewBuffer(jsonData))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+device.AuthToken)
|
|
|
|
client := &http.Client{}
|
|
resp, err := client.Do(req)
|
|
require.NoError(t, err)
|
|
defer resp.Body.Close()
|
|
|
|
// Should succeed but warn about stale data
|
|
assert.Equal(t, http.StatusOK, resp.StatusCode)
|
|
|
|
// NEW: Verify database handling of stale data
|
|
pgMediaID := pgtype.UUID{Bytes: [16]byte(mediaID), Valid: true}
|
|
pgUserID := pgtype.UUID{Bytes: [16]byte(setup.User.ID), Valid: true}
|
|
progress, err := setup.DB.GetReadingProgress(context.Background(), database.GetReadingProgressParams{
|
|
MediaItemID: pgMediaID,
|
|
UserID: pgUserID,
|
|
})
|
|
require.NoError(t, err, "Should retrieve progress")
|
|
|
|
// Stale data should either:
|
|
// 1. Be rejected (keep existing newer data)
|
|
// 2. Be accepted with warning (user notification)
|
|
// Verify behavior matches expected
|
|
assert.LessOrEqual(t, progress.Percentage.Float64, 50.0, "Should handle stale data appropriately")
|
|
}
|
|
```
|
|
|
|
##### Pattern 2: Sync Queue Overflow
|
|
|
|
```go
|
|
// NEW TEST: Sync queue exceeds limits
|
|
func TestSync_QueueOverflow(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
device := setup.CreateDevice(t, "Test Device", "koreader", "test-queue-overflow")
|
|
|
|
// Simulate many rapid sync operations (queue overflow)
|
|
const numOps = 100
|
|
var wg sync.WaitGroup
|
|
errors := make(chan error, numOps)
|
|
|
|
for i := 0; i < numOps; i++ {
|
|
wg.Add(1)
|
|
go func(opNum int) {
|
|
defer wg.Done()
|
|
|
|
progressData := map[string]interface{}{
|
|
"media_item_id": uuid.New(), // Different items
|
|
"percentage": float64(opNum),
|
|
}
|
|
jsonData, _ := json.Marshal(progressData)
|
|
|
|
req := httptest.NewRequest("POST", setup.Server.URL+"/api/sync/koreader/progress", bytes.NewBuffer(jsonData))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+device.AuthToken)
|
|
|
|
client := &http.Client{Timeout: 5 * time.Second}
|
|
resp, err := client.Do(req)
|
|
if err != nil {
|
|
errors <- err
|
|
return
|
|
}
|
|
defer resp.Body.Close()
|
|
|
|
// Queue might be full (503) or accepted (202)
|
|
if resp.StatusCode != http.StatusOK &&
|
|
resp.StatusCode != http.StatusAccepted &&
|
|
resp.StatusCode != http.StatusServiceUnavailable {
|
|
errors <- fmt.Errorf("unexpected status: %d", resp.StatusCode)
|
|
}
|
|
}(i)
|
|
}
|
|
|
|
wg.Wait()
|
|
close(errors)
|
|
|
|
// Some operations might fail due to queue overflow (expected)
|
|
errorCount := 0
|
|
for range errors {
|
|
errorCount++
|
|
}
|
|
t.Logf("Queue overflow: %d/%d operations failed", errorCount, numOps)
|
|
|
|
// NEW: Verify queue didn't deadlock or crash
|
|
// Queue should process or reject, not hang
|
|
}
|
|
```
|
|
|
|
#### File-by-File Plan
|
|
|
|
##### File: `cmd/server/tests/sync_integration_test.go`
|
|
|
|
**Add new test functions**:
|
|
|
|
1. **TestSync_OfflineDevice** (after concurrent tests)
|
|
- Lines: Add ~60 lines
|
|
- Purpose: Old sync data arrives after newer data
|
|
- Verification: Appropriate handling (reject or warn)
|
|
|
|
2. **TestSync_QueueOverflow** (after previous test)
|
|
- Lines: Add ~70 lines
|
|
- Purpose: Many rapid sync operations
|
|
- Verification: Queue handles gracefully, no deadlocks
|
|
|
|
3. **TestSync_ConflictingData** (after previous test)
|
|
- Lines: Add ~70 lines
|
|
- Purpose: Two devices send conflicting progress
|
|
- Verification: Conflict detection or last-write-wins with notification
|
|
|
|
4. **TestSync_OrphanedQueueItems** (after previous test)
|
|
- Lines: Add ~50 lines
|
|
- Purpose: Queue items for deleted books
|
|
- Verification: Orphaned items cleaned up
|
|
|
|
**Action**: Add 4 new test functions to `sync_integration_test.go`.
|
|
|
|
---
|
|
|
|
### 3.3 OPDS Edge Cases
|
|
|
|
#### Problem Areas
|
|
|
|
| Scenario | Missing Tests | Risk |
|
|
|-----------|---------------|-------|
|
|
| **Empty library** | OPDS feed with no books | Empty feed parsing errors |
|
|
| **Very large feeds** | Library with thousands of books | Pagination failures |
|
|
| **Corrupted metadata** | Books with invalid EPUB metadata | Feed generation crashes |
|
|
| **Special characters** | Titles with Unicode, emojis | XML/JSON encoding issues |
|
|
|
|
#### Test Patterns
|
|
|
|
##### Pattern 1: Empty Library
|
|
|
|
```go
|
|
// NEW TEST: OPDS feed for empty library
|
|
func TestOPDS_EmptyLibrary(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
|
|
// Create empty library
|
|
libReq := map[string]interface{}{
|
|
"name": "Empty Library",
|
|
"type": "ebooks",
|
|
}
|
|
libBody, _ := json.Marshal(libReq)
|
|
req := httptest.NewRequest("POST", setup.Server.URL+"/api/libraries", bytes.NewBuffer(libBody))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
|
|
client := &http.Client{}
|
|
resp, err := client.Do(req)
|
|
require.NoError(t, err)
|
|
defer resp.Body.Close()
|
|
require.Equal(t, http.StatusCreated, resp.StatusCode)
|
|
|
|
var libResponse map[string]interface{}
|
|
json.NewDecoder(resp.Body).Decode(&libResponse)
|
|
libraryID := libResponse["id"].(string)
|
|
|
|
// Get OPDS feed
|
|
req = httptest.NewRequest("GET", fmt.Sprintf("%s/api/opds/libraries/%s", setup.Server.URL, libraryID), nil)
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
|
|
resp, err = client.Do(req)
|
|
require.NoError(t, err)
|
|
defer resp.Body.Close()
|
|
|
|
assert.Equal(t, http.StatusOK, resp.StatusCode)
|
|
|
|
// NEW: Verify feed structure
|
|
var feed map[string]interface{}
|
|
json.NewDecoder(resp.Body).Decode(&feed)
|
|
|
|
assert.Contains(t, feed, "entries", "Feed should have entries array")
|
|
entries := feed["entries"].([]interface{})
|
|
assert.Empty(t, entries, "Entries should be empty for empty library")
|
|
assert.Contains(t, feed, "total", "Feed should have total count")
|
|
assert.Equal(t, 0, int(feed["total"].(float64)), "Total should be 0")
|
|
}
|
|
```
|
|
|
|
##### Pattern 2: Large Feed
|
|
|
|
```go
|
|
// NEW TEST: OPDS feed with many books
|
|
func TestOPDS_LargeFeed(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
|
|
// Create library
|
|
libReq := map[string]interface{}{
|
|
"name": "Large Library",
|
|
"type": "ebooks",
|
|
}
|
|
libBody, _ := json.Marshal(libReq)
|
|
req := httptest.NewRequest("POST", setup.Server.URL+"/api/libraries", bytes.NewBuffer(libBody))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
|
|
client := &http.Client{}
|
|
resp, err := client.Do(req)
|
|
require.NoError(t, err)
|
|
defer resp.Body.Close()
|
|
require.Equal(t, http.StatusCreated, resp.StatusCode)
|
|
|
|
var libResponse map[string]interface{}
|
|
json.NewDecoder(resp.Body).Decode(&libResponse)
|
|
libraryID := libResponse["id"].(string)
|
|
|
|
// Add folder
|
|
folderReq := map[string]interface{}{
|
|
"folder_path": "/app/uploads",
|
|
}
|
|
folderBody, _ := json.Marshal(folderReq)
|
|
req = httptest.NewRequest("POST", fmt.Sprintf("%s/api/libraries/%s/folders", setup.Server.URL, libraryID), bytes.NewBuffer(folderBody))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
resp, err = client.Do(req)
|
|
require.NoError(t, err)
|
|
resp.Body.Close()
|
|
|
|
// Add many books (simulate large library)
|
|
const numBooks = 1000
|
|
for i := 0; i < numBooks; i++ {
|
|
mediaReq := map[string]interface{}{
|
|
"library_id": libraryID,
|
|
"title": fmt.Sprintf("Book %d", i),
|
|
"author": "Test Author",
|
|
"file_path": fmt.Sprintf("/tmp/test%d.epub", i),
|
|
"file_size": 1024,
|
|
"mime_type": "application/epub+zip",
|
|
}
|
|
mediaBody, _ := json.Marshal(mediaReq)
|
|
|
|
req = httptest.NewRequest("POST", setup.Server.URL+"/api/media-items", bytes.NewBuffer(mediaBody))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
resp, err = client.Do(req)
|
|
if err != nil {
|
|
t.Logf("Failed to create book %d: %v", i, err)
|
|
continue
|
|
}
|
|
resp.Body.Close()
|
|
}
|
|
|
|
// Get OPDS feed with pagination
|
|
req = httptest.NewRequest("GET", fmt.Sprintf("%s/api/opds/libraries/%s?page=1&limit=100", setup.Server.URL, libraryID), nil)
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
|
|
resp, err = client.Do(req)
|
|
require.NoError(t, err)
|
|
defer resp.Body.Close()
|
|
|
|
// NEW: Verify pagination
|
|
assert.Equal(t, http.StatusOK, resp.StatusCode)
|
|
|
|
var feed map[string]interface{}
|
|
json.NewDecoder(resp.Body).Decode(&feed)
|
|
|
|
assert.Contains(t, feed, "entries", "Feed should have entries array")
|
|
entries := feed["entries"].([]interface{})
|
|
assert.LessOrEqual(t, len(entries), 100, "Should respect page limit")
|
|
assert.Contains(t, feed, "total", "Feed should have total count")
|
|
assert.GreaterOrEqual(t, int(feed["total"].(float64)), numBooks-10, "Total should reflect all books")
|
|
}
|
|
```
|
|
|
|
#### File-by-File Plan
|
|
|
|
##### File: `cmd/server/tests/opds_test.go`
|
|
|
|
**Add new test functions**:
|
|
|
|
1. **TestOPDS_EmptyLibrary** (after existing tests)
|
|
- Lines: Add ~50 lines
|
|
- Purpose: OPDS feed with no books
|
|
- Verification: Valid feed structure, empty entries
|
|
|
|
2. **TestOPDS_LargeFeed** (after previous test)
|
|
- Lines: Add ~90 lines
|
|
- Purpose: Library with 1000+ books
|
|
- Verification: Pagination works, total count accurate
|
|
|
|
3. **TestOPDS_SpecialCharacters** (after previous test)
|
|
- Lines: Add ~60 lines
|
|
- Purpose: Book titles with Unicode, emojis, RTL
|
|
- Verification: Proper XML/JSON encoding
|
|
|
|
4. **TestOPDS_CorruptedMetadata** (after previous test)
|
|
- Lines: Add ~50 lines
|
|
- Purpose: Book with invalid/malformed metadata
|
|
- Verification: Feed generation continues, skips bad entries
|
|
|
|
**Action**: Add 4 new test functions to `opds_test.go`.
|
|
|
|
---
|
|
|
|
### 3.4 Book Matching Edge Cases
|
|
|
|
#### Problem Areas
|
|
|
|
| Scenario | Missing Tests | Risk |
|
|
|-----------|---------------|-------|
|
|
| **No matches found** | Book has no close matches | Empty results handling |
|
|
| **Multiple equal matches** | Same confidence for multiple books | Ambiguity handling |
|
|
| **Special characters** | Titles with quotes, apostrophes | Query failures |
|
|
| **Unicode normalization** | Different Unicode representations | Duplicate matches |
|
|
|
|
#### Test Patterns
|
|
|
|
##### Pattern 1: No Matches Found
|
|
|
|
```go
|
|
// NEW TEST: Book matching finds no results
|
|
func TestBookMatching_NoMatches(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
device := setup.CreateDevice(t, "Test Device", "koreader", "test-no-matches")
|
|
|
|
// Device reports book that doesn't exist in library
|
|
unmatchedBook := map[string]interface{}{
|
|
"title": "Nonexistent Book Title That Definitely Doesn't Exist",
|
|
"author": "Unknown Author",
|
|
"file_path": "/mnt/sd/Nonexistent.epub",
|
|
"file_size": 1024,
|
|
}
|
|
|
|
jsonData, _ := json.Marshal(unmatchedBook)
|
|
req := httptest.NewRequest("POST", setup.Server.URL+"/api/sync/koreader/match", bytes.NewBuffer(jsonData))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+device.AuthToken)
|
|
|
|
client := &http.Client{}
|
|
resp, err := client.Do(req)
|
|
require.NoError(t, err)
|
|
defer resp.Body.Close()
|
|
|
|
// NEW: Verify handling of no matches
|
|
var response map[string]interface{}
|
|
json.NewDecoder(resp.Body).Decode(&response)
|
|
|
|
// Should either:
|
|
// 1. Return empty matches array
|
|
// 2. Return null match with confidence 0
|
|
// 3. Suggest manual match
|
|
matches, hasMatches := response["matches"]
|
|
if hasMatches {
|
|
matchArray := matches.([]interface{})
|
|
assert.Empty(t, matchArray, "Should have no matches")
|
|
}
|
|
|
|
// Or might have suggestion flag
|
|
if suggestion, hasSuggestion := response["suggest_manual"]; hasSuggestion {
|
|
assert.True(t, suggestion.(bool), "Should suggest manual match")
|
|
}
|
|
}
|
|
```
|
|
|
|
##### Pattern 2: Multiple Equal Matches
|
|
|
|
```go
|
|
// NEW TEST: Multiple books with same match confidence
|
|
func TestBookMatching_MultipleEqualConfidence(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
|
|
// Create library
|
|
libReq := map[string]interface{}{
|
|
"name": "Test Library",
|
|
"type": "ebooks",
|
|
}
|
|
libBody, _ := json.Marshal(libReq)
|
|
req := httptest.NewRequest("POST", setup.Server.URL+"/api/libraries", bytes.NewBuffer(libBody))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
|
|
client := &http.Client{}
|
|
resp, err := client.Do(req)
|
|
require.NoError(t, err)
|
|
defer resp.Body.Close()
|
|
require.Equal(t, http.StatusCreated, resp.StatusCode)
|
|
|
|
var libResponse map[string]interface{}
|
|
json.NewDecoder(resp.Body).Decode(&libResponse)
|
|
libraryID := libResponse["id"].(string)
|
|
|
|
// Add folder
|
|
folderReq := map[string]interface{}{
|
|
"folder_path": "/app/uploads",
|
|
}
|
|
folderBody, _ := json.Marshal(folderReq)
|
|
req = httptest.NewRequest("POST", fmt.Sprintf("%s/api/libraries/%s/folders", setup.Server.URL, libraryID), bytes.NewBuffer(folderBody))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
resp, err = client.Do(req)
|
|
require.NoError(t, err)
|
|
resp.Body.Close()
|
|
|
|
// Add multiple books with similar titles
|
|
for _, title := range []string{"Book One", "Book Two", "Book Three"} {
|
|
mediaReq := map[string]interface{}{
|
|
"library_id": libraryID,
|
|
"title": title,
|
|
"author": "Same Author",
|
|
"file_path": "/tmp/test.epub",
|
|
"file_size": 1024,
|
|
"mime_type": "application/epub+zip",
|
|
}
|
|
mediaBody, _ := json.Marshal(mediaReq)
|
|
|
|
req = httptest.NewRequest("POST", setup.Server.URL+"/api/media-items", bytes.NewBuffer(mediaBody))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
resp, err = client.Do(req)
|
|
require.NoError(t, err)
|
|
resp.Body.Close()
|
|
}
|
|
|
|
// Device reports book with ambiguous title
|
|
device := setup.CreateDevice(t, "Test Device", "koreader", "test-ambiguous")
|
|
ambiguousBook := map[string]interface{}{
|
|
"title": "Book", // Could match any of the three
|
|
"author": "Same Author",
|
|
}
|
|
|
|
jsonData, _ := json.Marshal(ambiguousBook)
|
|
req = httptest.NewRequest("POST", setup.Server.URL+"/api/sync/koreader/match", bytes.NewBuffer(jsonData))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+device.AuthToken)
|
|
|
|
resp, err = client.Do(req)
|
|
require.NoError(t, err)
|
|
defer resp.Body.Close()
|
|
|
|
// NEW: Verify ambiguous match handling
|
|
assert.Equal(t, http.StatusOK, resp.StatusCode)
|
|
|
|
var response map[string]interface{}
|
|
json.NewDecoder(resp.Body).Decode(&response)
|
|
|
|
// Should return multiple matches with similar confidence
|
|
matches, hasMatches := response["matches"]
|
|
require.True(t, hasMatches, "Should have matches")
|
|
matchArray := matches.([]interface{})
|
|
assert.Greater(t, len(matchArray), 1, "Should have multiple matches for ambiguous title")
|
|
|
|
// All matches should have similar confidence scores
|
|
if len(matchArray) > 1 {
|
|
firstMatch := matchArray[0].(map[string]interface{})
|
|
secondMatch := matchArray[1].(map[string]interface{})
|
|
|
|
firstConfidence := firstMatch["confidence"].(float64)
|
|
secondConfidence := secondMatch["confidence"].(float64)
|
|
|
|
// Confidence scores should be similar (within 10%)
|
|
confidenceDiff := firstConfidence - secondConfidence
|
|
assert.Less(t, confidenceDiff, 0.1, "Similar matches should have close confidence")
|
|
}
|
|
}
|
|
```
|
|
|
|
#### File-by-File Plan
|
|
|
|
##### File: `cmd/server/tests/book_matching_test.go`
|
|
|
|
**Add new test functions**:
|
|
|
|
1. **TestBookMatching_NoMatches** (after existing tests)
|
|
- Lines: Add ~50 lines
|
|
- Purpose: Book with no close matches
|
|
- Verification: Empty matches or manual match suggestion
|
|
|
|
2. **TestBookMatching_MultipleEqualConfidence** (after previous test)
|
|
- Lines: Add ~80 lines
|
|
- Purpose: Multiple books with similar titles
|
|
- Verification: Returns multiple matches, suggests disambiguation
|
|
|
|
3. **TestBookMatching_SpecialCharacters** (after previous test)
|
|
- Lines: Add ~60 lines
|
|
- Purpose: Titles with quotes, apostrophes, emojis
|
|
- Verification: Proper query escaping, correct matches
|
|
|
|
4. **TestBookMatching_UnicodeNormalization** (after previous test)
|
|
- Lines: Add ~60 lines
|
|
- Purpose: Same title in different Unicode forms (NFD vs NFC)
|
|
- Verification: Correct match despite Unicode differences
|
|
|
|
**Action**: Add 4 new test functions to `book_matching_test.go`.
|
|
|
|
---
|
|
|
|
### Phase 3 Summary
|
|
|
|
**New Test Functions**: 16+
|
|
**Lines Added**: ~1000 lines
|
|
**Error Path Coverage**: WebSocket, sync, OPDS, matching
|
|
**Edge Case Coverage**: Empty, large, corrupted, special characters
|
|
|
|
**Verification Steps**:
|
|
1. ✅ WebSocket has error path tests
|
|
2. ✅ Sync has failure scenario tests
|
|
3. ✅ OPDS has edge case tests
|
|
4. ✅ Book matching has ambiguity tests
|
|
|
|
---
|
|
|
|
## Phase 4: Load and Security
|
|
|
|
**Priority**: 🟢 LOW - **Do after Phase 3**
|
|
**Time Estimate**: 10-12 new test functions
|
|
**Risk Level**: Low (new tests only)
|
|
**ROI**: Medium - prevents DoS and encoding issues
|
|
|
|
### Objectives
|
|
1. Add very large payload tests
|
|
2. Add comprehensive Unicode/special character tests
|
|
3. Add max boundary tests
|
|
|
|
---
|
|
|
|
### 4.1 Very Large Payloads
|
|
|
|
#### Problem Areas
|
|
|
|
| Area | Current | Missing | Risk |
|
|
|-------|---------|----------|-------|
|
|
| **Single test** | `edge_cases_test.go:327` - 100KB string | Bulk operations with 1000+ items | Memory exhaustion |
|
|
| **Response size** | Not tested | User with 10,000 books | Timeouts |
|
|
| **WebSocket messages** | Not tested | Very large sync payloads | Frame size limits |
|
|
| **Upload limits** | Not tested | Huge book files | Disk space exhaustion |
|
|
|
|
#### Test Patterns
|
|
|
|
##### Pattern 1: Bulk Operation Limits
|
|
|
|
```go
|
|
// NEW TEST: Bulk operation with maximum items
|
|
func TestBulk_MaximumItems(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
device := setup.CreateDevice(t, "Test Device", "koreader", "test-bulk-max")
|
|
|
|
// Create library with folder
|
|
libReq := map[string]interface{}{
|
|
"name": "Bulk Test Library",
|
|
"type": "ebooks",
|
|
}
|
|
libBody, _ := json.Marshal(libReq)
|
|
req := httptest.NewRequest("POST", setup.Server.URL+"/api/libraries", bytes.NewBuffer(libBody))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
|
|
client := &http.Client{}
|
|
resp, err := client.Do(req)
|
|
require.NoError(t, err)
|
|
defer resp.Body.Close()
|
|
require.Equal(t, http.StatusCreated, resp.StatusCode)
|
|
|
|
var libResponse map[string]interface{}
|
|
json.NewDecoder(resp.Body).Decode(&libResponse)
|
|
libraryID := libResponse["id"].(string)
|
|
|
|
// Add folder
|
|
folderReq := map[string]interface{}{
|
|
"folder_path": "/app/uploads",
|
|
}
|
|
folderBody, _ := json.Marshal(folderReq)
|
|
req = httptest.NewRequest("POST", fmt.Sprintf("%s/api/libraries/%s/folders", setup.Server.URL, libraryID), bytes.NewBuffer(folderBody))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
resp, err = client.Do(req)
|
|
require.NoError(t, err)
|
|
resp.Body.Close()
|
|
|
|
// Create many books (1000 items)
|
|
const numBooks = 1000
|
|
var mediaIDs []string
|
|
for i := 0; i < numBooks; i++ {
|
|
mediaReq := map[string]interface{}{
|
|
"library_id": libraryID,
|
|
"title": fmt.Sprintf("Book %d", i),
|
|
"author": "Test Author",
|
|
"file_path": fmt.Sprintf("/tmp/test%d.epub", i),
|
|
"file_size": 1024,
|
|
"mime_type": "application/epub+zip",
|
|
}
|
|
mediaBody, _ := json.Marshal(mediaReq)
|
|
|
|
req = httptest.NewRequest("POST", setup.Server.URL+"/api/media-items", bytes.NewBuffer(mediaBody))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
resp, err = client.Do(req)
|
|
if err != nil {
|
|
t.Logf("Failed to create book %d: %v", i, err)
|
|
continue
|
|
}
|
|
defer resp.Body.Close()
|
|
|
|
if resp.StatusCode == http.StatusCreated {
|
|
var mediaResponse map[string]interface{}
|
|
json.NewDecoder(resp.Body).Decode(&mediaResponse)
|
|
mediaIDs = append(mediaIDs, mediaResponse["id"].(string))
|
|
}
|
|
}
|
|
|
|
// Test bulk update with all 1000 items
|
|
bulkUpdateReq := map[string]interface{}{
|
|
"media_item_ids": mediaIDs,
|
|
"tags": []string{"bulk-updated"},
|
|
}
|
|
bulkBody, _ := json.Marshal(bulkUpdateReq)
|
|
|
|
req = httptest.NewRequest("POST", setup.Server.URL+"/api/media-items/bulk-update", bytes.NewBuffer(bulkBody))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
|
|
start := time.Now()
|
|
resp, err = client.Do(req)
|
|
duration := time.Since(start)
|
|
|
|
if err != nil {
|
|
t.Logf("Bulk update with %d items failed: %v", numBooks, err)
|
|
return
|
|
}
|
|
defer resp.Body.Close()
|
|
|
|
// NEW: Verify behavior under load
|
|
t.Logf("Bulk update of %d items took %v", numBooks, duration)
|
|
|
|
// Should succeed but might take time
|
|
if resp.StatusCode == http.StatusOK {
|
|
var result map[string]interface{}
|
|
json.NewDecoder(resp.Body).Decode(&result)
|
|
|
|
// Verify count
|
|
if total, ok := result["total"]; ok {
|
|
assert.Equal(t, numBooks, int(total.(float64)), "Should process all items")
|
|
}
|
|
|
|
// Verify DB state for sample
|
|
if len(mediaIDs) > 0 {
|
|
pgID := pgtype.UUID{Bytes: [16]byte(mediaIDs[0]), Valid: true}
|
|
media, err := setup.DB.GetMediaItem(context.Background(), pgID)
|
|
require.NoError(t, err)
|
|
assert.Contains(t, media.Tags, "bulk-updated", "Should have updated tag")
|
|
}
|
|
} else if resp.StatusCode == http.StatusRequestEntityTooLarge {
|
|
t.Logf("Bulk update rejected as too large (acceptable)")
|
|
} else {
|
|
t.Logf("Unexpected status: %d", resp.StatusCode)
|
|
}
|
|
}
|
|
```
|
|
|
|
##### Pattern 2: Response Size Limits
|
|
|
|
```go
|
|
// NEW TEST: API response with very large payload
|
|
func TestResponse_VeryLargePayload(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
|
|
// Create library with many books (from previous test or reuse)
|
|
// Assuming we have 1000 books from previous test
|
|
|
|
// Request list of all books (large response)
|
|
req := httptest.NewRequest("GET", setup.Server.URL+"/api/media-items?limit=10000", nil)
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
|
|
client := &http.Client{Timeout: 30 * time.Second}
|
|
start := time.Now()
|
|
resp, err := client.Do(req)
|
|
duration := time.Since(start)
|
|
|
|
if err != nil {
|
|
t.Logf("Large payload request failed: %v", err)
|
|
return
|
|
}
|
|
defer resp.Body.Close()
|
|
|
|
// NEW: Verify large response handling
|
|
t.Logf("Large payload response (%d status) took %v", resp.StatusCode, duration)
|
|
|
|
if resp.StatusCode == http.StatusOK {
|
|
// Read body (might be large)
|
|
body, err := io.ReadAll(resp.Body)
|
|
if err != nil {
|
|
t.Logf("Failed to read large response: %v", err)
|
|
return
|
|
}
|
|
|
|
t.Logf("Response size: %d bytes", len(body))
|
|
|
|
// Should have reasonable response size (< 10MB)
|
|
assert.Less(t, len(body), 10*1024*1024, "Response should be < 10MB")
|
|
|
|
// Verify response is valid JSON
|
|
var response map[string]interface{}
|
|
err = json.Unmarshal(body, &response)
|
|
assert.NoError(t, err, "Large response should be valid JSON")
|
|
|
|
// Verify pagination
|
|
if items, ok := response["items"]; ok {
|
|
itemArray := items.([]interface{})
|
|
t.Logf("Returned %d items", len(itemArray))
|
|
}
|
|
} else if resp.StatusCode == http.StatusRequestEntityTooLarge {
|
|
t.Logf("Large payload rejected (acceptable)")
|
|
}
|
|
}
|
|
```
|
|
|
|
#### File-by-File Plan
|
|
|
|
##### File: `cmd/server/tests/edge_cases_test.go`
|
|
|
|
**Add new test functions**:
|
|
|
|
1. **TestBulk_MaximumItems** (after existing edge cases)
|
|
- Lines: Add ~120 lines
|
|
- Purpose: Bulk operation with 1000 items
|
|
- Verification: Succeeds or rejected gracefully, no crashes
|
|
|
|
2. **TestResponse_VeryLargePayload** (after previous test)
|
|
- Lines: Add ~80 lines
|
|
- Purpose: Request all items (large response)
|
|
- Verification: Reasonable size, valid JSON, proper pagination
|
|
|
|
3. **TestWebSocket_LargeMessage** (after previous test)
|
|
- Lines: Add ~60 lines
|
|
- Purpose: Send very large WebSocket message
|
|
- Verification: Rejected or handled gracefully
|
|
|
|
**Action**: Add 3 new test functions to `edge_cases_test.go`.
|
|
|
|
---
|
|
|
|
### 4.2 Comprehensive Unicode Tests
|
|
|
|
#### Problem Areas
|
|
|
|
| Character Type | Missing Tests | Risk |
|
|
|--------------|---------------|-------|
|
|
| **Emojis** | Only 1 test | Database encoding issues |
|
|
| **RTL languages** | None | Display corruption |
|
|
| **Zero-width characters** | None | Silent duplicates |
|
|
| **Unicode normalization** | None | Same text appears different |
|
|
| **Combining characters** | None | Search failures |
|
|
|
|
#### Test Pattern
|
|
|
|
```go
|
|
// NEW TEST: Comprehensive Unicode handling
|
|
func TestUnicode_Comprehensive(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
|
|
testCases := []struct {
|
|
name string
|
|
title string
|
|
author string
|
|
expectOK bool
|
|
}{
|
|
{
|
|
name: "Emojis in title",
|
|
title: "The Great Adventure 🚀📚✨",
|
|
author: "Jane Doe",
|
|
expectOK: true,
|
|
},
|
|
{
|
|
name: "Multiple emojis",
|
|
title: "📖 Book 🌟 of 🎉 Joy 😊",
|
|
author: "Happy Author 👩🚀",
|
|
expectOK: true,
|
|
},
|
|
{
|
|
name: "RTL text (Arabic)",
|
|
title: "كتاب عظيم",
|
|
author: "مؤلف بارز",
|
|
expectOK: true,
|
|
},
|
|
{
|
|
name: "RTL text (Hebrew)",
|
|
title: "ספר נהדר",
|
|
author: "סופר מפורסם",
|
|
expectOK: true,
|
|
},
|
|
{
|
|
name: "Mixed RTL and LTR",
|
|
title: "Hello שלום Bonjour",
|
|
author: "Author نبي",
|
|
expectOK: true,
|
|
},
|
|
{
|
|
name: "Zero-width characters",
|
|
title: "Book\u200BTitle", // Zero-width space
|
|
author: "Author\u200CName", // Zero-width non-joiner
|
|
expectOK: true,
|
|
},
|
|
{
|
|
name: "Combining diacritics",
|
|
title: "Café", // Precomposed é
|
|
author: "cafe\u0301", // Combining acute
|
|
expectOK: true,
|
|
},
|
|
{
|
|
name: "Multiple combining marks",
|
|
title: "ñaïve", // Precomposed ï
|
|
author: "nai\u0308ve", // Combining diaeresis
|
|
expectOK: true,
|
|
},
|
|
{
|
|
name: "CJK characters",
|
|
title: "伟大的书",
|
|
author: "作者",
|
|
expectOK: true,
|
|
},
|
|
{
|
|
name: "Special punctuation",
|
|
title: "Book: A Story; Of—Things—Like—This",
|
|
author: "O'Brien, St. John",
|
|
expectOK: true,
|
|
},
|
|
}
|
|
|
|
for _, tc := range testCases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
// Create library
|
|
libReq := map[string]interface{}{
|
|
"name": fmt.Sprintf("Unicode Library %s", tc.name),
|
|
"type": "ebooks",
|
|
}
|
|
libBody, _ := json.Marshal(libReq)
|
|
req := httptest.NewRequest("POST", setup.Server.URL+"/api/libraries", bytes.NewBuffer(libBody))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
|
|
client := &http.Client{}
|
|
resp, err := client.Do(req)
|
|
require.NoError(t, err)
|
|
defer resp.Body.Close()
|
|
require.Equal(t, http.StatusCreated, resp.StatusCode)
|
|
|
|
var libResponse map[string]interface{}
|
|
json.NewDecoder(resp.Body).Decode(&libResponse)
|
|
libraryID := libResponse["id"].(string)
|
|
|
|
// Add folder
|
|
folderReq := map[string]interface{}{
|
|
"folder_path": "/app/uploads",
|
|
}
|
|
folderBody, _ := json.Marshal(folderReq)
|
|
req = httptest.NewRequest("POST", fmt.Sprintf("%s/api/libraries/%s/folders", setup.Server.URL, libraryID), bytes.NewBuffer(folderBody))
|
|
req.Header.Set("Content-Type", "application/json")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
resp, err = client.Do(req)
|
|
require.NoError(t, err)
|
|
resp.Body.Close()
|
|
|
|
// Create book with Unicode title/author
|
|
mediaReq := map[string]interface{}{
|
|
"library_id": libraryID,
|
|
"title": tc.title,
|
|
"author": tc.author,
|
|
"file_path": "/tmp/test.epub",
|
|
"file_size": 1024,
|
|
"mime_type": "application/epub+zip",
|
|
}
|
|
mediaBody, _ := json.Marshal(mediaReq)
|
|
|
|
req = httptest.NewRequest("POST", setup.Server.URL+"/api/media-items", bytes.NewBuffer(mediaBody))
|
|
req.Header.Set("Content-Type", "application/json; charset=utf-8")
|
|
req.Header.Set("Authorization", "Bearer "+token)
|
|
resp, err = client.Do(req)
|
|
require.NoError(t, err)
|
|
defer resp.Body.Close()
|
|
|
|
if tc.expectOK {
|
|
assert.Equal(t, http.StatusCreated, resp.StatusCode, tc.name)
|
|
|
|
// NEW: Verify database stores Unicode correctly
|
|
var mediaResponse map[string]interface{}
|
|
json.NewDecoder(resp.Body).Decode(&mediaResponse)
|
|
mediaID := mediaResponse["id"].(string)
|
|
|
|
pgID := pgtype.UUID{Bytes: [16]byte(mediaID), Valid: true}
|
|
media, err := setup.DB.GetMediaItem(context.Background(), pgID)
|
|
require.NoError(t, err, "Should retrieve media item")
|
|
|
|
// Verify Unicode preserved
|
|
assert.Equal(t, tc.title, media.Title, "Title should preserve Unicode")
|
|
assert.Equal(t, tc.author, media.Author.String, "Author should preserve Unicode")
|
|
|
|
// Test search with Unicode
|
|
searchReq := httptest.NewRequest("GET", fmt.Sprintf("%s/api/search?q=%s", setup.Server.URL, url.QueryEscape(tc.title)), nil)
|
|
searchReq.Header.Set("Authorization", "Bearer "+token)
|
|
searchResp, err := client.Do(searchReq)
|
|
require.NoError(t, err)
|
|
defer searchResp.Body.Close()
|
|
|
|
assert.Equal(t, http.StatusOK, searchResp.StatusCode, "Search should work with Unicode")
|
|
} else {
|
|
assert.NotEqual(t, http.StatusCreated, resp.StatusCode, tc.name)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
```
|
|
|
|
#### File-by-File Plan
|
|
|
|
##### File: `cmd/server/tests/edge_cases_test.go`
|
|
|
|
**Add new test function**:
|
|
|
|
1. **TestUnicode_Comprehensive** (after existing edge cases)
|
|
- Lines: Add ~150 lines
|
|
- Purpose: Emojis, RTL, zero-width, combining marks, CJK
|
|
- Verification: Stored correctly, searchable
|
|
|
|
**Action**: Add new test function to `edge_cases_test.go`.
|
|
|
|
---
|
|
|
|
### 4.3 Max Boundary Tests
|
|
|
|
#### Problem Areas
|
|
|
|
| Field Type | Missing | Risk |
|
|
|-----------|----------|-------|
|
|
| **Email** | Exact boundary (254, 255, 256) | Truncation |
|
|
| **Username** | Boundary (49, 50, 51, 500, 5000) | Validation inconsistency |
|
|
| **Device name** | Max length | DB constraint violation |
|
|
| **Book title** | Max length | Display issues |
|
|
| **Array items** | Max items in bulk | Performance degradation |
|
|
|
|
#### Test Pattern
|
|
|
|
```go
|
|
// NEW TEST: Boundary value testing
|
|
func TestBoundaries_MaxLengths(t *testing.T) {
|
|
setup := setupTestServer(t)
|
|
token := loginTestUser(t, setup.Server, setup.DB)
|
|
|
|
t.Run("Email at exact boundaries", func(t *testing.T) {
|
|
testCases := []struct {
|
|
name string
|
|
email string
|
|
expectOK bool
|
|
}{
|
|
{"254 chars (one under limit)", strings.Repeat("a", 250) + "@example.com", true},
|
|
{"255 chars (exact limit)", strings.Repeat("a", 251) + "@example.com", true},
|
|
{"256 chars (one over limit)", strings.Repeat("a", 252) + "@example.com", false},
|
|
{"Very long (5000 chars)", strings.Repeat("a", 4995) + "@example.com", false},
|
|
}
|
|
|
|
for _, tc := range testCases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
// Create user with boundary email
|
|
// ... test code ...
|
|
})
|
|
}
|
|
})
|
|
|
|
t.Run("Username at exact boundaries", func(t *testing.T) {
|
|
testCases := []struct {
|
|
name string
|
|
username string
|
|
expectOK bool
|
|
}{
|
|
{"2 chars (one under min)", "ab", false},
|
|
{"3 chars (exact min)", "abc", true},
|
|
{"50 chars (exact max)", strings.Repeat("a", 50), true},
|
|
{"51 chars (one over max)", strings.Repeat("a", 51), false},
|
|
{"500 chars (way over)", strings.Repeat("a", 500), false},
|
|
{"5000 chars (extreme)", strings.Repeat("a", 5000), false},
|
|
}
|
|
|
|
for _, tc := range testCases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
// Create user with boundary username
|
|
// ... test code ...
|
|
})
|
|
}
|
|
})
|
|
|
|
t.Run("Device name at max length", func(t *testing.T) {
|
|
// Test device name at database column limit (likely 255)
|
|
// ... test code ...
|
|
})
|
|
|
|
t.Run("Book title at max length", func(t *testing.T) {
|
|
// Test title at database column limit
|
|
// ... test code ...
|
|
})
|
|
|
|
t.Run("Bulk array size limits", func(t *testing.T) {
|
|
// Test bulk operations with arrays at limits
|
|
// ... test code ...
|
|
})
|
|
}
|
|
```
|
|
|
|
#### File-by-File Plan
|
|
|
|
##### File: `cmd/server/tests/edge_cases_test.go`
|
|
|
|
**Add new test function**:
|
|
|
|
1. **TestBoundaries_MaxLengths** (after Unicode tests)
|
|
- Lines: Add ~200 lines
|
|
- Purpose: Exact boundary testing for all text fields
|
|
- Verification: Consistent validation, no truncation
|
|
|
|
**Action**: Add new test function to `edge_cases_test.go`.
|
|
|
|
---
|
|
|
|
### Phase 4 Summary
|
|
|
|
**New Test Functions**: 5+
|
|
**Lines Added**: ~600 lines
|
|
**Load Testing**: Bulk operations, large responses
|
|
**Unicode Coverage**: Emojis, RTL, zero-width, combining, CJK
|
|
**Boundary Testing**: Exact limits for all text fields
|
|
|
|
**Verification Steps**:
|
|
1. ✅ Large payloads handled gracefully
|
|
2. ✅ Unicode characters work end-to-end
|
|
3. ✅ Boundary values consistent
|
|
|
|
---
|
|
|
|
## Implementation Order
|
|
|
|
### Phase 1: Stop Silent Failures (HIGHEST ROI)
|
|
|
|
**Week 1-2**:
|
|
1. Replace map-based assertions with structs (12 files)
|
|
2. Add database verification (8 files)
|
|
3. Add null/empty/missing tests (3 files)
|
|
|
|
**Deliverables**:
|
|
- All tests use struct-based assertions
|
|
- All mutations verify database state
|
|
- Critical edge cases covered
|
|
|
|
**Verification**:
|
|
```bash
|
|
# All tests compile
|
|
go test ./cmd/server/tests/... -v
|
|
|
|
# No map[string]interface{} in assertions
|
|
grep -r "map\[string\]interface" cmd/server/tests/ | wc -l # Should be 0
|
|
|
|
# All mutations have DB verification
|
|
# (Manual review of test files)
|
|
```
|
|
|
|
---
|
|
|
|
### Phase 2: Concurrency Protection (HIGH)
|
|
|
|
**Week 3**:
|
|
1. Add sync race condition tests (4 tests)
|
|
2. Add bulk concurrent tests (3 tests)
|
|
3. Add profile + device tests (2 tests)
|
|
|
|
**Deliverables**:
|
|
- All sync operations have concurrent tests
|
|
- All bulk operations have concurrent tests
|
|
- Profile + device interactions tested
|
|
|
|
**Verification**:
|
|
```bash
|
|
# Run with race detector
|
|
go test ./cmd/server/tests/... -race -v
|
|
|
|
# All concurrent tests pass
|
|
```
|
|
|
|
---
|
|
|
|
### Phase 3: Hardened Error Handling (MEDIUM)
|
|
|
|
**Week 4**:
|
|
1. Add WebSocket error paths (4 tests)
|
|
2. Add sync failures (4 tests)
|
|
3. Add OPDS edge cases (4 tests)
|
|
4. Add book matching edge cases (4 tests)
|
|
|
|
**Deliverables**:
|
|
- WebSocket has comprehensive error tests
|
|
- Sync has failure scenario tests
|
|
- OPDS handles edge cases
|
|
- Book matching handles ambiguity
|
|
|
|
**Verification**:
|
|
```bash
|
|
# All error path tests pass
|
|
go test ./cmd/server/tests/... -v -run "TestWebSocket|TestSync|TestOPDS|TestBookMatching"
|
|
```
|
|
|
|
---
|
|
|
|
### Phase 4: Load and Security (LOW)
|
|
|
|
**Week 5**:
|
|
1. Add large payload tests (3 tests)
|
|
2. Add Unicode tests (1 comprehensive test)
|
|
3. Add boundary tests (1 comprehensive test)
|
|
|
|
**Deliverables**:
|
|
- Large payloads handled gracefully
|
|
- Unicode works end-to-end
|
|
- Boundary values consistent
|
|
|
|
**Verification**:
|
|
```bash
|
|
# All load/security tests pass
|
|
go test ./cmd/server/tests/edge_cases_test.go -v
|
|
```
|
|
|
|
---
|
|
|
|
## Verification Checklist
|
|
|
|
### Pre-Implementation
|
|
|
|
- [ ] All current tests pass
|
|
```bash
|
|
go test ./cmd/server/tests/... -v
|
|
```
|
|
- [ ] No compilation errors
|
|
```bash
|
|
go build ./...
|
|
```
|
|
- [ ] Guidelines verification passes
|
|
```bash
|
|
bash scripts/verify-guidelines.sh
|
|
```
|
|
|
|
### Phase 1 Verification
|
|
|
|
- [ ] All `map[string]interface{}` replaced with structs
|
|
```bash
|
|
grep -r "map\[string\]interface" cmd/server/tests/*.go | wc -l # Should be 0
|
|
```
|
|
- [ ] All mutation operations have DB verification
|
|
```bash
|
|
# Manual review: Every POST/PUT/DELETE test should query DB after
|
|
```
|
|
- [ ] Null/empty/missing tests added for critical fields
|
|
```bash
|
|
grep -r "NullVsEmpty" cmd/server/tests/*.go | wc -l # Should be > 0
|
|
```
|
|
|
|
### Phase 2 Verification
|
|
|
|
- [ ] All sync operations have concurrent tests
|
|
```bash
|
|
grep -r "Concurrent" cmd/server/tests/sync_integration_test.go | wc -l # Should be > 0
|
|
```
|
|
- [ ] All bulk operations have concurrent tests
|
|
```bash
|
|
grep -r "Concurrent" cmd/server/tests/*_bulk_test.go | wc -l # Should be > 0
|
|
```
|
|
- [ ] Race detector passes
|
|
```bash
|
|
go test ./cmd/server/tests/... -race -v
|
|
```
|
|
|
|
### Phase 3 Verification
|
|
|
|
- [ ] WebSocket has error path tests
|
|
```bash
|
|
grep -r "DisconnectMid\|MalformedFrames\|Timeout" cmd/server/tests/websocket_test.go | wc -l # Should be > 0
|
|
```
|
|
- [ ] Sync has failure tests
|
|
```bash
|
|
grep -r "Offline\|QueueOverflow\|Conflicting" cmd/server/tests/sync_integration_test.go | wc -l # Should be > 0
|
|
```
|
|
- [ ] OPDS has edge case tests
|
|
```bash
|
|
grep -r "Empty\|Large\|Special" cmd/server/tests/opds_test.go | wc -l # Should be > 0
|
|
```
|
|
- [ ] Book matching has ambiguity tests
|
|
```bash
|
|
grep -r "NoMatches\|MultipleEqual\|Unicode" cmd/server/tests/book_matching_test.go | wc -l # Should be > 0
|
|
```
|
|
|
|
### Phase 4 Verification
|
|
|
|
- [ ] Large payload tests exist
|
|
```bash
|
|
grep -r "Maximum\|VeryLarge" cmd/server/tests/edge_cases_test.go | wc -l # Should be > 0
|
|
```
|
|
- [ ] Unicode tests exist
|
|
```bash
|
|
grep -r "Emoji\|RTL\|Zero-width\|Combining" cmd/server/tests/edge_cases_test.go | wc -l # Should be > 0
|
|
```
|
|
- [ ] Boundary tests exist
|
|
```bash
|
|
grep -r "Boundaries\|254\|255\|256" cmd/server/tests/edge_cases_test.go | wc -l # Should be > 0
|
|
```
|
|
|
|
### Post-Implementation
|
|
|
|
- [ ] All tests pass
|
|
```bash
|
|
go test ./cmd/server/tests/... -v
|
|
```
|
|
- [ ] No compilation errors
|
|
```bash
|
|
go build ./...
|
|
```
|
|
- [ ] Guidelines verification passes
|
|
```bash
|
|
bash scripts/verify-guidelines.sh
|
|
```
|
|
- [ ] Test coverage increased
|
|
```bash
|
|
go test ./cmd/server/tests/... -cover
|
|
```
|
|
|
|
---
|
|
|
|
## Success Metrics
|
|
|
|
### Before
|
|
- **507+** map-based assertions (brittle)
|
|
- **0** concurrency tests
|
|
- **0** database verification for mutations
|
|
- **0** null/empty/missing edge cases
|
|
- **Happy path only** for WebSocket, sync, OPDS, matching
|
|
|
|
### After (All Phases Complete)
|
|
- **0** map-based assertions (100% struct-based)
|
|
- **10+** concurrency tests
|
|
- **100%** database verification for mutations
|
|
- **15+** null/empty/missing edge case tests
|
|
- **Comprehensive error paths** for WebSocket, sync, OPDS, matching
|
|
- **Load testing** for bulk operations
|
|
- **Unicode coverage** for emojis, RTL, zero-width, combining marks
|
|
- **Boundary testing** for all text fields
|
|
|
|
### Reliability Score
|
|
|
|
| Area | Before | After | Improvement |
|
|
|-------|---------|--------|-------------|
|
|
| API contract safety | ❌ 0% | ✅ 100% | Compile-time detection |
|
|
| Data integrity | ⚠️ 20% | ✅ 100% | DB verification |
|
|
| Concurrency | ❌ 0% | ✅ 80% | Race tests |
|
|
| Edge cases | ⚠️ 10% | ✅ 90% | Comprehensive |
|
|
| Error handling | ⚠️ 30% | ✅ 90% | Failure scenarios |
|
|
| Load handling | ⚠️ 5% | ✅ 70% | Large payloads |
|
|
| **Overall** | **13%** | **90%** | **+77%** |
|
|
|
|
---
|
|
|
|
## Notes
|
|
|
|
1. **Follow PROJECT_GUIDELINES.md**:
|
|
- Use `setupTestServer()` from test_helpers.go
|
|
- Share one test setup across subtests (call once at function level)
|
|
- Use table-driven tests with `t.Run()`
|
|
- Configure database with `max_conns=1` via `pgxpool.ParseConfig()`
|
|
|
|
2. **No cascading fixes**: Stop on compilation error, review `git diff`, recover deliberately
|
|
|
|
3. **Post-edit verification**: Run `go build` for affected packages after each edit
|
|
|
|
4. **Multiple logical commits**: Commit changes in logical steps with clear messages
|
|
|
|
5. **Test execution speed**: If tests become too slow, consider:
|
|
- Parallel test execution (`t.Parallel()`)
|
|
- Test-specific database fixtures
|
|
- Reduced dataset sizes for load tests
|
|
|
|
---
|
|
|
|
## Appendix: Conversion Examples
|
|
|
|
### Example 1: Simple Response
|
|
|
|
**BEFORE:**
|
|
```go
|
|
var response map[string]interface{}
|
|
json.Unmarshal(body, &response)
|
|
deviceName := response["device"].(map[string]interface{})["device_name"].(string)
|
|
assert.Equal(t, "Test Device", deviceName)
|
|
```
|
|
|
|
**AFTER:**
|
|
```go
|
|
import "bookhoard/internal/handlers"
|
|
|
|
var response handlers.DeviceListResponse
|
|
err := json.Unmarshal(body, &response)
|
|
require.NoError(t, err)
|
|
assert.Equal(t, "Test Device", response.Devices[0].DeviceName)
|
|
```
|
|
|
|
### Example 2: Array Response
|
|
|
|
**BEFORE:**
|
|
```go
|
|
var response map[string]interface{}
|
|
json.Unmarshal(body, &response)
|
|
devices := response["devices"].([]interface{})
|
|
for _, d := range devices {
|
|
device := d.(map[string]interface{})
|
|
name := device["device_name"].(string)
|
|
// ...
|
|
}
|
|
```
|
|
|
|
**AFTER:**
|
|
```go
|
|
import "bookhoard/internal/handlers"
|
|
|
|
var response handlers.DeviceListResponse
|
|
err := json.Unmarshal(body, &response)
|
|
require.NoError(t, err)
|
|
for _, device := range response.Devices {
|
|
name := device.DeviceName
|
|
// ...
|
|
}
|
|
```
|
|
|
|
### Example 3: Database Verification
|
|
|
|
**BEFORE:**
|
|
```go
|
|
assert.Equal(t, http.StatusNoContent, rec.Code)
|
|
```
|
|
|
|
**AFTER:**
|
|
```go
|
|
assert.Equal(t, http.StatusNoContent, rec.Code)
|
|
|
|
pgID := pgtype.UUID{Bytes: [16]byte(id), Valid: true}
|
|
_, err := setup.DB.GetDevice(context.Background(), pgID)
|
|
assert.Error(t, err, "Device should be deleted")
|
|
```
|
|
|
|
---
|
|
|
|
## Appendix: test_helpers.go Improvements
|
|
|
|
### Critical Bug Fixes
|
|
|
|
#### Issue 1: Return Type Mismatch (Line 210)
|
|
|
|
**Problem:**
|
|
```go
|
|
// Line 210 - WRONG: Returns UserTestData instead of error
|
|
return fmt.Errorf("user already exists: %s", existingUser.Email)
|
|
```
|
|
|
|
**Fix:**
|
|
```go
|
|
// Line 210 - CORRECT: Return error
|
|
return fmt.Errorf("user already exists: %s", existingUser.Email), UserTestData{}
|
|
// OR remove the early return entirely since user exists check should delete and recreate
|
|
```
|
|
|
|
**Impact:** This causes compilation errors and prevents `createTestUserOnce` from working correctly.
|
|
|
|
---
|
|
|
|
#### Issue 2: Dead Code (Lines 207-214)
|
|
|
|
**Problem:**
|
|
```go
|
|
// Lines 207-214: Early return makes code below unreachable
|
|
if err == nil {
|
|
return fmt.Errorf("user already exists: %s", existingUser.Email)
|
|
}
|
|
return UserTestData{} // NEVER REACHED
|
|
|
|
// Lines 216-236: Never executed due to early return
|
|
passwordHash := "$2a$10$JjAtK7PPa1WexQC3AUGe8OXLeuseZ/haN1Mz7emMo6CfOvMiTVXWq"
|
|
user, err := db.CreateUser(ctx, database.CreateUserParams{
|
|
// ...
|
|
})
|
|
```
|
|
|
|
**Fix:**
|
|
```go
|
|
// Remove early return and always delete + recreate
|
|
ctx := context.Background()
|
|
|
|
// Check if user exists and delete for fresh state
|
|
user, err := db.GetUserByEmail(ctx, "testuser@example.com")
|
|
if err == nil {
|
|
// User exists, delete them to ensure fresh password
|
|
err = db.DeleteUser(ctx, user.ID)
|
|
if err != nil {
|
|
// If delete fails (user might be referenced elsewhere), log and continue
|
|
t.Logf("Warning: Could not delete existing test user: %v", err)
|
|
}
|
|
}
|
|
|
|
// Create a fresh test user with a valid password
|
|
passwordHash := "$2a$10$JjAtK7PPa1WexQC3AUGe8OXLeuseZ/haN1Mz7emMo6CfOvMiTVXWq"
|
|
newUser, err := db.CreateUser(ctx, database.CreateUserParams{
|
|
Email: "testuser@example.com",
|
|
Username: "testuser",
|
|
PasswordHash: passwordHash,
|
|
FirstName: pgtype.Text{String: "Test", Valid: true},
|
|
LastName: pgtype.Text{String: "User", Valid: true},
|
|
Role: "admin",
|
|
})
|
|
require.NoError(t, err, "Failed to create test user")
|
|
|
|
userUUID, err := uuid.FromBytes(newUser.ID.Bytes[:])
|
|
require.NoError(t, err, "Failed to parse user UUID")
|
|
return UserTestData{
|
|
ID: userUUID,
|
|
Email: "testuser@example.com",
|
|
Username: "testuser",
|
|
Password: "Test@Pass123!",
|
|
}
|
|
```
|
|
|
|
**Impact:** Dead code prevents test user creation from working properly.
|
|
|
|
---
|
|
|
|
### New Helper Functions
|
|
|
|
Add these helpers to `test_helpers.go` to reduce code duplication across tests and provide consistent database verification.
|
|
|
|
#### Helper 1: Database Verification Functions
|
|
|
|
```go
|
|
// Add to test_helpers.go after line 562
|
|
|
|
// verifyDeviceCreated verifies a device exists in database with expected values
|
|
func verifyDeviceCreated(t *testing.T, db *database.Queries, deviceID uuid.UUID, expected DeviceTestData) {
|
|
pgDeviceID := pgtype.UUID{Bytes: [16]byte(deviceID), Valid: true}
|
|
device, err := db.GetDevice(context.Background(), pgDeviceID)
|
|
require.NoError(t, err, "Device should exist in database")
|
|
|
|
assert.Equal(t, expected.Name, device.DeviceName, "Device name should match")
|
|
assert.Equal(t, expected.Type, device.DeviceType, "Device type should match")
|
|
assert.Equal(t, expected.Identifier, device.DeviceIdentifier, "Device identifier should match")
|
|
assert.NotEmpty(t, device.AuthToken, "Device should have auth token")
|
|
}
|
|
|
|
// verifyDeviceDeleted verifies a device does not exist in database
|
|
func verifyDeviceDeleted(t *testing.T, db *database.Queries, deviceID uuid.UUID) {
|
|
pgDeviceID := pgtype.UUID{Bytes: [16]byte(deviceID), Valid: true}
|
|
_, err := db.GetDevice(context.Background(), pgDeviceID)
|
|
assert.Error(t, err, "Device should be deleted from database")
|
|
}
|
|
|
|
// verifyUserUpdated verifies a user has expected field values in database
|
|
func verifyUserUpdated(t *testing.T, db *database.Queries, userID uuid.UUID, expected map[string]interface{}) {
|
|
pgUserID := pgtype.UUID{Bytes: [16]byte(userID), Valid: true}
|
|
user, err := db.GetUser(context.Background(), pgUserID)
|
|
require.NoError(t, err, "User should exist in database")
|
|
|
|
if firstName, ok := expected["first_name"]; ok {
|
|
if fn, ok := firstName.(string); ok {
|
|
assert.Equal(t, fn, user.FirstName.String, "First name should match")
|
|
}
|
|
}
|
|
if lastName, ok := expected["last_name"]; ok {
|
|
if ln, ok := lastName.(string); ok {
|
|
assert.Equal(t, ln, user.LastName.String, "Last name should match")
|
|
}
|
|
}
|
|
if email, ok := expected["email"]; ok {
|
|
if em, ok := email.(string); ok {
|
|
assert.Equal(t, em, user.Email, "Email should match")
|
|
}
|
|
}
|
|
}
|
|
|
|
// verifyMediaItemUpdated verifies a media item has expected values in database
|
|
func verifyMediaItemUpdated(t *testing.T, db *database.Queries, mediaID uuid.UUID, expected map[string]interface{}) {
|
|
pgMediaID := pgtype.UUID{Bytes: [16]byte(mediaID), Valid: true}
|
|
media, err := db.GetMediaItem(context.Background(), pgMediaID)
|
|
require.NoError(t, err, "Media item should exist in database")
|
|
|
|
if title, ok := expected["title"]; ok {
|
|
if t, ok := title.(string); ok {
|
|
assert.Equal(t, t, media.Title, "Title should match")
|
|
}
|
|
}
|
|
if tags, ok := expected["tags"]; ok {
|
|
if tagArray, ok := tags.([]string); ok {
|
|
assert.ElementsMatch(t, tagArray, media.Tags, "Tags should match")
|
|
}
|
|
}
|
|
// Add more fields as needed
|
|
}
|
|
```
|
|
|
|
**Usage in tests:**
|
|
```go
|
|
// Instead of manual DB queries:
|
|
device := setup.CreateDevice(t, "Test Device", "koreader", "test-123")
|
|
|
|
// NEW: Use helper
|
|
verifyDeviceCreated(t, setup.DB, device.ID, DeviceTestData{
|
|
Name: "Test Device",
|
|
Type: "koreader",
|
|
Identifier: "test-123",
|
|
})
|
|
```
|
|
|
|
---
|
|
|
|
#### Helper 2: Request Builder Functions
|
|
|
|
```go
|
|
// Add to test_helpers.go after database verification functions
|
|
|
|
// buildLoginRequest creates a login request map
|
|
func buildLoginRequest(email, password string) map[string]interface{} {
|
|
return map[string]interface{}{
|
|
"login": email,
|
|
"password": password,
|
|
}
|
|
}
|
|
|
|
// buildDeviceUpdateRequest creates a device update request map
|
|
func buildDeviceUpdateRequest(name string, syncEnabled *bool, syncFreq *int32) map[string]interface{} {
|
|
req := map[string]interface{}{}
|
|
if name != "" {
|
|
req["device_name"] = name
|
|
}
|
|
if syncEnabled != nil {
|
|
req["sync_enabled"] = *syncEnabled
|
|
}
|
|
if syncFreq != nil {
|
|
req["sync_frequency_minutes"] = *syncFreq
|
|
}
|
|
return req
|
|
}
|
|
|
|
// buildMediaItemRequest creates a media item request map
|
|
func buildMediaItemRequest(libraryID, title, author, filePath, mimeType string, fileSize int64) map[string]interface{} {
|
|
return map[string]interface{}{
|
|
"library_id": libraryID,
|
|
"title": title,
|
|
"author": author,
|
|
"file_path": filePath,
|
|
"file_size": fileSize,
|
|
"mime_type": mimeType,
|
|
}
|
|
}
|
|
|
|
// buildUserUpdateRequest creates a user profile update request map
|
|
func buildUserUpdateRequest(firstName, lastName, email, theme string) map[string]interface{} {
|
|
req := map[string]interface{}{}
|
|
if firstName != "" {
|
|
req["first_name"] = firstName
|
|
}
|
|
if lastName != "" {
|
|
req["last_name"] = lastName
|
|
}
|
|
if email != "" {
|
|
req["email"] = email
|
|
}
|
|
if theme != "" {
|
|
req["theme"] = theme
|
|
}
|
|
return req
|
|
}
|
|
```
|
|
|
|
**Usage in tests:**
|
|
```go
|
|
// BEFORE: Manually build maps
|
|
loginRequest := map[string]interface{}{
|
|
"login": email,
|
|
"password": password,
|
|
}
|
|
|
|
// AFTER: Use helper
|
|
loginRequest := buildLoginRequest(email, password)
|
|
```
|
|
|
|
---
|
|
|
|
#### Helper 3: Struct-Based Response Assertions
|
|
|
|
```go
|
|
// Add to test_helpers.go after request builder functions
|
|
|
|
import (
|
|
"bookhoard/internal/handlers"
|
|
// ... existing imports
|
|
)
|
|
|
|
// assertDeviceListResponse parses and asserts DeviceListResponse
|
|
func assertDeviceListResponse(t *testing.T, body []byte) handlers.DeviceListResponse {
|
|
var response handlers.DeviceListResponse
|
|
err := json.Unmarshal(body, &response)
|
|
require.NoError(t, err, "Response should match DeviceListResponse schema")
|
|
return response
|
|
}
|
|
|
|
// assertDeviceUpdateResponse parses and asserts DeviceUpdateResponse
|
|
func assertDeviceUpdateResponse(t *testing.T, body []byte) handlers.DeviceUpdateResponse {
|
|
var response handlers.DeviceUpdateResponse
|
|
err := json.Unmarshal(body, &response)
|
|
require.NoError(t, err, "Response should match DeviceUpdateResponse schema")
|
|
return response
|
|
}
|
|
|
|
// assertLoginResponse parses and asserts LoginResponse
|
|
func assertLoginResponse(t *testing.T, body []byte) handlers.LoginResponse {
|
|
var response handlers.LoginResponse
|
|
err := json.Unmarshal(body, &response)
|
|
require.NoError(t, err, "Response should match LoginResponse schema")
|
|
return response
|
|
}
|
|
|
|
// assertMediaListResponse parses and asserts MediaListResponse
|
|
func assertMediaListResponse(t *testing.T, body []byte) handlers.MediaListResponse {
|
|
var response handlers.MediaListResponse
|
|
err := json.Unmarshal(body, &response)
|
|
require.NoError(t, err, "Response should match MediaListResponse schema")
|
|
return response
|
|
}
|
|
|
|
// assertLibraryResponse parses and asserts LibraryResponse
|
|
func assertLibraryResponse(t *testing.T, body []byte) handlers.LibraryResponse {
|
|
var response handlers.LibraryResponse
|
|
err := json.Unmarshal(body, &response)
|
|
require.NoError(t, err, "Response should match LibraryResponse schema")
|
|
return response
|
|
}
|
|
```
|
|
|
|
**Usage in tests:**
|
|
```go
|
|
// BEFORE: Map-based parsing
|
|
var response map[string]interface{}
|
|
json.Unmarshal(rec.Body.Bytes(), &response)
|
|
deviceName := response["device"].(map[string]interface{})["device_name"].(string)
|
|
assert.Equal(t, "Test Device", deviceName)
|
|
|
|
// AFTER: Struct-based parsing
|
|
response := assertDeviceListResponse(t, rec.Body.Bytes())
|
|
assert.Equal(t, "Test Device", response.Devices[0].DeviceName)
|
|
```
|
|
|
|
---
|
|
|
|
#### Helper 4: Concurrent Test Utilities
|
|
|
|
```go
|
|
// Add to test_helpers.go after assertion helpers
|
|
|
|
// runConcurrent executes functions concurrently and waits for completion
|
|
func runConcurrent(t *testing.T, numWorkers int, fns []func() error) []error {
|
|
errors := make(chan error, len(fns))
|
|
var wg sync.WaitGroup
|
|
|
|
for i := 0; i < len(fns); i++ {
|
|
wg.Add(1)
|
|
go func(idx int) {
|
|
defer wg.Done()
|
|
if err := fns[idx](); err != nil {
|
|
errors <- err
|
|
}
|
|
}(i)
|
|
}
|
|
|
|
wg.Wait()
|
|
close(errors)
|
|
|
|
var allErrors []error
|
|
for err := range errors {
|
|
allErrors = append(allErrors, err)
|
|
}
|
|
return allErrors
|
|
}
|
|
|
|
// retryOperation retries a function with exponential backoff
|
|
func retryOperation(maxRetries int, initialDelay time.Duration, fn func() error) error {
|
|
var err error
|
|
delay := initialDelay
|
|
|
|
for attempt := 0; attempt < maxRetries; attempt++ {
|
|
if err = fn(); err == nil {
|
|
return nil
|
|
}
|
|
|
|
if attempt < maxRetries-1 {
|
|
time.Sleep(delay)
|
|
delay *= 2 // Exponential backoff
|
|
}
|
|
}
|
|
return err
|
|
}
|
|
```
|
|
|
|
**Usage in tests:**
|
|
```go
|
|
// BEFORE: Manual goroutine management
|
|
var wg sync.WaitGroup
|
|
errors := make(chan error, 3)
|
|
for i := 0; i < 3; i++ {
|
|
wg.Add(1)
|
|
go func(idx int) {
|
|
defer wg.Done()
|
|
// ... operation ...
|
|
}(i)
|
|
}
|
|
wg.Wait()
|
|
close(errors)
|
|
|
|
// AFTER: Helper function
|
|
fns := []func() error{
|
|
func() error { /* operation 1 */ return nil },
|
|
func() error { /* operation 2 */ return nil },
|
|
func() error { /* operation 3 */ return nil },
|
|
}
|
|
errors := runConcurrent(t, len(fns), fns)
|
|
```
|
|
|
|
---
|
|
|
|
### Updated test_helpers.go Structure
|
|
|
|
```go
|
|
// Line 1: Add handlers import
|
|
import (
|
|
"bookhoard/internal/config"
|
|
"bookhoard/internal/database"
|
|
"bookhoard/internal/handlers" // NEW
|
|
"bookhoard/internal/middleware"
|
|
// ... rest of imports
|
|
)
|
|
|
|
// Line 210: FIX return type
|
|
// BEFORE:
|
|
return fmt.Errorf("user already exists: %s", existingUser.Email)
|
|
|
|
// AFTER:
|
|
// Remove early return, let code continue to delete + recreate
|
|
|
|
// Line 213-236: FIX dead code
|
|
// These lines are now reachable after removing early return
|
|
// Keep the CreateUser logic
|
|
|
|
// After line 562: ADD new helper functions
|
|
// (See Helper Functions section above)
|
|
```
|
|
|
|
---
|
|
|
|
### File-by-File Changes: test_helpers.go
|
|
|
|
| Lines | Change | Impact |
|
|
|--------|---------|--------|
|
|
| **4-31** | Add `"bookhoard/internal/handlers"` import | Enables struct-based assertions |
|
|
| **210** | Fix return type from `UserTestData` to error, or remove early return | Fixes compilation error |
|
|
| **207-214** | Remove early return or fix dead code | Makes user creation code reachable |
|
|
| **562+** | Add verification helpers (5 functions) | Reduces duplication, ensures DB verification |
|
|
| **562+** | Add request builders (4 functions) | Reduces map duplication |
|
|
| **562+** | Add struct assertion helpers (5 functions) | Enables compile-time safety |
|
|
| **562+** | Add concurrent utilities (2 functions) | Simplifies concurrent tests |
|
|
|
|
**Total Lines Added**: ~300 lines
|
|
**Total Lines Modified**: ~10 lines
|
|
**Functions Added**: 16 new helper functions
|
|
|
|
---
|
|
|
|
### Summary of test_helpers.go Improvements
|
|
|
|
| Category | Before | After | Improvement |
|
|
|----------|---------|--------|-------------|
|
|
| **Critical Bugs** | 2 compilation errors | 0 | All tests compile |
|
|
| **Code Duplication** | Map-based requests repeated everywhere | Shared builder functions | Consistent request building |
|
|
| **Database Verification** | Manual queries scattered | Helper functions | Guaranteed verification |
|
|
| **Type Safety** | map[string]interface{} | Struct-based assertions | Compile-time error detection |
|
|
| **Concurrent Tests** | Manual goroutine management | Utility functions | Consistent patterns |
|
|
| **Maintainability** | Logic duplicated in tests | Centralized helpers | Easier to update |
|
|
|
|
---
|
|
|
|
**END OF PLAN**
|
|
|
|
|
|
---
|
|
|
|
# END OF COMPREHENSIVE REVIEW REPORT
|
|
|
|
**Date**: 2025-02-13
|
|
**Scope**: Complete review of entire test system (30 test files, ~10,941 lines)
|
|
**Purpose**: Identify ALL gaps in TEST_RELIABILITY_PLAN.md and ensure complete coverage
|
|
|
|
---
|
|
|
|
## Executive Summary
|
|
|
|
The comprehensive review identified **69 individual issues** across **30 test files**. Critical findings:
|
|
|
|
### Most Critical Gaps
|
|
|
|
| Gap | Impact | Priority |
|
|
|------|---------|----------|
|
|
| **test_helpers.go has 2 critical bugs** | Tests fail compilation, user creation broken | 🔴 CRITICAL |
|
|
| **69% of tests use map-based assertions** | Silent API changes, type errors | 🔴 CRITICAL |
|
|
| **85% of tests lack DB verification** | Data corruption, silent failures | 🔴 CRITICAL |
|
|
| **96% of tests lack concurrency** | Race conditions in production | 🟠 HIGH |
|
|
| **10 test files not in plan** | Missing coverage, unknown gaps | 🟡 MEDIUM |
|
|
| **Zero Unicode edge cases** | Encoding failures, search issues | 🟠 HIGH |
|
|
| **Unit tests give false confidence** | Mock handlers != real integration | 🟡 MEDIUM |
|
|
|
|
### Recommended Priority Change
|
|
|
|
**Original**: Phase 1 → 2 → 3 → 4
|
|
**Revised**: **Phase 0** (NEW) → 1 → 2 → 3
|
|
|
|
**Phase 0: Fix Test Infrastructure** (NEW - 2-3 hours)
|
|
1. Fix 2 critical bugs in test_helpers.go
|
|
2. Add 6 missing helper functions
|
|
3. Move Unicode tests from Phase 4 → Phase 1
|
|
4. Create test isolation improvements
|
|
|
|
**Rationale**: Tests depend on test_helpers.go. Bugs there block all other work. Unicode too important to delay.
|
|
|