Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds debugging instrumentation to SQL query construction, disables most e2e test suites in the base adapter class (retaining only DocumentTests), configures PHPUnit to stop execution on first test failure, and appends an inert test block to DocumentTests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@phpunit.xml`:
- Line 10: The shared PHPUnit configuration currently enables global fast-fail
via the stopOnFailure="true" attribute, which halts CI on the first test
failure; remove or set stopOnFailure to "false" in the phpunit.xml so the test
suite runs to completion and surfaces all failures; locate the stopOnFailure
attribute in the phpunit.xml root <phpunit> element and change or remove it, and
ensure any project-level CI overrides (if desired) handle fast-fail behavior per
job rather than committing a global fast-fail.
In `@src/Database/Adapter/SQL.php`:
- Around line 3151-3157: Remove the unconditional var_dump calls that leak SQL
and bind data: delete the var_dump($sql) and the var_dump($key)/var_dump($value)
calls in the find() execution path (the places that prepare the statement and
iterate $binds), and if you need runtime diagnostics replace them with a
conditional debug log (e.g. $this->logger->debug(...) or a check against a debug
flag) so sensitive data is not printed to output in normal operation.
In `@tests/e2e/Adapter/Base.php`:
- Around line 26-38: The Base e2e test harness currently only uses DocumentTests
and has commented out the other trait suites (CollectionTests,
CustomDocumentTypeTests, AttributeTests, IndexTests, OperatorTests,
PermissionTests, RelationshipTests, SpatialTests, SchemalessTests,
ObjectAttributeTests, VectorTests, GeneralTests); restore CI coverage by
uncommenting those `use` lines in tests/e2e/Adapter/Base.php so the full set of
trait test suites are included again, ensuring the test class imports all
previously-disabled traits and runs their tests alongside DocumentTests.
In `@tests/e2e/Adapter/Scopes/DocumentTests.php`:
- Around line 2181-2189: The test currently contains a guaranteed failing
assertion (999 vs 999999) and no real checks for the queries; replace the bogus
assert with meaningful assertions that validate the results from $database->find
when using Query::search('ft', $phrase). For each $phrase in $phrases, capture
the result of $database->find($collection, [ Query::search('ft', $phrase) ]) and
assert expected behavior (e.g., assert that the returned result is not null,
assertCount > 0, or assert the returned documents contain the expected value
"Álvaro"); update the loop to perform these assertions instead of the hardcoded
failing check so the test verifies actual query output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f95a2c88-d6dc-4d04-a823-9ee0e843e770
📒 Files selected for processing (4)
phpunit.xmlsrc/Database/Adapter/SQL.phptests/e2e/Adapter/Base.phptests/e2e/Adapter/Scopes/DocumentTests.php
| processIsolation="false" | ||
| stopOnFailure="false" | ||
| > | ||
| stopOnFailure="true"> |
Contributor
There was a problem hiding this comment.
Avoid enabling global fast-fail in committed PHPUnit config.
Setting stopOnFailure="true" in shared config cuts off CI at the first failure, which hides additional failures and weakens debugging signal for contributors.
Suggested change
- stopOnFailure="true">
+ stopOnFailure="false">📝 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
| stopOnFailure="true"> | |
| stopOnFailure="false"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@phpunit.xml` at line 10, The shared PHPUnit configuration currently enables
global fast-fail via the stopOnFailure="true" attribute, which halts CI on the
first test failure; remove or set stopOnFailure to "false" in the phpunit.xml so
the test suite runs to completion and surfaces all failures; locate the
stopOnFailure attribute in the phpunit.xml root <phpunit> element and change or
remove it, and ensure any project-level CI overrides (if desired) handle
fast-fail behavior per job rather than committing a global fast-fail.
Comment on lines
+3151
to
+3157
| var_dump($sql); | ||
| try { | ||
| $stmt = $this->getPDO()->prepare($sql); | ||
|
|
||
| foreach ($binds as $key => $value) { | ||
| var_dump($key); | ||
| var_dump($value); |
Contributor
There was a problem hiding this comment.
Remove unconditional debug dumps from find() execution path.
Line 3151 and Lines 3156-3157 dump raw SQL and bind values on every request. This can leak sensitive data and can break consumers expecting clean response output.
Proposed fix
- $sql = $this->trigger(Database::EVENT_DOCUMENT_FIND, $sql);
-var_dump($sql);
+ $sql = $this->trigger(Database::EVENT_DOCUMENT_FIND, $sql);
try {
$stmt = $this->getPDO()->prepare($sql);
foreach ($binds as $key => $value) {
- var_dump($key);
- var_dump($value);
if (gettype($value) === 'double') {
$stmt->bindValue($key, $this->getFloatPrecision($value), \PDO::PARAM_STR);
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Adapter/SQL.php` around lines 3151 - 3157, Remove the
unconditional var_dump calls that leak SQL and bind data: delete the
var_dump($sql) and the var_dump($key)/var_dump($value) calls in the find()
execution path (the places that prepare the statement and iterate $binds), and
if you need runtime diagnostics replace them with a conditional debug log (e.g.
$this->logger->debug(...) or a check against a debug flag) so sensitive data is
not printed to output in normal operation.
Comment on lines
+26
to
+38
| // use CollectionTests; | ||
| // use CustomDocumentTypeTests; | ||
| use DocumentTests; | ||
| use AttributeTests; | ||
| use IndexTests; | ||
| use OperatorTests; | ||
| use PermissionTests; | ||
| use RelationshipTests; | ||
| use SpatialTests; | ||
| use SchemalessTests; | ||
| use ObjectAttributeTests; | ||
| use VectorTests; | ||
| use GeneralTests; | ||
| // use AttributeTests; | ||
| // use IndexTests; | ||
| // use OperatorTests; | ||
| // use PermissionTests; | ||
| // use RelationshipTests; | ||
| // use SpatialTests; | ||
| // use SchemalessTests; | ||
| // use ObjectAttributeTests; | ||
| // use VectorTests; | ||
| // use GeneralTests; |
Contributor
There was a problem hiding this comment.
Re-enable the disabled e2e trait suites in base test harness.
Commenting out most trait use lines here significantly reduces CI signal and can hide regressions outside DocumentTests, which is high-risk for a production PR.
Proposed fix
-// use CollectionTests;
-// use CustomDocumentTypeTests;
+ use CollectionTests;
+ use CustomDocumentTypeTests;
use DocumentTests;
-// use AttributeTests;
-// use IndexTests;
-// use OperatorTests;
-// use PermissionTests;
-// use RelationshipTests;
-// use SpatialTests;
-// use SchemalessTests;
-// use ObjectAttributeTests;
-// use VectorTests;
-// use GeneralTests;
+ use AttributeTests;
+ use IndexTests;
+ use OperatorTests;
+ use PermissionTests;
+ use RelationshipTests;
+ use SpatialTests;
+ use SchemalessTests;
+ use ObjectAttributeTests;
+ use VectorTests;
+ use GeneralTests;📝 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
| // use CollectionTests; | |
| // use CustomDocumentTypeTests; | |
| use DocumentTests; | |
| use AttributeTests; | |
| use IndexTests; | |
| use OperatorTests; | |
| use PermissionTests; | |
| use RelationshipTests; | |
| use SpatialTests; | |
| use SchemalessTests; | |
| use ObjectAttributeTests; | |
| use VectorTests; | |
| use GeneralTests; | |
| // use AttributeTests; | |
| // use IndexTests; | |
| // use OperatorTests; | |
| // use PermissionTests; | |
| // use RelationshipTests; | |
| // use SpatialTests; | |
| // use SchemalessTests; | |
| // use ObjectAttributeTests; | |
| // use VectorTests; | |
| // use GeneralTests; | |
| use CollectionTests; | |
| use CustomDocumentTypeTests; | |
| use DocumentTests; | |
| use AttributeTests; | |
| use IndexTests; | |
| use OperatorTests; | |
| use PermissionTests; | |
| use RelationshipTests; | |
| use SpatialTests; | |
| use SchemalessTests; | |
| use ObjectAttributeTests; | |
| use VectorTests; | |
| use GeneralTests; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/Adapter/Base.php` around lines 26 - 38, The Base e2e test harness
currently only uses DocumentTests and has commented out the other trait suites
(CollectionTests, CustomDocumentTypeTests, AttributeTests, IndexTests,
OperatorTests, PermissionTests, RelationshipTests, SpatialTests,
SchemalessTests, ObjectAttributeTests, VectorTests, GeneralTests); restore CI
coverage by uncommenting those `use` lines in tests/e2e/Adapter/Base.php so the
full set of trait test suites are included again, ensuring the test class
imports all previously-disabled traits and runs their tests alongside
DocumentTests.
Comment on lines
+2181
to
+2189
| $phrases = ["Álvaro"]; | ||
|
|
||
| foreach ($phrases as $phrase) { | ||
| $database->find($collection, [ | ||
| Query::search('ft', $phrase), | ||
| ]); | ||
| } | ||
|
|
||
| $this->assertEquals(999, 999999); |
Contributor
There was a problem hiding this comment.
This block introduces a guaranteed test failure and no meaningful verification.
Line 2189 always fails (999 !== 999999), and the loop currently performs queries without asserting behavior.
Suggested fix
- $phrases = ["Álvaro"];
-
- foreach ($phrases as $phrase) {
- $database->find($collection, [
- Query::search('ft', $phrase),
- ]);
- }
-
- $this->assertEquals(999, 999999);
+ $phrases = ['Álvaro'];
+
+ foreach ($phrases as $phrase) {
+ $documents = $database->find($collection, [
+ Query::search('ft', $phrase),
+ ]);
+ $this->assertIsArray($documents);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/Adapter/Scopes/DocumentTests.php` around lines 2181 - 2189, The
test currently contains a guaranteed failing assertion (999 vs 999999) and no
real checks for the queries; replace the bogus assert with meaningful assertions
that validate the results from $database->find when using Query::search('ft',
$phrase). For each $phrase in $phrases, capture the result of
$database->find($collection, [ Query::search('ft', $phrase) ]) and assert
expected behavior (e.g., assert that the returned result is not null,
assertCount > 0, or assert the returned documents contain the expected value
"Álvaro"); update the loop to perform these assertions instead of the hardcoded
failing check so the test verifies actual query output.
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
1 participant
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.
Summary by CodeRabbit
Tests
Chores