fix: testListSites 2 not matching expected 1#10726
Conversation
📝 WalkthroughWalkthroughThe test file tests/e2e/Services/Sites/SitesCustomServerTest.php was updated. In testListSites the created site names were changed from "Test Site" / "Test Site 2" to "Test List Sites" / "Test List Sites 2". The test now calls listSites with a search parameter matching the new names instead of listing without filters, and related assertions were updated to expect the new names and search-driven results. No public APIs or helper methods were modified. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/e2e/Services/Sites/SitesCustomServerTest.php (2)
653-690: Consider adding search filtering for better test isolation.These pagination and filter tests don't include the
searchparameter, which means they could still be affected by sites created by other concurrent tests. For example:
- Line 670 expects 0 results with
offset(1), assuming only 1 site exists- Line 680 expects exactly 1 enabled site
- Line 690 expects 0 disabled sites
Adding the search parameter to these test cases would make them more reliable in parallel test execution.
Apply this diff to add search filtering to pagination tests:
// Test pagination offset $sites = $this->listSites([ + 'search' => 'Test List Sites', 'queries' => [ Query::offset(1)->toString(), ], ]);Similar changes should be applied to the filter tests at lines 673-690.
702-708: Framework search may match sites from other tests.The search for framework
'other'could match sites created by other concurrent tests, as many tests in this file useframework => 'other'(e.g.,testCreateSite,testGetSite,testUpdateSite). Consider combining this with the site name search for better isolation.Apply this diff:
// Test search framework $sites = $this->listSites([ - 'search' => 'other' + 'search' => 'Test List Sites', + 'queries' => [ + Query::equal('framework', ['other'])->toString(), + ], ]);
🧹 Nitpick comments (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
735-753: Cursor pagination should include search parameter for consistency.The cursor values are derived from the search-filtered results at line 724, but the subsequent
listSitescalls at lines 735 and 745 don't include the same search filter. This creates an inconsistency where the cursor is from a filtered result set, but the pagination query operates on an unfiltered set.Apply this diff to add search filtering:
$sites1 = $this->listSites([ + 'search' => 'Test List Sites', 'queries' => [ Query::cursorAfter(new Document(['$id' => $sites['body']['sites'][0]['$id']]))->toString(), ], ]);$sites2 = $this->listSites([ + 'search' => 'Test List Sites', 'queries' => [ Query::cursorBefore(new Document(['$id' => $sites['body']['sites'][1]['$id']]))->toString(), ], ]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
tests/e2e/Services/Sites/SitesBase.php (1)
listSites(207-215)
⏰ 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). (20)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (2)
tests/e2e/Services/Sites/SitesCustomServerTest.php (2)
637-650: Good test isolation improvement.The addition of the search parameter and more specific site name properly isolates this test from other concurrent tests.
717-733: LGTM!The search correctly leverages partial string matching to return both sites. The assertions properly verify both site names are present in the results.
✨ Benchmark results
⚡ Benchmark Comparison
|
2 participants
fixes -
this can happen due to parallel executions of sites tests, that can lead to more sites being there than expected. so we narrow the scope of the lists function to only check for sites in that scope by using custom names and search