Preserve interface for SQL store testing #279
Conversation
…ure/preserve-interface-for-sql-store-testing
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the test suite to support testing multiple store implementations (InMemory, SQL, etc.) by introducing a parameterized store_fixture that can be easily extended. The main changes prepare the codebase for future SQL store implementation.
- Introduced a parameterized
store_fixturein conftest.py that currently tests only InMemoryLightningStore but can be extended to include SQL stores - Updated all test functions to use the generic
store_fixtureparameter instead of the specificinmemory_storeparameter - Updated docstrings to reflect the broader scope of tests covering multiple implementations
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/store/test_implementation.py | Replaced all inmemory_store parameter references with generic store_fixture to enable testing across multiple store implementations |
| tests/store/test_client_server.py | Updated server_client fixture to use store_fixture parameter for flexibility |
| tests/store/conftest.py | Added parameterized store_fixture and placeholder sql_store fixture to support multiple store implementations |
Comments suppressed due to low confidence (1)
tests/store/conftest.py:15
- The
__all__list should be updated to include the new fixturesstore_fixtureandsql_storesince they are now part of the module's public API.
__all__ = [
"inmemory_store",
"mock_readable_span",
]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| @pytest.fixture | ||
| def sql_store(): |
There was a problem hiding this comment.
The sql_store fixture lacks a docstring explaining its purpose as a placeholder. Consider adding a docstring like \"\"\"Placeholder fixture for SQL store implementation. Returns None until SQL store is ready.\"\"\".
| def sql_store(): | |
| def sql_store(): | |
| """Placeholder fixture for SQL store implementation. Returns None until SQL store is ready.""" |
tests/store/conftest.py
Outdated
| # Uncomment this when sql store is ready | ||
| # @pytest.fixture(params=["inmemory_store", "sql_store"]) | ||
| @pytest.fixture(params=["inmemory_store"]) | ||
| def store_fixture(request: FixtureRequest): |
There was a problem hiding this comment.
The store_fixture function lacks a docstring. Consider adding documentation explaining its purpose as a parameterized fixture that enables testing across multiple store implementations, e.g., \"\"\"Parameterized fixture that provides different store implementations for testing. Currently supports InMemoryLightningStore, with SQL store support planned.\"\"\".
| def store_fixture(request: FixtureRequest): | |
| def store_fixture(request: FixtureRequest): | |
| """ | |
| Parameterized fixture that provides different store implementations for testing. | |
| Currently supports InMemoryLightningStore, with SQL store support planned. | |
| """ |
tests/store/conftest.py
Outdated
| # Uncomment this when sql store is ready | ||
| # @pytest.fixture(params=["inmemory_store", "sql_store"]) | ||
| @pytest.fixture(params=["inmemory_store"]) | ||
| def store_fixture(request: FixtureRequest): |
There was a problem hiding this comment.
Consider adding a type annotation for the return type of store_fixture. It should return LightningStore to match the type used throughout the test file.
| def store_fixture(request: FixtureRequest): | |
| def store_fixture(request: FixtureRequest) -> InMemoryLightningStore: |
|
/ci |
|
🚀 CI Watcher for correlation id-3491869159-mhm5dqe4 triggered by comment 3491869159
✅ All runs completed. |
No description provided.