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:

# 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:

# 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:

# 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:

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:

# 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:

# 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:

# 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