Refactor ProjectsConsoleClientTest to remove test dependencies#10655
Refactor ProjectsConsoleClientTest to remove test dependencies#10655
Conversation
- Remove @Depends annotations to make tests independent - Each test now creates its own team and project instead of relying on shared state - Replace sleep() calls with assertEventually() for more reliable async testing - Change test methods to return void instead of passing data between tests - Remove unnecessary sleep() call that was causing test flakiness
📝 WalkthroughWalkthroughThis PR refactors tests in tests/e2e/Services/Projects/ProjectsConsoleClientTest.php to be self-contained. Test methods that previously accepted a $data parameter and returned arrays/objects were converted to void methods that create required entities (teams, projects) within each test. Inter-test data flow and returned payloads were removed or inlined. Fixed sleep-based waits were replaced with eventual/assertive synchronization to reduce timing flakiness. Multiple method signatures were updated to reflect these changes. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (3)
⏰ 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). (2)
🔇 Additional comments (2)
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! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (1)
115-1466: Consider extracting common setup logic into a helper method.Multiple test methods now duplicate the same team and project creation logic (lines 117-141, 233-291, 496-519, 609-633, 674-698, 745-769, 1108-1131, 1177-1201, 1306-1330). This increases maintenance burden—if the setup process changes, all tests need updating.
Consider extracting this into a reusable helper method:
private function createTestTeamAndProject(string $teamName, string $projectName): array { $team = $this->client->call(Client::METHOD_POST, '/teams', array_merge([ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], ], $this->getHeaders()), [ 'teamId' => ID::unique(), 'name' => $teamName, ]); $this->assertEquals(201, $team['headers']['status-code']); $project = $this->client->call(Client::METHOD_POST, '/projects', array_merge([ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], ], $this->getHeaders()), [ 'projectId' => ID::unique(), 'name' => $projectName, 'teamId' => $team['body']['$id'], 'region' => System::getEnv('_APP_REGION', 'default') ]); $this->assertEquals(201, $project['headers']['status-code']); return [ 'teamId' => $team['body']['$id'], 'projectId' => $project['body']['$id'] ]; }Then update each test to use:
$setup = $this->createTestTeamAndProject('Test Team', 'Test Project'); $id = $setup['projectId'];
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (2)
tests/e2e/Client.php (1)
Client(8-342)tests/e2e/Scopes/ProjectCustom.php (1)
getProject(21-185)
⏰ 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). (1)
- GitHub Check: scan
🔇 Additional comments (4)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (4)
115-159: LGTM! Test is now properly isolated.The test correctly creates its own team and project for testing duplicate project creation, making it independent of other tests.
3567-3575: Verify if assertEventually is necessary here.The
assertEventuallywrapper seems unnecessary for a simple GET request that checks the platform count. Unless platform creation is known to be asynchronous with eventual consistency issues, a direct assertion should suffice.If platform creation is synchronous and the count should be immediately available, consider removing the
assertEventuallywrapper:- $this->assertEventually(function () use ($id) { - $response = $this->client->call(Client::METHOD_GET, '/projects/' . $id . '/platforms', array_merge([ - 'content-type' => 'application/json', - 'x-appwrite-project' => $this->getProject()['$id'], - ], $this->getHeaders()), []); + $response = $this->client->call(Client::METHOD_GET, '/projects/' . $id . '/platforms', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), []); - $this->assertEquals(200, $response['headers']['status-code']); - $this->assertEquals(8, $response['body']['total']); - }); + $this->assertEquals(200, $response['headers']['status-code']); + $this->assertEquals(8, $response['body']['total']);However, if there's a known issue with platform creation being asynchronous, keep the wrapper and add a comment explaining why it's needed.
4226-4234: LGTM! Appropriate use of assertEventually.Using
assertEventuallyhere is correct—after deleting project1, we need to verify that project2's user wasn't accidentally affected. The eventual assertion accounts for any async cleanup or cache invalidation delays.
231-334: Ensure deterministic ordering in project listing
Add an explicit ordering clause to the GET /projects call (e.g.,
'queries' => [ Query::orderAsc('createdAt')->toString() ]at line 297)
or change the assertion to locate the project by its ID instead of assuming it’s at index 0.
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (1)
3482-3496: Incomplete refactoring: test still depends on shared state.This test was only partially refactored. While
sleep()was replaced withassertEventually()(which is good), the test still has the@depends testCreateProjectPlatformannotation and accepts the$dataparameter. It continues to rely on shared state from the dependent test.For consistency with the other refactored tests in this PR (e.g.,
testGetProject,testUpdateProject,testUpdateProjectOAuth), this test should also be made independent by:
- Removing the
@depends testCreateProjectPlatformannotation- Removing the
$dataparameter and changing return type tovoid- Creating its own team and project within the test body
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (3)
tests/e2e/Client.php (1)
Client(8-342)tests/e2e/Scopes/ProjectCustom.php (1)
getProject(21-185)tests/e2e/Scopes/Scope.php (1)
getHeaders(145-145)
🔇 Additional comments (8)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (8)
115-159: LGTM!The test has been successfully refactored to be self-contained. It now creates its own team and project instead of depending on shared state from
testCreateProject.
437-462: LGTM!The test has been correctly refactored to be independent. It creates its own team and project instead of relying on data from
testCreateProject.
550-576: LGTM!The test has been correctly refactored to be independent. It creates its own team and project setup instead of depending on shared state.
664-690: LGTM!The test has been correctly refactored to be independent. It creates its own team and project instead of relying on data from other tests.
1027-1052: LGTM!The test has been correctly refactored to be independent. It creates its own team and project instead of depending on shared state.
1096-1122: LGTM!The test has been correctly refactored to be independent. It creates its own team and project setup for testing OAuth functionality.
1225-1251: LGTM!The test has been correctly refactored to be independent. It creates its own team and project for testing authentication status updates.
4147-4155: LGTM!Good refactoring to replace implicit timing assumptions with explicit eventual consistency checks. The
assertEventually()wrapper ensures the test waits for the asynchronous deletion to complete before verifying that project 2's user is still accessible.
Summary
@dependsannotations to make tests independent and more maintainablesleep()calls withassertEventually()for more reliable async testingvoidinstead of passing data arrays between testssleep(5)call that was causing test flakinessTest plan
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php