Gondulf/docs/designs/phase-5b-clarifications.md

# Phase 5b Implementation Clarifications

This document provides clear answers to the Developer's implementation questions for Phase 5b.

## Questions and Answers

### 1. E2E Browser Automation

**Question**: Should we use Playwright/Selenium for browser automation, or TestClient-based flow simulation?

**Decision**: Use TestClient-based flow simulation.

**Rationale**:
- Simpler and more maintainable - no browser drivers to manage
- Faster execution - no browser startup overhead
- Better CI/CD compatibility - no headless browser configuration
- Sufficient for protocol compliance testing - we're testing OAuth flows, not UI rendering
- Aligns with existing test patterns in the codebase

**Implementation Guidance**:
```python
# Use FastAPI TestClient with session persistence
from fastapi.testclient import TestClient

def test_full_authorization_flow():
    client = TestClient(app)
    # Simulate full OAuth flow through TestClient
    # Parse HTML responses where needed for form submission
```

### 2. Database Fixtures

**Question**: Design shows async SQLAlchemy but codebase uses sync. Should tests use existing sync patterns?

**Decision**: Use existing sync patterns.

**Rationale**:
- Consistency with current codebase (Database class uses sync SQLAlchemy)
- No need to introduce async complexity for testing
- Simpler fixture management

**Implementation Guidance**:
```python
# Keep using sync patterns as in existing database/connection.py
@pytest.fixture
def test_db():
    """Create test database with sync SQLAlchemy."""
    db = Database("sqlite:///:memory:")
    db.initialize()
    yield db
    # cleanup
```

### 3. Parallel Test Execution

**Question**: Should pytest-xdist be added for parallel test execution?

**Decision**: No, not for Phase 5b.

**Rationale**:
- Current test suite is small enough for sequential execution
- Avoids complexity of test isolation for parallel runs
- Can be added later if test execution time becomes a problem
- KISS principle - don't add infrastructure we don't need yet

**Implementation Guidance**:
- Run tests sequentially with standard pytest
- Document in test README that parallel execution can be considered for future optimization

### 4. Performance Benchmarks

**Question**: Should pytest-benchmark be added? How to handle potentially flaky CI tests?

**Decision**: No benchmarking in Phase 5b.

**Rationale**:
- Performance testing is not in Phase 5b scope
- Focus on functional correctness and security first
- Performance optimization is premature at this stage
- Can be added in a dedicated performance phase if needed

**Implementation Guidance**:
- Skip any performance-related tests for now
- Focus on correctness and security tests only

### 5. Coverage Thresholds

**Question**: Per-module thresholds aren't natively supported by coverage.py. What approach?

**Decision**: Use global threshold of 80% for Phase 5b.

**Rationale**:
- Simple to implement and verify
- coverage.py supports this natively with `fail_under`
- Per-module thresholds add unnecessary complexity
- 80% is a reasonable target for this phase

**Implementation Guidance**:
```ini
# In pyproject.toml
[tool.coverage.report]
fail_under = 80
```

### 6. Consent Flow Testing

**Question**: Design shows `/consent` with JSON but implementation is `/authorize/consent` with HTML forms. Which to follow?

**Decision**: Follow the actual implementation: `/authorize/consent` with HTML forms.

**Rationale**:
- Test the system as it actually works
- The design document was conceptual; implementation is authoritative
- HTML form testing is more realistic for IndieAuth flows

**Implementation Guidance**:
```python
def test_consent_form_submission():
    # POST to /authorize/consent with form data
    response = client.post(
        "/authorize/consent",
        data={
            "client_id": "...",
            "redirect_uri": "...",
            # ... other form fields
        }
    )
```

### 7. Fixtures Directory

**Question**: Create new `tests/fixtures/` or keep existing `conftest.py` pattern?

**Decision**: Keep existing `conftest.py` pattern.

**Rationale**:
- Consistency with current test structure
- pytest naturally discovers fixtures in conftest.py
- No need to introduce new patterns
- Can organize fixtures within conftest.py with clear sections

**Implementation Guidance**:
```python
# In tests/conftest.py, add new fixtures with clear sections:

# === Database Fixtures ===
@pytest.fixture
def test_database():
    """Test database fixture."""
    pass

# === Client Fixtures ===
@pytest.fixture
def registered_client():
    """Pre-registered client fixture."""
    pass

# === Authorization Fixtures ===
@pytest.fixture
def valid_auth_code():
    """Valid authorization code fixture."""
    pass
```

### 8. CI/CD Workflow

**Question**: Is GitHub Actions workflow in scope for Phase 5b?

**Decision**: No, CI/CD is out of scope for Phase 5b.

**Rationale**:
- Phase 5b focuses on test implementation, not deployment infrastructure
- CI/CD should be a separate phase with its own design
- Keeps Phase 5b scope manageable

**Implementation Guidance**:
- Focus only on making tests runnable via `pytest`
- Document test execution commands in tests/README.md
- CI/CD integration can come later

### 9. DNS Mocking

**Question**: Global patching vs dependency injection override (existing pattern)?

**Decision**: Use dependency injection override pattern (existing in codebase).

**Rationale**:
- Consistency with existing patterns (see get_database, get_verification_service)
- More explicit and controllable
- Easier to reason about in tests
- Avoids global state issues

**Implementation Guidance**:
```python
# Use FastAPI dependency override pattern
def test_with_mocked_dns():
    def mock_dns_service():
        service = Mock()
        service.resolve_txt.return_value = ["expected", "values"]
        return service

    app.dependency_overrides[get_dns_service] = mock_dns_service
    # run test
    app.dependency_overrides.clear()
```

### 10. HTTP Mocking

**Question**: Use `responses` library (for requests) or `respx` (for httpx)?

**Decision**: Neither - use unittest.mock for urllib.

**Rationale**:
- The codebase uses urllib.request (see HTMLFetcherService), not requests or httpx
- httpx is only in test dependencies, not used in production code
- Existing tests already mock urllib successfully
- No need to add new mocking libraries

**Implementation Guidance**:
```python
# Follow existing pattern from test_html_fetcher.py
@patch('gondulf.services.html_fetcher.urllib.request.urlopen')
def test_http_fetch(mock_urlopen):
    mock_response = MagicMock()
    mock_response.read.return_value = b"<html>...</html>"
    mock_urlopen.return_value = mock_response
    # test the fetch
```

## Summary of Decisions

1. **E2E Testing**: TestClient-based simulation (no browser automation)
2. **Database**: Sync SQLAlchemy (match existing patterns)
3. **Parallel Tests**: No (keep it simple)
4. **Benchmarks**: No (out of scope)
5. **Coverage**: Global 80% threshold
6. **Consent Endpoint**: `/authorize/consent` with HTML forms (match implementation)
7. **Fixtures**: Keep conftest.py pattern
8. **CI/CD**: Out of scope
9. **DNS Mocking**: Dependency injection pattern
10. **HTTP Mocking**: unittest.mock for urllib

## Implementation Priority

Focus on these test categories in order:
1. Integration tests for complete OAuth flows
2. Security tests for timing attacks and injection
3. Error handling tests
4. Edge case coverage

## Key Principle

**Simplicity and Consistency**: Every decision above favors simplicity and consistency with existing patterns over introducing new complexity. The goal is comprehensive testing that works with what we have, not a perfect test infrastructure.

CLARIFICATIONS PROVIDED: Phase 5b - Developer may proceed