test(integration): standardize setup and rollback fixtures PQE-426#91
Open
ddlees wants to merge 8 commits into
Open
test(integration): standardize setup and rollback fixtures PQE-426#91ddlees wants to merge 8 commits into
ddlees wants to merge 8 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughRefactors integration-test setup to a session-based API (Open/Session) with rollback fixtures; updates tests (cypher templates, metamorphic, PostgreSQL) to use the new Session helpers; adds traversal collection unit tests with a mock graph DB and an error-collector unit test. ChangesIntegration Test Harness Refactoring
Traversal Collection Test Coverage
Error Collector Test
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ykaiboussiSO
changed the title
ykaiboussiSO
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
integration/harness.go (1)
77-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix cleanup behavior in
SetupDBWithKindsNoGraphCleanup
SetupDBWithKindsNoGraphCleanupdoes not enforce close-only cleanup:cleanupMode=0maps toCleanupGraph, so node deletion happens during cleanup and contradicts the helper’s “NoGraphCleanup” contract.- Drop the
bolt://scheme suggestion: the repo’s test setup already normalizesbolt://→neo4j://elsewhere, and there are nobolt://literals in the codebase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration/harness.go` around lines 77 - 90, SetupDBWithKindsNoGraphCleanup currently lets cleanupMode=0 map to CleanupGraph so nodes get deleted; change the logic in SetupDBWithKindsNoGraphCleanup so that when it intends “no graph cleanup” it sets/uses the close-only cleanup mode (e.g., CleanupCloseOnly or the existing CloseOnly/Close-only enum/value used by the harness) and ensure the teardown path only closes DB handles and does NOT call graph-deletion code (references: SetupDBWithKindsNoGraphCleanup, cleanupMode, CleanupGraph, the close-only cleanup constant). Also remove any special-case mapping of a "bolt" scheme in DriverFromConnectionString (stop treating "bolt" as a suggested scheme or mapping it to neo4j); only accept the canonical neo4j schemes already present (references: DriverFromConnectionString, neo4j.DriverName).
🧹 Nitpick comments (2)
traversal/util_test.go (1)
39-44: ⚡ Quick winInvoke delegates in successful write/batch paths.
WriteTransactionandBatchOperationcurrently return nil without calling their delegates. That can let tests pass even when callback logic is broken.Proposed fix
func (m *mockGraphDB) WriteTransaction(ctx context.Context, txDelegate graph.TransactionDelegate, options ...graph.TransactionOption) error { if m.errorMsg.Error() == "Write Transaction Failed" { return fmt.Errorf("Write Transaction Failed") } - return nil + return txDelegate(m.tx) } @@ func (m *mockGraphDB) BatchOperation(ctx context.Context, batchDelegate graph.BatchDelegate, options ...graph.BatchOption) error { if m.errorMsg.Error() == "Batch Transaction Failed" { return fmt.Errorf("Batch Transaction Failed") } - return nil + return batchDelegate(nil) }Also applies to: 53-58
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@traversal/util_test.go` around lines 39 - 44, The mock implementations for WriteTransaction and BatchOperation must invoke their provided delegates on the successful path instead of just returning nil; update mockGraphDB.WriteTransaction to call the supplied txDelegate (and return any error it returns) when m.errorMsg does not indicate failure, and likewise update mockGraphDB.BatchOperation to call the provided batch delegate (and return its error) when no failure is simulated; ensure you pass the context (and any required args) into the delegate and propagate its returned error so tests exercise callback logic.util/errors_test.go (1)
41-50: ⚡ Quick winConsider adding test coverage for the empty collector case.
The test validates the multiple-error path correctly, but does not verify that
Combined()returnsnilwhen no errors have been added. This is important behavior since callers likeOperation.Done()rely on this nil-return contract.🧪 Suggested addition for empty collector case
func TestNewErrorCollector(t *testing.T) { errCollector := util.NewErrorCollector() // Test empty collector returns nil err := errCollector.Combined() require.Nil(t, err) errCollector.Add(fmt.Errorf("errorOne")) errCollector.Add(fmt.Errorf("errorTwo")) err = errCollector.Combined() require.NotNil(t, err) require.Equal(t, err.Error(), "errorOne\nerrorTwo") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@util/errors_test.go` around lines 41 - 50, Add a test case in TestNewErrorCollector to assert that a newly created util.NewErrorCollector() returns nil from Combined() when no errors have been added; call errCollector := util.NewErrorCollector(), then err := errCollector.Combined() and require.Nil(t, err) before exercising errCollector.Add(...) and the existing assertions on Combined(), so the test covers both the empty-collector nil contract relied on by callers like Operation.Done() and the multi-error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration/harness.go`:
- Around line 349-361: The helper SetupDBWithKindsNoGraphCleanup currently
forwards the passed CleanupMode allowing callers to select CleanupGraph; change
it to always use the CloseOnly mode before calling Open so the graph is never
cleared. Specifically, in SetupDBWithKindsNoGraphCleanup override the incoming
cleanupMode value to CleanupMode.CloseOnly (or the CloseOnly constant used in
your CleanupMode type) and pass that into Open via Options (Options.CleanupMode)
while keeping other fields (ExtraNodeKinds, ExtraEdgeKinds, DatasetPath,
Datasets) unchanged.
In `@traversal/collection_test.go`:
- Around line 61-63: Replace the direct dereference of err.Error() with the
proper testify helper to avoid a panic when err is nil: in the test that calls
newCollector.PopulateProperties(ctx, db, "name") (the failing assertion around
err), use assert.EqualError(t, err, "Read Transaction Failed") instead of
assert.Equal(t, err.Error(), "Read Transaction Failed") so the assertion handles
a nil error safely and reports failures cleanly.
In `@traversal/util_test.go`:
- Around line 22-25: The mockGraphDB.SetDefaultGraph implementation returns an
inconsistent error message; when m.errorMsg.Error() == "Failed setting default
graph namespace" it should return that same error instead of "Failed to refresh
kinds". Update SetDefaultGraph (method on mockGraphDB) to return the matching
error (either by returning m.errorMsg or fmt.Errorf("Failed setting default
graph namespace")) so the returned error accurately reflects the condition
checked.
- Around line 22-25: The mock methods (e.g., mockGraphDB.SetDefaultGraph and the
other similar methods in this file) call m.errorMsg.Error() without nil checks
which can panic; update each mock method to first guard that m.errorMsg != nil
before calling .Error() (or use a safe helper like hasErrorMsg := m.errorMsg !=
nil && m.errorMsg.Error() == "Some message"), and keep the existing conditional
behavior intact so a zero-value mockGraphDB behaves as a successful mock.
---
Outside diff comments:
In `@integration/harness.go`:
- Around line 77-90: SetupDBWithKindsNoGraphCleanup currently lets cleanupMode=0
map to CleanupGraph so nodes get deleted; change the logic in
SetupDBWithKindsNoGraphCleanup so that when it intends “no graph cleanup” it
sets/uses the close-only cleanup mode (e.g., CleanupCloseOnly or the existing
CloseOnly/Close-only enum/value used by the harness) and ensure the teardown
path only closes DB handles and does NOT call graph-deletion code (references:
SetupDBWithKindsNoGraphCleanup, cleanupMode, CleanupGraph, the close-only
cleanup constant). Also remove any special-case mapping of a "bolt" scheme in
DriverFromConnectionString (stop treating "bolt" as a suggested scheme or
mapping it to neo4j); only accept the canonical neo4j schemes already present
(references: DriverFromConnectionString, neo4j.DriverName).
---
Nitpick comments:
In `@traversal/util_test.go`:
- Around line 39-44: The mock implementations for WriteTransaction and
BatchOperation must invoke their provided delegates on the successful path
instead of just returning nil; update mockGraphDB.WriteTransaction to call the
supplied txDelegate (and return any error it returns) when m.errorMsg does not
indicate failure, and likewise update mockGraphDB.BatchOperation to call the
provided batch delegate (and return its error) when no failure is simulated;
ensure you pass the context (and any required args) into the delegate and
propagate its returned error so tests exercise callback logic.
In `@util/errors_test.go`:
- Around line 41-50: Add a test case in TestNewErrorCollector to assert that a
newly created util.NewErrorCollector() returns nil from Combined() when no
errors have been added; call errCollector := util.NewErrorCollector(), then err
:= errCollector.Combined() and require.Nil(t, err) before exercising
errCollector.Add(...) and the existing assertions on Combined(), so the test
covers both the empty-collector nil contract relied on by callers like
Operation.Done() and the multi-error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 779cd8a6-ac6b-4d95-9fdd-7ed249d4cacf
📒 Files selected for processing (10)
integration/cypher_template_test.gointegration/cypher_test.gointegration/harness.gointegration/pgsql_aggregate_traversal_plan_test.gointegration/pgsql_count_fast_path_test.gointegration/pgsql_property_equality_test.gointegration/pgsql_property_index_plan_test.gotraversal/collection_test.gotraversal/util_test.goutil/errors_test.go
Comment on lines
+22
to
+25
| func (m *mockGraphDB) SetDefaultGraph(ctx context.Context, graph graph.Graph) error { | ||
| if m.errorMsg.Error() == "Failed setting default graph namespace" { | ||
| return fmt.Errorf("Failed to refresh kinds") | ||
| } |
There was a problem hiding this comment.
Return the matching error in SetDefaultGraph.
Line 24 returns "Failed to refresh kinds" for a "Failed setting default graph namespace" condition, which is inconsistent and makes failure intent ambiguous.
Proposed fix
func (m *mockGraphDB) SetDefaultGraph(ctx context.Context, graph graph.Graph) error {
if m.errorMsg.Error() == "Failed setting default graph namespace" {
- return fmt.Errorf("Failed to refresh kinds")
+ return fmt.Errorf("Failed setting default graph namespace")
}
return nil
}📝 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.
Suggested change
| func (m *mockGraphDB) SetDefaultGraph(ctx context.Context, graph graph.Graph) error { | |
| if m.errorMsg.Error() == "Failed setting default graph namespace" { | |
| return fmt.Errorf("Failed to refresh kinds") | |
| } | |
| func (m *mockGraphDB) SetDefaultGraph(ctx context.Context, graph graph.Graph) error { | |
| if m.errorMsg.Error() == "Failed setting default graph namespace" { | |
| return fmt.Errorf("Failed setting default graph namespace") | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@traversal/util_test.go` around lines 22 - 25, The mockGraphDB.SetDefaultGraph
implementation returns an inconsistent error message; when m.errorMsg.Error() ==
"Failed setting default graph namespace" it should return that same error
instead of "Failed to refresh kinds". Update SetDefaultGraph (method on
mockGraphDB) to return the matching error (either by returning m.errorMsg or
fmt.Errorf("Failed setting default graph namespace")) so the returned error
accurately reflects the condition checked.
Guard errorMsg before calling .Error() across mock methods.
Line 23 (and the other similar checks) dereferences m.errorMsg without a nil guard. A zero-value mockGraphDB will panic instead of behaving like a successful mock.
Proposed fix
+func (m *mockGraphDB) hasError(msg string) bool {
+ return m.errorMsg != nil && m.errorMsg.Error() == msg
+}
+
func (m *mockGraphDB) SetDefaultGraph(ctx context.Context, graph graph.Graph) error {
- if m.errorMsg.Error() == "Failed setting default graph namespace" {
+ if m.hasError("Failed setting default graph namespace") {
return fmt.Errorf("Failed to refresh kinds")
}
return nil
}
func (m *mockGraphDB) ReadTransaction(ctx context.Context, txDelegate graph.TransactionDelegate, options ...graph.TransactionOption) error {
@@
- if m.errorMsg.Error() == "Read Transaction Failed" {
+ if m.hasError("Read Transaction Failed") {
return fmt.Errorf("Read Transaction Failed")
}
@@
func (m *mockGraphDB) WriteTransaction(ctx context.Context, txDelegate graph.TransactionDelegate, options ...graph.TransactionOption) error {
- if m.errorMsg.Error() == "Write Transaction Failed" {
+ if m.hasError("Write Transaction Failed") {
return fmt.Errorf("Write Transaction Failed")
}
return nil
}Also applies to: 29-37, 39-44, 46-51, 53-58, 60-65, 67-72, 74-79, 81-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@traversal/util_test.go` around lines 22 - 25, The mock methods (e.g.,
mockGraphDB.SetDefaultGraph and the other similar methods in this file) call
m.errorMsg.Error() without nil checks which can panic; update each mock method
to first guard that m.errorMsg != nil before calling .Error() (or use a safe
helper like hasErrorMsg := m.errorMsg != nil && m.errorMsg.Error() == "Some
message"), and keep the existing conditional behavior intact so a zero-value
mockGraphDB behaves as a successful mock.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This change centralizes integration test setup, schema assertion, driver detection, graph cleanup, and rollback fixture handling in the shared integration harness. It keeps the existing setup helpers available while adding a Session-based API for clearer cleanup behavior and reusable fixture rollback flows across Cypher integration tests.
Resolves: PQE-426
Type of Change
Testing
make test_allwithCONNECTION_STRINGset)Screenshots (if appropriate):
Driver Impact
drivers/pg)drivers/neo4j)Checklist
go.mod/go.sumare up to date if dependencies changedSummary by CodeRabbit
Tests
Chores