StarPunk/docs/decisions/ADR-051-phase1-test-strategy.md

# ADR-051: Phase 1 Test Strategy and Implementation Review

## Status
Accepted

## Context

The developer has completed Phase 1 of the IndieAuth authorization server removal, which involved:
- Removing the `/auth/authorization` endpoint
- Deleting the authorization UI template
- Removing authorization and PKCE-specific test files
- Cleaning up related imports

The implementation has resulted in 539 of 569 tests passing (94.7%), with 30 tests failing. These failures fall into six categories:
1. OAuth metadata endpoint tests (10 tests)
2. State token tests (6 tests)
3. Callback tests (4 tests)
4. Migration tests (2 tests)
5. IndieAuth client discovery tests (5 tests)
6. Development auth tests (1 test)

## Decision

### On Phase 1 Implementation Quality
Phase 1 has been executed correctly and according to plan. The developer properly:
- Removed only the authorization-specific code
- Preserved admin login functionality
- Documented all changes comprehensively
- Identified and categorized all test failures

### On Handling the 30 Failing Tests
**We choose Option A: Delete all 30 failing tests now.**

Rationale:
1. **All failures are expected** - Every failing test is testing functionality we intentionally removed
2. **Clean state principle** - Leaving failing tests creates confusion and technical debt
3. **No value in preservation** - These tests will never be relevant again in V1
4. **Simplified maintenance** - A green test suite is easier to maintain and gives confidence

### On the Overall Implementation Plan
**The 5-phase approach remains correct, but we should accelerate execution.**

Recommended adjustments:
1. **Combine Phases 2 and 3** - Remove token functionality AND database tables together
2. **Keep Phase 4 separate** - External verification is complex enough to warrant isolation
3. **Keep Phase 5 separate** - Documentation deserves dedicated attention

### On Immediate Next Steps
1. **Clean up the 30 failing tests immediately** (before committing Phase 1)
2. **Commit Phase 1 with clean test suite**
3. **Proceed directly to combined Phase 2+3**

## Rationale

### Why Delete Tests Now
- **False positives harm confidence**: Failing tests that "should" fail train developers to ignore test failures
- **Git preserves history**: If we ever need these tests, they're in git history
- **Clear intention**: Deleted tests make it explicit that functionality is gone
- **Faster CI/CD**: No time wasted running irrelevant tests

### Why Accelerate Phases
- **Momentum preservation**: The developer understands the codebase now
- **Reduced intermediate states**: Fewer partially-functional states reduces confusion
- **Coherent changes**: Token removal and database cleanup are logically connected

### Why Not Fix Tests
- **Wasted effort**: Fixing tests for removed functionality is pure waste
- **Misleading coverage**: Tests for non-existent features inflate coverage metrics
- **Future confusion**: Future developers would wonder why we test things that don't exist

## Consequences

### Positive
- **Clean test suite**: 100% passing tests after cleanup
- **Clear boundaries**: Each phase has unambiguous completion
- **Faster delivery**: Combined phases reduce total implementation time
- **Reduced complexity**: Fewer intermediate states to manage

### Negative
- **Larger commits**: Combined phases create bigger changesets
- **Rollback complexity**: Larger changes are harder to revert
- **Testing gaps**: Need to ensure no valid tests are accidentally removed

### Mitigations
- **Careful review**: Double-check each test deletion is intentional
- **Git granularity**: Use separate commits for test deletion vs. code removal
- **Backup branch**: Keep Phase 1 isolated in case rollback needed

## Implementation Instructions

### Immediate Actions (30 minutes)

1. **Delete OAuth metadata tests**:
```bash
# Remove the entire TestOAuthMetadataEndpoint class from test_routes_public.py
# Also remove TestIndieAuthMetadataLink class
```

2. **Delete state token tests**:
```bash
# Review each state token test - some may be testing admin login
# Only delete tests specific to authorization flow
```

3. **Delete callback tests**:
```bash
# Verify these are authorization callbacks, not admin login callbacks
# If admin login, fix them; if authorization, delete them
```

4. **Delete migration tests expecting PKCE**:
```bash
# Update tests to not expect code_verifier column
# These tests should verify current schema, not old schema
```

5. **Delete h-app microformat tests**:
```bash
# Remove all IndieAuth client discovery tests
# These are no longer relevant without authorization endpoint
```

6. **Verify clean suite**:
```bash
uv run pytest
# Should show 100% passing
```

### Commit Strategy

Create two commits:

**Commit 1**: Test cleanup
```bash
git add tests/
git commit -m "test: Remove tests for deleted IndieAuth authorization functionality

- Remove OAuth metadata endpoint tests (no longer serving authorization metadata)
- Remove authorization-specific state token tests
- Remove authorization callback tests
- Remove h-app client discovery tests
- Update migration tests to reflect current schema

All removed tests were for functionality intentionally deleted in Phase 1.
Tests preserved in git history if ever needed for reference."
```

**Commit 2**: Phase 1 implementation
```bash
git add .
git commit -m "feat!: Phase 1 - Remove IndieAuth authorization server

BREAKING CHANGE: Removed built-in IndieAuth authorization endpoint

- Remove /auth/authorization endpoint
- Delete authorization consent UI template
- Remove authorization-related imports
- Clean up PKCE test file
- Update version to 1.0.0-rc.4

This is Phase 1 of 5 in the IndieAuth removal plan.
Admin login functionality remains fully operational.
Token endpoint preserved for Phase 2 removal.

See: docs/architecture/indieauth-removal-phases.md"
```

### Phase 2+3 Combined Plan (Next)

After committing Phase 1:

1. **Remove token endpoint** (`/auth/token`)
2. **Remove token module** (`starpunk/tokens.py`)
3. **Create and run database migration** to drop tables
4. **Remove all token-related tests**
5. **Update version** to 1.0.0-rc.5

This combined approach will complete the removal faster while maintaining coherent system states.

## Alternatives Considered

### Alternative 1: Fix Failing Tests
**Rejected** because:
- Effort to fix tests for removed features is wasted
- Creates false sense that features still exist
- Contradicts the removal intention

### Alternative 2: Leave Tests Failing Until End
**Rejected** because:
- Creates confusion about system state
- Makes it hard to identify real failures
- Violates principle of maintaining green test suite

### Alternative 3: Comment Out Failing Tests
**Rejected** because:
- Dead code accumulates
- Comments tend to persist forever
- Git history is better for preservation

### Alternative 4: Keep Original 5 Phases
**Rejected** because:
- Unnecessary granularity
- More intermediate states to manage
- Slower overall delivery

## Review Checklist

Before proceeding:
- [ ] Verify each deleted test was actually testing removed functionality
- [ ] Confirm admin login tests are preserved and passing
- [ ] Ensure no accidental deletion of valid tests
- [ ] Document test removal in commit messages
- [ ] Verify 100% test pass rate after cleanup
- [ ] Create backup branch before Phase 2+3

## References

- `docs/architecture/indieauth-removal-phases.md` - Original phase plan
- `docs/reports/2025-11-24-phase1-indieauth-server-removal.md` - Phase 1 implementation report
- ADR-030 - External token verification architecture
- ADR-050 - Decision to remove custom IndieAuth server

---

**Decision Date**: 2025-11-24
**Decision Makers**: StarPunk Architecture Team
**Status**: Accepted and ready for immediate implementation