Restructure tests into unit, integration, and e2e tiers #5
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughReorganizes tests into unit/integration/e2e tiers, updates CI test targets to the new locations, replaces legacy test helpers with modular helpers/fixtures, adds provider-specific integration fixtures and placeholders, adjusts pytest markers/docs, and removes an obsolete MCP debug doc. No production API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI Runner
participant Collector as Pytest Discovery
participant IntConftest as tests/integration/conftest.py
participant Fixtures as fixtures_services.graph_driver
participant Services as tests.helpers.services
participant Driver as GraphDriver
CI->>Collector: discover tests (unit/integration/e2e)
Collector->>IntConftest: load integration conftest
IntConftest->>Fixtures: request graph_driver fixture
Fixtures->>Services: available_providers()
alt provider available
Fixtures->>Services: prepare_driver(provider)
Services->>Driver: instantiate & clear test groups
Services-->>Fixtures: return driver
Fixtures-->>Collector: yield driver to tests
Collector-->>Fixtures: tests complete
Fixtures->>Services: teardown_driver(driver)
Services->>Driver: close()
else no provider
Fixtures-->>Collector: skip tests (no provider)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
tests/unit/search/test_lucene_utils.py (1)
7-17: Consider using@pytest.mark.parametrizefor better test isolation.The current loop-based approach works, but using
@pytest.mark.parametrizewould provide clearer test names and better failure reporting for each case.Apply this diff to refactor using parametrize:
+@pytest.mark.parametrize( + 'query,expected', + [ + ( + 'This has every escape character + - && || ! ( ) { } [ ] ^ " ~ * ? : \\ /', + '\\This has every escape character \\+ \\- \\&\\& \\|\\| \\! \\( \\) \\{ \\} \\[ \\] \\^ \\" \\~ \\* \\? \\: \\\\ \\/', + ), + ('this has no escape characters', 'this has no escape characters'), + ], +) -def test_lucene_sanitize(): - queries = [ - ( - 'This has every escape character + - && || ! ( ) { } [ ] ^ " ~ * ? : \\ /', - '\\This has every escape character \\+ \\- \\&\\& \\|\\| \\! \\( \\) \\{ \\} \\[ \\] \\^ \\" \\~ \\* \\? \\: \\\\ \\/', - ), - ('this has no escape characters', 'this has no escape characters'), - ] - - for query, expected in queries: - assert lucene_sanitize(query) == expected +def test_lucene_sanitize(query, expected): + assert lucene_sanitize(query) == expectedtests/helpers/embeddings.py (1)
18-46: Consider setting a numpy seed for deterministic embeddings.The embeddings are generated with
np.random.uniform()without a seed, which means they'll differ across test runs. If tests rely on consistent embedding values (beyond just shape/presence), this could cause non-deterministic behavior.Apply this diff to add deterministic seeding:
def default_embeddings() -> dict[str, list[float]]: + # Set seed for deterministic test embeddings + rng = np.random.Generator(np.random.PCG64(42)) keys = [ 'Alice', 'Bob', ... ] embeddings = { - key: np.random.uniform(0.0, 0.9, EMBEDDING_DIM).tolist() for key in keys + key: rng.uniform(0.0, 0.9, EMBEDDING_DIM).tolist() for key in keys } embeddings['Alice Smith'] = embeddings['Alice'] return embeddingstests/integration/conftest.py (1)
6-6: Consider removing thenoqadirective.Ruff indicates that
F401is not enabled in your configuration, making thenoqa: F401directive unnecessary for Ruff. If you're not using other linters (like flake8) that would flag this import, you can safely remove the directive.Apply this diff if Ruff is your only linter:
-from tests.integration.shared.fixtures_services import graph_driver # noqa: F401 +from tests.integration.shared.fixtures_services import graph_driver
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.github/workflows/ci.yml(1 hunks)MCP_SMOKE_DEBUG.md(0 hunks)TEST_RESTRUCTURE_PLAN.md(1 hunks)conftest.py(2 hunks)docs/testing.md(3 hunks)pytest.ini(1 hunks)tests/driver/__init__.py(0 hunks)tests/e2e/graph/eval_cli.py(1 hunks)tests/e2e/graph/eval_graph_building.py(6 hunks)tests/e2e/graph/test_eval_harness.py(1 hunks)tests/evals/pytest.ini(0 hunks)tests/helpers/embeddings.py(1 hunks)tests/helpers/services.py(1 hunks)tests/helpers_test.py(0 hunks)tests/integration/conftest.py(1 hunks)tests/integration/core/shared/test_entity_exclusion.py(5 hunks)tests/integration/core/shared/test_graphium_bootstrap.py(1 hunks)tests/integration/core/shared/test_ingestion_pipeline.py(2 hunks)tests/integration/core/shared/test_repository_edges.py(18 hunks)tests/integration/core/shared/test_repository_nodes.py(9 hunks)tests/integration/falkordb/test_placeholder.py(1 hunks)tests/integration/kuzu/test_placeholder.py(1 hunks)tests/integration/neo4j/test_placeholder.py(1 hunks)tests/integration/shared/fixtures_services.py(1 hunks)tests/unit/embedder/test_gemini.py(1 hunks)tests/unit/embedder/test_openai.py(1 hunks)tests/unit/embedder/test_voyage.py(1 hunks)tests/unit/search/test_lucene_utils.py(1 hunks)
💤 Files with no reviewable changes (4)
- tests/driver/init.py
- MCP_SMOKE_DEBUG.md
- tests/helpers_test.py
- tests/evals/pytest.ini
🧰 Additional context used
📓 Path-based instructions (4)
tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Author tests under tests/ alongside features
Files:
tests/integration/kuzu/test_placeholder.pytests/integration/falkordb/test_placeholder.pytests/unit/embedder/test_gemini.pytests/e2e/graph/eval_cli.pytests/unit/embedder/test_voyage.pytests/integration/core/shared/test_repository_nodes.pytests/e2e/graph/eval_graph_building.pytests/integration/core/shared/test_graphium_bootstrap.pytests/integration/conftest.pytests/helpers/services.pytests/integration/shared/fixtures_services.pytests/integration/neo4j/test_placeholder.pytests/integration/core/shared/test_repository_edges.pytests/e2e/graph/test_eval_harness.pytests/unit/embedder/test_openai.pytests/unit/search/test_lucene_utils.pytests/integration/core/shared/test_entity_exclusion.pytests/helpers/embeddings.pytests/integration/core/shared/test_ingestion_pipeline.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indentation, 100-character line length, and prefer single quotes for Python code (as configured in pyproject.toml)
Use snake_case for module, file, and function names in Python
Files:
tests/integration/kuzu/test_placeholder.pytests/integration/falkordb/test_placeholder.pyconftest.pytests/unit/embedder/test_gemini.pytests/e2e/graph/eval_cli.pytests/unit/embedder/test_voyage.pytests/integration/core/shared/test_repository_nodes.pytests/e2e/graph/eval_graph_building.pytests/integration/core/shared/test_graphium_bootstrap.pytests/integration/conftest.pytests/helpers/services.pytests/integration/shared/fixtures_services.pytests/integration/neo4j/test_placeholder.pytests/integration/core/shared/test_repository_edges.pytests/e2e/graph/test_eval_harness.pytests/unit/embedder/test_openai.pytests/unit/search/test_lucene_utils.pytests/integration/core/shared/test_entity_exclusion.pytests/helpers/embeddings.pytests/integration/core/shared/test_ingestion_pipeline.py
conftest.py
📄 CodeRabbit inference engine (AGENTS.md)
Pytest configuration should reside in repository-level conftest.py
Files:
conftest.py
pytest.ini
📄 CodeRabbit inference engine (AGENTS.md)
Maintain pytest configuration in pytest.ini at the repo root
Files:
pytest.ini
🧠 Learnings (2)
📚 Learning: 2025-10-11T16:59:48.957Z
Learnt from: CR
PR: CaliLuke/graphium#0
File: AGENTS.md:0-0
Timestamp: 2025-10-11T16:59:48.957Z
Learning: Applies to tests/*_int.py : Integration tests live in tests/*_int.py and must be marked with pytest.mark.integration
Applied to files:
docs/testing.mdpytest.ini
📚 Learning: 2025-10-11T16:59:48.957Z
Learnt from: CR
PR: CaliLuke/graphium#0
File: AGENTS.md:0-0
Timestamp: 2025-10-11T16:59:48.957Z
Learning: Use pytest.mark.integration to gate database-reliant scenarios in CI
Applied to files:
pytest.ini
🧬 Code graph analysis (15)
conftest.py (1)
tests/helpers/embeddings.py (1)
make_mock_embedder(49-61)
tests/unit/embedder/test_gemini.py (1)
tests/unit/embedder/embedder_fixtures.py (1)
create_embedding_values(5-7)
tests/e2e/graph/eval_cli.py (1)
tests/e2e/graph/eval_graph_building.py (2)
build_baseline_graph(98-115)eval_graph(118-173)
tests/unit/embedder/test_voyage.py (1)
tests/unit/embedder/embedder_fixtures.py (1)
create_embedding_values(5-7)
tests/integration/core/shared/test_repository_nodes.py (2)
tests/helpers/services.py (4)
assert_community_node_equals(204-215)assert_entity_node_equals(188-201)assert_episodic_node_equals(218-227)get_node_count(160-169)graphium_core/repository/node_repository.py (1)
delete_by_group_id(110-166)
tests/e2e/graph/eval_graph_building.py (1)
graphium_core/settings.py (1)
Neo4jSettings(277-280)
tests/integration/core/shared/test_graphium_bootstrap.py (1)
graphium_core/driver/driver.py (1)
GraphProvider(32-36)
tests/integration/conftest.py (1)
tests/integration/shared/fixtures_services.py (1)
graph_driver(30-39)
tests/helpers/services.py (7)
graphium_core/driver/driver.py (3)
GraphDriver(70-119)GraphProvider(32-36)repositories(115-119)graphium_core/edges.py (3)
CommunityEdge(111-112)EntityEdge(47-108)EpisodicEdge(43-44)graphium_core/nodes.py (3)
CommunityNode(152-163)EntityNode(91-149)EpisodicNode(78-88)graphium_core/orchestration/maintenance/graph_data_operations.py (1)
clear_data(218-242)graphium_core/settings.py (4)
FalkorSettings(283-290)KuzuSettings(299-300)Neo4jSettings(277-280)NeptuneSettings(293-296)graphium_core/repository/graph_repository.py (4)
delete_node(65-66)delete_edge(75-76)save_node(62-63)save_edge(72-73)graphium_core/repository/edge_repository.py (1)
load_fact_embedding(421-452)
tests/integration/shared/fixtures_services.py (2)
graphium_core/driver/driver.py (1)
GraphProvider(32-36)tests/helpers/services.py (3)
available_providers(56-72)prepare_driver(109-113)teardown_driver(116-117)
tests/integration/core/shared/test_repository_edges.py (2)
tests/helpers/services.py (2)
get_edge_count(172-185)get_node_count(160-169)graphium_core/repository/node_repository.py (1)
get_by_entity_node_uuid(360-376)
tests/unit/embedder/test_openai.py (1)
tests/unit/embedder/embedder_fixtures.py (1)
create_embedding_values(5-7)
tests/integration/core/shared/test_entity_exclusion.py (2)
tests/integration/shared/fixtures_services.py (1)
graph_driver(30-39)graphium_core/graphium.py (1)
Graphium(40-235)
tests/helpers/embeddings.py (3)
graphium_core/embedder/client.py (1)
EmbedderClient(18-26)tests/unit/orchestration/test_initializer_factory.py (1)
create(68-69)tests/unit/search/test_search_helpers.py (4)
create(26-27)create(34-35)create(43-44)create(97-99)
tests/integration/core/shared/test_ingestion_pipeline.py (2)
graphium_core/driver/driver.py (1)
GraphProvider(32-36)tests/helpers/services.py (7)
assert_entity_edge_equals(230-248)assert_entity_node_equals(188-201)assert_episodic_edge_equals(251-256)assert_episodic_node_equals(218-227)delete_all(149-157)get_edge_count(172-185)get_node_count(160-169)
🪛 GitHub Actions: CI
tests/integration/shared/fixtures_services.py
[error] 25-25: pytest: No graph providers enabled for integration tests (Skipped by fixture).
🪛 markdownlint-cli2 (0.18.1)
TEST_RESTRUCTURE_PLAN.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
61-61: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.13.3)
tests/integration/conftest.py
6-6: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
tests/helpers/services.py
106-106: Avoid specifying long messages outside the exception class
(TRY003)
146-146: Avoid specifying long messages outside the exception class
(TRY003)
157-157: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (32)
tests/unit/embedder/test_gemini.py (1)
4-4: LGTM!The test path comment and relative import correctly reflect the new test structure under
tests/unit/embedder/.Also applies to: 11-12
tests/unit/embedder/test_voyage.py (1)
12-13: LGTM!The relative import correctly aligns with the restructured test layout under
tests/unit/embedder/.docs/testing.md (3)
21-23: LGTM!The test layout description clearly documents the three-tier structure (unit/integration/e2e) with appropriate marker annotations.
53-53: LGTM!The test commands accurately reflect the new paths and marker-based filtering for unit, integration, and e2e tiers.
Also applies to: 59-66
95-96: LGTM!The example paths correctly point to the new integration test structure.
tests/e2e/graph/eval_graph_building.py (2)
6-6: LGTM!Using
Path(__file__).parentforBASE_DIRand loading Neo4j settings dynamically improves portability and aligns with the project's settings pattern.Also applies to: 16-24
72-73: Data file verifiedThe required JSON is present at tests/e2e/graph/data/longmemeval_data/longmemeval_oracle.json.
tests/e2e/graph/eval_cli.py (1)
4-4: LGTM!The import path correctly reflects the relocation of evaluation utilities to
tests/e2e/graph/.tests/integration/core/shared/test_graphium_bootstrap.py (1)
9-9: LGTM!Importing
GraphProviderfrom its canonical location ingraphium_core.driver.driverremoves unnecessary indirection and is the correct practice.tests/helpers/embeddings.py (1)
49-61: Incomplete input type handling in mock embedder.The
mock_embedfunction handlesstrandIterable[str], butEmbedderClient.createalso acceptsIterable[int]andIterable[Iterable[int]]according to the interface. If tests pass integer iterables, the mock will fail.If integer inputs are expected in tests, extend the mock:
def mock_embed(input_data: str | Iterable[str]) -> list[float]: if isinstance(input_data, str): return store[input_data] + # Handle list of ints or nested iterables by converting to string + if hasattr(input_data, '__iter__') and not isinstance(input_data, str): + try: + combined_input = ' '.join(str(x) for x in input_data) + return store.get(combined_input, store['Alice']) # fallback + except Exception: + return store['Alice'] # fallback for unexpected types - combined_input = ' '.join(input_data) - return store[combined_input]Alternatively, if integer inputs are never used in practice, this can remain as-is.
Likely an incorrect or invalid review comment.
pytest.ini (1)
3-8: LGTM!The expanded marker system provides clear test categorization and enables selective execution by provider or test type. The descriptions are helpful and align well with the test restructuring objectives.
tests/integration/neo4j/test_placeholder.py (1)
7-10: LGTM!The placeholder test is properly marked with
integrationandneo4jmarkers and clearly communicates the pending work. This approach enables the test infrastructure to be in place while implementation is in progress.tests/unit/embedder/test_openai.py (1)
13-13: LGTM!The change from absolute to relative import is appropriate and cleaner for a module in the same directory.
tests/integration/falkordb/test_placeholder.py (1)
7-10: LGTM!The placeholder test follows the established pattern and is properly marked with
integrationandfalkordbmarkers for selective execution.tests/e2e/graph/test_eval_harness.py (1)
7-9: LGTM!The e2e placeholder is properly marked and clearly communicates the requirements for running this test suite.
tests/integration/core/shared/test_repository_nodes.py (3)
15-21: LGTM!The refactoring to use a shared
GROUP_IDconstant improves test consistency and aligns with the helpers module reorganization.
34-34: LGTM!The fixtures now use the shared
GROUP_IDconstant consistently.Also applies to: 51-51, 66-66
95-95: LGTM!All test method calls have been updated to use the shared
GROUP_IDconstant consistently.Also applies to: 117-117, 145-145, 166-166, 194-194, 215-215
tests/integration/kuzu/test_placeholder.py (1)
7-10: LGTM!The placeholder test follows the established pattern and is properly marked with
integrationandkuzumarkers.conftest.py (1)
42-44: Fixture reuse looks great.Providing
mock_embeddervia conftest keeps the test surface consistent across tiers and removes the import boilerplate. Nice cleanup.tests/integration/core/shared/test_repository_edges.py (1)
11-92: Consistent GROUP_ID usage is a win.Switching everything over to the shared
GROUP_IDhelper keeps the fixtures and assertions aligned with the rest of the integration suite. Thanks for normalizing this..github/workflows/ci.yml (1)
44-45: Targeted CI commands look good.Running the fast unit tier plus the focused ingestion smoke matches the restructure plan and keeps the workflow lean.
tests/integration/core/shared/test_ingestion_pipeline.py (1)
47-57: Nice consolidation of shared helpers.Re-routing the constants and helper assertions through
tests.helpers.serviceskeeps the ingestion suite aligned with the new helper module.tests/integration/core/shared/test_entity_exclusion.py (1)
82-347: Great adoption of the shared graph_driver fixture.Swapping the bespoke driver plumbing for the common fixture simplifies the suite and keeps it aligned with the new integration scaffold.
tests/helpers/services.py (8)
56-73: LGTM: Clean lazy provider detection.The pattern of checking environment flags and lazily importing provider drivers keeps the module lightweight and avoids import-time failures when optional dependencies are missing. The logic correctly enables providers when their
DISABLE_*flag is not set.
75-107: Driver instantiation logic is correct.The function correctly handles all four supported providers (Neo4j, FalkorDB, Kuzu, Neptune) and loads their settings appropriately.
Note: The static analysis hint (TRY003) about the error message on line 106 is acceptable for test helper code where readability is prioritized.
109-118: LGTM: Clean driver lifecycle management.Both functions properly handle driver setup and teardown. The
prepare_driverfunction ensures a clean state by clearing test groups before tests run.
120-158: LGTM: Comprehensive CRUD helpers.The functions provide both type-specific operations (
save_nodes,save_edges) and polymorphic helpers (save_all,delete_all) that handle multiple node and edge types. This gives test authors flexibility in how they manage test data.Note: Static analysis hints (TRY003) on lines 146 and 157 are acceptable for test helper code where explicit error messages aid debugging.
160-186: LGTM: Provider-aware counting queries.The
get_edge_countfunction correctly handles Kuzu's special edge representation (RelatesToNode_) by using aUNION ALLquery. Both count functions properly convert iterable UUIDs to lists for the Cypher query.
188-202: LGTM: Thorough entity node assertions.The function validates all critical properties including UUID, name, labels, embeddings (with
np.allclosefor floating-point comparison), and attributes. Properly loads embeddings before comparison.
230-257: LGTM: Comprehensive edge assertions.Both entity and episodic edge assertion functions properly validate all critical properties. The entity edge function correctly loads embeddings and uses
np.allclosefor floating-point comparison. Temporal properties and attributes are thoroughly checked.
259-280: LGTM: Clean lazy import pattern.The resolver functions properly import provider drivers on-demand, avoiding import-time failures when optional dependencies are unavailable. This pattern keeps the test helpers lightweight and allows selective provider testing.
|
|
||
| load_dotenv() | ||
|
|
||
| os.environ.setdefault('DISABLE_NEPTUNE', 'True') |
There was a problem hiding this comment.
Reconsider hardcoding DISABLE_NEPTUNE at import time.
Setting this environment variable as a module-level side effect may interfere with tests that want to explicitly enable Neptune. Since _provider_enabled already checks for the absence of the flag, tests can control provider availability through their own environment setup or fixtures.
Consider removing this line and documenting that Neptune is disabled by default unless explicitly enabled:
-os.environ.setdefault('DISABLE_NEPTUNE', 'True')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| os.environ.setdefault('DISABLE_NEPTUNE', 'True') |
🤖 Prompt for AI Agents
In tests/helpers/services.py at line 35, remove the module-level side effect
that sets os.environ.setdefault('DISABLE_NEPTUNE', 'True'); instead delete that
line so tests can control Neptune via their own environment setup or fixtures
before importing this module, and add a brief comment or module docstring
stating that Neptune is disabled by default unless tests explicitly set or unset
DISABLE_NEPTUNE (and examples for enabling it via test fixtures or environment
variables).
| def _integration_providers() -> list[GraphProvider]: | ||
| providers = services.available_providers() | ||
| if not providers: | ||
| pytest.skip('No graph providers enabled for integration tests') | ||
| return providers | ||
|
|
||
|
|
||
| @pytest.fixture(params=_integration_providers()) | ||
| async def graph_driver(request: pytest.FixtureRequest): | ||
| provider: GraphProvider = request.param | ||
| marker = PROVIDER_MARKER.get(provider) | ||
| if marker: | ||
| request.node.add_marker(marker) | ||
| driver = await services.prepare_driver(provider) | ||
| try: | ||
| yield driver | ||
| finally: | ||
| await services.teardown_driver(driver) |
There was a problem hiding this comment.
Fix import-time skip that breaks CI.
params=_integration_providers() runs at import time; when no graph providers are available (CI sets DISABLE_*='1'), _integration_providers raises pytest.skip, which aborts collection. The GitHub Actions failure (“pytest: No graph providers enabled for integration tests”) comes straight from this change.
Let the fixture parameterize over a sentinel and defer the skip until fixture execution so Pytest can report a normal skip instead of failing the session.
Apply this diff:
def _integration_providers() -> list[GraphProvider]:
- providers = services.available_providers()
- if not providers:
- pytest.skip('No graph providers enabled for integration tests')
- return providers
+ providers = services.available_providers()
+ return providers or [None]
@pytest.fixture(params=_integration_providers())
async def graph_driver(request: pytest.FixtureRequest):
- provider: GraphProvider = request.param
- marker = PROVIDER_MARKER.get(provider)
- if marker:
- request.node.add_marker(marker)
- driver = await services.prepare_driver(provider)
- try:
- yield driver
- finally:
- await services.teardown_driver(driver)
+ provider: GraphProvider | None = request.param
+ if provider is None:
+ pytest.skip('No graph providers enabled for integration tests')
+
+ marker = PROVIDER_MARKER.get(provider)
+ if marker:
+ request.node.add_marker(marker)
+
+ driver = await services.prepare_driver(provider)
+ try:
+ yield driver
+ finally:
+ await services.teardown_driver(driver)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _integration_providers() -> list[GraphProvider]: | |
| providers = services.available_providers() | |
| if not providers: | |
| pytest.skip('No graph providers enabled for integration tests') | |
| return providers | |
| @pytest.fixture(params=_integration_providers()) | |
| async def graph_driver(request: pytest.FixtureRequest): | |
| provider: GraphProvider = request.param | |
| marker = PROVIDER_MARKER.get(provider) | |
| if marker: | |
| request.node.add_marker(marker) | |
| driver = await services.prepare_driver(provider) | |
| try: | |
| yield driver | |
| finally: | |
| await services.teardown_driver(driver) | |
| def _integration_providers() -> list[GraphProvider]: | |
| providers = services.available_providers() | |
| return providers or [None] | |
| @pytest.fixture(params=_integration_providers()) | |
| async def graph_driver(request: pytest.FixtureRequest): | |
| provider: GraphProvider | None = request.param | |
| if provider is None: | |
| pytest.skip('No graph providers enabled for integration tests') | |
| marker = PROVIDER_MARKER.get(provider) | |
| if marker: | |
| request.node.add_marker(marker) | |
| driver = await services.prepare_driver(provider) | |
| try: | |
| yield driver | |
| finally: | |
| await services.teardown_driver(driver) |
🧰 Tools
🪛 GitHub Actions: CI
[error] 25-25: pytest: No graph providers enabled for integration tests (Skipped by fixture).
🤖 Prompt for AI Agents
In tests/integration/shared/fixtures_services.py around lines 22 to 39, the
fixture currently evaluates _integration_providers() at import time causing
pytest.skip to run during collection; change params to a single sentinel value
(e.g. SENTINEL = object()) so parametrization happens with that sentinel instead
of calling the providers function at import time, and update the fixture body so
that when request.param is the sentinel it calls services.available_providers(),
calls pytest.skip if the returned list is empty, otherwise iterates over those
providers to prepare/teardown the driver for each provider (or reparametrize via
pytest.skip+pytest.mark if you prefer), ensuring the skip happens inside fixture
execution rather than at import time.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
TEST_RESTRUCTURE_PLAN.md (2)
19-44: Add language identifier to fenced code block.The directory structure code block should specify a language identifier for proper syntax highlighting and linting compliance.
Apply this diff:
-``` +```text tests/ ├─ unit/
61-69: Add language identifier to fenced code block.The pytest.ini snippet should specify a language identifier.
Apply this diff:
-``` +```ini [pytest] markers =tests/helpers/services.py (1)
106-106: Consider extracting exception messages to constants.Static analysis flags these lines for specifying long messages directly in exception constructors, which can make exceptions harder to test and maintain.
Consider extracting common error patterns:
+# Exception message templates +_MSG_UNSUPPORTED_PROVIDER = 'Unsupported provider: {provider!r}' +_MSG_UNSUPPORTED_TYPE_SAVE = 'Unsupported item type for save_all: {type!r}' +_MSG_UNSUPPORTED_TYPE_DELETE = 'Unsupported item type for delete_all: {type!r}' + ... - raise ValueError(f'Unsupported provider: {provider!r}') + raise ValueError(_MSG_UNSUPPORTED_PROVIDER.format(provider=provider))Also applies to: 146-146, 157-157
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.github/workflows/ci.yml(1 hunks)MCP_SMOKE_DEBUG.md(0 hunks)TEST_RESTRUCTURE_PLAN.md(1 hunks)conftest.py(2 hunks)docs/testing.md(3 hunks)pytest.ini(1 hunks)tests/driver/__init__.py(0 hunks)tests/e2e/graph/eval_cli.py(1 hunks)tests/e2e/graph/eval_graph_building.py(6 hunks)tests/e2e/graph/test_eval_harness.py(1 hunks)tests/evals/pytest.ini(0 hunks)tests/helpers/embeddings.py(1 hunks)tests/helpers/services.py(1 hunks)tests/helpers_test.py(0 hunks)tests/integration/conftest.py(1 hunks)tests/integration/core/shared/test_entity_exclusion.py(5 hunks)tests/integration/core/shared/test_graphium_bootstrap.py(1 hunks)tests/integration/core/shared/test_ingestion_pipeline.py(2 hunks)tests/integration/core/shared/test_repository_edges.py(18 hunks)tests/integration/core/shared/test_repository_nodes.py(9 hunks)tests/integration/falkordb/test_placeholder.py(1 hunks)tests/integration/kuzu/test_placeholder.py(1 hunks)tests/integration/neo4j/test_placeholder.py(1 hunks)tests/integration/shared/fixtures_services.py(1 hunks)tests/unit/embedder/test_gemini.py(1 hunks)tests/unit/embedder/test_openai.py(1 hunks)tests/unit/embedder/test_voyage.py(1 hunks)tests/unit/search/test_lucene_utils.py(1 hunks)
💤 Files with no reviewable changes (4)
- tests/evals/pytest.ini
- tests/helpers_test.py
- tests/driver/init.py
- MCP_SMOKE_DEBUG.md
🧰 Additional context used
📓 Path-based instructions (4)
tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Author tests under tests/ alongside features
Files:
tests/unit/embedder/test_openai.pytests/integration/falkordb/test_placeholder.pytests/e2e/graph/test_eval_harness.pytests/integration/core/shared/test_repository_edges.pytests/unit/search/test_lucene_utils.pytests/unit/embedder/test_gemini.pytests/helpers/embeddings.pytests/unit/embedder/test_voyage.pytests/e2e/graph/eval_cli.pytests/integration/conftest.pytests/integration/core/shared/test_graphium_bootstrap.pytests/e2e/graph/eval_graph_building.pytests/integration/kuzu/test_placeholder.pytests/integration/neo4j/test_placeholder.pytests/integration/core/shared/test_repository_nodes.pytests/integration/shared/fixtures_services.pytests/helpers/services.pytests/integration/core/shared/test_entity_exclusion.pytests/integration/core/shared/test_ingestion_pipeline.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indentation, 100-character line length, and prefer single quotes for Python code (as configured in pyproject.toml)
Use snake_case for module, file, and function names in Python
Files:
tests/unit/embedder/test_openai.pytests/integration/falkordb/test_placeholder.pytests/e2e/graph/test_eval_harness.pytests/integration/core/shared/test_repository_edges.pytests/unit/search/test_lucene_utils.pytests/unit/embedder/test_gemini.pyconftest.pytests/helpers/embeddings.pytests/unit/embedder/test_voyage.pytests/e2e/graph/eval_cli.pytests/integration/conftest.pytests/integration/core/shared/test_graphium_bootstrap.pytests/e2e/graph/eval_graph_building.pytests/integration/kuzu/test_placeholder.pytests/integration/neo4j/test_placeholder.pytests/integration/core/shared/test_repository_nodes.pytests/integration/shared/fixtures_services.pytests/helpers/services.pytests/integration/core/shared/test_entity_exclusion.pytests/integration/core/shared/test_ingestion_pipeline.py
pytest.ini
📄 CodeRabbit inference engine (AGENTS.md)
Maintain pytest configuration in pytest.ini at the repo root
Files:
pytest.ini
conftest.py
📄 CodeRabbit inference engine (AGENTS.md)
Pytest configuration should reside in repository-level conftest.py
Files:
conftest.py
🧠 Learnings (2)
📚 Learning: 2025-10-11T16:59:48.957Z
Learnt from: CR
PR: CaliLuke/graphium#0
File: AGENTS.md:0-0
Timestamp: 2025-10-11T16:59:48.957Z
Learning: Applies to tests/*_int.py : Integration tests live in tests/*_int.py and must be marked with pytest.mark.integration
Applied to files:
pytest.inidocs/testing.md
📚 Learning: 2025-10-11T16:59:48.957Z
Learnt from: CR
PR: CaliLuke/graphium#0
File: AGENTS.md:0-0
Timestamp: 2025-10-11T16:59:48.957Z
Learning: Use pytest.mark.integration to gate database-reliant scenarios in CI
Applied to files:
pytest.ini
🧬 Code graph analysis (15)
tests/unit/embedder/test_openai.py (1)
tests/unit/embedder/embedder_fixtures.py (1)
create_embedding_values(5-7)
tests/integration/core/shared/test_repository_edges.py (2)
tests/helpers/services.py (2)
get_edge_count(172-185)get_node_count(160-169)graphium_core/repository/node_repository.py (1)
get_by_entity_node_uuid(360-376)
tests/unit/embedder/test_gemini.py (1)
tests/unit/embedder/embedder_fixtures.py (1)
create_embedding_values(5-7)
conftest.py (1)
tests/helpers/embeddings.py (1)
make_mock_embedder(49-61)
tests/helpers/embeddings.py (3)
graphium_core/embedder/client.py (1)
EmbedderClient(18-26)tests/unit/orchestration/test_initializer_factory.py (1)
create(68-69)tests/unit/search/test_search_helpers.py (4)
create(26-27)create(34-35)create(43-44)create(97-99)
tests/unit/embedder/test_voyage.py (1)
tests/unit/embedder/embedder_fixtures.py (1)
create_embedding_values(5-7)
tests/e2e/graph/eval_cli.py (1)
tests/e2e/graph/eval_graph_building.py (2)
build_baseline_graph(98-115)eval_graph(118-173)
tests/integration/conftest.py (1)
tests/integration/shared/fixtures_services.py (1)
graph_driver(32-43)
tests/integration/core/shared/test_graphium_bootstrap.py (1)
graphium_core/driver/driver.py (1)
GraphProvider(32-36)
tests/e2e/graph/eval_graph_building.py (2)
graphium_core/settings.py (1)
Neo4jSettings(277-280)graphium_core/graphium.py (1)
Graphium(40-235)
tests/integration/core/shared/test_repository_nodes.py (2)
tests/helpers/services.py (4)
assert_community_node_equals(204-215)assert_entity_node_equals(188-201)assert_episodic_node_equals(218-227)get_node_count(160-169)graphium_core/repository/node_repository.py (1)
delete_by_group_id(110-166)
tests/integration/shared/fixtures_services.py (2)
graphium_core/driver/driver.py (1)
GraphProvider(32-36)tests/helpers/services.py (3)
available_providers(56-72)prepare_driver(109-113)teardown_driver(116-117)
tests/helpers/services.py (7)
graphium_core/driver/driver.py (3)
GraphDriver(70-119)GraphProvider(32-36)repositories(115-119)graphium_core/edges.py (3)
CommunityEdge(111-112)EntityEdge(47-108)EpisodicEdge(43-44)graphium_core/nodes.py (3)
CommunityNode(152-163)EntityNode(91-149)EpisodicNode(78-88)graphium_core/orchestration/maintenance/graph_data_operations.py (1)
clear_data(218-242)graphium_core/settings.py (4)
FalkorSettings(283-290)KuzuSettings(299-300)Neo4jSettings(277-280)NeptuneSettings(293-296)graphium_core/repository/graph_repository.py (4)
delete_node(65-66)delete_edge(75-76)save_node(62-63)save_edge(72-73)graphium_core/repository/edge_repository.py (1)
load_fact_embedding(421-452)
tests/integration/core/shared/test_entity_exclusion.py (2)
tests/integration/shared/fixtures_services.py (1)
graph_driver(32-43)graphium_core/graphium.py (1)
Graphium(40-235)
tests/integration/core/shared/test_ingestion_pipeline.py (2)
graphium_core/driver/driver.py (1)
GraphProvider(32-36)tests/helpers/services.py (7)
assert_entity_edge_equals(230-248)assert_entity_node_equals(188-201)assert_episodic_edge_equals(251-256)assert_episodic_node_equals(218-227)delete_all(149-157)get_edge_count(172-185)get_node_count(160-169)
🪛 markdownlint-cli2 (0.18.1)
TEST_RESTRUCTURE_PLAN.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
61-61: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.13.3)
tests/integration/conftest.py
6-6: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
tests/helpers/services.py
106-106: Avoid specifying long messages outside the exception class
(TRY003)
146-146: Avoid specifying long messages outside the exception class
(TRY003)
157-157: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (28)
tests/unit/search/test_lucene_utils.py (1)
1-17: LGTM! Comprehensive unit test for Lucene sanitization.The test effectively validates both the escaping of special characters and the pass-through of plain text. The test data covers all Lucene special characters, ensuring thorough coverage of the sanitization logic.
.github/workflows/ci.yml (1)
44-45: LGTM! CI test paths updated correctly.The test paths have been updated to match the new unit/integration structure. The selected tests provide good coverage while remaining fast enough for CI execution.
tests/e2e/graph/eval_cli.py (1)
4-4: LGTM! Import path updated for test restructuring.The import path correctly reflects the new e2e test organization under
tests/e2e/graph/.tests/unit/embedder/test_openai.py (1)
12-13: LGTM! Relative import improves test package organization.Using relative imports within the embedder test package makes the code more maintainable and follows Python best practices for intra-package imports.
tests/integration/conftest.py (1)
6-6: LGTM! Fixture import is correct despite static analysis warning.The
noqa: F401is intentionally used here to suppress unused import warnings for thegraph_driverfixture. This is a standard pytest pattern where fixtures are imported inconftest.pyto make them available to all tests in the directory tree. The Ruff RUF100 warning is a false positive in this case.tests/e2e/graph/test_eval_harness.py (1)
7-9: LGTM! Placeholder test documents e2e requirements.The placeholder test with a skip message is a good approach for documenting that eval harness tests require manual orchestration. This preserves the test structure while clearly communicating the manual setup requirement.
tests/integration/core/shared/test_ingestion_pipeline.py (2)
11-11: LGTM! GraphProvider import updated correctly.The import path now correctly references
graphium_core.driver.driverfor theGraphProviderenum.
47-60: LGTM! Test helper imports and aliases updated correctly.The import paths now correctly reference
tests.helpers.services, and the GROUP_ID constants are appropriately aliased to lowercase names for local usage, following Python naming conventions.tests/integration/core/shared/test_graphium_bootstrap.py (1)
9-9: LGTM! GraphProvider import updated correctly.The import path now correctly references
graphium_core.driver.driver, consistent with other integration test files in this PR.tests/unit/embedder/test_gemini.py (1)
12-12: LGTM! Relative import aligns with package structure.The change from absolute to relative import is appropriate for fixtures within the same test package.
tests/unit/embedder/test_voyage.py (1)
13-13: LGTM! Consistent relative import pattern.The relative import matches the pattern established in other unit tests within this package.
tests/integration/core/shared/test_repository_nodes.py (3)
15-21: LGTM! Standardized constant usage improves consistency.The migration from a local variable to the shared
GROUP_IDconstant fromtests.helpers.servicesprovides consistent test data across integration tests.
34-34: LGTM! Fixtures correctly use the shared constant.All sample node fixtures consistently use
GROUP_ID, ensuring uniform test data.Also applies to: 51-51, 66-66
95-95: LGTM! Test assertions properly use the standardized constant.All
get_by_group_idsanddelete_by_group_idcalls correctly referenceGROUP_ID, maintaining consistency with the fixtures.Also applies to: 117-117, 145-145, 166-166, 194-194, 215-215
pytest.ini (1)
3-8: LGTM! Enhanced marker system supports selective test execution.The updated integration marker description and new provider-specific markers (neo4j, falkordb, kuzu, neptune, e2e) enable granular test selection in CI and local development, aligning with the test restructuring objectives.
Based on learnings
tests/integration/kuzu/test_placeholder.py (1)
1-10: LGTM! Placeholder structure enables future Kuzu-specific tests.The placeholder test with
@pytest.mark.integrationand@pytest.mark.kuzumarkers correctly reserves space for Kuzu-backed integration tests and aligns with the new marker system defined in pytest.ini.tests/integration/neo4j/test_placeholder.py (1)
1-10: LGTM! Neo4j placeholder follows established pattern.The placeholder test structure with appropriate markers (
@pytest.mark.integrationand@pytest.mark.neo4j) is consistent with other provider placeholders and integrates with the expanded marker system.conftest.py (2)
15-15: LGTM! Centralized embedding helper improves maintainability.The import of
make_mock_embedderfrom the newtests.helpers.embeddingsmodule consolidates embedding test utilities.
17-17: graph_driver fixture available in integration tests Verified definition in tests/integration/shared/fixtures_services.py (line 32).tests/e2e/graph/eval_graph_building.py (2)
6-6: LGTM! Modernized path and settings management.Introducing
pathlib.Pathfor filesystem operations and sourcing Neo4j connection parameters fromNeo4jSettingsimproves maintainability and follows Python best practices. TheBASE_DIRandDATA_DIRconstants provide clear, reusable path references.Also applies to: 19-24
72-73: LGTM! Consistent Path usage throughout the file.All file operations now use
Path-based paths (DATA_DIR / ...,BASE_DIR / ...) instead of string concatenation, ensuring cross-platform compatibility and cleaner code.Also applies to: 101-101, 107-107, 121-122, 133-133
tests/integration/core/shared/test_repository_edges.py (1)
11-11: LGTM! Consolidation of test constants.The import of
GROUP_IDfrom the new centralizedtests.helpers.servicesmodule improves maintainability by eliminating hardcoded group IDs throughout the test suite.tests/integration/falkordb/test_placeholder.py (1)
7-10: LGTM! Provider-specific integration placeholder.The placeholder test correctly uses integration and provider-specific markers, consistent with the new test organization strategy outlined in the PR.
docs/testing.md (1)
21-23: LGTM! Documentation accurately reflects new test organization.The updated test layout description correctly documents the separation of unit, integration, and e2e tests with appropriate markers.
tests/integration/core/shared/test_entity_exclusion.py (1)
82-90: LGTM! Consistent fixture usage.The migration from parameterized driver fixtures to the centralized
graph_driverfixture improves test consistency and maintainability across the integration test suite.tests/helpers/services.py (1)
56-72: LGTM! Flexible provider availability checking.The lazy provider resolution approach correctly avoids import-time side effects while still enabling environment-driven test configuration.
tests/helpers/embeddings.py (1)
54-58: Iterable branch unused in mock_embed; no change required.tests/integration/shared/fixtures_services.py (1)
36-38: Use pytest.mark decorators instead of string markers
Passing a string torequest.node.add_markerdoesn’t register markers. Update as follows in tests/integration/shared/fixtures_services.py:- marker = PROVIDER_MARKER.get(provider) - if marker: - request.node.add_marker(marker) + marker_name = PROVIDER_MARKER.get(provider) + if marker_name: + request.node.add_marker(getattr(pytest.mark, marker_name))Likely an incorrect or invalid review comment.
|
|
||
| load_dotenv() | ||
|
|
||
| os.environ.setdefault('DISABLE_NEPTUNE', 'True') |
There was a problem hiding this comment.
Review environment variable override behavior.
Setting DISABLE_NEPTUNE=True after load_dotenv() will override any user-specified value in .env. If the intent is to provide a default only when unset, use os.environ.setdefault() before load_dotenv(), or check if the variable exists before setting it.
If this is intentional (forcing Neptune to be disabled by default), consider documenting the behavior. Otherwise, apply this diff:
-load_dotenv()
-
os.environ.setdefault('DISABLE_NEPTUNE', 'True')
+
+load_dotenv()This ensures the default is set only if not already present in the environment, and .env values take precedence.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| os.environ.setdefault('DISABLE_NEPTUNE', 'True') | |
| os.environ.setdefault('DISABLE_NEPTUNE', 'True') | |
| load_dotenv() |
🤖 Prompt for AI Agents
In tests/helpers/services.py around line 35, the code unconditionally sets
DISABLE_NEPTUNE after load_dotenv which overrides any .env value; change this to
only set a default when the variable is absent by either moving
os.environ.setdefault('DISABLE_NEPTUNE','True') to before load_dotenv or by
checking if 'DISABLE_NEPTUNE' not in os.environ before assigning, so .env values
take precedence (or document intentional forced override if that was desired).
|
Addressed the actionable review notes:
Let me know if anything else needs tweaking! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/integration/shared/fixtures_services.py (1)
31-43: Well-designed fixture orchestration.The fixture correctly orchestrates per-provider graph driver setup and teardown. The implementation properly adds provider-specific markers, prepares the driver, yields it for test use, and ensures cleanup via the finally block.
The defensive check at lines 34-35 is appropriate even though
pytest.paramwith skip mark should prevent fixture execution, as it guards against edge cases.For improved type safety with strict type checkers, consider adding an assertion after the skip check:
provider = request.param if provider is None: pytest.skip(_NO_PROVIDER_MSG) assert provider is not None # Type guard for stricter type checkers marker_name = PROVIDER_MARKER.get(provider) # ...This helps type checkers understand that
providercannot beNoneafter the skip check (thoughpytest.skip()already raises an exception, so this is optional).tests/unit/search/test_lucene_utils.py (2)
6-6: Consider simplifying the import.The alias
lucene_sanitizeis clear but adds little value since the module path already indicates it's from Lucene. You could simplify to:-from graphium_core.search.lucene import sanitize as lucene_sanitize +from graphium_core.search.lucene import sanitizeAnd then update line 20 to use
sanitize(query).Also note: there are excessive blank lines between imports (lines 5 and 7-8). PEP 8 recommends one blank line between import groups, but since there's only one import from
graphium_core, the extra spacing is unnecessary.
9-20: Consider adding edge case tests.The current test coverage is good for the happy path, but consider adding tests for:
- Empty string:
('', '')- String with only special characters:
('+-()[]{}', '\\+\\-\\(\\)\\[\\]\\{\\}')- Consecutive special characters without spaces
- Leading/trailing special characters
These additional tests would improve confidence in the sanitize function's robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
TEST_RESTRUCTURE_PLAN.md(1 hunks)tests/helpers/embeddings.py(1 hunks)tests/helpers/services.py(1 hunks)tests/integration/shared/fixtures_services.py(1 hunks)tests/unit/search/test_lucene_utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/helpers/embeddings.py
- tests/helpers/services.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Author tests under tests/ alongside features
Files:
tests/unit/search/test_lucene_utils.pytests/integration/shared/fixtures_services.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indentation, 100-character line length, and prefer single quotes for Python code (as configured in pyproject.toml)
Use snake_case for module, file, and function names in Python
Files:
tests/unit/search/test_lucene_utils.pytests/integration/shared/fixtures_services.py
🧬 Code graph analysis (1)
tests/integration/shared/fixtures_services.py (2)
graphium_core/driver/driver.py (1)
GraphProvider(32-36)tests/helpers/services.py (3)
available_providers(60-76)prepare_driver(113-117)teardown_driver(120-121)
🔇 Additional comments (5)
TEST_RESTRUCTURE_PLAN.md (1)
1-442: Well-structured test restructure plan.The document provides comprehensive guidance for reorganizing tests into unit, integration, and e2e tiers. The goals, pain points, directory layout, execution strategy, and migration roadmap are clearly defined and will serve as an effective reference during implementation.
Minor note: The pytest.ini snippet (lines 61-69) doesn't include a
neptunemarker, buttests/integration/shared/fixtures_services.pyincludes Neptune support inPROVIDER_MARKER. Consider addingneptune: requires a running Neptune instanceto the markers list when updating the actual pytest.ini configuration.tests/integration/shared/fixtures_services.py (2)
14-19: LGTM!The provider-to-marker mapping is correct and follows pytest naming conventions. All four providers (NEO4J, FALKORDB, KUZU, NEPTUNE) are properly mapped to their corresponding marker strings.
24-28: Correctly defers skip to test execution time.The function properly addresses the past review concern by returning
pytest.param(None, marks=pytest.mark.skip(...))instead of callingpytest.skip()at import time. This ensures that collection succeeds even when no providers are available, with the skip happening during test execution.tests/unit/search/test_lucene_utils.py (2)
1-3: LGTM!Standard license header with appropriate modification notice.
12-15: Remove unintended letter mappings from sanitize
sanitize’sescape_mapcurrently includes'O','R','N','T','A','D', so non-special letters (e.g.T) get escaped and the test is correct. Remove those entries fromescape_mapin graphium_core/search/lucene.py so only true Lucene special characters are escaped.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/helpers/services.py (2)
62-78: Simplify provider checks after fixing the type hint.Following the type hint correction in
_import_if_available, the truthiness checks on lines 66, 69, 72, and 76 become unnecessary since the function either returns a driver class or raises an exception.After applying the previous type hint fix, apply this diff:
def available_providers() -> list[GraphProvider]: """Return the graph providers that can be exercised for integration tests.""" providers: list[GraphProvider] = [] if _provider_enabled('DISABLE_NEO4J'): - if _import_if_available(GraphProvider.NEO4J, _resolve_neo4j_driver): - providers.append(GraphProvider.NEO4J) + _import_if_available(GraphProvider.NEO4J, _resolve_neo4j_driver) + providers.append(GraphProvider.NEO4J) if _provider_enabled('DISABLE_FALKORDB'): - if _import_if_available(GraphProvider.FALKORDB, _resolve_falkordb_driver): - providers.append(GraphProvider.FALKORDB) + _import_if_available(GraphProvider.FALKORDB, _resolve_falkordb_driver) + providers.append(GraphProvider.FALKORDB) if _provider_enabled('DISABLE_KUZU'): - if _import_if_available(GraphProvider.KUZU, _resolve_kuzu_driver): - providers.append(GraphProvider.KUZU) + _import_if_available(GraphProvider.KUZU, _resolve_kuzu_driver) + providers.append(GraphProvider.KUZU) if _provider_enabled('DISABLE_NEPTUNE'): - driver = _import_if_available(GraphProvider.NEPTUNE, _resolve_neptune_driver) - if driver: - providers.append(GraphProvider.NEPTUNE) + _import_if_available(GraphProvider.NEPTUNE, _resolve_neptune_driver) + providers.append(GraphProvider.NEPTUNE) return providers
81-112: Remove redundantint()casts.Lines 96 and 109 cast
settings.porttoint, but the settings classes already declareportasint(perFalkorSettingsandNeptuneSettings). While harmless, these casts are unnecessary.Apply this diff if you want to remove the redundancy:
if provider == GraphProvider.FALKORDB: settings = FalkorSettings() driver_cls = _resolve_falkordb_driver() return driver_cls( host=settings.host, - port=int(settings.port), + port=settings.port, username=settings.username, password=settings.password, ) ... if provider == GraphProvider.NEPTUNE: settings = NeptuneSettings() driver_cls = _resolve_neptune_driver() return driver_cls( host=settings.host or 'localhost', - port=int(settings.port), + port=settings.port, aoss_host=settings.aoss_host, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/helpers/services.py(1 hunks)tests/integration/conftest.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/conftest.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Author tests under tests/ alongside features
Files:
tests/helpers/services.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indentation, 100-character line length, and prefer single quotes for Python code (as configured in pyproject.toml)
Use snake_case for module, file, and function names in Python
Files:
tests/helpers/services.py
🧬 Code graph analysis (1)
tests/helpers/services.py (6)
graphium_core/driver/driver.py (3)
GraphDriver(70-119)GraphProvider(32-36)repositories(115-119)graphium_core/nodes.py (3)
CommunityNode(152-163)EntityNode(91-149)EpisodicNode(78-88)graphium_core/orchestration/maintenance/graph_data_operations.py (1)
clear_data(218-242)graphium_core/settings.py (4)
FalkorSettings(283-290)KuzuSettings(299-300)Neo4jSettings(277-280)NeptuneSettings(293-296)graphium_core/repository/graph_repository.py (4)
delete_node(65-66)delete_edge(75-76)save_node(62-63)save_edge(72-73)graphium_core/repository/edge_repository.py (1)
load_fact_embedding(421-452)
🔇 Additional comments (7)
tests/helpers/services.py (7)
1-44: LGTM! Module setup is well-structured.The environment variable initialization order is correct: existing environment values and
.envsettings take precedence, withDISABLE_NEPTUNE=Trueserving only as a fallback default. The module docstring accurately documents this behavior. Using constants for error messages ensures consistency across the module.
115-123: LGTM! Driver lifecycle management is clean.The
prepare_driverandteardown_driverfunctions correctly handle driver provisioning and cleanup. Clearing specific test groups before tests run prevents cross-contamination between test runs.
126-141: LGTM! Save and delete helpers correctly use available repository APIs.The batch operations for saves and individual operations for deletes reflect the underlying repository API design. The implementation is clean and correct.
144-167: LGTM! Generic save/delete helpers with proper type checking.The
save_allanddelete_allfunctions correctly dispatch to the appropriate repository methods based on instance type. Using shared error message constants ensures consistency.
170-195: LGTM! Count helpers correctly handle provider-specific edge models.The
get_edge_countfunction'sUNION ALLquery correctly accounts for Kuzu's hybrid edge representation (both relationship edges andRelatesToNode_nodes), while remaining compatible with other providers.
198-267: LGTM! Assertion helpers correctly compare against sample properties.All five assertion functions now properly compare
retrieved.group_idagainstsample.group_id(lines 204, 220, 231, 245, 263), addressing the concerns from previous reviews. The embedding comparisons usingnp.allcloseare appropriate for floating-point arrays.
269-290: LGTM! Lazy resolver functions follow consistent pattern.The private resolver functions correctly implement lazy loading by importing driver classes only when called. This approach keeps test collection fast and avoids unnecessary imports, as documented in the module docstring.
| def _import_if_available( | ||
| provider: GraphProvider, | ||
| import_factory: Callable[[], type[GraphDriver]], | ||
| ) -> type[GraphDriver] | None: | ||
| try: | ||
| return import_factory() | ||
| except ImportError as exc: | ||
| msg = f'Provider {provider.value} is enabled but unavailable: {exc}' | ||
| raise ImportError(msg) from exc |
There was a problem hiding this comment.
Type hint doesn't match implementation behavior.
The function is typed to return type[GraphDriver] | None, but the implementation always raises ImportError on failure and never returns None. This causes callers to perform redundant truthiness checks.
Apply this diff to make the type hint accurate:
def _import_if_available(
provider: GraphProvider,
import_factory: Callable[[], type[GraphDriver]],
-) -> type[GraphDriver] | None:
+) -> type[GraphDriver]:
try:
return import_factory()
except ImportError as exc:
msg = f'Provider {provider.value} is enabled but unavailable: {exc}'
raise ImportError(msg) from exc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _import_if_available( | |
| provider: GraphProvider, | |
| import_factory: Callable[[], type[GraphDriver]], | |
| ) -> type[GraphDriver] | None: | |
| try: | |
| return import_factory() | |
| except ImportError as exc: | |
| msg = f'Provider {provider.value} is enabled but unavailable: {exc}' | |
| raise ImportError(msg) from exc | |
| def _import_if_available( | |
| provider: GraphProvider, | |
| import_factory: Callable[[], type[GraphDriver]], | |
| ) -> type[GraphDriver]: | |
| try: | |
| return import_factory() | |
| except ImportError as exc: | |
| msg = f'Provider {provider.value} is enabled but unavailable: {exc}' | |
| raise ImportError(msg) from exc |
🤖 Prompt for AI Agents
In tests/helpers/services.py around lines 51 to 59, the function
_import_if_available is annotated to return type[GraphDriver] | None but in
practice it never returns None and instead raises ImportError on failure; update
the function signature to return type[GraphDriver] (remove the | None) so the
annotation matches behavior and remove any redundant None-handling in callers if
present.
Summary
Testing
Summary by CodeRabbit
Tests
Documentation
Chores