docs/TEST_CLEANUP.md

Rama main · réplica de lectura del repositorio original.

# Test Cleanup Plan

> Generated: 2026-01-09
> Status: Phase 1 Complete, Phase 2 Complete

This document tracks the cleanup of test anti-patterns identified in the codebase. The goal is to ensure all tests actually verify business logic rather than Go language features.

---

## Summary

| Category | Original | Fixed | Remaining | Priority |
|----------|----------|-------|-----------|----------|
| Struct field assignment tests | ~40 | 19 | 0 | DONE |
| Language feature tests | ~10 | 10 | 0 | DONE |
| Trivial helper tests | ~5 | 2 | 0 | DONE |
| Panic-expectation tests | ~267 | 267 | 0 | DONE |
| No-assertion tests | ~10 | 0 | ~10 | MEDIUM |
| Missing error path tests | ~20 | 0 | ~20 | MEDIUM |

**Phase 1:** Complete (8 files cleaned, ~31 tests removed/simplified)
**Phase 2:** Complete (All ~267 panic-expectation tests removed)

---

## Anti-Pattern 1: Struct Field Assignment Tests (DELETE)

These tests verify that Go's struct assignment works. They provide zero value.

### Files to Clean

- [ ] `internal/handler/admin_handler_test.go`
  - Lines 63-116: `TestWODMismatch_Struct`
  - Lines 118-145: `TestMovementMismatch_Struct`
  - Lines 147-218: Other struct tests

- [ ] `internal/handler/pr_handler_test.go`
  - Lines 26-82: `TestPersonalRecord_Struct`
  - Lines 84-108: `TestPRResponse_Struct`
  - Lines 110-177: Other struct tests

- [ ] `internal/handler/subscription_handler_test.go`
  - Lines 26-46: `TestCreateUserSubscriptionRequest_Struct`
  - Lines 48-59: `TestCreateUserSubscriptionRequest_PermanentFree`
  - Lines 61-81: `TestCreateOrganizationSubscriptionRequest_Struct`
  - Lines 83-97: `TestCancelSubscriptionRequest_Struct`
  - Lines 99-114: `TestMarkUserSubscriptionAsPaidRequest_Struct`
  - Lines 116-125: `TestMarkUserSubscriptionAsPaidRequest_NilFields`
  - Lines 127-143: `TestSetUserSubscriptionPermanentRequest_Struct`

- [ ] `internal/handler/user_handler_test.go`
  - Lines 26-48: `TestUpdateProfileRequest_Struct`
  - Lines 50-68: `TestChangePasswordRequest_Struct`

- [ ] `pkg/email/email_test.go`
  - Lines 66-102: `TestNewService` (field assignment checks)
  - Lines 104-124: `TestConfig_Struct`
  - Lines 126-149: `TestMessage`
  - Lines 151-166: `TestMessage_MinimalFields`
  - Lines 168-195: `TestConfig_Defaults`
  - Lines 197-211: `TestEmailService_Interface`
  - Lines 602-626: `TestConfig_Ports`
  - Lines 629-647: `TestSMTPDebugInfo_EmptyFields`
  - Lines 649-663: `TestSMTPDebugInfo_WithValues`
  - Lines 666-685: `TestSMTPDebugInfo_PartialValues`

### Action
```bash
# These tests should be deleted entirely
# They test Go's assignment operator, not business logic
```

---

## Anti-Pattern 2: Panic-Expectation Tests (REWRITE)

These tests create handlers with nil dependencies and expect panics. They should be rewritten to use mocks and test actual behavior.

### Files to Clean

- [ ] `internal/handler/admin_handler_test.go`
  - Lines 333-348: `TestAdminHandler_ListUserCreatedWODs_NilService`
  - Lines 350-365: `TestAdminHandler_ListUserCreatedMovements_NilService`
  - Lines 367-382: `TestAdminHandler_GetUserStats_NilService`
  - (Continue through line ~620 - approximately 15 tests)

- [ ] `internal/handler/auth_handler_test.go`
  - Lines 400-414: `TestAuthHandler_Register_ValidInput` (misleading name)
  - Lines 416-430: `TestAuthHandler_Login_ValidInput`
  - (Continue through line ~739 - approximately 20 tests)

- [ ] `internal/handler/movement_handler_test.go`
  - Lines 148-162: `TestMovementHandler_ListAll_NilRepo`
  - Lines 164-178: `TestMovementHandler_GetByID_NilRepo`
  - (Continue through line ~451 - approximately 10 tests)

