docs: Update Phase 0 with specific test fix requirements
Per ADR-012, Phase 0 now specifies: - 5 tests to REMOVE (broken multiprocessing) - 4 tests to FIX (brittle assertions) - 1 test to RENAME (misleading name) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
181
docs/decisions/ADR-012-flaky-test-removal.md
Normal file
181
docs/decisions/ADR-012-flaky-test-removal.md
Normal file
@@ -0,0 +1,181 @@
|
||||
# ADR-012: Flaky Test Removal and Test Quality Standards
|
||||
|
||||
## Status
|
||||
Proposed
|
||||
|
||||
## Context
|
||||
|
||||
The test suite contains several categories of flaky tests that pass inconsistently. These tests consume developer time without providing proportional value. Per the project philosophy ("Every line of code must justify its existence"), we must evaluate whether these tests should be kept, fixed, or removed.
|
||||
|
||||
## Analysis by Test Category
|
||||
|
||||
### 1. Migration Race Condition Tests (`test_migration_race_condition.py`)
|
||||
|
||||
**Failing Tests:**
|
||||
- `test_debug_level_for_early_retries` - Log message matching
|
||||
- `test_new_connection_per_retry` - Connection count assertions
|
||||
- `test_concurrent_workers_barrier_sync` - Multiprocessing pickle errors
|
||||
- `test_sequential_worker_startup` - Missing table errors
|
||||
- `test_worker_late_arrival` - Missing table errors
|
||||
- `test_single_worker_performance` - Missing table errors
|
||||
- `test_concurrent_workers_performance` - Pickle errors
|
||||
|
||||
**Value Analysis:**
|
||||
- The migration retry logic with exponential backoff is *critical* for production deployments with multiple Gunicorn workers
|
||||
- However, the flaky tests are testing implementation details (log levels, exact connection counts) rather than behavior
|
||||
- The multiprocessing tests fundamentally cannot work reliably because:
|
||||
1. `multiprocessing.Manager().Barrier()` objects cannot be pickled for `Pool.map()`
|
||||
2. The worker functions require Flask app context that doesn't transfer across processes
|
||||
3. SQLite database files in temp directories may not be accessible across process boundaries
|
||||
|
||||
**Root Cause:** Test design is flawed. These are attempting integration/stress tests using unit test infrastructure.
|
||||
|
||||
**Recommendation: REMOVE the multiprocessing tests entirely. KEEP and FIX the unit tests.**
|
||||
|
||||
Specifically:
|
||||
- **REMOVE:** `TestConcurrentExecution` class (all 3 tests) - fundamentally broken by design
|
||||
- **REMOVE:** `TestPerformance` class (both tests) - same multiprocessing issues
|
||||
- **KEEP:** `TestRetryLogic` - valuable, just needs mock fixes
|
||||
- **KEEP:** `TestGraduatedLogging` - valuable, needs logger configuration fixes
|
||||
- **KEEP:** `TestConnectionManagement` - valuable, needs assertion fixes
|
||||
- **KEEP:** `TestErrorHandling` - valuable, tests critical rollback behavior
|
||||
- **KEEP:** `TestBeginImmediateTransaction` - valuable, tests locking mechanism
|
||||
|
||||
**Rationale for removal:** If we need to test concurrent migration behavior, that requires:
|
||||
1. A proper integration test framework (not pytest unit tests)
|
||||
2. External process spawning (not multiprocessing.Pool)
|
||||
3. Real filesystem isolation
|
||||
4. This is out of scope for V1 - the code works; the tests are the problem
|
||||
|
||||
---
|
||||
|
||||
### 2. Feed Route Tests (`test_routes_feeds.py`)
|
||||
|
||||
**Failing Assertions:**
|
||||
- Tests checking for exact `<?xml version="1.0"` but code produces `<?xml version='1.0'` (single quotes)
|
||||
- Tests checking for exact Content-Type with charset but response may vary
|
||||
- Tests checking for exact `<rss version="2.0"` string
|
||||
|
||||
**Value Analysis:**
|
||||
- These tests ARE valuable - they verify feed output format
|
||||
- The tests are NOT flaky per se; they are *brittle* due to over-specific assertions
|
||||
|
||||
**Root Cause:** Tests are asserting implementation details (quote style) rather than semantics (valid XML).
|
||||
|
||||
**Recommendation: FIX by loosening assertions**
|
||||
|
||||
Current (brittle):
|
||||
```python
|
||||
assert b'<?xml version="1.0"' in response.data
|
||||
```
|
||||
|
||||
Better (semantic):
|
||||
```python
|
||||
assert b'<?xml version=' in response.data
|
||||
assert b"encoding=" in response.data
|
||||
```
|
||||
|
||||
The test file already has SOME tests using the correct pattern (lines 72, 103, 265). The ATOM test on line 84 is the outlier - it should match the RSS tests.
|
||||
|
||||
---
|
||||
|
||||
### 3. Feed Streaming Test (`test_routes_feed.py`)
|
||||
|
||||
**Failing Test:** `test_feed_route_streaming`
|
||||
|
||||
**Current assertion (line 124):**
|
||||
```python
|
||||
assert "ETag" in response.headers
|
||||
```
|
||||
|
||||
**But the test comment says:**
|
||||
```python
|
||||
# Cached responses include ETags for conditional requests
|
||||
# (Phase 3 caching was added, replacing streaming for better performance)
|
||||
```
|
||||
|
||||
**Value Analysis:**
|
||||
- The test title says "streaming" but the implementation uses caching
|
||||
- The test is actually correct (ETag SHOULD be present)
|
||||
- If ETag is NOT present, that's a bug in the feed caching implementation
|
||||
|
||||
**Root Cause:** This is not a flaky test - if it fails, there's an actual bug. The test name is misleading but the assertion is correct.
|
||||
|
||||
**Recommendation: KEEP and RENAME to `test_feed_route_caching`**
|
||||
|
||||
---
|
||||
|
||||
### 4. Search Security Tests (`test_search_security.py`)
|
||||
|
||||
**Analysis of the file:** After reviewing, I see no obviously flaky tests. All tests are:
|
||||
- Testing XSS prevention (correct)
|
||||
- Testing SQL injection prevention (correct)
|
||||
- Testing input validation (correct)
|
||||
- Testing pagination limits (correct)
|
||||
|
||||
**Possible flakiness sources:**
|
||||
- FTS5 special character handling varies by SQLite version
|
||||
- Tests that accept multiple status codes (200, 400, 500) are defensive, not flaky
|
||||
|
||||
**Recommendation: KEEP all tests**
|
||||
|
||||
If specific flakiness is identified, it's likely due to SQLite FTS5 version differences, which should be documented rather than the tests removed.
|
||||
|
||||
---
|
||||
|
||||
## Decision
|
||||
|
||||
### Remove Entirely
|
||||
1. `TestConcurrentExecution` class from `test_migration_race_condition.py`
|
||||
2. `TestPerformance` class from `test_migration_race_condition.py`
|
||||
|
||||
### Fix Tests (Developer Action Items)
|
||||
1. **`test_routes_feeds.py` line 84:** Change `assert b'<?xml version="1.0"'` to `assert b'<?xml version='`
|
||||
2. **`test_routes_feed.py` line 117:** Rename test from `test_feed_route_streaming` to `test_feed_route_caching`
|
||||
3. **`test_migration_race_condition.py`:** Fix logger configuration in `TestGraduatedLogging` tests to ensure DEBUG level is captured
|
||||
4. **`test_migration_race_condition.py`:** Fix mock setup in `test_new_connection_per_retry` to accurately count connection attempts
|
||||
|
||||
### Keep As-Is
|
||||
1. All tests in `test_search_security.py`
|
||||
2. All non-multiprocessing tests in `test_migration_race_condition.py` (after fixes)
|
||||
3. All other tests in `test_routes_feeds.py` and `test_routes_feed.py`
|
||||
|
||||
## Rationale
|
||||
|
||||
1. **Project philosophy alignment:** Tests that cannot reliably pass do not justify their existence. They waste developer time and erode confidence in the test suite.
|
||||
|
||||
2. **Pragmatic approach:** The migration concurrency code is tested in production by virtue of running with multiple Gunicorn workers. Manual testing during deployment is more reliable than broken multiprocessing tests.
|
||||
|
||||
3. **Test semantics over implementation:** Tests should verify behavior, not implementation details like quote styles in XML.
|
||||
|
||||
4. **Maintainability:** A smaller, reliable test suite is better than a larger, flaky one.
|
||||
|
||||
## Consequences
|
||||
|
||||
### Positive
|
||||
- Faster, more reliable CI/CD pipeline
|
||||
- Increased developer confidence in test results
|
||||
- Reduced time spent debugging test infrastructure
|
||||
- Tests that fail actually indicate bugs
|
||||
|
||||
### Negative
|
||||
- Reduced test coverage for concurrent migration scenarios
|
||||
- Manual testing required for multi-worker deployments
|
||||
|
||||
### Mitigations
|
||||
- Document the multi-worker testing procedure in deployment docs
|
||||
- Consider adding integration tests in a separate test category (not run in CI) for concurrent scenarios
|
||||
|
||||
## Alternatives Considered
|
||||
|
||||
### 1. Fix the multiprocessing tests
|
||||
**Rejected:** Would require significant refactoring to use subprocess spawning instead of multiprocessing.Pool. The complexity is not justified for V1 given the code works correctly in production.
|
||||
|
||||
### 2. Mark tests as `@pytest.mark.skip`
|
||||
**Rejected:** Skipped tests are just noise. They either work and should run, or they don't work and should be removed. "Skip" is procrastination.
|
||||
|
||||
### 3. Use pytest-xdist for parallel testing
|
||||
**Rejected:** Does not solve the fundamental issue of needing to spawn external processes with proper app context.
|
||||
|
||||
### 4. Move to integration test framework (e.g., testcontainers)
|
||||
**Considered for future:** This is the correct long-term solution but is out of scope for V1. Should be considered for V2 if concurrent migration testing is deemed critical.
|
||||
@@ -22,27 +22,42 @@ v1.5.0 is a quality-focused release that addresses failing tests, increases test
|
||||
**Priority**: Must complete first - unblocks all other phases
|
||||
|
||||
#### Scope
|
||||
Fix the 19 failing tests identified in the current test suite:
|
||||
Address flaky and broken tests per ADR-012 (Flaky Test Removal):
|
||||
|
||||
| Category | Count | Tests |
|
||||
|----------|-------|-------|
|
||||
| Migration Performance | 2 | `test_single_worker_performance`, `test_concurrent_workers_performance` |
|
||||
| Feed Route (Streaming) | 1 | `test_feed_route_streaming` |
|
||||
| Feed Endpoints | 3 | `test_feed_rss_endpoint`, `test_feed_json_endpoint`, `test_feed_xml_legacy_endpoint` |
|
||||
| Content Negotiation | 6 | `test_accept_rss`, `test_accept_json_feed`, `test_accept_json_generic`, `test_accept_wildcard`, `test_no_accept_header`, `test_quality_factor_json_wins` |
|
||||
| Backward Compatibility | 1 | `test_feed_xml_contains_rss` |
|
||||
| Search Security | 1 | `test_search_escapes_html_in_note_content` |
|
||||
**REMOVE (5 tests)** - Architecturally broken multiprocessing tests:
|
||||
| Test | Reason |
|
||||
|------|--------|
|
||||
| `test_concurrent_workers_barrier_sync` | Cannot pickle Barrier objects |
|
||||
| `test_sequential_worker_startup` | Missing Flask app context |
|
||||
| `test_worker_late_arrival` | Missing Flask app context |
|
||||
| `test_single_worker_performance` | Cannot pickle local functions |
|
||||
| `test_concurrent_workers_performance` | Cannot pickle local functions |
|
||||
|
||||
**FIX (4 tests)** - Valuable tests needing adjustments:
|
||||
| Test | Fix Required |
|
||||
|------|--------------|
|
||||
| `test_debug_level_for_early_retries` | Configure logger level in test |
|
||||
| `test_new_connection_per_retry` | Adjust assertion count |
|
||||
| Feed XML tests | Change `<?xml version="1.0"` to `<?xml version=` (don't assert quote style) |
|
||||
| `test_feed_json_endpoint` | Don't require charset in Content-Type |
|
||||
|
||||
**RENAME (1 test)**:
|
||||
| Test | New Name |
|
||||
|------|----------|
|
||||
| `test_feed_route_streaming` | `test_feed_route_caching` (test is correct, name misleading) |
|
||||
|
||||
#### Approach
|
||||
1. Investigate each failing test category
|
||||
2. Determine if failure is test issue or code issue
|
||||
3. Fix appropriately (prefer fixing tests over changing working code)
|
||||
4. Document any behavioral changes
|
||||
1. Remove the 5 broken multiprocessing tests (they cannot work due to Python limitations)
|
||||
2. Fix the brittle feed assertion tests (check semantics, not quote style)
|
||||
3. Fix the 4 migration tests that have value but need mock/assertion adjustments
|
||||
4. Rename misleading test
|
||||
5. Document changes in implementation report
|
||||
|
||||
#### Acceptance Criteria
|
||||
- [ ] All 879 tests pass
|
||||
- [ ] No test skips added (unless justified)
|
||||
- [ ] No test timeouts
|
||||
- [ ] All remaining tests pass consistently (run 3x to verify no flakiness)
|
||||
- [ ] 5 broken tests removed with justification in ADR-012
|
||||
- [ ] No new test skips added
|
||||
- [ ] Test count reduced from 879 to 874
|
||||
|
||||
#### Dependencies
|
||||
None - this is the first phase
|
||||
|
||||
Reference in New Issue
Block a user