Fix flaky Windows tests by using function-scoped fixtures #235
Conversation
Changed DiskStore and MultiDiskStore test fixtures from session-scoped to function-scoped. This ensures each test gets a fresh cache instance with its own temporary directory, preventing SQLite locking conflicts that caused pytest-xdist worker crashes on Windows. Fixes #166 Co-authored-by: William Easton <strawgate@users.noreply.github.com>
📝 WalkthroughWalkthroughSession-scoped shared disk and multi-disk store fixtures were removed; store fixtures now create and yield per-test DiskStore or MultiDiskStore instances in temporary directories. Dependent fixtures (e.g., disk_cache) were updated to use the per-test store. DuckDB fixtures were simplified to directly return/yield DuckDBStore instances. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Pytest
participant TempDir
participant Store
rect rgba(211,228,205,0.5)
note over Pytest,Store: Old flow (session-scoped)
Test->>Pytest: request `disk_store` (session)
Pytest->>TempDir: create shared temp dir
TempDir-->>Pytest: path
Pytest->>Store: instantiate DiskStore(path) [shared]
Store-->>Pytest: shared store
Pytest->>Test: provide `disk_store` (reused across tests)
end
rect rgba(252,231,214,0.5)
note over Pytest,Store: New flow (per-test)
Test->>Pytest: request `store` (per-test)
Pytest->>TempDir: create temp dir (per-test)
TempDir-->>Pytest: path
Pytest->>Store: instantiate DiskStore(path) (isolated)
Store-->>Pytest: store instance
Pytest->>Test: provide `store` (unique per test)
end
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py (2)
key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@claude can we create the directory in another session fixture and use the store fixture still so that the same temp dir is used across all tests? |
|
Claude finished @strawgate's task —— View job Analysis CompleteI've tested the requested approach of using a session-scoped temp directory with function-scoped store fixtures. Unfortunately, this approach won't work correctly and would break the tests. The ProblemWhen tests share the same temp directory:
Why It FailsThe diskcache SQLite database persists in the temp directory. Even though we create a fresh Test ResultsWith session-scoped directory: With function-scoped directory (current PR): Current Solution is CorrectThe current PR implementation (function-scoped fixtures with fresh temp dirs) is the right approach because:
RecommendationKeep the current implementation. The filesystem overhead of creating separate temp directories per test is minimal and necessary for proper test isolation. The shared directory approach fundamentally breaks test isolation with persistent storage backends like SQLite. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Test Failure AnalysisSummary: The workflow run failed due to a pytest-xdist worker crash in Root Cause: The
Suggested Solution: Apply the same fixture pattern fix to the DuckDB tests that was applied to DiskStore tests. Files to Modify:
Required Changes:The Wait, let me re-examine... Looking more closely at the fixture: @pytest.fixture
async def store(self) -> AsyncGenerator[DuckDBStore, None]:
"""Test with persistent DuckDB database file."""
with TemporaryDirectory() as temp_dir:
db_path = Path(temp_dir) / "test.db"
duckdb_store = DuckDBStore(database_path=db_path)
yield duckdb_store
await duckdb_store.close()This fixture:
But it ALSO manually calls The Real Issue:The fixture shouldn't manually close the store because
Correct Fix:Remove the manual @pytest.fixture
async def store(self) -> AsyncGenerator[DuckDBStore, None]:
"""Test with persistent DuckDB database file."""
with TemporaryDirectory() as temp_dir:
db_path = Path(temp_dir) / "test.db"
duckdb_store = DuckDBStore(database_path=db_path)
yield duckdb_store
# ContextManagerStoreTestMixin handles closingThe same issue exists in Detailed AnalysisWorkflow Run Details
Failure Log ExcerptPattern ComparisonDiskStore (Fixed in this PR):
DuckDBStore (Still Broken):
Related Files
|
|
@claude can you address the PR feedback above? |
|
Claude finished @strawgate's task —— View job Addressing PR Feedback ✅I've successfully addressed the feedback about DuckDB tests having a similar double-close issue. SummaryThe DuckDB tests had a double-close issue similar to what this PR originally fixed for DiskStore:
Changes MadeFixed DuckDB test fixtures:
Verified no other issues:
CommitCommitted and pushed the fix in commit |
The DuckDB test fixtures were manually calling await store.close() after yielding, but ContextManagerStoreTestMixin.enter_exit_store already handles closing the store. This caused a double-close which could lead to errors, particularly on Windows. Changes: - Removed manual close() from TestDuckDBStore.store fixture - Removed manual close() from TestDuckDBStorePersistent.store fixture - Kept manual close() in TestDuckDBStoreSpecific.store (doesn't use mixin) - Simplified code to directly yield the store instance This follows the same pattern as the DiskStore and MultiDiskStore fixes in this PR. Co-authored-by: William Easton <strawgate@users.noreply.github.com>
Test Failure AnalysisSummary: Multiple CI checks are failing due to an incomplete fix in the DuckDB test fixtures. The previous commit (50b9522) incorrectly simplified the async fixtures by directly yielding the store constructor call without storing it in a variable first. Root Cause: The commit that "fixed" the DuckDB double-close issue made an error in the async fixtures: TestDuckDBStore fixture (lines 23-27): async def store(self) -> AsyncGenerator[DuckDBStore, None]:
"""Test with in-memory DuckDB database."""
yield DuckDBStore() # ← Problem: directly yielding constructor call
# ContextManagerStoreTestMixin handles closingTestDuckDBStorePersistent fixture (lines 37-43): async def store(self) -> AsyncGenerator[DuckDBStore, None]:
"""Test with persistent DuckDB database file."""
with TemporaryDirectory() as temp_dir:
db_path = Path(temp_dir) / "test.db"
yield DuckDBStore(database_path=db_path) # ← Problem: directly yielding
# ContextManagerStoreTestMixin handles closingThis pattern triggers Ruff lint rule PT022 (
Additionally, the fixtures need to keep the variable assignment so that:
Suggested Solution: Restore the variable assignment pattern in both fixtures: File: Change 1 (lines 23-27): @pytest.fixture
async def store(self) -> AsyncGenerator[DuckDBStore, None]:
"""Test with in-memory DuckDB database."""
duckdb_store = DuckDBStore()
yield duckdb_store
# ContextManagerStoreTestMixin handles closingChange 2 (lines 37-43): @pytest.fixture
async def store(self) -> AsyncGenerator[DuckDBStore, None]:
"""Test with persistent DuckDB database file."""
with TemporaryDirectory() as temp_dir:
db_path = Path(temp_dir) / "test.db"
duckdb_store = DuckDBStore(database_path=db_path)
yield duckdb_store
# ContextManagerStoreTestMixin handles closingThen run Why This Will Fix the Issues:
Detailed AnalysisFailed Checks
Error Log ExcerptsCodegen check failure: diff --git a/key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py
- yield DuckDBStore()
+ return DuckDBStore()
# ContextManagerStoreTestMixin handles closingType check error: Pattern ComparisonRocksDBStore (correct pattern): async def store(self) -> AsyncGenerator[RocksDBStore, None]:
with TemporaryDirectory() as temp_dir:
db_path = Path(temp_dir) / "test_db"
rocksdb_store = RocksDBStore(path=db_path) # ← Variable assignment
yield rocksdb_store # ← Yield variableDuckDBStore (current, incorrect): async def store(self) -> AsyncGenerator[DuckDBStore, None]:
with TemporaryDirectory() as temp_dir:
db_path = Path(temp_dir) / "test.db"
yield DuckDBStore(database_path=db_path) # ← Direct yieldRelated Files
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py (1)
23-29: Fix the return type annotation to match the return statement.The fixture correctly uses
returnsince there's no teardown logic, but the return type annotation isGenerator[DuckDBStore, None, None]. This type mismatch is causing the static analysis and pipeline failures.Apply this diff:
@override @pytest.fixture - def store(self) -> Generator[DuckDBStore, None, None]: + def store(self) -> DuckDBStore: """Test with in-memory DuckDB database.""" return DuckDBStore() # ContextManagerStoreTestMixin handles closingNote: Since this is an auto-generated file, you'll need to fix the source file (
key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py) and regenerate this file by runningmake codegen.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py(3 hunks)key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py (1)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (1)
DuckDBStore(62-380)
key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (1)
DuckDBStore(65-330)
🪛 GitHub Actions: Run Tests
key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py
[error] 22-22: Modified files detected after running 'make codegen lint'. Please run 'make codegen lint' locally and commit the changes. Change in test_duckdb.py: replace 'yield DuckDBStore()' with 'return DuckDBStore()'.
key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py
[error] 28-28: Type "DuckDBStore" is not assignable to return type "Generator[DuckDBStore, None, None]".
🪛 GitHub Check: static_analysis (key-value/key-value-sync)
key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py
[failure] 28-28:
Type "DuckDBStore" is not assignable to return type "Generator[DuckDBStore, None, None]"
"DuckDBStore" is incompatible with protocol "Generator[DuckDBStore, None, None]"
"next" is not present
"send" is not present
"throw" is not present
"iter" is not present (reportReturnType)
🔇 Additional comments (4)
key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py (2)
35-41: LGTM!The persistent fixture correctly uses
yieldbecause theTemporaryDirectorycontext manager must remain active during the test. The return type annotation is correct, and the cleanup delegation to the mixin is appropriate.
51-56: LGTM!The clarifying comment helps explain why this fixture explicitly closes the store. Since
TestDuckDBStoreSpecificdoes not useContextManagerStoreTestMixin, explicit cleanup is appropriate.key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py (2)
36-44: LGTM!The persistent fixture correctly uses
yieldto maintain theTemporaryDirectorycontext manager during the test. The return type annotation is correct, and the cleanup delegation to the mixin is appropriate.
54-59: LGTM!The clarifying comment explains why explicit cleanup is required. Since
TestDuckDBStoreSpecificdoes not inherit fromContextManagerStoreTestMixin, explicit store closure is appropriate.
| @pytest.fixture | ||
| async def store(self) -> AsyncGenerator[DuckDBStore, None]: | ||
| """Test with in-memory DuckDB database.""" | ||
| duckdb_store = DuckDBStore() | ||
| yield duckdb_store | ||
| await duckdb_store.close() | ||
| yield DuckDBStore() | ||
| # ContextManagerStoreTestMixin handles closing |
There was a problem hiding this comment.
Fix the fixture to use return and update the return type annotation.
The pipeline failure indicates that when a fixture has no teardown logic, it should use return instead of yield. Additionally, the return type annotation must change from AsyncGenerator[DuckDBStore, None] to just DuckDBStore.
Apply this diff:
@override
@pytest.fixture
- async def store(self) -> AsyncGenerator[DuckDBStore, None]:
+ async def store(self) -> DuckDBStore:
"""Test with in-memory DuckDB database."""
- yield DuckDBStore()
+ return DuckDBStore()
# ContextManagerStoreTestMixin handles closingCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Run Tests
[error] 22-22: Modified files detected after running 'make codegen lint'. Please run 'make codegen lint' locally and commit the changes. Change in test_duckdb.py: replace 'yield DuckDBStore()' with 'return DuckDBStore()'.
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py around lines 22 to
26, the pytest fixture currently uses an async generator signature and yield
even though there is no teardown logic; change the fixture to return a
DuckDBStore instance and update the type annotation from
AsyncGenerator[DuckDBStore, None] to DuckDBStore. Specifically, replace the
async generator fixture definition to an async def that returns DuckDBStore (use
return DuckDBStore()) and remove the yield and comment about teardown.
Test Failure AnalysisSummary: The workflow run failed with 1 test failure due to a pytest-xdist worker crash on . However, when the test was automatically retried by pytest-xdist, it passed. This intermittent flakiness on Windows is exactly what this PR aims to fix, but the crash indicates the fix may not be complete. Root Cause: Despite this PR's changes to use function-scoped fixtures with fresh temporary directories, the
The logs show: Then later when retried: Suggested Solution: The PR changes are correct, but
Options to resolve:
The fact that the test passes on retry suggests the issue is a timing/cleanup race condition, not a logic error in the test itself. Detailed AnalysisWorkflow Run Details
Test BehaviorThe async def test_not_unbounded(self, store: BaseStore):
"""Tests that the store is not unbounded."""
for i in range(1000):
value = hashlib.sha256(f"test_{i}".encode()).hexdigest()
await store.put(collection="test_collection", key=f"test_key_{i}", value={"test": value})
assert await store.get(collection="test_collection", key="test_key_0") is None
assert await store.get(collection="test_collection", key="test_key_999") is not NoneThis test:
Windows-Specific IssueThe
The Related Files
|
Test Failure AnalysisSummary: The workflow run failed with 1 test failure due to a pytest-xdist worker crash on Root Cause: Despite this PR's changes to use function-scoped fixtures with fresh temporary directories, the
The logs show: Then later when retried: Suggested Solution: The PR changes are correct, but
Options to resolve:
The fact that the test passes on retry suggests the issue is a timing/cleanup race condition, not a logic error in the test itself. Detailed AnalysisWorkflow Run Details
Test BehaviorThe async def test_not_unbounded(self, store: BaseStore):
"""Tests that the store is not unbounded."""
for i in range(1000):
value = hashlib.sha256(f"test_{i}".encode()).hexdigest()
await store.put(collection="test_collection", key=f"test_key_{i}", value={"test": value})
assert await store.get(collection="test_collection", key="test_key_0") is None
assert await store.get(collection="test_collection", key="test_key_999") is not NoneThis test:
Windows-Specific IssueThe
The Related Files
|
|
Summary
Fixed flaky Windows tests for DiskStore and MultiDiskStore by changing test fixtures from session-scoped to function-scoped.
Problem
Tests were randomly failing on Windows CI with pytest-xdist worker crashes:
replacing crashed worker gw1[gw2] node down: Not properly terminatedtest_not_unbounded[with_ctx_manager]failing intermittentlyRoot Cause
Session-scoped fixtures created ONE store instance for all tests, but
ContextManagerStoreTestMixinclosed the store after EACH test. This caused:Solution
Changed fixtures to function scope, creating a fresh cache instance with its own temporary directory for each test. This:
Fixes #166
Generated with Claude Code
Summary by CodeRabbit