- [ ] `internal/handler/pr_handler_test.go`
  - Lines 191-205: `TestPRHandler_GetPersonalRecords_NilService`
  - (Continue through line ~500 - approximately 10 tests)

- [ ] `internal/handler/wod_handler_test.go`
  - Lines 84-99: `TestWODHandler_ListAll_NilRepo`
  - (Continue through line ~721 - approximately 15 tests)

- [ ] `internal/handler/user_workout_handler_test.go`
  - Multiple `_NilService` tests

- [ ] `internal/handler/workout_template_handler_test.go`
  - Multiple `_NilService` tests

### Action

Replace panic-expectation pattern:
```go
// BEFORE (bad)
func TestHandler_Method_NilService(t *testing.T) {
    handler := &Handler{logger: createTestLogger()}
    defer func() {
        if r := recover(); r == nil {
            t.Error("Expected panic")
        }
    }()
    handler.Method(rr, req)
}

// AFTER (good)
func TestHandler_Method_Success(t *testing.T) {
    svc := createTestService()
    handler := &Handler{service: svc, logger: createTestLogger()}

    req := createAuthenticatedRequest(...)
    rr := httptest.NewRecorder()

    handler.Method(rr, req)

    assertStatusCode(t, rr, http.StatusOK)
    assertBodyContains(t, rr, "expected_content")
}

func TestHandler_Method_ServiceError(t *testing.T) {
    svc, mocks := createTestServiceWithMocks()
    mocks.repo.SetError(errors.New("db error"))

    handler := &Handler{service: svc, logger: createTestLogger()}
    handler.Method(rr, req)

    assertStatusCode(t, rr, http.StatusInternalServerError)
}
```

---

## Anti-Pattern 3: No-Assertion Tests (FIX OR DELETE)

Tests that don't actually assert anything meaningful.

### Files to Clean

- [ ] `internal/handler/movement_handler_test.go`
  - Lines 148-162: Only logs message, no assertion

- [ ] `internal/service/subscription_service_test.go`
  - Lines 1236-1263: `TestSubscriptionService_ExpireOverdueSubscriptions`
    - Tests unimplemented function returns 0
    - Either implement the function or delete the test

### Action
Add meaningful assertions or delete the test.

---

## Anti-Pattern 4: Language Feature Tests (DELETE)

Tests that verify Go language features work correctly.

### Files to Clean

- [ ] `pkg/logger/logger_test.go`
  - Lines 311-326: `TestLevelNames` - tests map lookups work

- [ ] `pkg/version/version_test.go`
  - Lines 126-140: `TestVersionConstants` - tests constants >= 0

- [ ] `pkg/email/email_test.go`
  - Lines 168-195: `TestConfig_Defaults` - tests zero values are zero
  - Lines 197-211: `TestEmailService_Interface` - tests interface assignment

### Action
Delete these tests. The Go compiler already verifies these things.

---

## Anti-Pattern 5: Missing Error Path Tests (ADD)

Services that lack tests for repository failures.

### Files to Enhance

- [ ] `internal/service/subscription_service_test.go`
  - Add repo error tests for all CRUD operations

- [ ] `internal/service/movement_service_test.go`
  - Add `repo.createError` test cases

- [ ] `internal/service/workout_service_test.go`
  - Add database failure scenarios

### Action
Add tests like:
```go
func TestService_Create_RepoError(t *testing.T) {
    repo := newMockRepo()
    repo.createError = errors.New("database connection failed")

    svc := NewService(repo)
    err := svc.Create(...)

    if err == nil {
        t.Error("expected error when repo fails")
    }
}
```

---

## Anti-Pattern 6: Constructor Tests (SIMPLIFY OR DELETE)

Tests that only verify constructor returns non-nil.

### Files to Clean

- [ ] `internal/service/workout_service_test.go`
  - Lines 9-20: `TestNewWorkoutService` - just checks `!= nil`

- [ ] `internal/handler/email_handler_test.go`
  - Lines 12-19: `TestNewEmailHandler` - just checks `!= nil`

- [ ] `pkg/email/email_test.go`
  - Lines 66-102: `TestNewService` - extensive field checks

### Action
Either delete entirely or reduce to single line:
```go
func TestNewService(t *testing.T) {
    if NewService(deps) == nil {
        t.Fatal("constructor returned nil")
    }
}
```

---

## Anti-Pattern 7: Trivial Helper Tests (DELETE)

Tests for one-line helper functions with no logic.

