test: assert the client returns a valid execution plan, not which plan the engine picked - #609
Open
Naseem77 wants to merge 10 commits into
Open
test: assert the client returns a valid execution plan, not which plan the engine picked#609Naseem77 wants to merge 10 commits into
Naseem77 wants to merge 10 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughAdded shared helpers for execution-plan and profile validation. Updated graph, query, and profile tests to use the helpers. Increased slow-query workloads from 200,000 to 1,000,000. Removed legacy Docker Compose installation from CI and simplified a relationship-creation query. ChangesQuery test updates
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🔵 Low · up to The PR improves plan and slowlog assertions, but the slowlog tests still depend on shared log state and the first result, which could cause flaky or misleading test outcomes; the change is mergeable with explicit owner awareness or follow-up. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/graphAndQuery.spec.ts`:
- Around line 336-337: Update the single-query plan assertions in the relevant
test to verify the GRAPH.EXPLAIN array’s exact leading sequence: assert
“Project” is the first entry and “ Node By Label Scan | (n:Person)” is the
second, rather than using toContain; ensure the assertion also fails if an
unexpected “Results” step is present.
- Line 277: Update the slowlog test around longQuery to clear the graph slowlog
before execution, then assert that the slowlog results contain an entry for this
query rather than assuming slowLogResults[0] is the match. Keep the assertion
host-speed independent while preserving validation that the long query is
logged.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dfd8c575-51fd-47a2-944b-acc2d36257ce
📒 Files selected for processing (2)
tests/graphAndQuery.spec.tstests/profile.spec.ts
…n the engine picked The specs pinned exact operation names, arguments and indentation, so any change to how the engine plans a query broke them even though the client was behaving correctly. Which operations a query compiles into is the engine's business. Assert what is actually the client's responsibility instead: a plan comes back, it holds at least as many operations as the query needs, every line names an operation, and the indentation that conveys nesting is well formed. Profiles additionally have to carry per operation statistics, and the row count the query yields is checked since that is a property of the query rather than of the engine. These specs now pass against both engines.
…t test The RETURN exists((a)-[:KNOWS]->(b)) result was never asserted, so the call only added a dependency on a function that behaves differently between engines without testing anything.
Naseem77
force-pushed
the
naseem77-align-plan-assertions-with-engine
branch
from
August 10, 2026 14:01
7503940 to
0169bd2
Compare
Naseem77
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #609 +/- ##
==========================================
+ Coverage 97.32% 97.45% +0.12%
==========================================
Files 28 29 +1
Lines 2204 2316 +112
Branches 319 140 -179
==========================================
+ Hits 2145 2257 +112
- Misses 58 59 +1
+ Partials 1 0 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the test suite to validate execution plans/profiles in an engine-agnostic way, focusing on what the client must return rather than pinning exact planner output.
Changes:
- Introduces
tests/planHelpers.tswith reusable assertions forGRAPH.EXPLAINandGRAPH.PROFILEoutput shape and nesting. - Refactors existing plan/profile specs to use the new helper assertions instead of exact operation-name matching.
- Adjusts slowlog tests to use a larger workload and removes an unasserted
RETURN exists(...)from the constraints test.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/single.spec.ts | Increases slowlog query workload to better trigger slowlog recording. |
| tests/profile.spec.ts | Replaces exact plan-step assertions with expectProfile(...) helper checks. |
| tests/planHelpers.ts | Adds engine-agnostic execution plan/profile validation helpers. |
| tests/graphAndQuery.spec.ts | Switches explain-plan assertions to expectExecutionPlan(...) and increases slowlog workload. |
| tests/constraints.spec.ts | Removes an unasserted RETURN exists(...) expression from a constraint setup query. |
| tests/cluster.spec.ts | Increases slowlog query workload to better trigger slowlog recording. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (4)
tests/planHelpers.ts:37
- The indentation assertion uses
toBe(index === 0 ? 0 : Math.min(...)), which is harder to read and can still throw if the previous entry is not a string. Splitting the root vs non-root cases and usingtoBeLessThanOrEqualmakes the intent clearer and keeps the helper from crashing on malformed input.
// indentation is what conveys nesting, so it has to be whole levels, and
// an operation can only ever be one level deeper than the one above it
const indent = indentOf(line);
expect(Number.isInteger(indent)).toBe(true);
expect(indent).toBe(index === 0 ? 0 : Math.min(indent, indentOf(plan[index - 1]) + 1));
tests/planHelpers.ts:52
- The profile time regex requires a decimal fraction (
\d+\.\d+). If the engine reports an integer value (e.g.Execution time: 1 ms) this helper will fail even though the profile is valid. Making the fractional part optional keeps the assertion engine-agnostic while still rejecting missing timing stats.
const records = line.match(/Records produced: (\d+)/);
const time = line.match(/Execution time: (\d+\.\d+) ms/);
tests/planHelpers.ts:28
expectExecutionPlanassertstypeof line === "string"but then immediately callsoperationName(line)/indentOf(line). If a malformed reply contains a non-string entry, this will throw a TypeError instead of producing a clear assertion failure (and it can also break the rest of the validation). Add a runtime guard after the type assertion to keep the helper reporting a test failure rather than crashing.
expect(typeof line).toBe("string");
// every line names an operation, optionally followed by its arguments
expect(operationName(line)).not.toBe("");
tests/planHelpers.ts:63
expectProfileclaims to verify the number of rows yielded by the query, but it currently checksMath.max(...counts). In profiled plans, intermediate operations (e.g., scans) can produce more records than the final result after filters/limits, somaxis not equivalent to the query result count. SinceexpectExecutionPlanalready enforces the first line is the root (indent 0), assert againstcounts[0]instead.
if (recordsProduced !== undefined) {
// how many rows the query yields is a property of the query, not of the
// engine, so the client must report it whichever engine answered
expect(Math.max(...counts)).toBe(recordsProduced);
}
Co-Authored-By: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/planHelpers.ts:43
expectExecutionPlancan throw (e.g.,plan.length,plan.forEach,operationName(line)) even when it’s meant to assert malformed replies. Because Jestexpect()doesn’t stop execution, a non-array plan, an empty plan, or a non-string line will cause a TypeError and hide the real failure. Add early returns / guards so the helper fails with assertions instead of crashing.
export function expectExecutionPlan(plan: string[], minOperations = 1): void {
expect(Array.isArray(plan)).toBe(true);
expect(plan.length).toBeGreaterThanOrEqual(minOperations);
plan.forEach((line, index) => {
expect(typeof line).toBe("string");
// every line names an operation, optionally followed by its arguments
expect(operationName(line)).not.toBe("");
// indentation is what conveys nesting, so it has to be whole levels, and
// an operation can only ever be one level deeper than the one above it
const indent = indentOf(line);
expect(Number.isInteger(indent)).toBe(true);
expect(indent).toBe(index === 0 ? 0 : Math.min(indent, indentOf(plan[index - 1]) + 1));
});
tests/planHelpers.ts:12
PROFILE_STATSis currently very strict (requires a comma +\d+\.\d+ ms). That makesoperationLine()stripping fragile: if the engine outputs a valid profile with a slightly different time format,expectPlanShapewill start comparing operation+args including stats and fail. Relax the stripping regex to remove everything from| Records produced:to EOL;expectProfile()already validates the stats separately.
const INDENT = " ";
const ENGINE_ROOT_OPERATIONS = new Set(["Results", "Commit"]);
const PROFILE_STATS = /\s*\|\s*Records produced: \d+,\s*Execution time: \d+\.\d+ ms\s*$/;
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (6)
tests/planHelpers.ts:41
- The indentation assertion uses
Math.min(indent, prevIndent + 1), which obscures the intent and yields less clear failures. A directtoBeLessThanOrEqual(prevIndent + 1)expresses the constraint (indent may only increase by 1 level) more clearly.
expect(indent).toBe(index === 0 ? 0 : Math.min(indent, indentOf(plan[index - 1]) + 1));
tests/planHelpers.ts:88
- The profile time assertion only matches values with a fractional part (e.g.
0.01 ms). If an engine returns0 ms/1 ms, this will incorrectly fail even though the profile contains valid timing data.
const time = line.match(/Execution time: (\d+\.\d+) ms/);
tests/single.spec.ts:254
- The test indexes
slowLog[0]without asserting the slow log contains any entries. If the query isn't recorded (e.g. threshold not exceeded in a fast CI run), this will fail with a TypeError instead of a clear assertion failure.
const slowLog = await graph.slowLog();
expect(Array.isArray(slowLog)).toBe(true);
expect(slowLog[0].command).toBe("GRAPH.QUERY");
expect(slowLog[0].query).toBe(longQuery);
tests/cluster.spec.ts:199
- The test indexes
result[0]without asserting the slow log contains any entries. If nothing is recorded (e.g. the query still runs under the threshold), this will fail with an unhelpful TypeError.
const result = await graph.slowLog();
expect(Array.isArray(result)).toBe(true);
expect(result[0].command).toBe("GRAPH.QUERY");
expect(result[0].query).toBe(longQuery);
tests/planHelpers.ts:12
PROFILE_STATSand the profile-time matcher only accept timings with a fractional part (e.g.1.23 ms) and will fail on valid outputs like1 ms. Making the fractional part optional and allowing additional trailing stats makes these assertions more resilient to engine formatting changes while still enforcing presence of the stats.
const PROFILE_STATS = /\s*\|\s*Records produced: \d+,\s*Execution time: \d+\.\d+ ms\s*$/;
tests/graphAndQuery.spec.ts:281
- The test assumes
slowLogResults[0]exists. If the slow log is empty (e.g. threshold not exceeded on a fast run), this can throw before producing a clear Jest assertion failure. Assert the array is non-empty before indexing.
const slowLogResults = await graph.slowLog();
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
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.
The plan specs pinned exact operation names, arguments and indentation, so any change to how the engine plans a query broke them even though the client was behaving correctly. That is testing the engine's planner, not the client.
Assert what is actually the client's responsibility instead: a plan comes back, it holds at least as many operations as the query needs, every line names an operation, and the indentation that conveys nesting is well formed. Profiles additionally have to carry per operation statistics, and the row count the query yields is still checked by value since that is a property of the query rather than of the engine.
The new assertions live in
tests/planHelpers.ts. They are not a rubber stamp — each one was verified to reject a malformed plan: an empty reply, too few operations, a blank operation name, a half indent, an indent that skips a level, a root that is indented, a profile missing its statistics, and a wrong row count.Also drops a
RETURN exists((a)-[:KNOWS]->(b))from the constraint test. Its result was never asserted, so the call only added a dependency on a function that behaves differently between engines without testing anything.Separately, the slowlog tests queried 200k rows, which runs in roughly 6ms and so falls under the ~10ms slowlog threshold. Bumped to 1M rows so the query is actually recorded.
Verification
graphAndQuery,profileandconstraints: 35 passed against both engines, and the slowlog tests pass on both. Lint is clean.Not covered here
Two groups of failures on edge are engine side decisions and are deliberately left alone: the
GRAPH.INFOspecs, and settingCMD_INFO/MAX_INFO_QUERIESat run time.Summary by CodeRabbit