Nested relationship zoo tets #655
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
phpunit.xml (1)
10-10: Consider revertingstopOnFailurebefore mergingSetting
stopOnFailure="true"is useful during local development for debugging specific test failures, but it may not be ideal for CI/CD pipelines where you want to see all test failures in a single run. Consider whether this change should be committed or kept as a local development configuration.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
phpunit.xml(1 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/e2e/Adapter/Scopes/RelationshipTests.php (4)
src/Database/Adapter.php (3)
createDocument(680-680)getDocument(670-670)count(805-805)src/Database/Database.php (4)
createDocument(3580-3671)getDocument(3193-3314)count(6257-6293)findOne(6228-6243)src/Database/Document.php (3)
Document(12-462)getId(63-66)getAttribute(216-223)src/Database/Query.php (4)
getAttribute(130-133)Query(8-836)equal(370-373)select(515-518)
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/RelationshipTests.php
110-110: Avoid unused local variables such as '$iguana'. (Unused Code Rules)
(UnusedLocalVariable)
130-130: Avoid unused local variables such as '$tiger'. (Unused Code Rules)
(UnusedLocalVariable)
150-150: Avoid unused local variables such as '$lama'. (Unused Code Rules)
(UnusedLocalVariable)
189-189: Avoid unused local variables such as '$trump'. (Unused Code Rules)
(UnusedLocalVariable)
203-203: Avoid unused local variables such as '$bush'. (Unused Code Rules)
(UnusedLocalVariable)
214-214: Avoid unused local variables such as '$biden'. (Unused Code Rules)
(UnusedLocalVariable)
🔇 Additional comments (3)
tests/e2e/Adapter/Scopes/RelationshipTests.php (3)
98-109: Good improvements to test clarity and validationThe variable renaming from
$zoo1to$zooimproves readability, and the added assertions properly validate the document creation.
110-168: Excellent variable naming improvementsThe descriptive variable names (
$iguana,$tiger,$lama) make the test much more readable and intuitive compared to generic names like$animal1,$animal2, etc. The static analysis warnings about unused variables can be safely ignored as these documents are created and referenced by ID in subsequent assertions.
189-223: Clear and descriptive variable namingThe renamed variables (
$trump,$bush,$biden) enhance test readability. These are appropriately used for document creation despite the static analysis warnings.
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant ZooCollection
participant AnimalCollection
participant VeterinarianCollection
participant PresidentCollection
participant Database
TestRunner->>ZooCollection: Create zoo document
TestRunner->>AnimalCollection: Create iguana, tiger, lama documents (linked to zoo)
TestRunner->>VeterinarianCollection: Create veterinarians (linked to animals, presidents)
TestRunner->>PresidentCollection: Create presidents (linked to animals, veterinarians)
TestRunner->>Database: Query zoo with relationships and selects
Database-->>TestRunner: Return zoo with related animals (filtered by selects)
Database-->>TestRunner: Return animals with related veterinarians and presidents
Database-->>TestRunner: Return veterinarians with related presidents and animals
Database-->>TestRunner: Return presidents with related animals and votes
TestRunner->>TestRunner: Assert relationship and attribute integrity
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
322-377: Excellent selective query testing with one unresolved commentThis is sophisticated testing of relationship query selection that properly validates:
- Basic
*select behavior (no relationship resolution)- Partial relationship selection (
animals.*)- Nested relationship resolution (
animals.president.*)However, the commented assertion on line 347 still needs attention as flagged in previous reviews.
Action Required: Please address the commented assertion:
//$this->assertArrayNotHasKey('presidents', $veterinarian); // ???Either uncomment it if the check is valid, remove it if not needed, or add a clear comment explaining why it's intentionally omitted.
🧹 Nitpick comments (2)
tests/e2e/Adapter/Scopes/RelationshipTests.php (2)
110-167: Excellent variable naming improvementsThe rename from generic
$animal1/$animal2/$animal3to descriptive$iguana/$tiger/$lamasignificantly improves test readability and makes the test structure much clearer.Note: While static analysis flags these as unused variables, they serve as clear documentation of the test data structure and could be valuable for debugging. Consider if you want to keep them for clarity or remove them to satisfy static analysis.
189-223: Clear and descriptive variable namingThe rename from generic
$president1/$president2/$president3to specific$trump/$bush/$bidenmakes the test much more readable and easier to follow.Same note as with the animal variables - these are flagged as unused but serve as useful documentation of the test data structure.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/Adapter/Scopes/RelationshipTests.php(8 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/RelationshipTests.php
110-110: Avoid unused local variables such as '$iguana'. (Unused Code Rules)
(UnusedLocalVariable)
130-130: Avoid unused local variables such as '$tiger'. (Unused Code Rules)
(UnusedLocalVariable)
150-150: Avoid unused local variables such as '$lama'. (Unused Code Rules)
(UnusedLocalVariable)
189-189: Avoid unused local variables such as '$trump'. (Unused Code Rules)
(UnusedLocalVariable)
203-203: Avoid unused local variables such as '$bush'. (Unused Code Rules)
(UnusedLocalVariable)
214-214: Avoid unused local variables such as '$biden'. (Unused Code Rules)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (MariaDB)
🔇 Additional comments (5)
tests/e2e/Adapter/Scopes/RelationshipTests.php (5)
98-109: LGTM! Good variable naming and immediate verificationThe variable rename from
$zoo1to$zooimproves readability, and adding immediate assertions after document creation is excellent practice for verifying the operation succeeded and the document has expected relationship attributes.
225-245: Comprehensive zoo data validationExcellent test coverage that validates document structure, relationship attributes, proper counts, and nested relationships. Testing both
getDocument()andfindOne()methods ensures comprehensive coverage of different retrieval patterns.
246-272: Thorough veterinarian relationship validationWell-structured assertions that validate the bidirectional relationships (presidents and animals) and ensure nested relationship attributes are properly populated. Good coverage of the complex relationship structure.
273-299: Complete animal relationship validationExcellent validation of animal documents ensuring all relationship attributes (zoo, veterinarian, president) are properly populated and accessible. The nested attribute checks confirm the relationship data integrity.
300-321: Solid president relationship testingGood validation of president documents covering both animal relationships and voting relationships with veterinarians. Testing different vote counts (2 vs 0) ensures the relationships work correctly across various scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Database/Database.php (1)
3327-3335: Selective relationship resolution looks good; consider a small robustness tweakThe explicit loop that filters only relationship attributes and respects nested selections aligns with how
processRelationshipQueries()builds$selects. This avoids unnecessary fetches and matches the new tests.Optionally, to be defensive against metadata inconsistencies, fall back to
'$id'whenkeyis absent:- foreach ($attributes as $attribute) { - if ($attribute['type'] === Database::VAR_RELATIONSHIP) { - if (empty($selects) || array_key_exists($attribute['key'], $selects)) { - $relationships[] = $attribute; - } - } - } + foreach ($attributes as $attribute) { + if ($attribute['type'] !== Database::VAR_RELATIONSHIP) { + continue; + } + $attrKey = $attribute['key'] ?? $attribute['$id'] ?? null; + if ($attrKey === null) { + continue; + } + if (empty($selects) || array_key_exists($attrKey, $selects)) { + $relationships[] = $attribute; + } + }tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
110-128: Clean up unused locals flagged by static analysisThe return values are not used; keep the side effects by dropping the assignments:
- $iguana = $database->createDocument('__animals', new Document([ + $database->createDocument('__animals', new Document([ ... ])); - $tiger = $database->createDocument('__animals', new Document([ + $database->createDocument('__animals', new Document([ ... ])); - $lama = $database->createDocument('__animals', new Document([ + $database->createDocument('__animals', new Document([ ... ])); - $trump = $database->createDocument('presidents', new Document([ + $database->createDocument('presidents', new Document([ ... ])); - $bush = $database->createDocument('presidents', new Document([ + $database->createDocument('presidents', new Document([ ... ])); - $biden = $database->createDocument('presidents', new Document([ + $database->createDocument('presidents', new Document([ ... ]));This addresses PHPMD “UnusedLocalVariable” hints without altering behavior. If you prefer to keep the names for readability, prefix with
_to signal intentional non-use.Also applies to: 130-148, 150-167, 189-201, 203-213, 214-223
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Database.php(1 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(8 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/RelationshipTests.php
110-110: Avoid unused local variables such as '$iguana'. (Unused Code Rules)
(UnusedLocalVariable)
130-130: Avoid unused local variables such as '$tiger'. (Unused Code Rules)
(UnusedLocalVariable)
150-150: Avoid unused local variables such as '$lama'. (Unused Code Rules)
(UnusedLocalVariable)
189-189: Avoid unused local variables such as '$trump'. (Unused Code Rules)
(UnusedLocalVariable)
203-203: Avoid unused local variables such as '$bush'. (Unused Code Rules)
(UnusedLocalVariable)
214-214: Avoid unused local variables such as '$biden'. (Unused Code Rules)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MySQL)
Summary by CodeRabbit