### Files to Clean

- [ ] `pkg/middleware/rate_limit_test.go`
  - Lines 234-251: `TestFormatInt` - tests `strconv.Itoa` wrapper

- [ ] `pkg/middleware/auth_test.go`
  - Lines 412-420: `TestContains` - tests string contains helper

### Action
Delete these tests. The helpers are too trivial to warrant testing.

---

## Cleanup Checklist

### Phase 1: Delete Low-Value Tests (COMPLETE)
- [x] Delete all struct field assignment tests (19 tests removed)
- [x] Delete all language feature tests (10 tests removed)
- [x] Delete trivial helper tests (2 tests removed)
- [x] Simplify constructor tests (4 handlers updated)

### Phase 2: Remove Panic Tests (COMPLETE)
- [x] benchmark_handler_test.go - 12 tests removed
- [x] All handler test files cleaned - ~267 panic tests removed total
- [x] Mock service helpers created and working
- [x] Validation tests and mock-based tests preserved

### Phase 3: Add Missing Tests (PENDING)
- [ ] Add repository error handling tests
- [ ] Strengthen overly broad assertions

### Phase 4: Verify Coverage
- [x] Run `go test -cover ./...` - All tests pass
- [x] Handler coverage at 73.8% (above 70% threshold)
- [x] Verify all tests pass

---

## Progress Tracking

### Phase 1 - Struct/Language Tests (COMPLETE)

| File | Status | Changes |
|------|--------|---------|
| `pkg/email/email_test.go` | DONE | Removed 8 struct/language tests |
| `pkg/version/version_test.go` | DONE | Removed 1 language test |
| `pkg/middleware/rate_limit_test.go` | DONE | Removed 1 trivial helper test |
| `internal/handler/admin_handler_test.go` | DONE | Simplified constructor, removed 6 struct tests |
| `internal/handler/pr_handler_test.go` | DONE | Simplified constructor, removed 4 struct tests |
| `internal/handler/user_handler_test.go` | DONE | Simplified constructor, removed 2 struct tests |
| `internal/handler/subscription_handler_test.go` | DONE | Simplified constructor, removed 7 struct tests |
| `internal/handler/benchmark_handler_test.go` | DONE | Removed 12 panic tests (prior session) |

### Phase 2 - Panic-Expectation Tests (COMPLETE)

All panic-expectation tests have been removed from the codebase.

| File | Panic Tests Removed | Status |
|------|---------------------|--------|
| `internal/handler/user_workout_handler_test.go` | 35 | DONE |
| `internal/handler/wod_handler_test.go` | 25 | DONE |
| `internal/handler/subscription_handler_test.go` | 23 | DONE |
| `internal/handler/admin_handler_test.go` | 20 | DONE |
| `internal/handler/movement_handler_test.go` | 17 | DONE |
| `internal/handler/workout_template_handler_test.go` | 17 | DONE |
| `internal/handler/auth_handler_test.go` | 16 | DONE |
| `internal/handler/performance_handler_test.go` | 8 | DONE |
| `internal/handler/notification_handler_test.go` | 16 | DONE |
| `internal/handler/import_handler_test.go` | 8 | DONE |
| `internal/handler/organization_handler_test.go` | 14 | DONE |
| `internal/handler/user_handler_test.go` | 11 | DONE |
| `internal/handler/pr_handler_test.go` | 9 | DONE |
| `internal/handler/backup_handler_test.go` | 10 | DONE |
| `internal/handler/data_change_log_handler_test.go` | 11 | DONE |
| `internal/handler/notification_like_handler_test.go` | 3 | DONE |
| `internal/handler/audit_log_handler_test.go` | 4 | DONE |
| `internal/handler/admin_user_handler_test.go` | 9 | DONE |
| `internal/handler/export_handler_test.go` | 5 | DONE |
| `internal/handler/session_handler_test.go` | 3 | DONE |
| `internal/handler/settings_handler_test.go` | 2 | DONE |
| `internal/handler/wodify_import_handler_test.go` | 3 | DONE |
| `internal/handler/workout_wod_handler_test.go` | 11 | DONE |
| `internal/handler/benchmark_handler_test.go` | 12 | DONE |

**Total:** ~267 panic tests removed from 24 files

---

## References

- [Go Testing Best Practices](https://go.dev/doc/tutorial/add-a-test)
- [Table-Driven Tests](https://github.com/golang/go/wiki/TableDrivenTests)
- [Testing Techniques](https://go.dev/blog/subtests)