Conversation
📝 WalkthroughWalkthroughAdds multiple test helper traits and URL builders (ApiLegacy, ApiTablesDB, DatabasesUrlHelpers, SchemaPolling, ACIDBase, TransactionPermissionsBase), a shared DatabaseFixture, retry and test-finished PHPUnit subscribers, and a TestHook extension. Introduces many scaffold end-to-end test classes for legacy and TablesDB flows, refactors transaction tests to trait-based bases, and adds extensive permission/permission‑team test suites. Removes numerous legacy/TablesDB test files and a permissions helper trait. Migrates PHPUnit docblock annotations and data providers to PHP 8 attributes (making providers static), increases test client timeout, updates composer dev dependencies (phpunit → 12.*, adds paratest), and updates CI workflow to use paratest. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ 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 |
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: 8
🤖 Fix all issues with AI agents
In @tests/e2e/Services/Databases/LegacyCustomServerTest.php:
- Line 12: The class LegacyCustomServerTest references the DatabasesBase trait
but lacks an import; add the appropriate PHP "use" import for the DatabasesBase
trait (matching its declared namespace) at the top of the file and ensure the
class still uses the trait (e.g., the LegacyCustomServerTest class contains "use
DatabasesBase;") so PHP can locate the trait at runtime.
In @tests/e2e/Services/Databases/TablesDBConsoleClientTest.php:
- Line 12: The test class TablesDBConsoleClientTest uses the DatabasesBase trait
but does not import it; add the missing use/import for the DatabasesBase trait
(the trait symbol DatabasesBase referenced in TablesDBConsoleClientTest) at the
top of the file so PHP can resolve the trait, ensuring the namespace matches
where DatabasesBase is declared and no runtime fatal error occurs.
In @tests/e2e/Services/Databases/Transactions/ACIDBase.php:
- Line 288: The test currently accepts both 400 and 500 via
$this->assertContains($response['headers']['status-code'], [400, 500], ...),
which hides server errors; change the assertion to require a 400 status for
validation failures (e.g. replace the assertContains with
$this->assertEquals(400, $response['headers']['status-code'], 'Transaction
commit should fail due to validation. Response: ' .
json_encode($response['body'])) or an equivalent strict assertion) so only Bad
Request passes and 500s fail the test.
In
@tests/e2e/Services/Databases/Transactions/LegacyTransactionsCustomClientTest.php:
- Line 13: Add the missing import for the TransactionsBase trait: include a use
statement for TransactionsBase at the top of the test file (with other imports)
so the trait reference resolves; ensure the use statement references the trait's
correct namespace and is placed alongside the other use declarations before the
class declaration that uses TransactionsBase.
In
@tests/e2e/Services/Databases/Transactions/LegacyTransactionsCustomServerTest.php:
- Line 13: The test references the TransactionsBase trait but never imports it;
add a proper "use TransactionsBase;" import at the top of the class/file where
other trait imports live so the TransactionsBase trait is available to the test
(ensure you place the use statement alongside other use declarations and/or
trait imports to match project namespace conventions and resolve the symbol
TransactionsBase).
In @tests/e2e/Services/Databases/Transactions/TablesDBACIDTest.php:
- Line 13: The file references the ACIDBase trait but does not import its
namespace; add the correct import for the trait (e.g. add a top-level use
statement like use <Actual\Trait\Namespace>\ACIDBase;) or reference it with its
fully-qualified name where the trait is used (e.g. use
\Actual\Trait\Namespace\ACIDBase;) so PHP can locate the trait and avoid the
fatal runtime error.
In
@tests/e2e/Services/Databases/Transactions/TablesDBTransactionPermissionsCustomClientTest.php:
- Line 12: The file uses the TransactionPermissionsBase trait but lacks its
namespace import; add a top-level PHP "use" import for the
TransactionPermissionsBase trait that matches its actual namespace (so the trait
referenced by the class resolves), keeping the existing in-class "use
TransactionPermissionsBase;" line intact.
In
@tests/e2e/Services/Databases/Transactions/TablesDBTransactionsConsoleClientTest.php:
- Line 13: The TransactionsBase trait is referenced but not imported; add the
missing use statement for the TransactionsBase trait at the top of the test
class file so PHP can resolve it (e.g., add "use TransactionsBase;" or the
correct namespaced import matching the trait's namespace where the other imports
are declared), ensuring the class that uses TransactionsBase can compile and
run.
🧹 Nitpick comments (9)
tests/e2e/Services/Databases/LegacyConsoleClientTest.php (1)
1-16: Looks fine; double-check namespace/file-path convention for “Legacy” tests.*
If the suite expectsLegacy*tests to live under aTests\E2E\Services\Databases\Legacynamespace (PSR-4-ish), this could be a discovery/autoload footgun; if PHPUnit loads by path, it’s fine.tests/e2e/Services/Databases/Permissions/DatabasesPermissionsBase.php (1)
15-47: Duplicate URL helper methods – consider usingDatabasesUrlHelperstrait instead.These four URL helper methods (
getDatabaseUrl,getContainerUrl,getSchemaUrl,getRecordUrl) duplicate the implementations intests/e2e/Traits/DatabasesUrlHelpers.php. Additionally, thegetSchemaUrlimplementation here differs fromDatabasesUrlHelpers– the trait version has special handling for relationship attributes that this version lacks.Consider using the
DatabasesUrlHelperstrait to avoid duplication and ensure consistent behavior:trait DatabasesPermissionsBase { + use \Tests\E2E\Traits\DatabasesUrlHelpers; + public array $users = []; public array $teams = []; - // URL Helper Methods - uses methods from ApiLegacy/ApiTablesDB traits - protected function getDatabaseUrl(string $databaseId = ''): string - { - ... - } - // ... remove other duplicated URL methodstests/e2e/Services/Databases/Permissions/LegacyPermissionsGuestTest.php (1)
86-86: Consider replacingsleep()with a polling mechanism for attribute readiness.Using fixed
sleep(2)delays is fragile and can cause flaky tests if attribute indexing takes longer than expected, or wastes time if it completes faster. A polling approach that checks attribute status would be more reliable.tests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.php (1)
96-99: Remove debug output statements before merging.These
echostatements will pollute test output in CI/CD pipelines. Consider using PHPUnit's assertion messages or removing them entirely since the assertion on line 100 will fail with a clear message if the status code is wrong.🔧 Suggested fix
// This should fail with 401 Unauthorized - if ($staged['headers']['status-code'] !== 401) { - echo "\nDEBUG - Actual response code: " . $staged['headers']['status-code'] . "\n"; - echo "DEBUG - Response body: " . json_encode($staged['body'], JSON_PRETTY_PRINT) . "\n"; - } - $this->assertEquals(401, $staged['headers']['status-code']); + $this->assertEquals(401, $staged['headers']['status-code'], + 'Expected 401 Unauthorized, got: ' . $staged['headers']['status-code'] . + '. Response: ' . json_encode($staged['body']));tests/e2e/Traits/DatabaseFixture.php (2)
56-77: Implicit method dependencies not defined in trait.The trait calls
getSchemaApiConfig(),waitForAllAttributes(), andwaitForAllIndexes()(lines 58, 184-185, 200) which are not defined within this trait. This creates implicit dependencies on the consuming class.Consider one of:
- Declaring abstract methods in the trait to make dependencies explicit
- Adding a docblock documenting required methods
- Using the existing
ApiLegacy/ApiTablesDBtraits' methods instead ofgetSchemaApiConfig()📖 Example: Document dependencies
/** * Provides pre-created database fixtures for read-only tests. * * Required methods from consuming class: * - getSchemaApiConfig(): array * - waitForAllAttributes(string $databaseId, string $collectionId): void * - waitForAllIndexes(string $databaseId, string $collectionId): void * - getProject(): array * - getHeaders(): array * - getUser(): array */ trait DatabaseFixture
74-77: Consider using trait methods instead of hardcoded key names.The conditional key assignments duplicate logic that exists in
ApiLegacyandApiTablesDBtraits. Using the trait methods would be more consistent:♻️ Suggested refactor
- $collectionKey = $isTablesDB ? 'tableId' : 'collectionId'; - $docKey = $isTablesDB ? 'rowId' : 'documentId'; - $docEndpoint = $isTablesDB ? 'rows' : 'documents'; + $collectionKey = $this->getContainerIdParam(); + $docKey = $this->getRecordIdParam(); + $docEndpoint = $this->getRecordResource();This requires the consuming class to use an API trait (
ApiLegacyorApiTablesDB), which aligns with the PR's trait composition pattern.tests/e2e/Services/Databases/Permissions/TablesDBPermissionsTeamTest.php (1)
21-22: Consider using unique database IDs for test isolation.The hardcoded
databaseId = 'testpermissiondb'could cause test conflicts if tests run in parallel or if cleanup fails. Consider usingID::unique()like other tests in this PR.Suggested approach
public array $collections = []; - public string $databaseId = 'testpermissiondb'; + public string $databaseId = '';Then in
createCollections():+ $this->databaseId = ID::unique(); $db = $this->client->call( Client::METHOD_POST, $this->getDatabaseUrl(), $this->getServerHeader(), [ 'databaseId' => $this->databaseId,tests/e2e/Services/Databases/Permissions/LegacyPermissionsTeamTest.php (1)
21-22: Same hardcoded databaseId concern as TablesDBPermissionsTeamTest.Consider using
ID::unique()for test isolation, as noted in the TablesDB variant review.tests/e2e/Traits/DatabasesUrlHelpers.php (1)
12-61: Investigate getSchemaUrl duplication between DatabasesPermissionsBase and DatabasesUrlHelpers.Both files define URL helper methods, but implementations differ. Most notably:
getSchemaUrlin DatabasesPermissionsBase lacks the relationship-specific logic present in DatabasesUrlHelpers (relationship URLs need/attributes/{key}/relationshipvs/attributes/{type}/{key})- Parameter types differ (
stringvs?stringfor optional parameters)- DatabasesUrlHelpers includes two additional methods (
getTransactionUrl,getIndexUrl) not in DatabasesPermissionsBaseVerify whether both versions of
getSchemaUrlare needed for different test scenarios or if one implementation should be unified. If the relationship logic should apply globally, consolidate to avoid future divergence.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (54)
tests/e2e/Scopes/ApiLegacy.phptests/e2e/Scopes/ApiTablesDB.phptests/e2e/Services/Databases/DatabasesBase.phptests/e2e/Services/Databases/Legacy/DatabasesConsoleClientTest.phptests/e2e/Services/Databases/Legacy/DatabasesCustomClientTest.phptests/e2e/Services/Databases/Legacy/DatabasesCustomServerTest.phptests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsGuestTest.phptests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsTeamTest.phptests/e2e/Services/Databases/Legacy/Transactions/TransactionsBase.phptests/e2e/Services/Databases/Legacy/Transactions/TransactionsConsoleClientTest.phptests/e2e/Services/Databases/Legacy/Transactions/TransactionsCustomClientTest.phptests/e2e/Services/Databases/Legacy/Transactions/TransactionsCustomServerTest.phptests/e2e/Services/Databases/LegacyConsoleClientTest.phptests/e2e/Services/Databases/LegacyCustomClientTest.phptests/e2e/Services/Databases/LegacyCustomServerTest.phptests/e2e/Services/Databases/Permissions/DatabasesPermissionsBase.phptests/e2e/Services/Databases/Permissions/LegacyPermissionsGuestTest.phptests/e2e/Services/Databases/Permissions/LegacyPermissionsMemberTest.phptests/e2e/Services/Databases/Permissions/LegacyPermissionsTeamTest.phptests/e2e/Services/Databases/Permissions/TablesDBPermissionsGuestTest.phptests/e2e/Services/Databases/Permissions/TablesDBPermissionsMemberTest.phptests/e2e/Services/Databases/Permissions/TablesDBPermissionsTeamTest.phptests/e2e/Services/Databases/TablesDB/DatabasesBase.phptests/e2e/Services/Databases/TablesDB/DatabasesConsoleClientTest.phptests/e2e/Services/Databases/TablesDB/DatabasesCustomClientTest.phptests/e2e/Services/Databases/TablesDB/DatabasesCustomServerTest.phptests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsGuestTest.phptests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsMemberTest.phptests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsScope.phptests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsTeamTest.phptests/e2e/Services/Databases/TablesDB/Transactions/ACIDTest.phptests/e2e/Services/Databases/TablesDB/Transactions/PermissionsCustomClientTest.phptests/e2e/Services/Databases/TablesDB/Transactions/TransactionsConsoleClientTest.phptests/e2e/Services/Databases/TablesDB/Transactions/TransactionsCustomClientTest.phptests/e2e/Services/Databases/TablesDB/Transactions/TransactionsCustomServerTest.phptests/e2e/Services/Databases/TablesDBConsoleClientTest.phptests/e2e/Services/Databases/TablesDBCustomClientTest.phptests/e2e/Services/Databases/TablesDBCustomServerTest.phptests/e2e/Services/Databases/Transactions/ACIDBase.phptests/e2e/Services/Databases/Transactions/LegacyACIDTest.phptests/e2e/Services/Databases/Transactions/LegacyTransactionPermissionsCustomClientTest.phptests/e2e/Services/Databases/Transactions/LegacyTransactionsConsoleClientTest.phptests/e2e/Services/Databases/Transactions/LegacyTransactionsCustomClientTest.phptests/e2e/Services/Databases/Transactions/LegacyTransactionsCustomServerTest.phptests/e2e/Services/Databases/Transactions/TablesDBACIDTest.phptests/e2e/Services/Databases/Transactions/TablesDBTransactionPermissionsCustomClientTest.phptests/e2e/Services/Databases/Transactions/TablesDBTransactionsConsoleClientTest.phptests/e2e/Services/Databases/Transactions/TablesDBTransactionsCustomClientTest.phptests/e2e/Services/Databases/Transactions/TablesDBTransactionsCustomServerTest.phptests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.phptests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.php.tmptests/e2e/Services/Databases/Transactions/TransactionsBase.phptests/e2e/Traits/DatabaseFixture.phptests/e2e/Traits/DatabasesUrlHelpers.php
💤 Files with no reviewable changes (18)
- tests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsGuestTest.php
- tests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsScope.php
- tests/e2e/Services/Databases/TablesDB/Transactions/TransactionsConsoleClientTest.php
- tests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsMemberTest.php
- tests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsGuestTest.php
- tests/e2e/Services/Databases/TablesDB/Transactions/TransactionsCustomServerTest.php
- tests/e2e/Services/Databases/TablesDB/Transactions/PermissionsCustomClientTest.php
- tests/e2e/Services/Databases/Legacy/DatabasesCustomClientTest.php
- tests/e2e/Services/Databases/TablesDB/Transactions/TransactionsCustomClientTest.php
- tests/e2e/Services/Databases/Legacy/DatabasesConsoleClientTest.php
- tests/e2e/Services/Databases/Legacy/Transactions/TransactionsCustomClientTest.php
- tests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsTeamTest.php
- tests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsTeamTest.php
- tests/e2e/Services/Databases/Legacy/Transactions/TransactionsCustomServerTest.php
- tests/e2e/Services/Databases/TablesDB/DatabasesCustomClientTest.php
- tests/e2e/Services/Databases/TablesDB/DatabasesConsoleClientTest.php
- tests/e2e/Services/Databases/Legacy/Transactions/TransactionsConsoleClientTest.php
- tests/e2e/Services/Databases/TablesDB/Transactions/ACIDTest.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T10:16:48.075Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10023
File: src/Appwrite/Databases/TransactionState.php:174-219
Timestamp: 2025-10-02T10:16:48.075Z
Learning: In `src/Appwrite/Databases/TransactionState.php`, the `listDocuments` method intentionally applies queries only to the initial database fetch of committed documents. Transaction state changes (creates, updates, deletes) are then overlaid on top of the filtered committed set without re-evaluating the queries against the final merged result. This is the intended behavior for transaction-aware document listing.
Applied to files:
tests/e2e/Services/Databases/Transactions/ACIDBase.php
🧬 Code graph analysis (23)
tests/e2e/Services/Databases/TablesDBConsoleClientTest.php (1)
tests/e2e/Scopes/Scope.php (1)
Scope(12-256)
tests/e2e/Services/Databases/Transactions/TablesDBACIDTest.php (2)
src/Appwrite/Platform/Modules/Databases/Workers/Databases.php (1)
Databases(21-644)tests/e2e/Scopes/Scope.php (1)
Scope(12-256)
tests/e2e/Services/Databases/TablesDBCustomClientTest.php (2)
src/Appwrite/Platform/Modules/Databases/Workers/Databases.php (1)
Databases(21-644)tests/e2e/Scopes/Scope.php (1)
Scope(12-256)
tests/e2e/Services/Databases/LegacyCustomClientTest.php (1)
tests/e2e/Scopes/Scope.php (1)
Scope(12-256)
tests/e2e/Services/Databases/Transactions/TablesDBTransactionPermissionsCustomClientTest.php (1)
tests/e2e/Scopes/Scope.php (1)
Scope(12-256)
tests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.php (4)
tests/e2e/Scopes/ApiLegacy.php (4)
getApiBasePath(11-14)getContainerIdParam(26-29)getSecurityParam(51-54)getRecordIdParam(46-49)tests/e2e/Scopes/ApiTablesDB.php (4)
getApiBasePath(11-14)getContainerIdParam(26-29)getSecurityParam(51-54)getRecordIdParam(46-49)tests/e2e/Services/Databases/Permissions/DatabasesPermissionsBase.php (3)
getContainerUrl(22-27)getSchemaUrl(29-40)getRecordUrl(42-47)tests/e2e/Traits/DatabasesUrlHelpers.php (4)
getContainerUrl(18-23)getSchemaUrl(25-42)getTransactionUrl(51-55)getRecordUrl(44-49)
tests/e2e/Services/Databases/LegacyCustomServerTest.php (1)
tests/e2e/Scopes/Scope.php (1)
Scope(12-256)
tests/e2e/Services/Databases/Permissions/TablesDBPermissionsGuestTest.php (1)
tests/e2e/Scopes/ApiTablesDB.php (4)
getContainerIdParam(26-29)getSecurityParam(51-54)getRecordIdParam(46-49)getRecordResource(41-44)
tests/e2e/Services/Databases/Permissions/TablesDBPermissionsMemberTest.php (3)
tests/e2e/Traits/DatabasesUrlHelpers.php (4)
getDatabaseUrl(12-16)getContainerUrl(18-23)getSchemaUrl(25-42)getRecordUrl(44-49)tests/e2e/Scopes/ApiLegacy.php (3)
getContainerIdParam(26-29)getSecurityParam(51-54)getRecordIdParam(46-49)tests/e2e/Scopes/ApiTablesDB.php (3)
getContainerIdParam(26-29)getSecurityParam(51-54)getRecordIdParam(46-49)
tests/e2e/Services/Databases/Permissions/TablesDBPermissionsTeamTest.php (5)
tests/e2e/Scopes/Scope.php (1)
Scope(12-256)tests/e2e/Services/Databases/Permissions/DatabasesPermissionsBase.php (7)
createTeam(92-101)createUser(50-84)getDatabaseUrl(16-20)getServerHeader(119-126)getContainerUrl(22-27)getSchemaUrl(29-40)getRecordUrl(42-47)tests/e2e/Traits/DatabasesUrlHelpers.php (4)
getDatabaseUrl(12-16)getContainerUrl(18-23)getSchemaUrl(25-42)getRecordUrl(44-49)tests/e2e/Scopes/ApiLegacy.php (3)
getContainerIdParam(26-29)getRecordIdParam(46-49)getRecordResource(41-44)tests/e2e/Scopes/ApiTablesDB.php (3)
getContainerIdParam(26-29)getRecordIdParam(46-49)getRecordResource(41-44)
tests/e2e/Services/Databases/Permissions/LegacyPermissionsMemberTest.php (3)
tests/e2e/Services/Databases/Permissions/DatabasesPermissionsBase.php (5)
getDatabaseUrl(16-20)getServerHeader(119-126)getContainerUrl(22-27)getSchemaUrl(29-40)getRecordUrl(42-47)tests/e2e/Scopes/ApiLegacy.php (3)
getContainerIdParam(26-29)getSecurityParam(51-54)getRecordIdParam(46-49)tests/e2e/Scopes/ApiTablesDB.php (3)
getContainerIdParam(26-29)getSecurityParam(51-54)getRecordIdParam(46-49)
tests/e2e/Traits/DatabaseFixture.php (1)
tests/e2e/Client.php (1)
Client(8-342)
tests/e2e/Services/Databases/Transactions/TablesDBTransactionsConsoleClientTest.php (1)
tests/e2e/Scopes/Scope.php (1)
Scope(12-256)
tests/e2e/Scopes/ApiTablesDB.php (1)
tests/e2e/Scopes/ApiLegacy.php (14)
getApiBasePath(11-14)getDatabaseType(16-19)getContainerResource(21-24)getContainerIdParam(26-29)getSchemaResource(31-34)getSchemaParam(36-39)getRecordResource(41-44)getRecordIdParam(46-49)getSecurityParam(51-54)getRelatedIdParam(56-59)getRelatedResourceKey(61-64)getContainerIdResponseKey(66-69)getIndexAttributesParam(71-74)getSecurityResponseKey(76-79)
tests/e2e/Services/Databases/TablesDBCustomServerTest.php (1)
tests/e2e/Scopes/Scope.php (1)
Scope(12-256)
tests/e2e/Services/Databases/Transactions/ACIDBase.php (3)
tests/e2e/Services/Databases/Permissions/DatabasesPermissionsBase.php (4)
getDatabaseUrl(16-20)getContainerUrl(22-27)getSchemaUrl(29-40)getRecordUrl(42-47)tests/e2e/Scopes/ApiLegacy.php (4)
getContainerIdParam(26-29)getSecurityParam(51-54)getRecordIdParam(46-49)getSchemaParam(36-39)tests/e2e/Scopes/ApiTablesDB.php (4)
getContainerIdParam(26-29)getSecurityParam(51-54)getRecordIdParam(46-49)getSchemaParam(36-39)
tests/e2e/Services/Databases/Permissions/LegacyPermissionsTeamTest.php (3)
tests/e2e/Traits/DatabasesUrlHelpers.php (4)
getDatabaseUrl(12-16)getContainerUrl(18-23)getSchemaUrl(25-42)getRecordUrl(44-49)tests/e2e/Scopes/ApiLegacy.php (3)
getContainerIdParam(26-29)getRecordIdParam(46-49)getRecordResource(41-44)tests/e2e/Scopes/ApiTablesDB.php (3)
getContainerIdParam(26-29)getRecordIdParam(46-49)getRecordResource(41-44)
tests/e2e/Services/Databases/Transactions/LegacyACIDTest.php (1)
tests/e2e/Scopes/Scope.php (1)
Scope(12-256)
tests/e2e/Services/Databases/Permissions/DatabasesPermissionsBase.php (3)
tests/e2e/Traits/DatabasesUrlHelpers.php (4)
getDatabaseUrl(12-16)getContainerUrl(18-23)getSchemaUrl(25-42)getRecordUrl(44-49)tests/e2e/Scopes/ApiLegacy.php (4)
getApiBasePath(11-14)getContainerResource(21-24)getSchemaResource(31-34)getRecordResource(41-44)tests/e2e/Scopes/ApiTablesDB.php (4)
getApiBasePath(11-14)getContainerResource(21-24)getSchemaResource(31-34)getRecordResource(41-44)
tests/e2e/Traits/DatabasesUrlHelpers.php (3)
tests/e2e/Services/Databases/Permissions/DatabasesPermissionsBase.php (4)
getDatabaseUrl(16-20)getContainerUrl(22-27)getSchemaUrl(29-40)getRecordUrl(42-47)tests/e2e/Scopes/ApiLegacy.php (4)
getApiBasePath(11-14)getContainerResource(21-24)getSchemaResource(31-34)getRecordResource(41-44)tests/e2e/Scopes/ApiTablesDB.php (4)
getApiBasePath(11-14)getContainerResource(21-24)getSchemaResource(31-34)getRecordResource(41-44)
tests/e2e/Services/Databases/Transactions/LegacyTransactionPermissionsCustomClientTest.php (1)
tests/e2e/Scopes/Scope.php (1)
Scope(12-256)
tests/e2e/Services/Databases/LegacyConsoleClientTest.php (1)
tests/e2e/Scopes/Scope.php (1)
Scope(12-256)
tests/e2e/Scopes/ApiLegacy.php (1)
tests/e2e/Scopes/ApiTablesDB.php (14)
getApiBasePath(11-14)getDatabaseType(16-19)getContainerResource(21-24)getContainerIdParam(26-29)getSchemaResource(31-34)getSchemaParam(36-39)getRecordResource(41-44)getRecordIdParam(46-49)getSecurityParam(51-54)getRelatedIdParam(56-59)getRelatedResourceKey(61-64)getContainerIdResponseKey(66-69)getIndexAttributesParam(71-74)getSecurityResponseKey(76-79)
🪛 GitHub Actions: Linter
tests/e2e/Services/Databases/Permissions/DatabasesPermissionsBase.php
[error] 1-1: Pint linting failed. PSR-12 style issues detected. Script 'vendor/bin/pint --test --config pint.json' exited with code 1.
⏰ 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). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (41)
tests/e2e/Services/Databases/Transactions/LegacyTransactionPermissionsCustomClientTest.php (1)
1-16: Good composition-only scaffold for the “custom client + legacy API” permissions suite.
AssumingTransactionPermissionsBasecarries thetest*methods andProjectCustom/SideClientprovideScope’s abstract methods, this is good to go.tests/e2e/Services/Databases/LegacyCustomClientTest.php (1)
1-16: LGTM scaffold for “legacy + custom client” DB tests.
Consistent with the trait-composition pattern introduced in this PR.tests/e2e/Services/Databases/Transactions/LegacyACIDTest.php (1)
1-18: Good setup; watch for trait method-name collisions (ACIDBasevs URL/API helper traits).
If any two traits define the same method name withoutinsteadof/aliasing, PHP will fatal at load time—worth a quick sanity check as the trait stack grows.tests/e2e/Services/Databases/TablesDBCustomServerTest.php (1)
1-16: Scaffold is correct; trait composition properly satisfiesScopeabstract methods.All referenced traits (
DatabasesBase,ProjectCustom,SideServer,ApiTablesDB) are properly defined. TheScopeabstract methodsgetHeaders()andgetProject()are provided bySideServer(line 12 in SideServer.php) andProjectCustom(line 21 in ProjectCustom.php) respectively. No runtime errors will occur.tests/e2e/Services/Databases/Transactions/LegacyTransactionsCustomClientTest.php (1)
3-3: Verify namespace includesLegacysubdirectory.Ensure the namespace aligns with the file path. The filename
LegacyTransactionsCustomClientTest.phpsuggests the file may be located in aLegacysubdirectory underTransactions/, in which case the namespace should beTests\E2E\Services\Databases\Transactions\Legacyrather thanTests\E2E\Services\Databases\Transactions.tests/e2e/Services/Databases/Transactions/LegacyTransactionsConsoleClientTest.php (2)
3-3: [No action required] The namespace is correct. The file pathtests/e2e/Services/Databases/Transactions/LegacyTransactionsConsoleClientTest.phpcorrectly maps to the namespaceTests\E2E\Services\Databases\Transactions. The "Legacy" prefix is part of the class name, not a directory level—the subdirectory structure isServices/Databases/Transactions/, notServices/Databases/Legacy/Transactions/.Likely an incorrect or invalid review comment.
13-13: No action needed. TheTransactionsBasetrait is in the same namespace as the test class (Tests\E2E\Services\Databases\Transactions) and resolves correctly without an explicit import statement. Unqualified trait names automatically resolve within the current namespace in PHP.Likely an incorrect or invalid review comment.
tests/e2e/Services/Databases/Transactions/TablesDBTransactionsCustomClientTest.php (2)
3-3: No action required. The namespaceTests\E2E\Services\Databases\Transactionscorrectly aligns with the actual file pathtests/e2e/Services/Databases/Transactions/TablesDBTransactionsCustomClientTest.phpper PSR-4 autoloading standards. The concern was based on an inaccurate path suggestion.Likely an incorrect or invalid review comment.
13-13: No action needed. TheTransactionsBasetrait is properly accessible in the same namespace without an explicit import statement.Likely an incorrect or invalid review comment.
tests/e2e/Services/Databases/Transactions/LegacyTransactionsCustomServerTest.php (1)
3-3: No action needed — namespace correctly matches file path.The namespace
Tests\E2E\Services\Databases\Transactionsaccurately reflects the file's location attests/e2e/Services/Databases/Transactions/. The "Legacy" designation in the class name does not require a separate directory level. Other similar test files in the same directory follow the identical namespace pattern.Likely an incorrect or invalid review comment.
tests/e2e/Services/Databases/TablesDBCustomClientTest.php (1)
12-12: > Likely an incorrect or invalid review comment.tests/e2e/Services/Databases/Transactions/TablesDBTransactionsCustomServerTest.php (1)
11-18: LGTM!The test scaffold correctly composes the required traits for TablesDB transaction testing with server-side authentication. The trait composition pattern is consistent with the PR's architecture.
tests/e2e/Services/Databases/Permissions/DatabasesPermissionsBase.php (1)
50-126: LGTM!The user management, team management, and server header helper methods are well-structured and follow the established test patterns in the codebase.
tests/e2e/Services/Databases/Permissions/LegacyPermissionsGuestTest.php (2)
14-20: LGTM!The test class correctly composes the required traits for legacy API guest permission testing. The trait composition aligns with the PR's architecture pattern.
107-172: LGTM!The
testReadDocumentstest method properly validates read permissions across public and private collections with various permission configurations. The logic correctly handles theRole::any()permission check for private collection access.tests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.php (3)
11-14: LGTM!The trait correctly uses
DatabasesUrlHelpersfor URL construction and declares the$permissionsDatabaseproperty for storing the test database ID.
674-785: Well-designed cross-batch dependency test.The
testCanUpdateDocumentCreatedInPreviousBatchmethod effectively validates the transaction state management for cross-batch dependencies. The test properly covers create → update → delete sequences and includes cleanup via rollback.
787-1009: Comprehensive user isolation tests.The user isolation tests (
testUserCannotReadAnotherUsersTransaction,testUserCannotListAnotherUsersTransactions,testUserCannotUpdateAnotherUsersTransaction,testUserCannotDeleteAnotherUsersTransaction) provide excellent coverage for transaction access control. Each test properly creates fresh users and validates that unauthorized access returns 404.tests/e2e/Scopes/ApiTablesDB.php (1)
5-80: LGTM!The
ApiTablesDBtrait provides a consistent API configuration pattern that mirrorsApiLegacy. The method signatures and return values correctly map to the TablesDB API resources (/tablesdb,tables,columns,rows).tests/e2e/Traits/DatabaseFixture.php (1)
229-238: LGTM!The
tearDownAfterClassmethod properly resets all static fixture state and calls the parent implementation, ensuring clean test isolation between test classes.tests/e2e/Services/Databases/Permissions/LegacyPermissionsMemberTest.php (3)
14-19: LGTM!The class correctly migrates to the new trait composition pattern with
DatabasesPermissionsBaseandApiLegacy, aligning with the PR's architecture.
117-229: LGTM!The
testSetupDatabasemethod is well-refactored to use URL helper methods (getDatabaseUrl,getContainerUrl,getSchemaUrl) and parameter helpers (getContainerIdParam,getSecurityParam) consistently throughout. This improves maintainability and allows the same test logic to work with different API implementations.
236-334: LGTM!The
testReadDocumentstest method correctly uses the URL helpers for all API calls and properly tests document visibility across different collection permission configurations.tests/e2e/Services/Databases/Transactions/ACIDBase.php (3)
11-166: LGTM on testAtomicity implementation.The atomicity test correctly validates that either all operations succeed or none are applied when a unique constraint violation occurs. The use of dynamic URL helpers (
getDatabaseUrl,getContainerUrl, etc.) and parameter helpers (getContainerIdParam,getRecordIdParam) enables proper reuse across Legacy and TablesDB API variants.
302-454: LGTM on testIsolation.The isolation test correctly validates that concurrent transactions on the same data are properly handled, with both increments applied atomically (0 + 10 + 5 = 15).
459-618: LGTM on testDurability.The durability test properly validates that committed transaction data persists and can be subsequently modified outside the transaction context.
tests/e2e/Services/Databases/Permissions/TablesDBPermissionsGuestTest.php (5)
14-20: LGTM on trait composition and class structure.The test class properly composes the required traits (
DatabasesPermissionsBase,ProjectCustom,SideClient,ApiTablesDB) for TablesDB guest permission testing, enabling reuse of URL helpers and API parameter mappings.
21-105: LGTM on collection setup and data provider.The
createCollectionmethod properly creates both public (withRole::any()permissions) and private (withrowSecurityenabled) collections, andpermissionsProvidercovers key permission scenarios for data-driven testing.
107-172: LGTM on testReadDocuments.The test correctly validates guest read access based on document permissions, properly checking that guests can only read documents with
Permission::read(Role::any())in private collections.
174-286: LGTM on testWriteDocument.The test comprehensively validates guest write permissions, correctly asserting 401 for unauthorized operations on private collections while allowing operations on public collections with
Role::any()permissions.
288-357: LGTM on testWriteDocumentWithPermissions.The test properly validates that guests can create documents with specific permissions when the collection allows
Permission::create(Role::any()).tests/e2e/Services/Databases/Permissions/TablesDBPermissionsTeamTest.php (2)
24-147: LGTM on team permission setup and data providers.The setup correctly creates team-based permission scenarios with:
- Collection 1: Team 1 read access, admin role for CUD
- Collection 2: Team 2 read access, owner role for CUD
- User3 in both teams without roles to test basic membership
149-253: LGTM on test methods with proper @depends/@dataProvider usage.The tests correctly use PHPUnit's
@depends testSetupDatabasewith@dataProviderto share state while iterating over permission scenarios. The assertion logic properly handles both authorized (201/200) and unauthorized (401/404) responses.tests/e2e/Services/Databases/Permissions/TablesDBPermissionsMemberTest.php (3)
31-107: LGTM on comprehensive permission data provider.The 12 permission scenarios cover key cases including
Role::any(),Role::users(), specific user roles, and combinations. The cumulative count expectations (any: 1→12, users: 1→12, doconly: 1→7) correctly reflect document visibility based on permissions.
231-334: LGTM on testReadDocuments implementation.The test correctly validates document visibility across three collection types (public/private/doconly) for user1, using the cumulative count approach to verify permission-based filtering works correctly.
163-163: Status code 202 is correct for attribute creation.The expected status code of 202 is consistent across all database tests in the codebase. The TablesDB API returns 202 (Accepted) for asynchronous attribute creation, followed by a sleep period to allow the operation to complete. This is the standard behavior and requires no changes.
tests/e2e/Traits/DatabasesUrlHelpers.php (1)
5-9: Good documentation of trait dependencies.The docblock clearly states that this trait requires
ApiLegacyorApiTablesDBto providegetApiBasePath(),getContainerResource(), etc. This helps developers understand the composition requirements.tests/e2e/Services/Databases/Permissions/LegacyPermissionsTeamTest.php (2)
14-19: LGTM on trait composition for Legacy API.The class correctly uses
ApiLegacy(instead ofApiTablesDBin the TablesDB variant) to configure legacy API paths (/databases,collections,documents).
24-253: LGTM on test implementation.The test logic correctly mirrors
TablesDBPermissionsTeamTestfor the legacy API, validating team-based permissions for collections and documents.tests/e2e/Scopes/ApiLegacy.php (2)
5-9: Good documentation of trait purpose.The docblock clearly describes the trait's role in configuring legacy database API paths (
/databases,collections,attributes,documents).
11-79: LGTM on API configuration methods.All 14 getter methods correctly return legacy-specific configuration values that mirror the
ApiTablesDBtrait structure. The response keys with$prefix ($collectionId) correctly match Appwrite's response format.
tests/e2e/Services/Databases/Transactions/LegacyTransactionsCustomClientTest.php
Show resolved
Hide resolved
tests/e2e/Services/Databases/Transactions/LegacyTransactionsCustomServerTest.php
Show resolved
Hide resolved
tests/e2e/Services/Databases/Transactions/TablesDBTransactionPermissionsCustomClientTest.php
Show resolved
Hide resolved
tests/e2e/Services/Databases/Transactions/TablesDBTransactionsConsoleClientTest.php
Show resolved
Hide resolved
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @tests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.php:
- Around line 108-177: testCollectionUpdatePermissionDenied opens a transaction
($transaction from getTransactionUrl()) and leaves it open on failure; wrap the
transaction lifecycle in a try/finally and always close it in the finally block
(call the transaction delete/rollback endpoint via
getTransactionUrl($transaction['body']['$id']) with the same headers) so the
transaction is removed regardless of the staged operation result; ensure the
finally runs even when the staged call asserts a 401 so tests stay isolated.
- Around line 22-37: The test setup creates a database in setUp() but never
cleans it up; add a tearDown() method that checks $this->permissionsDatabase and
calls $this->client->call(Client::METHOD_DELETE, $this->getApiBasePath(), [same
headers used in setUp()], ['databaseId' => $this->permissionsDatabase]) to
remove it after each test; ensure tearDown() runs even on failures (override
tearDown(): void and call parent::tearDown() at the end), and handle/delete
errors safely (catch exceptions or ignore not-found responses) so cleanup does
not mask test failures.
- Around line 838-898: The test testUserCannotListAnotherUsersTransactions
relies on strict total checks (assertEquals(1 / 2 on listUser2/listUser1) which
is flaky if other transactions exist; instead remove the assertEquals on list
totals and verify ownership by checking returned transaction IDs: for listUser2
and listUser1 use array_column($response['body']['transactions'], '$id') and
assertContains/assertNotContains for expected IDs (transaction3 present in
listUser2, transaction1/transaction2 absent), and optionally assertCount on the
filtered arrays of IDs that match the expected set (e.g.,
count(array_intersect($returnedIds, [$transaction3['body']['$id']])) === 1).
Replace references to the total assertions in
testUserCannotListAnotherUsersTransactions with these ID-based presence/absence
checks using getTransactionUrl(), getUser(),
transaction1/transaction2/transaction3 and listUser1/listUser2 response
variables.
- Around line 1016-1105: Add explicit cleanup to remove/rollback user1's
transaction at the end of testUserCannotAddOperationsToAnotherUsersTransaction:
after the final assertions call the transactions delete/rollback endpoint for
$transactionId1 using the same $user1Headers (invoke Client::METHOD_DELETE on
$this->getTransactionUrl($transactionId1)) and assert the expected success
status (e.g., 204) so the staged transaction is removed and the test environment
stays clean.
- Around line 182-250: In testCollectionDeletePermissionDenied: after the staged
operations check, explicitly close the created transaction (using
getTransactionUrl with $transaction['body']['$id']) by calling the appropriate
commit/rollback/delete endpoint via $this->client->call and assert a successful
close status so the transaction is not left open and tests remain isolated.
- Around line 413-479: The test method testCannotSetUnauthorizedRolePermissions
currently accesses $staged['body']['message'] directly which can trigger
undefined index notices if the API returns an empty or differently-shaped error
body; update the assertions to first guard the shape of $staged (e.g., assert
that 'body' exists and is an array, and that 'message' key exists or use isset
on $staged['body']['message']) before asserting its contents, and if missing
fail with a clear assertion message so the test reports a meaningful error
rather than a PHP notice.
- Around line 42-103: The test prints debug output with echo and never closes
the created transaction; remove the echo statements and instead include the
response body/status in the assertion message for the $staged check, and wrap
the transaction lifecycle in a try/finally so the transaction created by
$transaction = $this->client->call(...) is cleaned up (rollback/delete) in the
finally block using the transaction id ($transaction['body']['$id']) and
getTransactionUrl; ensure you reference $staged for the assertion message and
always call the API to delete/rollback the transaction in the finally regardless
of test outcome.
🧹 Nitpick comments (8)
tests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.php (5)
12-18: Make the trait’s required host-class contract explicit (to avoid “magic” dependencies).This trait implicitly requires
$this->client,getProject(),getHeaders(),getUser(),getTransactionUrl(), etc. Consider adding a@mixin/@propertyphpdoc (or declaringabstractmethod stubs) so consumers fail fast and IDE/static analysis can help.
553-611: Status-code expectation looks reasonable; consider asserting error shape for debuggability.For failures like 404, asserting a stable error code/type field (if present) is usually less brittle than message strings.
616-675: Status-code expectation looks reasonable; consider asserting error shape for debuggability.Same note as the non-existent update case.
680-787: Good cross-batch dependency coverage; cleanup is handled here (rollback).Nice that this one explicitly rolls back—consider applying the same pattern to the other transaction-creating tests for consistency.
1110-1157: Test intent is good; consider using a fresh user here for determinism.Using
getUser()(non-fresh) plus>= 3makes the test tolerant, but also less precise; a fresh user would let you assert exact totals and better catch regressions.tests/e2e/Scopes/SchemaPolling.php (1)
28-32: Remove unnecessaryarray_merge()calls.Throughout this trait,
array_merge()is called with a single array argument (lines 28, 55, 94, 131, 157, 186). This is redundant since there's nothing to merge.♻️ Suggested simplification
- $attribute = $this->client->call( - Client::METHOD_GET, - $this->getSchemaUrl($databaseId, $containerId) . '/' . $attributeKey, - array_merge([ - 'content-type' => 'application/json', - 'x-appwrite-project' => $this->getProject()['$id'], - 'x-appwrite-key' => $this->getProject()['apiKey'] - ]) - ); + $attribute = $this->client->call( + Client::METHOD_GET, + $this->getSchemaUrl($databaseId, $containerId) . '/' . $attributeKey, + [ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + 'x-appwrite-key' => $this->getProject()['apiKey'] + ] + );Apply this same simplification to all similar occurrences in
waitForAttributes,waitForAttributeCount,waitForIndex,waitForAllIndexes, andwaitForAllAttributes.tests/e2e/Services/Databases/Transactions/ACIDBase.php (1)
292-292: UseassertContainscorrectly or prefer explicit assertions.
assertContains($needle, $haystack)in PHPUnit 9.x expects an iterable as the second argument. The current usage passes the result as$needleand an array as$haystack, which is correct. However, for clarity and type safety when checking against a set of expected status codes, consider using an explicit conditional orassertThatwithlogicalOr.♻️ Alternative approach for clearer intent
- $this->assertContains($response['headers']['status-code'], [400, 500], 'Transaction commit should fail due to validation. Response: ' . json_encode($response['body'])); + $statusCode = $response['headers']['status-code']; + $this->assertTrue( + in_array($statusCode, [400, 500], true), + "Transaction commit should fail due to validation (expected 400 or 500, got {$statusCode}). Response: " . json_encode($response['body']) + );tests/e2e/Services/Databases/Permissions/DatabasesPermissionsBase.php (1)
12-44: Code duplication: URL helpers are duplicated fromDatabasesUrlHelperstrait.These URL helper methods (
getDatabaseUrl,getContainerUrl,getSchemaUrl,getRecordUrl) duplicate the implementation intests/e2e/Traits/DatabasesUrlHelpers.php. Additionally, this implementation ofgetSchemaUrlis missing the special handling for relationship attributes that exists inDatabasesUrlHelpers.Consider using the
DatabasesUrlHelperstrait instead of duplicating these methods. This would eliminate the duplication and ensure consistency across the test suite.♻️ Suggested refactor to use existing trait
namespace Tests\E2E\Services\Databases\Permissions; use Tests\E2E\Client; +use Tests\E2E\Traits\DatabasesUrlHelpers; trait DatabasesPermissionsBase { + use DatabasesUrlHelpers; + public array $users = []; public array $teams = []; - // URL Helper Methods - uses methods from ApiLegacy/ApiTablesDB traits - protected function getDatabaseUrl(string $databaseId = ''): string - { - $base = $this->getApiBasePath(); - return $databaseId ? "{$base}/{$databaseId}" : $base; - } - - protected function getContainerUrl(string $databaseId, string $containerId = ''): string - { - $resource = $this->getContainerResource(); - $base = "{$this->getApiBasePath()}/{$databaseId}/{$resource}"; - return $containerId ? "{$base}/{$containerId}" : $base; - } - - protected function getSchemaUrl(string $databaseId, string $containerId, string $type = '', string $key = ''): string - { - $resource = $this->getSchemaResource(); - $base = "{$this->getContainerUrl($databaseId, $containerId)}/{$resource}"; - if ($type) { - $base .= "/{$type}"; - } - if ($key) { - $base .= "/{$key}"; - } - return $base; - } - - protected function getRecordUrl(string $databaseId, string $containerId, string $recordId = ''): string - { - $resource = $this->getRecordResource(); - $base = "{$this->getContainerUrl($databaseId, $containerId)}/{$resource}"; - return $recordId ? "{$base}/{$recordId}" : $base; - } - // User Management Methods
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/tests.ymltests/e2e/Scopes/ApiLegacy.phptests/e2e/Scopes/ApiTablesDB.phptests/e2e/Scopes/SchemaPolling.phptests/e2e/Services/Databases/DatabasesBase.phptests/e2e/Services/Databases/Permissions/DatabasesPermissionsBase.phptests/e2e/Services/Databases/Transactions/ACIDBase.phptests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.phptests/e2e/Services/Databases/Transactions/TransactionsBase.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T10:16:48.075Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10023
File: src/Appwrite/Databases/TransactionState.php:174-219
Timestamp: 2025-10-02T10:16:48.075Z
Learning: In `src/Appwrite/Databases/TransactionState.php`, the `listDocuments` method intentionally applies queries only to the initial database fetch of committed documents. Transaction state changes (creates, updates, deletes) are then overlaid on top of the filtered committed set without re-evaluating the queries against the final merged result. This is the intended behavior for transaction-aware document listing.
Applied to files:
tests/e2e/Services/Databases/Transactions/ACIDBase.php
🧬 Code graph analysis (6)
tests/e2e/Services/Databases/Transactions/ACIDBase.php (3)
tests/e2e/Scopes/ApiLegacy.php (6)
getContainerIdParam(26-29)getSecurityParam(51-54)getIndexAttributesParam(76-79)getRecordIdParam(46-49)getRecordResource(41-44)getSchemaParam(36-39)tests/e2e/Scopes/ApiTablesDB.php (6)
getContainerIdParam(26-29)getSecurityParam(51-54)getIndexAttributesParam(76-79)getRecordIdParam(46-49)getRecordResource(41-44)getSchemaParam(36-39)tests/e2e/Scopes/SchemaPolling.php (2)
waitForAllAttributes(180-199)waitForIndex(125-141)
tests/e2e/Services/Databases/Permissions/DatabasesPermissionsBase.php (3)
tests/e2e/Traits/DatabasesUrlHelpers.php (4)
getDatabaseUrl(12-16)getContainerUrl(18-23)getSchemaUrl(25-42)getRecordUrl(44-49)tests/e2e/Scopes/ApiLegacy.php (4)
getApiBasePath(11-14)getContainerResource(21-24)getSchemaResource(31-34)getRecordResource(41-44)tests/e2e/Scopes/ApiTablesDB.php (4)
getApiBasePath(11-14)getContainerResource(21-24)getSchemaResource(31-34)getRecordResource(41-44)
tests/e2e/Scopes/ApiTablesDB.php (1)
tests/e2e/Scopes/ApiLegacy.php (15)
getApiBasePath(11-14)getDatabaseType(16-19)getContainerResource(21-24)getContainerIdParam(26-29)getSchemaResource(31-34)getSchemaParam(36-39)getRecordResource(41-44)getRecordIdParam(46-49)getSecurityParam(51-54)getRelatedIdParam(56-59)getRelatedResourceKey(61-64)getContainerIdResponseKey(66-69)getOppositeContainerIdResponseKey(71-74)getIndexAttributesParam(76-79)getSecurityResponseKey(81-84)
tests/e2e/Scopes/SchemaPolling.php (4)
tests/e2e/Client.php (1)
Client(8-342)tests/e2e/Scopes/ApiLegacy.php (1)
getSchemaResource(31-34)tests/e2e/Scopes/ApiTablesDB.php (1)
getSchemaResource(31-34)tests/e2e/Traits/DatabasesUrlHelpers.php (1)
getIndexUrl(57-61)
tests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.php (3)
tests/e2e/Scopes/ApiLegacy.php (4)
getApiBasePath(11-14)getContainerIdParam(26-29)getSecurityParam(51-54)getRecordIdParam(46-49)tests/e2e/Scopes/ApiTablesDB.php (4)
getApiBasePath(11-14)getContainerIdParam(26-29)getSecurityParam(51-54)getRecordIdParam(46-49)tests/e2e/Traits/DatabasesUrlHelpers.php (3)
getContainerUrl(18-23)getSchemaUrl(25-42)getRecordUrl(44-49)
tests/e2e/Scopes/ApiLegacy.php (1)
tests/e2e/Scopes/ApiTablesDB.php (15)
getApiBasePath(11-14)getDatabaseType(16-19)getContainerResource(21-24)getContainerIdParam(26-29)getSchemaResource(31-34)getSchemaParam(36-39)getRecordResource(41-44)getRecordIdParam(46-49)getSecurityParam(51-54)getRelatedIdParam(56-59)getRelatedResourceKey(61-64)getContainerIdResponseKey(66-69)getOppositeContainerIdResponseKey(71-74)getIndexAttributesParam(76-79)getSecurityResponseKey(81-84)
⏰ 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: Benchmark
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Abuse enabled)
- GitHub Check: scan
🔇 Additional comments (16)
.github/workflows/tests.yml (2)
243-266: Same consolidation concern applies here for shared-mode tests.This matrix definition shows the same consolidation pattern as
e2e_service_test. Since this job tests database changes with shared table modes (V1 and V2), it's particularly important that both Legacy and TablesDB modes are adequately covered within the unified "Databases" test path.The consistency of this change across both matrices suggests it was intentional, but verification remains important to ensure no regression in coverage.
166-188: No coverage reduction risk from single "Databases" matrix entry.The test structure properly supports testing both Legacy and TablesDB modes through PHPUnit's auto-discovery. The tests/e2e/Services/Databases/ directory contains separate test classes for each mode (LegacyConsoleClientTest, TablesDBConsoleClientTest, etc.), each using the appropriate trait (ApiLegacy or ApiTablesDB). PHPUnit discovers and runs all classes within the directory when a single "Databases" matrix entry is used. Both modes are exercised within this single job, not reduced. The e2e_shared_mode_test job provides additional coverage through the tables-mode matrix dimension for shared table configurations.
tests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.php (7)
256-328: Good coverage of document-level permissions with row security enabled.The setup clearly isolates collection-level vs document-level permissions and validates the expected allow behavior.
334-408: Good coverage of document-level delete permissions with row security enabled.This complements the update case well and validates allow behavior at document scope.
484-548: Nice: positive-case staging validates expected operation count.This is a good counterpart to the denial tests and asserts both status and a key response field.
792-833: Good user-isolation behavior check (404 for other user).This validates the “don’t leak existence” behavior and also confirms the owner can still read.
903-958: Good coverage for commit/rollback authorization on чужие transactions.Verifies both mutation paths are blocked and owner can still commit.
963-1011: Good delete authorization coverage; also validates owner delete path.Looks solid for user isolation + correct deletion status code.
1162-1220: Solid CRUD coverage for user-owned transactions.Creates, reads, deletes, and validates 404 after deletion; plus repeats with a second transaction.
tests/e2e/Scopes/ApiLegacy.php (1)
5-85: LGTM! Clean trait implementation for Legacy API configuration.The trait provides a consistent set of accessor methods that mirror the
ApiTablesDBcounterpart. The naming is clear, and the return values correctly map to the Legacy database API's resource naming conventions (collections,attributes,documents, etc.).tests/e2e/Services/Databases/Transactions/ACIDBase.php (3)
12-14: Good trait composition with SchemaPolling.The trait correctly composes
SchemaPollingand uses its methods (waitForAllAttributes,waitForIndex) to replace sleep-based waits with polling-based synchronization. This makes tests more reliable and potentially faster.
18-170: Well-structured atomicity test.The
testAtomicitytest properly validates that when one operation in a transaction fails (due to unique constraint violation), none of the operations are committed. The test correctly handles both success and failure paths with appropriate assertions.
306-458: Isolation test correctly validates concurrent increment behavior.The test creates two independent transactions that increment a shared counter atomically. Both transactions succeed as expected (0 + 10 + 5 = 15) because increment operations use atomic SQL and are exempt from timestamp-based write-write conflict detection—by design, they are commutative and safe to execute concurrently. This properly tests the isolation guarantees for atomic operations.
tests/e2e/Scopes/ApiTablesDB.php (1)
5-85: LGTM! Symmetric implementation to ApiLegacy.The trait provides a consistent set of accessor methods that correctly mirror
ApiLegacywith TablesDB-specific values (tables,columns,rows, etc.). The symmetric design allows test code to be reused across both API types through trait composition.tests/e2e/Services/Databases/Permissions/DatabasesPermissionsBase.php (1)
46-123: User and team management methods are well-structured.The
createUser,getCreatedUser,createTeam,addToTeam, andgetServerHeadermethods provide clean abstractions for test setup. The user session handling and team membership creation follow proper API patterns.tests/e2e/Scopes/SchemaPolling.php (1)
7-14: Good use of polling pattern to replace sleep-based waits.The trait correctly documents its purpose and uses
assertEventuallyfor non-blocking waits. This is a cleaner approach than arbitrary sleep calls. All dependencies are properly satisfied:assertEventuallyis available via theAsynctrait used by theScopebase class; helper methods (getSchemaUrl,getContainerUrl,getIndexUrl) are provided byDatabasesUrlHelpersused by composing traits; andgetSchemaResourceis implemented by the scope helper classes (e.g.,ApiTablesDB) used by concrete test classes.
tests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.php
Outdated
Show resolved
Hide resolved
tests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.php
Show resolved
Hide resolved
tests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.php
Show resolved
Hide resolved
tests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.php
Show resolved
Hide resolved
tests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.php
Show resolved
Hide resolved
tests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.php
Show resolved
Hide resolved
tests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/tests.yml (1)
215-229: Make paratest--processescount configurable via environment variable.
The process count is currently hard-coded to 4 across both test jobs (lines 228 and 319). While this matches typical ubuntu-latest resources, making it configurable would allow dynamic adjustment based on runner specifications or enable testing with different parallelism levels. Export the value as an environment variable (e.g.,PARATEST_PROCESSES=4) to support future flexibility.
🤖 Fix all issues with AI agents
In @tests/extensions/Retryable.php:
- Around line 19-34: The repository lacks an executor for the Retry attribute:
implement a RetryExtension (or similarly named test runner hook) that locates
tests using the Retryable trait and their getNumberOfRetries() method and wraps
test invocation with retry semantics; specifically, create a RetryExtension that
hooks into the test lifecycle (e.g., before/around test execution), calls
$test->getNumberOfRetries() on Retryable tests, retries the test up to that
count on failure, and logs or rethrows the final failure—alternatively, if
retries are intentionally unsupported, add a comment in Retryable.php and remove
or mark getNumberOfRetries() unused so the code and tests reflect the intended
design.
🧹 Nitpick comments (3)
tests/e2e/Client.php (1)
212-213: Make the 120s cURL timeout configurable (avoid long hangs in CI/local).
Hard-coding 120s can mask regressions and slow failure feedback; consider an env override with a sane default.Proposed tweak
- curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 0); - curl_setopt($ch, CURLOPT_TIMEOUT, 120); + curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 0); + $timeout = (int)($_ENV['_APP_E2E_HTTP_TIMEOUT'] ?? \getenv('_APP_E2E_HTTP_TIMEOUT') ?: 120); + curl_setopt($ch, CURLOPT_TIMEOUT, $timeout);tests/e2e/Scopes/SchemaPolling.php (1)
24-33: DRY: extract common “project auth headers” builder (and drop no-oparray_merge).
The repeated header block is identical across all methods; a small helper will reduce churn and mistakes.Sketch
trait SchemaPolling { + private function getProjectAuthHeaders(): array + { + return [ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + 'x-appwrite-key' => $this->getProject()['apiKey'], + ]; + } @@ - array_merge([ - 'content-type' => 'application/json', - 'x-appwrite-project' => $this->getProject()['$id'], - 'x-appwrite-key' => $this->getProject()['apiKey'] - ]) + $this->getProjectAuthHeaders() );Also applies to: 51-60, 90-99, 127-136, 153-162, 182-191
tests/extensions/TestFinishedSubscriber.php (1)
15-29: Implementation is correct.The notify method correctly tracks and reports test completion times, with appropriate unit conversions (seconds to milliseconds) and threshold checks.
Optional observation: The completion message prints for every test, which may produce verbose output in large test suites. This might be intentional for debugging, but consider whether this level of output is necessary in all environments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/workflows/tests.ymlcomposer.jsonphpunit.xmltests/e2e/Client.phptests/e2e/Scopes/SchemaPolling.phptests/extensions/Retryable.phptests/extensions/TestFinishedSubscriber.phptests/extensions/TestHook.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/extensions/TestHook.php (1)
tests/extensions/TestFinishedSubscriber.php (1)
TestFinishedSubscriber(8-30)
🪛 PHPMD (2.15.0)
tests/extensions/TestHook.php
14-14: Avoid unused parameters such as '$configuration'. (undefined)
(UnusedFormalParameter)
14-14: Avoid unused parameters such as '$parameters'. (undefined)
(UnusedFormalParameter)
⏰ 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). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (8)
tests/extensions/Retryable.php (1)
6-10: LGTM! Clear documentation of the delegated retry mechanism.The docblock accurately describes the new architecture where retry logic is handled by RetryExtension, and clearly documents the attribute usage pattern.
.github/workflows/tests.yml (1)
166-188: LGTM on consolidatingDatabasesinto a single service entry.
Matrix simplification should reduce CI complexity while keeping coverage in one lane.tests/e2e/Scopes/SchemaPolling.php (1)
22-38: Good use ofassertEventuallyfor async readiness checks (attribute/index).
This should reduce test flakiness vssleep().
- Please confirm
assertEventually()retries on assertion failures and on transient call/parse failures (or explicitly document what it catches), otherwise a single intermittent failure could still abort polling.Also applies to: 125-141
phpunit.xml (1)
1-13: PHPUnit 10 configuration is valid. The<extensions><bootstrap class="Appwrite\Tests\TestHook" />element is correctly implemented. TestHook properly implements thePHPUnit\Runner\Extension\Extensioninterface with the correctbootstrap()method signature, and phpunit.xml validates successfully against the schema. No changes needed.composer.json (1)
90-100: ParaTest 7.4.x is compatible with PHPUnit 10.x and meets the PHP 8.2–8.3 requirement for this environment. No configuration concerns.tests/extensions/TestFinishedSubscriber.php (1)
10-13: LGTM!The constructor uses readonly property promotion effectively and clearly defines the threshold parameter.
tests/extensions/TestHook.php (2)
10-12: LGTM!The migration to the Extension interface aligns with PHPUnit 10's architecture, and the 15-second threshold for slow tests is reasonable for e2e tests.
14-17: LGTM!The bootstrap method correctly registers the TestFinishedSubscriber with the facade. The unused parameter warnings from PHPMD are false positives—the Extension interface requires these parameters in the signature, even when specific implementations don't need them.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/Appwrite/Messaging/Adapter/Realtime.php:
- Around line 41-48: The getPubSubPool method currently depends on the global
$register and assumes $register->get('pools')->get('pubsub') exists; replace
this by accepting a PubSubPool (or a pools factory/service) via constructor or a
setter to remove the global dependency, and add defensive checks in
getPubSubPool: verify the pools service and the 'pubsub' config are non-null and
of expected type before constructing PubSubPool, and throw a descriptive
exception (e.g., InvalidArgumentException or RuntimeException) if missing or
invalid so failures occur early and are testable.
In @tests/e2e/Scopes/Scope.php:
- Around line 157-160: getLastRequest() assumes getProject() returns an array
containing '$id' and will raise an undefined index notice if absent; update
getLastRequest() to retrieve $project = $this->getProject(), verify
isset($project['$id']) (or array_key_exists) before calling
$this->getLastRequestForProject($project['$id']), and if missing either return a
safe default (e.g., empty array) or throw a clear exception indicating the
missing project id so the real failure isn't masked.
🧹 Nitpick comments (5)
tests/unit/Migration/MigrationVersionsTest.php (3)
15-15: Avoidrequire_once app/init.phpinside the test (prefer PHPUnit bootstrap /setUpBeforeClass).If
phpunit.xmlalready bootstraps the app, re-requiring init here can add hidden side-effects and make ordering dependencies harder to spot. Consider moving this to PHPUnit’s bootstrap or asetUpBeforeClass()guarded include.
21-25: Make RC detection/constant assumptions more robust.
str_contains(APP_VERSION_STABLE, 'RC')is case-sensitive and assumesAPP_VERSION_STABLEis always defined by the time the test runs. If version formatting ever changes (e.g.,-rc.1), this can become flaky. Consider guardingdefined('APP_VERSION_STABLE')and using a case-insensitive check.Proposed change
- if (!(\str_contains(APP_VERSION_STABLE, 'RC'))) { - $this->assertArrayHasKey(APP_VERSION_STABLE, Migration::$versions); - } + if (\defined('APP_VERSION_STABLE') && \stripos(APP_VERSION_STABLE, 'rc') === false) { + $this->assertArrayHasKey(APP_VERSION_STABLE, Migration::$versions); + }
17-19: Add assertion message and build FQCN explicitly for better test diagnostics.The test currently concatenates the hardcoded namespace prefix without providing an assertion message. When a migration class is missing, the failure won't indicate which class, making debugging harder. Also, explicitly building the FQCN avoids string duplication and allows for handling both short names and full FQCNs defensively.
Proposed change
- foreach (Migration::$versions as $class) { - $this->assertTrue(class_exists('Appwrite\\Migration\\Version\\' . $class)); - } + foreach (Migration::$versions as $class) { + $fqcn = \str_starts_with($class, 'Appwrite\\Migration\\Version\\') + ? $class + : 'Appwrite\\Migration\\Version\\' . $class; + + $this->assertTrue( + \class_exists($fqcn), + "Missing migration version class: {$fqcn}" + ); + }tests/e2e/Scopes/Scope.php (2)
62-87: Harden MailDev fetch to avoid flaky warnings and reduce race risk.
file_get_contents()can returnfalseandjson_decode()can returnnull, which will emit warnings / cause odd downstream failures under parallel load. Also, a fixedsleep(3)is still race-prone: sometimes the email arrives later, sometimes sooner (wasted time).Proposed patch (defensive fetch + simple polling)
protected function getLastEmailByAddress(string $address): array { - sleep(3); - - $emails = json_decode(file_get_contents('http://maildev:1080/email'), true); + $maxAttempts = 10; + $delayMs = 300; + + for ($attempt = 0; $attempt < $maxAttempts; $attempt++) { + $raw = @file_get_contents('http://maildev:1080/email'); + if ($raw === false) { + usleep($delayMs * 1000); + continue; + } + + $emails = json_decode($raw, true); - if ($emails && is_array($emails)) { - // Search from the end (most recent) to the beginning - for ($i = count($emails) - 1; $i >= 0; $i--) { - $email = $emails[$i]; - if (isset($email['to']) && is_array($email['to'])) { - foreach ($email['to'] as $recipient) { - if (isset($recipient['address']) && $recipient['address'] === $address) { - return $email; - } - } - } - } - } + if (is_array($emails)) { + // Search from the end (most recent) to the beginning + for ($i = count($emails) - 1; $i >= 0; $i--) { + $email = $emails[$i]; + if (isset($email['to']) && is_array($email['to'])) { + foreach ($email['to'] as $recipient) { + if (($recipient['address'] ?? null) === $address) { + return $email; + } + } + } + } + } + + usleep($delayMs * 1000); + } return []; }
162-191: The header casing and value handling concerns are not substantiated by the actual request-catcher behavior.The existing webhook tests throughout the codebase (WebhooksCustomClientTest.php, WebhooksCustomServerTest.php, etc.) consistently access headers using exact casing—e.g.,
$webhook['headers']['X-Appwrite-Webhook-Project-Id']—and these tests pass. The request-catcher service (appwrite/requestcatcher:1.0.0) is a simple HTTP sink that preserves header names and values exactly as received; it does not normalize casing or convert header values to arrays. Header values in the current implementation are strings.The proposed defensive programming improvements (case-insensitive matching, array handling) would add robustness but are not addressing actual issues with the current implementation. Consider these enhancements as optional refactoring if you want to harden the code against hypothetical future changes to request-catcher's behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Appwrite/Messaging/Adapter/Realtime.phptests/e2e/Scopes/Scope.phptests/e2e/Services/Account/AccountBase.phptests/e2e/Services/Account/AccountConsoleClientTest.phptests/e2e/Services/Account/AccountCustomClientTest.phptests/e2e/Services/Account/AccountCustomServerTest.phptests/e2e/Services/Teams/TeamsBaseClient.phptests/e2e/Services/Teams/TeamsCustomClientTest.phptests/unit/Migration/MigrationTest.phptests/unit/Migration/MigrationVersionsTest.php
💤 Files with no reviewable changes (1)
- tests/unit/Migration/MigrationTest.php
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-16T06:37:59.054Z
Learnt from: hmacr
Repo: appwrite/appwrite PR: 10501
File: app/config/console.php:54-75
Timestamp: 2025-09-16T06:37:59.054Z
Learning: The Console project in Appwrite does not currently have email OTP flows implemented. Adding OTP session email templates to the Console configuration would cause test failures since the authentication flow is not defined for Console at this point.
Applied to files:
tests/e2e/Services/Account/AccountConsoleClientTest.php
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: In Appwrite's codebase, Utopia\Queue\Broker\Pool (aliased as BrokerPool) is a readonly class that implements both Publisher and Consumer interfaces, allowing it to be used directly in constructors expecting either interface type. This is extensively used throughout the codebase for queue operations.
Applied to files:
src/Appwrite/Messaging/Adapter/Realtime.php
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: The Utopia\Queue\Broker\Pool class (aliased as BrokerPool) implements both Publisher and Consumer interfaces, making it directly compatible with constructors expecting Publisher parameters like Func::__construct(protected Publisher $publisher). No wrapping is needed.
Applied to files:
src/Appwrite/Messaging/Adapter/Realtime.php
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: In the Utopia Queue system, the Pool class implements both Publisher and Consumer interfaces, so BrokerPool (Pool) can be passed directly to constructors expecting Publisher parameters like Func::__construct(protected Publisher $publisher).
Applied to files:
src/Appwrite/Messaging/Adapter/Realtime.php
🧬 Code graph analysis (8)
tests/unit/Migration/MigrationVersionsTest.php (1)
src/Appwrite/Platform/Appwrite.php (1)
Appwrite(17-32)
tests/e2e/Services/Account/AccountBase.php (1)
tests/e2e/Scopes/Scope.php (1)
getLastEmailByAddress(66-87)
tests/e2e/Services/Account/AccountConsoleClientTest.php (1)
tests/e2e/Scopes/Scope.php (1)
getLastEmailByAddress(66-87)
tests/e2e/Scopes/Scope.php (1)
src/Appwrite/Event/Event.php (1)
getProject(156-159)
tests/e2e/Services/Account/AccountCustomServerTest.php (1)
tests/e2e/Scopes/Scope.php (1)
getLastEmailByAddress(66-87)
tests/e2e/Services/Teams/TeamsBaseClient.php (1)
tests/e2e/Scopes/Scope.php (1)
getLastEmailByAddress(66-87)
tests/e2e/Services/Teams/TeamsCustomClientTest.php (1)
tests/e2e/Scopes/Scope.php (1)
getLastEmailByAddress(66-87)
tests/e2e/Services/Account/AccountCustomClientTest.php (1)
tests/e2e/Scopes/Scope.php (1)
getLastEmailByAddress(66-87)
⏰ 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 (Proxy)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Abuse enabled)
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (6)
src/Appwrite/Messaging/Adapter/Realtime.php (1)
154-165: Lazy initialization pattern is correct and safe.The change properly implements lazy initialization—the
PubSubPoolis only created on firstsend()call and cached thereafter. The early return guard (lines 147-149) ensures the pool is never instantiated for no-op calls when channels, roles, or projectId are empty.This maintains identical functionality while improving performance for Realtime adapter instances that are instantiated but never used to publish messages. The parallel test execution concern is not applicable here since paratest uses process isolation (separate PHP instances), not threading, so each test process has its own isolated
$registerand$pubSubPoolinstance.tests/e2e/Services/Account/AccountCustomClientTest.php (1)
943-946: Good change: address-scoped email lookup reduces cross-test interference; watch for shared OTP addresses.The
assertNotEmpty($lastEmail, ...)gate is a solid improvement before dereferencing['html']/['text']. Only remaining flake vector is places that use fixed recipient addresses (e.g.,otpuser2@appwrite.io) across multiple suites: address-scoping won’t help if multiple parallel tests send to the same address—consider making those addresses unique per test where possible (or additionally filter by subject/body marker).Also applies to: 1247-1250, 1430-1433, 1460-1463, 1487-1489, 3011-3013, 3082-3085
tests/e2e/Services/Account/AccountCustomServerTest.php (1)
217-219: LGTM: address-scoped email lookup for magic-url.This reduces false positives when tests run in parallel.
tests/e2e/Services/Teams/TeamsBaseClient.php (1)
221-224: LGTM: invitation emails fetched by recipient address + presence guard.This should reduce parallel flakes vs “global last email”.
Also applies to: 288-291, 311-313, 608-610
tests/e2e/Services/Account/AccountConsoleClientTest.php (1)
139-142: Please re-validate Console email behaviors (session alert + OTP) end-to-end.The address-scoped lookup is good, but the test still assumes Console will emit OTP emails (
/account/tokens/email) and session-alert emails. Based on learnings, Console OTP flows historically weren’t implemented—so this might still be flaky/failing depending on current Console behavior.Also applies to: 169-172, 195-197
tests/e2e/Services/Account/AccountBase.php (1)
166-169: Remove Console OTP concern; parallel-collision risk with sharedotpuser@appwrite.iopersists.The Console project does support OTP email flows (confirmed in
app/config/locale/templates.phpwhich includesotpSession), so that part of the concern is resolved. However, the parallel-execution risk remains: using a hardcoded shared email addressotpuser@appwrite.ioacross the trait (lines 166-169, 245-247) can still cause collisions when AccountBase is used by multiple test classes (AccountConsoleClientTest, AccountCustomClientTest, AccountCustomServerTest) running in parallel. WhilegetLastEmailByAddress()is an improvement, the shared address itself is the bottleneck. Consider using unique, test-scoped addresses or adding test isolation.Also applies to: 245-247
⛔ Skipped due to learnings
Learnt from: hmacr Repo: appwrite/appwrite PR: 10501 File: app/config/console.php:54-75 Timestamp: 2025-09-16T06:37:59.054Z Learning: The Console project in Appwrite does not currently have email OTP flows implemented. Adding OTP session email templates to the Console configuration would cause test failures since the authentication flow is not defined for Console at this point.
# Conflicts: # composer.lock # tests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsGuestTest.php # tests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsGuestTest.php
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
2712-2725: Dynamic assertions reduce test specificity for the canceled deployment scenario.At line 2691, the test explicitly verifies the deployment status is
'canceled'. Re-fetching and dynamically matching the status here could mask regressions — if a bug causes the status to unexpectedly change, the test would still pass with different assertions rather than failing.Since this section tests the canceled deployment error page, consider asserting the expected messages directly:
Proposed fix
- $deployment = $this->getDeployment($siteId, $deploymentId); - $status = $deployment['body']['status'] ?? ''; - $expectedMessage = match ($status) { - 'failed' => 'Deployment build failed', - 'canceled' => 'Deployment build canceled', - default => 'Deployment is still building', - }; - $this->assertStringContainsString($expectedMessage, $response['body']); - $expectedCta = match ($status) { - 'failed' => 'View logs', - 'canceled' => 'View deployments', - default => 'Reload', - }; - $this->assertStringContainsString($expectedCta, $response['body']); + $this->assertStringContainsString('Deployment build canceled', $response['body']); + $this->assertStringContainsString('View deployments', $response['body']);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php
⏰ 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
# Conflicts: # composer.lock
# Conflicts: # composer.lock # tests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsGuestTest.php # tests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsGuestTest.php
# Conflicts: # composer.lock # tests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsGuestTest.php # tests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsGuestTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
composer.json
🧰 Additional context used
🪛 GitHub Actions: Linter
composer.json
[error] 1-1: Composer.json is valid, but the lock file has some errors. Please run 'composer update' to resolve the mismatch.
⏰ 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). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (24)
tests/e2e/Services/GraphQL/TeamsClientTest.php (2)
136-157: Missing#[Depends]attributes will cause test failure.The
@dependsannotations fortestCreateTeamandtestCreateTeamMembershipwere removed but not replaced with PHP 8 attributes. This method requires both$teamand$membershipparameters, which PHPUnit will not provide without the dependency declarations.🐛 Proposed fix
- /** - */ + #[Depends('testCreateTeam')] + #[Depends('testCreateTeamMembership')] public function testGetTeamMembership($team, $membership)
159-180: Missing#[Depends]attributes will cause test failure.Same issue as
testGetTeamMembership. The@dependsannotations were removed without adding the equivalent PHP 8 attributes. Both$teamand$membershipparameters are required but will not be provided.🐛 Proposed fix
- /** - */ + #[Depends('testCreateTeam')] + #[Depends('testCreateTeamMembership')] public function testDeleteTeamMembership($team, $membership)tests/e2e/Services/GraphQL/TeamsServerTest.php (3)
188-210: Missing#[Depends]attributes cause test failure.This method expects
$teamand$membershipparameters but has no dependency attributes. PHPUnit 11 will not know where to source these values, causing the test to fail at runtime.The empty docblock suggests the migration from
@dependsannotations is incomplete.🐛 Proposed fix
- /** - */ + #[Depends('testCreateTeam')] + #[Depends('testCreateTeamMembership')] public function testGetTeamMembership($team, $membership)
235-259: Missing#[Depends]attributes cause test failure.Same incomplete migration as above - method requires
$teamand$membershipbut has no dependency attributes.🐛 Proposed fix
- /** - */ + #[Depends('testCreateTeam')] + #[Depends('testCreateTeamMembership')] public function testUpdateTeamMembershipRoles($team, $membership)
261-282: Missing#[Depends]attributes cause test failure.Same issue - method requires
$teamand$membershipparameters but has no dependency attributes.🐛 Proposed fix
- /** - */ + #[Depends('testCreateTeam')] + #[Depends('testCreateTeamMembership')] public function testDeleteTeamMembership($team, $membership)tests/e2e/Services/GraphQL/StorageClientTest.php (1)
91-286: Missing#[Depends]attributes will break test execution.The
@dependsannotations were removed from these test methods, but the corresponding#[Depends]attributes were not added. These methods have parameters expecting data from predecessor tests:
Method Parameter(s) Likely Dependencies testGetFiles$buckettestCreateBuckettestGetFile$bucket,$filetestCreateBucket,testCreateFiletestGetFilePreview$filetestCreateFiletestGetFileDownload$filetestCreateFiletestGetFileView$filetestCreateFiletestUpdateFile$filetestCreateFiletestDeleteFile$filetestUpdateFileWithout the
#[Depends]attributes, PHPUnit 11 will invoke these methods without passing the required data, causing test failures.Proposed fix (example for a few methods)
+ #[Depends('testCreateBucket')] public function testGetFiles($bucket): array {+ #[Depends('testCreateBucket')] + #[Depends('testCreateFile')] public function testGetFile($bucket, $file) {+ #[Depends('testCreateFile')] public function testGetFilePreview($file) {Apply the same pattern for
testGetFileDownload,testGetFileView,testUpdateFile, andtestDeleteFile, ensuring each has the appropriate#[Depends]attributes matching the original dependency chain.tests/e2e/Services/Webhooks/WebhooksBase.php (2)
317-320: Missing#[Depends]attribute fortestDeleteDocument.This method expects
array $datacontainingactorsIdanddatabaseIdfrom the previous test, but the dependency attribute is missing. Without it, PHPUnit 11 won't pass the required data, causing the test to fail or be skipped.🐛 Proposed fix
/** */ +#[Depends('testUpdateDocument')] #[Retry(count: 1)] public function testDeleteDocument(array $data): array
661-664: Missing#[Depends]attribute fortestDeleteRow.Same issue as
testDeleteDocument- this method expectsarray $datacontainingactorsIdanddatabaseIdbut lacks the dependency attribute.🐛 Proposed fix
/** */ +#[Depends('testUpdateRow')] #[Retry(count: 1)] public function testDeleteRow(array $data): arraytests/e2e/Services/GraphQL/TablesDB/DatabaseClientTest.php (5)
145-147: Missing#[Depends]attribute will cause test failure.
testCreateRowaccepts a$dataparameter and accesses$data['database']['_id']and$data['table']['_id'], but lacks the required#[Depends]attribute. Without it, PHPUnit 11 will not pass the return value from the prerequisite test, causing$datato be null and the test to fail.The AI summary indicates this method should have
#[Depends('testCreateTable')], but the attribute is missing from the code.Proposed fix
- /** - */ + #[Depends('testCreateIntegerColumn')] public function testCreateRow($data): arrayNote: Should depend on
testCreateIntegerColumn(ortestCreateStringColumn) to ensure columns exist before creating rows.
189-192: Missing#[Depends]attribute will cause test failure.
testGetRowstakes$dataparameter and accesses$data['database']['_id']and$data['table']['_id'], but has no#[Depends]attribute.Proposed fix
/** * `@throws` \Exception */ + #[Depends('testCreateRow')] public function testGetRows($data): void
214-217: Missing#[Depends]attribute will cause test failure.
testGetDocumentaccesses$data['row']['_id']but lacks dependency declaration.Proposed fix
/** * `@throws` \Exception */ + #[Depends('testCreateRow')] public function testGetDocument($data): void
240-243: Missing#[Depends]attribute will cause test failure.
testUpdateRowaccesses$data['row']['_id']but lacks dependency declaration.Proposed fix
/** * `@throws` \Exception */ + #[Depends('testCreateRow')] public function testUpdateRow($data): void
272-275: Missing#[Depends]attribute will cause test failure.
testDeleteRowaccesses$data['row']['_id']but lacks dependency declaration.Proposed fix
/** * `@throws` \Exception */ + #[Depends('testCreateRow')] public function testDeleteRow($data): voidtests/e2e/Services/Messaging/MessagingBase.php (2)
1426-1434: Undefined variable$fromwill cause test to always skip.The variable
$fromon line 1432 is never defined in this method. Since undefined variables evaluate tonull(and thusempty()), this condition will always triggermarkTestSkipped()even when the SMS DSN is properly configured.This appears to be a leftover from refactoring. The
$fromcheck should likely be removed since$senderIdserves as the sender identifier for SMS providers.🐛 Proposed fix
- if (empty($to) || empty($from) || empty($senderId) || empty($authKey)) { + if (empty($to) || empty($senderId) || empty($authKey)) { $this->markTestSkipped('SMS provider not configured'); }
1742-1750: Missing#[Depends]attribute will cause test failure.The
testDeleteMessagemethod expects$paramswithmessageandtopickeys, which matches the return value oftestSendEmail. However, the#[Depends('testSendEmail')]attribute is missing, so PHPUnit won't inject the dependency data and$paramswill be empty/null at runtime.🐛 Proposed fix
+ #[Depends('testSendEmail')] /** * `@return` void * `@throws` \Exception */ public function testDeleteMessage(array $params): voidtests/e2e/Services/Functions/FunctionsCustomServerTest.php (2)
710-715: Missing#[Depends]attribute causes test failure.The method
testCancelDeploymentBuildexpects$datacontainingfunctionId, but lacks a#[Depends]attribute. Without it, PHPUnit 11 won't inject the dependent data, causing a runtime error when accessing$data['functionId'].🐛 Proposed fix
/** */ #[Retry(count: 3)] + #[Depends('testCreateDeployment')] public function testCancelDeploymentBuild($data): void { $functionId = $data['functionId'];
2150-2152: Redundant double assignment.There's a typo with duplicate assignment that should be cleaned up.
✏️ Suggested fix
- $functionId = $functionId = $function['body']['$id'] ?? ''; + $functionId = $function['body']['$id'] ?? '';tests/e2e/Services/Storage/StorageCustomClientTest.php (1)
420-436: FixCLient::METHOD_PUTtypo (fatal error).
CLient::METHOD_PUTwill error (undefined class). Should beClient::METHOD_PUT.Proposed diff
- $this->client->call(CLient::METHOD_PUT, '/storage/buckets/' . $bucketId . '/files/' . $fileId, [ + $this->client->call(Client::METHOD_PUT, '/storage/buckets/' . $bucketId . '/files/' . $fileId, [ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], ], [ 'name' => 'permissions.png', ]);tests/e2e/Services/Webhooks/WebhooksCustomClientTest.php (1)
409-412: Missing#[Depends]attribute ontestUpdateAccountName.This method accepts a
$dataparameter (containingid,session), indicating it relies on data from a prior test. However, no#[Depends(...)]attribute is present. In PHPUnit 11, this will result in$databeing uninitialized, causing test failures.Based on the dependency chain, this method should depend on
testDeleteAccountSessionswhich returns the session data.🐛 Proposed fix to add the missing dependency
/** */ #[Retry(count: 1)] +#[Depends('testDeleteAccountSessions')] public function testUpdateAccountName($data): arraytests/e2e/Services/GraphQL/Legacy/DatabaseClientTest.php (1)
144-146: Add missing#[Depends]attribute totestCreateDocument.The method takes
$dataas a parameter but has no#[Depends]attribute. It should depend on eithertestCreateStringAttributeortestCreateIntegerAttribute(both return the same structure). Without this, the test will fail in PHPUnit 11 since the parameter won't be populated by the test framework.#[Depends('testCreateStringAttribute')] public function testCreateDocument($data): arraytests/e2e/Services/Account/AccountCustomClientTest.php (3)
482-486: Missing#[Depends]attribute ontestUpdateAccountPassword.The method signature
testUpdateAccountPassword($data): arrayexpects$dataparameter from a previous test, but there's no#[Depends]attribute. Based on the test flow and the fact that it uses$data['email'],$data['password'], and$data['session'], this should depend ontestCreateAccountSession.Proposed fix
- /** - */ + #[Depends('testCreateAccountSession')] #[Retry(count: 1)] public function testUpdateAccountPassword($data): array
1288-1291: Missing#[Depends]attribute ontestUpdateAccountRecovery.The method expects
$datacontaining'id'and'recovery'keys which are returned fromtestCreateAccountRecovery. The#[Depends]attribute is missing.Proposed fix
- /** - */ + #[Depends('testCreateAccountRecovery')] #[Retry(count: 1)] public function testUpdateAccountRecovery($data): array
2709-2711: Missing#[Depends]attribute ontestCreateSession.The method signature
testCreateSession(array $data): arrayexpects data containing'id'from a previous test (likely from the phone authentication chain), but there's no#[Depends]attribute.Proposed fix
- /** - */ + #[Depends('testUpdatePhone')] public function testCreateSession(array $data): arraytests/unit/Utopia/Response/Filters/V17Test.php (1)
160-173: Remove unusedwebhookProvider()method or add the corresponding test method.The static provider
webhookProvider()at line 160 is defined but never referenced by any test method. This indicates either dead code or an incomplete implementation from the PHPUnit 11 migration.
🤖 Fix all issues with AI agents
In `@tests/e2e/Services/Databases/Permissions/TablesDBPermissionsGuestTest.php`:
- Around line 322-331: The schema POST call using $this->client->call(...,
$this->getSchemaUrl($databaseId, $moviesId, 'string'), $this->getServerHeader(),
...) is missing any verification; capture the response returned by that call
(e.g. $response = $this->client->call(...)) and add assertions similar to the
earlier createCollection test: assert the expected HTTP status (created/201 or
appropriate), assert the response body contains the created schema key/name
('key' => 'title') and any expected fields (size, required) or an id, so the
test verifies the schema was actually created.
- Around line 66-85: The two Client::call() invocations that create schemas in
TablesDBPermissionsGuestTest.php are not checked; capture each call's response
(for the public and private collection schema creations), assert the response
indicates success (e.g., HTTP 201/200 or SDK success flag) and assert the
response body contains the expected schema attributes (key 'title', size 256,
required true) so failures surface immediately; reference the existing calls to
Client::METHOD_POST with getSchemaUrl($databaseId, $publicCollection['id'],
'string') and getSchemaUrl($databaseId, $privateCollection['id'], 'string') and
add assertions after each call validating status and returned schema fields.
In `@tests/e2e/Services/Databases/Permissions/TablesDBPermissionsTeamTest.php`:
- Around line 56-72: Add an assertion that the $collection1 response from
$this->client->call succeeded before using its body: verify the HTTP status is
201 (created) and that $collection1['body']['$id'] exists (mirroring the
validation used for $db creation) so the subsequent assignment
$this->collections['collection1'] = $collection1['body']['$id'] is safe.
- Around line 85-101: The creation response for $collection2 is not being
asserted; after the Client::METHOD_POST call that sets $collection2, add the
same status-code assertion used for collection1 (e.g. $this->assertEquals(201,
$collection2['headers']['status-code']) or the exact assertion style used for
$collection1) before assigning $this->collections['collection2'] =
$collection2['body']['$id']; to ensure the POST succeeded.
In `@tests/e2e/Services/Projects/ProjectsConsoleClientTest.php`:
- Around line 228-230: Several test methods (e.g., testListProject) accept a
$data parameter but lack the PHPUnit dependency attribute, causing $data to be
undefined at runtime; add the attribute #[Depends('testCreateProject')] above
each test method that declares a $data parameter (including testListProject and
the other methods referenced) so PHPUnit injects the result from
testCreateProject, and ensure the attribute is applied to the methods that use
$data['projectId'] (match by method names like testListProject, and the other
methods in the ranges noted).
In `@tests/e2e/Services/Users/UsersBase.php`:
- Around line 1164-1166: The tests use the #[Retry] attribute (e.g., on
testUpdateUserStatus) and the Retryable trait but there is no PHPUnit extension
to perform retries, so implement a RetryExtension that inspects the Retry
attribute (via Retryable::getNumberOfRetries()) and re-schedules failed tests,
register that extension alongside TestHook in the test bootstrap/extension
registration, or alternatively remove the unused #[Retry] attributes from tests
like testUpdateUserStatus to avoid dead code; ensure the new class is named
RetryExtension and wired into PHPUnit so Retryable::getNumberOfRetries() is
actually invoked when tests fail.
♻️ Duplicate comments (1)
composer.json (1)
90-100: Confirm PHPUnit 11 + ParaTest 7 compatibility and lockfile/config updates.You’re pinning
"phpunit/phpunit": "11.*"and adding"brianium/paratest": "7.*". Please confirm:
composer.lockis updated and committed for 1.8.x.phpunit.xml*is compatible with PHPUnit 11.- CI/locally, ParaTest executes this suite without breaking
#[Depends]chains (especially for e2e tests that pass arrays between methods).#!/bin/bash set -euo pipefail # 1) Ensure lockfile exists and includes the intended packages/versions if [[ ! -f composer.lock ]]; then echo "composer.lock is missing (expected committed change for pinned dev deps)." >&2 exit 1 fi echo "== composer.lock: phpunit/phpunit & brianium/paratest entries ==" rg -n '"name":\s*"phpunit/phpunit"|\"name\":\s*\"brianium/paratest\"' -C2 composer.lock || true rg -n '"version":\s*"v11\.' -C1 composer.lock || true rg -n '"version":\s*"v7\.' -C1 composer.lock || true # 2) Quick scan for leftover docblock annotations that PHPUnit 10/11 may not accept echo "== leftover phpunit docblock annotations (spot check) ==" rg -n --type=php '@depends|@dataProvider|@test\b' tests/ | head -n 50 || trueDoes ParaTest 7 support PHPUnit 11 (and does it preserve PHPUnit test method dependencies / #[Depends] execution order under parallel runs)?
🧹 Nitpick comments (12)
tests/e2e/Services/Databases/Permissions/TablesDBPermissionsGuestTest.php (2)
28-31: Consider a more descriptive database name.The database name
'InvalidDocumentDatabase'doesn't align with this test's purpose of testing guest permissions. A name like'GuestPermissionsDatabase'would better convey intent.
87-87: Consider consistent sleep durations or polling mechanism.The
sleep(2)here andsleep(1)at line 333 have inconsistent durations. If schema propagation timing is non-deterministic, consider either:
- Using a consistent duration across all schema waits
- Implementing a polling helper that waits until the schema is ready
tests/e2e/Services/Databases/Permissions/TablesDBPermissionsTeamTest.php (2)
16-24: Hardcoded databaseId may cause test isolation issues.The
$databaseIdis hardcoded as'testpermissiondb'. If tests run in parallel (e.g., with paratest as mentioned in the PR summary), this could cause conflicts. Consider usingID::unique()like inTablesDBPermissionsMemberTest.♻️ Suggested fix
public array $collections = []; -public string $databaseId = 'testpermissiondb'; +public string $databaseId = '';Then in
testSetupDatabase():+$this->databaseId = ID::unique(); $this->createUsers(); $this->createTeams();
201-221: Test uses collection name string instead of collection ID - potential bug.The data provider passes collection names like
'collection1'and'collection2'as strings (line 207), butgetRecordUrl()expects the actual collection ID. However, sincecreateCollections()usesID::custom('collection1'), the custom ID matches the string name, so this works correctly.The success path assertion (line 217) checks the count but doesn't verify the status code is 200. Consider adding a status assertion for completeness.
♻️ Suggested improvement
if ($success) { + $this->assertEquals(200, $documents['headers']['status-code']); $this->assertCount(1, $documents['body'][$this->getRecordResource()]); } else {tests/e2e/Services/GraphQL/TablesDB/DatabaseServerTest.php (1)
116-117: Pre-existing: Many test methods have undeclared dependencies.Several test methods like
testCreateStringColumn,testGetDatabase, andtestUpdateDatabaseaccept$dataor$databaseparameters but lack#[Depends]attributes. Without explicit dependencies, PHPUnit will call these methods withnullarguments, causing failures.This isn't introduced by this PR, but consider adding the missing dependency attributes for consistency with the PHPUnit 11 migration.
Also applies to: 922-923, 1197-1198
tests/e2e/Services/Teams/TeamsBase.php (1)
5-6: Add parameter types for depended-on data to avoid silent shape bugs.These methods are fed the return from
testCreateTeam(): array, so declaringarray $datamakes failures clearer.Proposed diff
#[Depends('testCreateTeam')] - public function testGetTeam($data): array + public function testGetTeam(array $data): array { #[Depends('testCreateTeam')] - public function testListTeams($data): array + public function testListTeams(array $data): array { #[Depends('testCreateTeam')] - public function testUpdateAndGetUserPrefs(array $data): void + public function testUpdateAndGetUserPrefs(array $data): void {Also applies to: 130-132, 159-161, 437-439
.github/workflows/tests.yml (1)
309-317: Consider using consistent process count strategy.The e2e_service_test uses
$(nproc)for dynamic scaling while e2e_shared_mode_test uses a fixed--processes 4. This inconsistency could lead to different behavior across environments.💡 Suggestion for consistent parallelism
Consider using the same strategy for both jobs, or document why shared mode needs a fixed count:
- appwrite vendor/bin/paratest --processes 4 "$SERVICE_PATH" --exclude-group abuseEnabled,screenshots + appwrite vendor/bin/paratest --processes $(nproc) "$SERVICE_PATH" --exclude-group abuseEnabled,screenshotsAlternatively, if there's a resource constraint reason for the fixed count in shared mode, consider adding a comment explaining it.
tests/e2e/Services/Account/AccountCustomServerTest.php (1)
214-216: Good: email lookup is now recipient-scoped; add a small guard before parsingtext.
Right after Line 215, consider assertingtextexists to avoid a noisy undefined-index failure if Maildev returns a different payload.Proposed tweak
$lastEmail = $this->getLastEmailByAddress($email); $this->assertNotEmpty($lastEmail, 'Email not found for address: ' . $email); + $this->assertArrayHasKey('text', $lastEmail); $this->assertEquals($this->getProject()['name'] . ' Login', $lastEmail['subject']);tests/e2e/Services/Databases/Permissions/LegacyPermissionsGuestTest.php (2)
108-171: Avoid order-sensitive$permissionsassertions in responses.
$permissionsmay come back in a different order than sent; using canonicalizing comparison prevents flaky failures. Also consider asserting list response status codes explicitly.Proposed fix (canonicalizing compare + strict in_array)
- $this->assertEquals($permissions, $publicDocuments['body'][$recordKey][0]['$permissions']); + $this->assertEqualsCanonicalizing($permissions, $publicDocuments['body'][$recordKey][0]['$permissions']); - if (\in_array(Permission::read(Role::any()), $permissions)) { + if (\in_array(Permission::read(Role::any()), $permissions, true)) { $this->assertEquals(1, $privateDocuments['body']['total']); - $this->assertEquals($permissions, $privateDocuments['body'][$recordKey][0]['$permissions']); + $this->assertEqualsCanonicalizing($permissions, $privateDocuments['body'][$recordKey][0]['$permissions']); } else { $this->assertEquals(0, $privateDocuments['body']['total']); }
287-355: Remove redundantarray_merge([...])and assert setup calls in this test too.This test currently doesn’t assert collection/schema creation responses, and the single-argument
array_mergeis a no-op.tests/e2e/Services/Databases/Permissions/LegacyPermissionsTeamTest.php (1)
43-117: Add response assertions for collection/schema creation (fail fast) and prefer polling oversleep(2).Right now the test can proceed with partially-created schema/collections and fail later with harder-to-debug errors.
tests/e2e/Services/Databases/Permissions/LegacyPermissionsMemberTest.php (1)
119-225: Replacesleep(2)with a readiness/polling helper if available.Fixed sleeps tend to be the top source of e2e flakes (too short) and slow builds (too long).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (60)
.github/workflows/tests.ymlcomposer.jsontests/e2e/General/UsageTest.phptests/e2e/Services/Account/AccountBase.phptests/e2e/Services/Account/AccountCustomClientTest.phptests/e2e/Services/Account/AccountCustomServerTest.phptests/e2e/Services/Databases/DatabasesBase.phptests/e2e/Services/Databases/Permissions/LegacyPermissionsGuestTest.phptests/e2e/Services/Databases/Permissions/LegacyPermissionsMemberTest.phptests/e2e/Services/Databases/Permissions/LegacyPermissionsTeamTest.phptests/e2e/Services/Databases/Permissions/TablesDBPermissionsGuestTest.phptests/e2e/Services/Databases/Permissions/TablesDBPermissionsMemberTest.phptests/e2e/Services/Databases/Permissions/TablesDBPermissionsTeamTest.phptests/e2e/Services/Functions/FunctionsConsoleClientTest.phptests/e2e/Services/Functions/FunctionsCustomServerTest.phptests/e2e/Services/GraphQL/AccountTest.phptests/e2e/Services/GraphQL/FunctionsClientTest.phptests/e2e/Services/GraphQL/FunctionsServerTest.phptests/e2e/Services/GraphQL/Legacy/DatabaseClientTest.phptests/e2e/Services/GraphQL/Legacy/DatabaseServerTest.phptests/e2e/Services/GraphQL/MessagingTest.phptests/e2e/Services/GraphQL/StorageClientTest.phptests/e2e/Services/GraphQL/StorageServerTest.phptests/e2e/Services/GraphQL/TablesDB/DatabaseClientTest.phptests/e2e/Services/GraphQL/TablesDB/DatabaseServerTest.phptests/e2e/Services/GraphQL/TeamsClientTest.phptests/e2e/Services/GraphQL/TeamsServerTest.phptests/e2e/Services/GraphQL/UsersTest.phptests/e2e/Services/Messaging/MessagingBase.phptests/e2e/Services/Messaging/MessagingConsoleClientTest.phptests/e2e/Services/Migrations/MigrationsBase.phptests/e2e/Services/Projects/ProjectsConsoleClientTest.phptests/e2e/Services/Realtime/RealtimeConsoleClientTest.phptests/e2e/Services/Realtime/RealtimeCustomClientTest.phptests/e2e/Services/Sites/SitesConsoleClientTest.phptests/e2e/Services/Storage/StorageBase.phptests/e2e/Services/Storage/StorageCustomClientTest.phptests/e2e/Services/Storage/StorageCustomServerTest.phptests/e2e/Services/Teams/TeamsBase.phptests/e2e/Services/Teams/TeamsBaseClient.phptests/e2e/Services/Teams/TeamsBaseServer.phptests/e2e/Services/Teams/TeamsConsoleClientTest.phptests/e2e/Services/Teams/TeamsCustomClientTest.phptests/e2e/Services/Tokens/TokensBase.phptests/e2e/Services/Tokens/TokensConsoleClientTest.phptests/e2e/Services/Tokens/TokensCustomServerTest.phptests/e2e/Services/Users/UsersBase.phptests/e2e/Services/VCS/VCSConsoleClientTest.phptests/e2e/Services/Webhooks/WebhooksBase.phptests/e2e/Services/Webhooks/WebhooksCustomClientTest.phptests/e2e/Services/Webhooks/WebhooksCustomServerTest.phptests/unit/Utopia/Database/Validator/ProjectIdTest.phptests/unit/Utopia/Request/Filters/V16Test.phptests/unit/Utopia/Request/Filters/V17Test.phptests/unit/Utopia/Request/Filters/V18Test.phptests/unit/Utopia/Request/Filters/V19Test.phptests/unit/Utopia/Response/Filters/V16Test.phptests/unit/Utopia/Response/Filters/V17Test.phptests/unit/Utopia/Response/Filters/V18Test.phptests/unit/Utopia/Response/Filters/V19Test.php
💤 Files with no reviewable changes (3)
- tests/e2e/Services/GraphQL/FunctionsServerTest.php
- tests/e2e/Services/GraphQL/AccountTest.php
- tests/e2e/Services/GraphQL/FunctionsClientTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Services/Teams/TeamsCustomClientTest.php
🧰 Additional context used
🧬 Code graph analysis (31)
tests/unit/Utopia/Request/Filters/V18Test.php (3)
tests/unit/Utopia/Request/Filters/V16Test.php (1)
DataProvider(46-54)tests/unit/Utopia/Request/Filters/V17Test.php (2)
DataProvider(45-53)DataProvider(77-85)tests/unit/Utopia/Request/Filters/V19Test.php (2)
DataProvider(63-71)DataProvider(73-81)
tests/e2e/Services/GraphQL/TablesDB/DatabaseServerTest.php (2)
tests/e2e/General/UsageTest.php (11)
Depends(184-226)Depends(228-359)Depends(361-416)Depends(418-582)Depends(584-656)Depends(658-828)Depends(830-907)Depends(909-1060)Depends(1062-1124)Depends(1192-1266)Depends(1268-1375)tests/e2e/Services/Account/AccountCustomClientTest.php (5)
Depends(24-128)Depends(130-175)Depends(177-209)Depends(211-264)Depends(266-415)
tests/e2e/Services/Messaging/MessagingConsoleClientTest.php (1)
tests/e2e/Services/Account/AccountCustomClientTest.php (5)
Depends(24-128)Depends(130-175)Depends(177-209)Depends(211-264)Depends(266-415)
tests/unit/Utopia/Request/Filters/V16Test.php (4)
tests/unit/Utopia/Database/Validator/ProjectIdTest.php (1)
DataProvider(35-39)tests/unit/Utopia/Request/Filters/V17Test.php (2)
DataProvider(45-53)DataProvider(77-85)tests/unit/Utopia/Request/Filters/V18Test.php (1)
DataProvider(41-49)tests/unit/Utopia/Request/Filters/V19Test.php (2)
DataProvider(63-71)DataProvider(73-81)
tests/e2e/Services/Storage/StorageBase.php (2)
tests/e2e/Services/GraphQL/StorageServerTest.php (1)
Depends(53-87)tests/e2e/Services/Storage/StorageCustomServerTest.php (4)
Depends(75-170)Depends(172-227)Depends(229-268)Depends(270-305)
tests/e2e/Services/GraphQL/Legacy/DatabaseClientTest.php (1)
tests/e2e/Services/Account/AccountCustomClientTest.php (5)
Depends(24-128)Depends(130-175)Depends(177-209)Depends(211-264)Depends(266-415)
tests/unit/Utopia/Response/Filters/V16Test.php (3)
tests/unit/Utopia/Response/Filters/V17Test.php (5)
DataProvider(56-64)DataProvider(80-88)DataProvider(103-111)DataProvider(126-134)projectProvider(26-54)tests/unit/Utopia/Response/Filters/V19Test.php (3)
deploymentProvider(113-139)functionProvider(26-47)projectProvider(222-243)tests/unit/Utopia/Response/Filters/V18Test.php (3)
executionProvider(53-104)functionProvider(26-40)projectProvider(116-134)
tests/e2e/Services/Webhooks/WebhooksCustomClientTest.php (1)
tests/e2e/Services/Account/AccountCustomClientTest.php (16)
Depends(24-128)Depends(130-175)Depends(177-209)Depends(211-264)Depends(266-415)Depends(419-480)Depends(719-793)Depends(795-898)Depends(900-973)Depends(975-1025)Depends(1027-1091)Depends(1093-1146)Depends(1148-1194)Depends(1196-1286)Depends(1462-1527)Depends(1962-1978)
tests/e2e/Services/Teams/TeamsConsoleClientTest.php (2)
tests/e2e/General/UsageTest.php (11)
Depends(184-226)Depends(228-359)Depends(361-416)Depends(418-582)Depends(584-656)Depends(658-828)Depends(830-907)Depends(909-1060)Depends(1062-1124)Depends(1192-1266)Depends(1268-1375)tests/e2e/Services/Account/AccountCustomClientTest.php (5)
Depends(24-128)Depends(130-175)Depends(177-209)Depends(211-264)Depends(266-415)
tests/e2e/Services/Migrations/MigrationsBase.php (1)
tests/e2e/Services/Account/AccountCustomClientTest.php (16)
Depends(24-128)Depends(130-175)Depends(177-209)Depends(211-264)Depends(266-415)Depends(419-480)Depends(719-793)Depends(795-898)Depends(900-973)Depends(975-1025)Depends(1027-1091)Depends(1093-1146)Depends(1148-1194)Depends(1196-1286)Depends(1462-1527)Depends(1962-1978)
tests/e2e/Services/Sites/SitesConsoleClientTest.php (2)
tests/e2e/Services/GraphQL/TeamsServerTest.php (1)
Group(284-305)tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (14)
Group(27-113)Group(161-226)Group(228-434)Group(436-693)Group(868-940)Group(942-1044)Group(1046-1133)Group(1975-2175)Group(4708-4764)Group(4767-4853)Group(4856-4896)Group(4898-4951)Group(4953-5030)Group(5032-5085)
tests/unit/Utopia/Response/Filters/V17Test.php (3)
tests/unit/Utopia/Request/Filters/V16Test.php (1)
DataProvider(46-54)tests/unit/Utopia/Response/Filters/V18Test.php (1)
projectProvider(116-134)tests/unit/Utopia/Response/Filters/V19Test.php (1)
projectProvider(222-243)
tests/e2e/Services/GraphQL/TablesDB/DatabaseClientTest.php (2)
tests/e2e/General/UsageTest.php (11)
Depends(184-226)Depends(228-359)Depends(361-416)Depends(418-582)Depends(584-656)Depends(658-828)Depends(830-907)Depends(909-1060)Depends(1062-1124)Depends(1192-1266)Depends(1268-1375)tests/e2e/Services/Account/AccountCustomClientTest.php (5)
Depends(24-128)Depends(130-175)Depends(177-209)Depends(211-264)Depends(266-415)
tests/e2e/Services/Webhooks/WebhooksBase.php (2)
tests/e2e/General/UsageTest.php (3)
Depends(184-226)Depends(228-359)Depends(361-416)tests/e2e/Services/Webhooks/WebhooksCustomServerTest.php (13)
Depends(26-67)Depends(69-136)Depends(209-250)Depends(252-318)Depends(442-479)Depends(481-524)Depends(526-564)Depends(603-653)Depends(655-701)Depends(703-748)Depends(750-817)Depends(819-860)Depends(862-897)
tests/e2e/Services/Teams/TeamsBaseClient.php (1)
tests/e2e/Scopes/Scope.php (1)
getLastEmailByAddress(66-87)
tests/e2e/Services/Realtime/RealtimeCustomClientTest.php (1)
tests/e2e/Services/Account/AccountCustomClientTest.php (5)
Depends(24-128)Depends(130-175)Depends(177-209)Depends(211-264)Depends(266-415)
tests/e2e/Services/Databases/Permissions/TablesDBPermissionsGuestTest.php (1)
tests/e2e/Scopes/ApiTablesDB.php (4)
getContainerIdParam(26-29)getSecurityParam(51-54)getRecordIdParam(46-49)getRecordResource(41-44)
tests/e2e/Services/GraphQL/UsersTest.php (2)
tests/e2e/General/UsageTest.php (11)
Depends(184-226)Depends(228-359)Depends(361-416)Depends(418-582)Depends(584-656)Depends(658-828)Depends(830-907)Depends(909-1060)Depends(1062-1124)Depends(1192-1266)Depends(1268-1375)tests/e2e/Services/Account/AccountCustomClientTest.php (5)
Depends(24-128)Depends(130-175)Depends(177-209)Depends(211-264)Depends(266-415)
tests/e2e/Services/Tokens/TokensCustomServerTest.php (2)
tests/e2e/General/UsageTest.php (11)
Depends(184-226)Depends(228-359)Depends(361-416)Depends(418-582)Depends(584-656)Depends(658-828)Depends(830-907)Depends(909-1060)Depends(1062-1124)Depends(1192-1266)Depends(1268-1375)tests/e2e/Services/Tokens/TokensConsoleClientTest.php (3)
Depends(124-187)Depends(189-241)Depends(243-255)
tests/e2e/Services/GraphQL/TeamsServerTest.php (2)
tests/e2e/Services/Account/AccountCustomClientTest.php (5)
Depends(24-128)Depends(130-175)Depends(177-209)Depends(211-264)Depends(266-415)tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (15)
Group(27-113)Group(161-226)Group(228-434)Group(436-693)Group(868-940)Group(942-1044)Group(1046-1133)Group(1975-2175)Group(4708-4764)Group(4767-4853)Group(4856-4896)Group(4898-4951)Group(4953-5030)Group(5032-5085)Group(5087-5188)
tests/unit/Utopia/Response/Filters/V19Test.php (2)
tests/unit/Utopia/Request/Filters/V16Test.php (1)
DataProvider(46-54)tests/unit/Utopia/Response/Filters/V18Test.php (1)
functionProvider(26-40)
tests/e2e/Services/Databases/Permissions/LegacyPermissionsMemberTest.php (1)
tests/e2e/Scopes/ApiLegacy.php (3)
getContainerIdParam(26-29)getSecurityParam(51-54)getRecordIdParam(46-49)
tests/e2e/Services/Databases/Permissions/TablesDBPermissionsTeamTest.php (2)
tests/e2e/Services/Databases/Permissions/DatabasesPermissionsBase.php (3)
createUser(47-81)getServerHeader(116-123)addToTeam(100-113)tests/e2e/Scopes/ApiTablesDB.php (3)
getContainerIdParam(26-29)getRecordIdParam(46-49)getRecordResource(41-44)
tests/e2e/Services/GraphQL/StorageClientTest.php (1)
tests/e2e/Services/Account/AccountCustomClientTest.php (5)
Depends(24-128)Depends(130-175)Depends(177-209)Depends(211-264)Depends(266-415)
tests/e2e/Services/Databases/Permissions/TablesDBPermissionsMemberTest.php (3)
tests/e2e/Services/Databases/Permissions/DatabasesPermissionsBase.php (1)
createUser(47-81)tests/e2e/Scopes/ApiLegacy.php (3)
getContainerIdParam(26-29)getSecurityParam(51-54)getRecordIdParam(46-49)tests/e2e/Scopes/ApiTablesDB.php (3)
getContainerIdParam(26-29)getSecurityParam(51-54)getRecordIdParam(46-49)
tests/unit/Utopia/Response/Filters/V18Test.php (1)
tests/unit/Utopia/Response/Filters/V16Test.php (7)
DataProvider(68-76)DataProvider(116-124)DataProvider(142-150)DataProvider(210-218)functionProvider(126-140)executionProvider(78-114)projectProvider(173-208)
tests/e2e/Services/GraphQL/TeamsClientTest.php (1)
tests/e2e/Services/Account/AccountCustomClientTest.php (5)
Depends(24-128)Depends(130-175)Depends(177-209)Depends(211-264)Depends(266-415)
tests/e2e/Services/Webhooks/WebhooksCustomServerTest.php (1)
tests/e2e/Services/Webhooks/WebhooksBase.php (16)
Depends(103-196)Depends(198-257)Depends(259-315)Depends(449-540)Depends(542-601)Depends(603-659)Depends(777-818)Depends(820-890)Depends(892-944)Depends(946-992)Depends(994-1028)Depends(1074-1115)Depends(1117-1160)Depends(1211-1287)Depends(1289-1347)Depends(1406-1469)
tests/e2e/Services/GraphQL/MessagingTest.php (1)
tests/e2e/Services/Messaging/MessagingBase.php (16)
Depends(133-242)Depends(270-283)Depends(285-295)Depends(297-308)Depends(342-367)Depends(369-403)Depends(405-418)Depends(420-508)Depends(559-574)Depends(576-647)Depends(649-772)Depends(774-795)Depends(797-806)Depends(808-874)Depends(1367-1418)Depends(1532-1580)
tests/e2e/Services/GraphQL/Legacy/DatabaseServerTest.php (2)
tests/e2e/General/UsageTest.php (11)
Depends(184-226)Depends(228-359)Depends(361-416)Depends(418-582)Depends(584-656)Depends(658-828)Depends(830-907)Depends(909-1060)Depends(1062-1124)Depends(1192-1266)Depends(1268-1375)tests/e2e/Services/Account/AccountCustomClientTest.php (5)
Depends(24-128)Depends(130-175)Depends(177-209)Depends(211-264)Depends(266-415)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (5)
tests/e2e/Services/Account/AccountCustomClientTest.php (16)
Depends(24-128)Depends(130-175)Depends(177-209)Depends(211-264)Depends(266-415)Depends(419-480)Depends(719-793)Depends(795-898)Depends(900-973)Depends(975-1025)Depends(1027-1091)Depends(1093-1146)Depends(1148-1194)Depends(1196-1286)Depends(1462-1527)Depends(1962-1978)tests/e2e/Services/Account/AccountBase.php (1)
Group(369-439)tests/e2e/Services/GraphQL/TeamsServerTest.php (1)
Group(284-305)tests/e2e/Services/Sites/SitesConsoleClientTest.php (1)
Group(18-149)tests/e2e/Services/Storage/StorageBase.php (1)
Group(18-299)
⏰ 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). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
tests/e2e/Services/Databases/Permissions/LegacyPermissionsMemberTest.php
Show resolved
Hide resolved
tests/e2e/Services/Databases/Permissions/TablesDBPermissionsGuestTest.php
Outdated
Show resolved
Hide resolved
tests/e2e/Services/Databases/Permissions/TablesDBPermissionsGuestTest.php
Outdated
Show resolved
Hide resolved
tests/e2e/Services/Databases/Permissions/TablesDBPermissionsTeamTest.php
Show resolved
Hide resolved
tests/e2e/Services/Databases/Permissions/TablesDBPermissionsTeamTest.php
Show resolved
Hide resolved
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 (1)
tests/extensions/Retryable.php (1)
1-13: Missing import forRetryclass.Similar to
RetrySubscriber.php, this file referencesRetry::classon line 31 without a correspondingusestatement.🐛 Proposed fix
<?php namespace Appwrite\Tests; +use Appwrite\Tests\Retry; + /** * Marker trait for classes that support retry functionality.
♻️ Duplicate comments (1)
tests/extensions/Retryable.php (1)
21-36:getNumberOfRetries()remains unused by the retry infrastructure.The
RetrySubscriberimplements its owngetRetryCountForTest()method rather than calling this trait method. This creates duplication and leaves the trait's method effectively dead.Consider either:
- Having
RetrySubscriberuse this trait method on test instances that implementRetryable- Removing this method if the trait is purely a marker
🧹 Nitpick comments (4)
tests/extensions/RetrySubscriber.php (3)
19-24:$pendingRetriesis declared but never used.This static property is only cleared in
reset()but never populated or read. Either remove it or complete the pending retry queue implementation.♻️ Remove dead code
- /** - * Track tests that should be retried - * - * `@var` array<string, array{test: TestMethod, remainingRetries: int, lastError: \Throwable|null}> - */ - private static array $pendingRetries = [];And update
reset():public static function reset(): void { self::$retryCounts = []; - self::$pendingRetries = []; }
31-35: Consider using$errorMessagefor enhanced retry logging or remove the unused parameter.The error message is extracted but never displayed. Including it in the retry output could help developers diagnose flaky tests.
♻️ Option 1: Use the error message in retry output
fwrite( STDOUT, sprintf( - "\e[33m[RETRY] Test %s failed (attempt %d/%d). %s\e[0m\n", + "\e[33m[RETRY] Test %s failed (attempt %d/%d). %s\nError: %s\e[0m\n", $testId, $currentAttempt + 1, $retryCount + 1, - $remainingRetries > 0 ? "Will retry {$remainingRetries} more time(s)." : "No more retries." + $remainingRetries > 0 ? "Will retry {$remainingRetries} more time(s)." : "No more retries.", + substr($errorMessage, 0, 200) // Truncate for readability ) );
63-93: This method duplicates logic inRetryable::getNumberOfRetries().Both implementations use reflection to read the
Retryattribute from test methods. Consider extracting shared logic to avoid divergence.tests/extensions/Retryable.php (1)
23-28: Consider adding a type assertion or documenting the TestCase requirement.
$this->name()assumes usage within aPHPUnit\Framework\TestCasesubclass. While this is likely the intended use, the trait doesn't enforce this constraint.♻️ Add defensive assertion
public function getNumberOfRetries(): int { + if (!method_exists($this, 'name')) { + return 0; + } + $root = new \ReflectionClass($this); $name = $this->name();Or document in the trait docblock:
/** * Marker trait for classes that support retry functionality. + * This trait must be used in a PHPUnit\Framework\TestCase subclass. * The actual retry logic is handled by the RetrySubscriber extension.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/e2e/Services/Databases/Permissions/TablesDBPermissionsGuestTest.phptests/e2e/Services/Databases/Permissions/TablesDBPermissionsTeamTest.phptests/e2e/Services/Projects/ProjectsConsoleClientTest.phptests/extensions/RetrySubscriber.phptests/extensions/Retryable.phptests/extensions/TestHook.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Services/Databases/Permissions/TablesDBPermissionsGuestTest.php
🧰 Additional context used
🧬 Code graph analysis (3)
tests/extensions/TestHook.php (2)
tests/extensions/TestFinishedSubscriber.php (1)
TestFinishedSubscriber(8-30)tests/extensions/RetrySubscriber.php (1)
RetrySubscriber(10-103)
tests/e2e/Services/Databases/Permissions/TablesDBPermissionsTeamTest.php (2)
tests/e2e/Scopes/ApiLegacy.php (3)
getContainerIdParam(26-29)getRecordIdParam(46-49)getRecordResource(41-44)tests/e2e/Scopes/ApiTablesDB.php (3)
getContainerIdParam(26-29)getRecordIdParam(46-49)getRecordResource(41-44)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (2)
tests/e2e/Services/Account/AccountCustomClientTest.php (14)
Depends(24-128)Depends(130-175)Depends(177-209)Depends(211-264)Depends(266-415)Depends(419-480)Depends(719-793)Depends(795-898)Depends(900-973)Depends(975-1025)Depends(1027-1091)Depends(1093-1146)Depends(1148-1194)Depends(1196-1286)tests/e2e/Services/GraphQL/TeamsServerTest.php (1)
Group(284-305)
🪛 PHPMD (2.15.0)
tests/extensions/TestHook.php
14-14: Avoid unused parameters such as '$configuration'. (undefined)
(UnusedFormalParameter)
14-14: Avoid unused parameters such as '$parameters'. (undefined)
(UnusedFormalParameter)
tests/extensions/RetrySubscriber.php
31-31: Avoid unused parameters such as '$errorMessage'. (undefined)
(UnusedFormalParameter)
⏰ 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 (Teams)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Service Test (Abuse enabled)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (17)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (6)
7-8: LGTM!The imports for
DependsandGroupattributes are correctly added for PHPUnit 11 compatibility.
27-29: LGTM!The
#[Group]attributes are correctly applied totestCreateProject, which serves as the root dependency for multiple downstream tests.
228-230: Previously flagged issue has been resolved.The
#[Depends('testCreateProject')]attribute is now correctly applied totestListProject, ensuring the$dataparameter receives the project ID from the dependency chain.
869-871: Previously flagged issue has been resolved.The
#[Depends('testCreateProject')]attribute is now correctly applied totestUpdateProjectSMTP.
1048-1050: Previously flagged issue has been resolved.The
#[Depends('testCreateProject')]attribute is now correctly applied totestUpdateTemplates.
1978-1981: Previously flagged issue has been resolved.The
#[Depends('testCreateProject')]attribute is now correctly applied totestUpdateMockNumbers, along with the appropriate#[Group]attributes.tests/e2e/Services/Databases/Permissions/TablesDBPermissionsTeamTest.php (9)
1-14: LGTM!Namespace and imports are correctly structured for PHPUnit 11 with appropriate attribute imports and Utopia database helpers.
16-24: LGTM!Class structure with trait composition follows the established pattern for TablesDB permission tests. The
ApiTablesDBtrait provides the correct URL helpers (getContainerIdParam()returns'tableId',getRecordResource()returns'rows').
26-32: LGTM!Team creation helper delegates to the base trait's
createTeammethod. The returned array structure is consistent with usage increateCollections.
34-41: LGTM!User creation follows the same pattern as team creation, delegating to the base trait's
createUsermethod.
43-121: LGTM!Collection setup is well-structured with proper assertions for all API calls. The permission model correctly grants:
- Collection 1: read access to team1 members, write access to team1 admins
- Collection 2: read access to team2 members, write access to team2 owners
The
sleep(2)at line 118 accommodates async schema indexing. Consider using a polling helper (per the PR'sSchemaPollingtrait) if flaky tests arise.
123-153: LGTM!Data providers correctly enumerate the expected permission matrix:
- Read: Team membership grants read access (no role required), so user3 can read both collections.
- Write: Requires specific roles (admin for team1, owner for team2), so user3 cannot write to either.
Test coverage is comprehensive for the permission combinations.
155-203: LGTM!Test setup correctly:
- Creates users and teams with appropriate role assignments
- Creates collections with team-based permissions
- Seeds each collection with a document using server credentials
- Returns
$this->usersfor dependent test methods (PHPUnit passes this as the last parameter)The PHPDoc explaining data provider state loss is helpful documentation.
205-225: LGTM!The test correctly validates read permissions using user sessions via cookie authentication. The
#[Depends]attribute ensures$usersis passed fromtestSetupDatabase, and#[DataProvider]supplies the test matrix.Minor suggestion: Consider also asserting
$this->assertEquals(200, $documents['headers']['status-code'])in the success branch for explicit status verification, though the count assertion implicitly confirms success.
227-255: LGTM!Write permission test correctly handles the two failure modes:
- 401: User is in the team (collection visible) but lacks the required role for write access
- 404: User is not in the team, so the collection is not discoverable
The
assertContainscheck appropriately validates either error response.tests/extensions/TestHook.php (1)
10-18: LGTM! Extension implementation follows PHPUnit 11 patterns correctly.The unused
$configurationand$parametersflagged by PHPMD are false positives—these parameters are mandated by theExtensioninterface contract and cannot be removed.tests/extensions/RetrySubscriber.php (1)
46-60: This feature is incomplete: tests decorated with#[Retry]will not actually re-execute on failure.The current implementation only logs retry attempts. While
FailedSubscriberis invoked after a test fails, it cannot trigger re-execution—PHPUnit's event system does not support re-running tests from this hook.The unused
$pendingRetriesproperty (line 24) suggests an attempt to build actual retry logic that was never completed. Update the implementation to either:
- Implement real retry execution using a suitable PHPUnit hook (if one exists for PHPUnit 11), or
- Remove the
#[Retry]attribute and clarify inRetryablethat retry tracking is informational onlyUntil resolved, tests using
#[Retry(count: N)]will fail despite the expectation of automatic retries.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
PHPUnit 11+ warns when using comma-separated values with --exclude-group. This warning causes the tests to return exit code 1 despite all tests passing. Fix by using separate --exclude-group flags for each group. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PHPUnit 12 requires PHP 8.3+ and uses PHP enums like CodeCoverageInitializationStatus. The old composer:2.0 image used PHP 8.0.6 which doesn't support enums, causing autoloader issues. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace @Depends annotations with static property assertions to fix compatibility issues with PHPUnit 12 and paratest parallel execution. Tests now use self::$databaseId and self::$collectionId/self::$tableId directly instead of passing data between dependent tests. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/e2e/Services/Databases/TablesDB/DatabasesStringTypesTest.php (2)
624-628:⚠️ Potential issue | 🔴 CriticalAssertions expect values from skipped tests - test will fail.
Lines 626-627 assert default values of
'updated default'and'Updated text default value', but these values would only exist iftestUpdateVarcharColumnandtestUpdateTextColumnhad run successfully. Both tests are marked as skipped (lines 425, 479).The actual defaults are:
varchar_with_default:'hello world'(line 93)text_with_default:'This is a longer default text value that can contain more content.'(line 261)🐛 Proposed fix to match actual default values
$this->assertEquals(201, $row['headers']['status-code']); // Check that default values are applied - $this->assertEquals('updated default', $row['body']['varchar_with_default']); - $this->assertEquals('Updated text default value', $row['body']['text_with_default']); + $this->assertEquals('hello world', $row['body']['varchar_with_default']); + $this->assertEquals('This is a longer default text value that can contain more content.', $row['body']['text_with_default']);
807-823:⚠️ Potential issue | 🔴 CriticalDeleting non-existent column
varchar_max- test will fail.The test attempts to delete column
varchar_max, but this column is never created. The varchar columns created intestCreateVarcharColumnare:
varchar_fieldvarchar_with_defaultvarchar_requiredvarchar_arrayvarchar_minThe DELETE call on line 808 will likely return 404 instead of 204.
🐛 Proposed fix to delete an existing column
// Test SUCCESS: Delete varchar column - $deleteVarchar = $this->client->call(Client::METHOD_DELETE, '/tablesdb/' . $databaseId . '/tables/' . $tableId . '/columns/varchar_max', [ + $deleteVarchar = $this->client->call(Client::METHOD_DELETE, '/tablesdb/' . $databaseId . '/tables/' . $tableId . '/columns/varchar_min', [ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], 'x-appwrite-key' => $this->getProject()['apiKey'] ]); $this->assertEquals(204, $deleteVarchar['headers']['status-code']); // Verify deletion - $getDeleted = $this->client->call(Client::METHOD_GET, '/tablesdb/' . $databaseId . '/tables/' . $tableId . '/columns/varchar_max', [ + $getDeleted = $this->client->call(Client::METHOD_GET, '/tablesdb/' . $databaseId . '/tables/' . $tableId . '/columns/varchar_min', [ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], 'x-appwrite-key' => $this->getProject()['apiKey'] ]);tests/e2e/Services/Databases/Legacy/DatabasesStringTypesTest.php (2)
734-758:⚠️ Potential issue | 🟠 MajorDelete uses
varchar_maxwhich is never created.No attribute with key
varchar_maxis created in this class, so the delete likely returns 404. Delete an existing attribute or createvarchar_maxearlier.✅ Suggested fix (delete an existing attribute)
- $deleteVarchar = $this->client->call(Client::METHOD_DELETE, '/databases/' . $databaseId . '/collections/' . $collectionId . '/attributes/varchar_max', [ + $deleteVarchar = $this->client->call(Client::METHOD_DELETE, '/databases/' . $databaseId . '/collections/' . $collectionId . '/attributes/varchar_min', [
597-628:⚠️ Potential issue | 🟠 MajorAssertions expect "updated" defaults that never happen—align with initial defaults.
After removing
@dependsannotations,testCreateDocumentWithDefaultValues()now runs regardless of whether the update tests execute. However,testUpdateVarcharAttribute()andtestUpdateTextAttribute()are unconditionally skipped, so attribute defaults remain at their initial values. The assertions expect the "updated" values that never get applied.Suggested fix
- $this->assertEquals('updated default', $document['body']['varchar_with_default']); - $this->assertEquals('Updated text default value', $document['body']['text_with_default']); + $this->assertEquals('hello world', $document['body']['varchar_with_default']); + $this->assertEquals('This is a longer default text value that can contain more content.', $document['body']['text_with_default']);
🤖 Fix all issues with AI agents
In `@tests/e2e/Services/Databases/Legacy/DatabasesStringTypesTest.php`:
- Around line 36-39: Initialize the test class's static ID properties so PHP
won't throw "Typed property must not be accessed before initialization" when
assertions like assertNotEmpty(self::$databaseId) run; update the class
declaration for DatabasesStringTypesTest to give each static ID a safe default
(e.g., empty string or null-acceptable default) for properties such as
self::$databaseId and any other static IDs referenced across the file
(collectionId, documentId, etc.), ensuring the typed static properties are
initialized before tests run so assertions execute instead of a fatal Error.
In `@tests/e2e/Services/Databases/TablesDB/DatabasesStringTypesTest.php`:
- Line 21: The test suite relies on ordering because testCreateDatabase() sets
self::$databaseId used by testCreateTable(); restore explicit PHPUnit
dependencies by adding a `@depends` annotation on testCreateTable (and any other
tests that use self::$databaseId) referencing testCreateDatabase, or
alternatively make tests independent by creating their own database in
setUp()/setUpBeforeClass() and storing it on the test class; the quick fix is to
add `@depends` on testCreateTable to reference testCreateDatabase (ensure the
static property self::$databaseId remains populated in testCreateDatabase).
🧹 Nitpick comments (1)
tests/e2e/Services/Databases/TablesDB/DatabasesStringTypesTest.php (1)
58-65: Consider extracting repeated precondition boilerplate.The pattern of asserting prerequisites and assigning local variables is repeated in every test method:
$this->assertNotEmpty(self::$databaseId, 'Database must be created first'); $this->assertNotEmpty(self::$tableId, 'Table must be created first'); $databaseId = self::$databaseId; $tableId = self::$tableId;A helper method could reduce duplication:
♻️ Optional: Extract to helper method
private function requireDatabaseAndTable(): array { $this->assertNotEmpty(self::$databaseId, 'Database must be created first'); $this->assertNotEmpty(self::$tableId, 'Table must be created first'); return [self::$databaseId, self::$tableId]; }Usage:
public function testCreateVarcharColumn(): void { [$databaseId, $tableId] = $this->requireDatabaseAndTable(); // ... }Also applies to: 143-149, 230-236, 295-301, 359-365
Remove @Depends annotation and create a new database within the test to fix compatibility with PHPUnit 12 and paratest parallel execution. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove @Depends annotation and set up phone number within the test to ensure it works with PHPUnit 12 and paratest parallel execution. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/e2e/Services/GraphQL/AccountTest.php`:
- Around line 126-140: Capture and validate the response from the precondition
phone update call so failures are detected immediately: call
$this->client->call(...) with $phonePayload into a $response variable, decode
the response body and assert the operation succeeded (e.g., no errors and
expected data present) before continuing the test; update references around
getQuery(self::UPDATE_ACCOUNT_PHONE), $phonePayload, client->call and
$this->getHeaders() to include the response capture and appropriate assertions.
- Update expected default values to match the actual defaults (update tests are skipped, so original defaults apply) - Fix varchar_max -> varchar_min (varchar_max was never created) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Attribute deletion is asynchronous - add sleep to wait for deletion to complete before verifying the attribute no longer exists. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Skip FunctionsCustomServerTest::testCreateFunctionAndDeploymentFromTemplateCommit when GitHub API is rate-limited or unavailable - Increase waitForAllAttributes timeout to 3 minutes for testBulkDeleteMatchingUpdatedDocuments to handle slow attribute processing in CI Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove explicit sleep and increase assertEventually timeout from 60s to 180s to account for slower deployment and scheduling in CI. Also use assertGreaterThanOrEqual to handle cases where multiple executions may have run. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Increase all default timeouts in SchemaPolling trait from 2 minutes to 3 minutes to handle slower attribute processing in CI environments. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
# Conflicts: # tests/e2e/Services/Databases/DatabasesBase.php # tests/e2e/Services/Databases/TablesDB/DatabasesBase.php
Increase all default timeouts in SchemaPolling trait from 3 minutes to 5 minutes to handle slower attribute processing when running tests in parallel under heavy CI load. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Increase all default timeouts in SchemaPolling trait from 5 minutes to 10 minutes. With tests running in parallel via paratest, the attribute processing worker can become backlogged under heavy CI load, requiring longer wait times for attributes to transition from 'processing' to 'available' status. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The root cause of flaky attribute processing tests is that a single appwrite-worker-databases container cannot keep up with attribute creation jobs when tests run in parallel via paratest. This fix: - Scales the databases worker to 4 instances during E2E service tests - Reverts timeout increases that masked the underlying issue Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
abnegate
force-pushed
the
feat-db-tests
branch
from
b7b3f90 to
fba43b3
Compare
The --functional flag cannot be used because tests rely on static properties to share state between test methods (e.g., $databaseId set in testCreateDatabase is used by testCreateTable). With --functional, each test method runs in a separate process so these properties are not initialized. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
abnegate
force-pushed
the
feat-db-tests
branch
from
fba43b3 to
4631a2a
Compare
- Fix typo CLient -> Client in DatabasesBase.php:4568 - Fix typo CLient -> Client in StorageCustomClientTest.php:428 - Fix cleanupFunction() call with extra parameter in FunctionsScheduleTest.php:175 - Increase message scheduling test timeouts from 30s to 180s for CI stability Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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/Storage/StorageCustomClientTest.php (1)
428-437:⚠️ Potential issue | 🟡 MinorAssert the PUT failure response instead of discarding it.
The PUT call’s response is ignored, so the test doesn’t actually verify update-permission failure. Capture and assert it to avoid false positives.
Suggested fix
- $this->client->call(Client::METHOD_PUT, '/storage/buckets/' . $bucketId . '/files/' . $fileId, [ + $update = $this->client->call(Client::METHOD_PUT, '/storage/buckets/' . $bucketId . '/files/' . $fileId, [ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], ], [ 'name' => 'permissions.png', ]); - $this->assertEquals(401, $file['headers']['status-code']); - - $this->assertEquals($file['headers']['status-code'], 401); + $this->assertEquals(401, $file['headers']['status-code']); + $this->assertEquals(401, $update['headers']['status-code']);
🤖 Fix all issues with AI agents
In `@tests/e2e/Services/Messaging/MessagingBase.php`:
- Around line 1072-1082: The assertEventually call that waits for the message to
become FAILED uses a 30000ms timeout; update the timeout argument in the
assertEventually invocation inside MessagingBase:: (the closure using $messageId
and client->call) from 30000 to 180000 to match other scheduling tests (ensure
the final call remains assertEventually(..., 180000, 1000) so CI has the same
180s tolerance).
| $messageId = $message['body']['$id']; | ||
| $this->assertEventually(function () use ($messageId) { | ||
| $message = $this->client->call(Client::METHOD_GET, '/messaging/messages/' . $messageId, [ | ||
| 'content-type' => 'application/json', | ||
| 'x-appwrite-project' => $this->getProject()['$id'], | ||
| 'x-appwrite-key' => $this->getProject()['apiKey'], | ||
| ]); | ||
|
|
||
| $this->assertEquals(200, $message['headers']['status-code']); | ||
| $this->assertEquals(MessageStatus::FAILED, $message['body']['status']); | ||
| $this->assertEquals(200, $message['headers']['status-code']); | ||
| $this->assertEquals(MessageStatus::FAILED, $message['body']['status']); | ||
| }, 30000, 1000); |
There was a problem hiding this comment.
Inconsistent timeout: consider using 180000ms for CI stability.
The PR objective mentions increasing message scheduling test timeouts from 30s to 180s for CI stability. However, this test still uses a 30000ms (30s) timeout while similar scheduling tests at lines 1195 and 1267 use 180000ms (180s). This inconsistency may lead to flaky test failures in CI.
Proposed fix
$this->assertEventually(function () use ($messageId) {
$message = $this->client->call(Client::METHOD_GET, '/messaging/messages/' . $messageId, [
'content-type' => 'application/json',
'x-appwrite-project' => $this->getProject()['$id'],
'x-appwrite-key' => $this->getProject()['apiKey'],
]);
$this->assertEquals(200, $message['headers']['status-code']);
$this->assertEquals(MessageStatus::FAILED, $message['body']['status']);
- }, 30000, 1000);
+ }, 180000, 1000);📝 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.
| $messageId = $message['body']['$id']; | |
| $this->assertEventually(function () use ($messageId) { | |
| $message = $this->client->call(Client::METHOD_GET, '/messaging/messages/' . $messageId, [ | |
| 'content-type' => 'application/json', | |
| 'x-appwrite-project' => $this->getProject()['$id'], | |
| 'x-appwrite-key' => $this->getProject()['apiKey'], | |
| ]); | |
| $this->assertEquals(200, $message['headers']['status-code']); | |
| $this->assertEquals(MessageStatus::FAILED, $message['body']['status']); | |
| $this->assertEquals(200, $message['headers']['status-code']); | |
| $this->assertEquals(MessageStatus::FAILED, $message['body']['status']); | |
| }, 30000, 1000); | |
| $messageId = $message['body']['$id']; | |
| $this->assertEventually(function () use ($messageId) { | |
| $message = $this->client->call(Client::METHOD_GET, '/messaging/messages/' . $messageId, [ | |
| 'content-type' => 'application/json', | |
| 'x-appwrite-project' => $this->getProject()['$id'], | |
| 'x-appwrite-key' => $this->getProject()['apiKey'], | |
| ]); | |
| $this->assertEquals(200, $message['headers']['status-code']); | |
| $this->assertEquals(MessageStatus::FAILED, $message['body']['status']); | |
| }, 180000, 1000); |
🤖 Prompt for AI Agents
In `@tests/e2e/Services/Messaging/MessagingBase.php` around lines 1072 - 1082, The
assertEventually call that waits for the message to become FAILED uses a 30000ms
timeout; update the timeout argument in the assertEventually invocation inside
MessagingBase:: (the closure using $messageId and client->call) from 30000 to
180000 to match other scheduling tests (ensure the final call remains
assertEventually(..., 180000, 1000) so CI has the same 180s tolerance).
Parallel test execution with paratest overwhelms the single databases worker when many tests create attributes simultaneously. Run these specific services sequentially with PHPUnit to ensure the worker can keep up with attribute processing jobs. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add helper methods createCollectionWithAttribute(), createTableWithAttribute(), createCollectionWithIndex(), createTableWithIndex() to RealtimeConsoleClientTest - Add createTeam() helper to RealtimeCustomClientTest - Replace all @Depends annotations with direct helper method calls - Enable tests to run in parallel with --functional flag Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
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/Realtime/RealtimeConsoleClientTest.php (1)
259-369:⚠️ Potential issue | 🔴 CriticalFix return type mismatch in
testAttributesCollectionsAPI.The method declares
: arraybut returns nothing; PHP will throw aTypeErrorwhen the test completes. Change to: void.Suggested fix
- public function testAttributesCollectionsAPI(): array + public function testAttributesCollectionsAPI(): void
🤖 Fix all issues with AI agents
In `@tests/e2e/Services/Realtime/RealtimeConsoleClientTest.php`:
- Around line 53-65: Replace the fixed sleep(2) calls with a polling loop that
queries the collection attributes until the new attribute appears or a timeout
is reached; specifically, after the POST that creates the attribute (around the
call using $this->client->call and the 'attributes/string' path in
RealtimeConsoleClientTest), repeatedly call the collection attributes GET
endpoint via $this->client->call and check for the 'name' attribute presence
with short sleeps between attempts, failing the test if the timeout elapses;
apply the same replacement for the other occurrences noted (near lines 104-105,
126-128, 148-150) so tests wait deterministically for schema readiness instead
of using hard-coded sleep.
- Around line 94-102: The test is calling the wrong endpoint for TablesDB
attribute creation: change the POST path used in RealtimeConsoleClientTest from
'/attributes/string' to the TablesDB endpoint '/columns/string' (keeping the
same concatenation with $databaseId and $actorsId and the same headers and
payload), so the call using Client::METHOD_POST targets '/tablesdb/' .
$databaseId . '/tables/' . $actorsId . '/columns/string' instead of
'/attributes/string'.
🧹 Nitpick comments (1)
tests/e2e/Services/Realtime/RealtimeConsoleClientTest.php (1)
27-52: Add status-code assertions in setup helpers.Without asserts, failed setup calls can cascade into confusing errors later. Consider asserting 201/202 after each create call.
♻️ Example pattern
$database = $this->client->call(Client::METHOD_POST, '/databases', array_merge([ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], ], $this->getHeaders()), [ 'databaseId' => ID::unique(), 'name' => 'Actors DB', ]); + $this->assertEquals(201, $database['headers']['status-code']);Also applies to: 74-92, 117-124, 139-146
| // Create attribute and wait for it to be available | ||
| $this->client->call(Client::METHOD_POST, '/databases/' . $databaseId . '/collections/' . $actorsId . '/attributes/string', array_merge([ | ||
| 'content-type' => 'application/json', | ||
| 'x-appwrite-project' => $this->getProject()['$id'], | ||
| ], $this->getHeaders()), [ | ||
| 'key' => 'name', | ||
| 'size' => 256, | ||
| 'required' => true, | ||
| ]); | ||
|
|
||
| // Wait for attribute to be available | ||
| sleep(2); | ||
|
|
There was a problem hiding this comment.
Replace fixed sleeps with polling for schema readiness.
Hard-coded sleep(2) can be flaky under parallel load. Prefer polling with a timeout (or an existing schema polling helper) to avoid intermittent failures.
Also applies to: 104-105, 126-128, 148-150
🤖 Prompt for AI Agents
In `@tests/e2e/Services/Realtime/RealtimeConsoleClientTest.php` around lines 53 -
65, Replace the fixed sleep(2) calls with a polling loop that queries the
collection attributes until the new attribute appears or a timeout is reached;
specifically, after the POST that creates the attribute (around the call using
$this->client->call and the 'attributes/string' path in
RealtimeConsoleClientTest), repeatedly call the collection attributes GET
endpoint via $this->client->call and check for the 'name' attribute presence
with short sleeps between attempts, failing the test if the timeout elapses;
apply the same replacement for the other occurrences noted (near lines 104-105,
126-128, 148-150) so tests wait deterministically for schema readiness instead
of using hard-coded sleep.
This reverts commit 8dadcfe.
- Use /databases endpoint instead of /tablesdb for database creation - Use /columns/string instead of /attributes/string for table columns - Fix testAttributesCollectionsAPI return type from array to void Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Enables --functional mode for parallel test execution by: - Adding helper methods for independent test setup - Removing all #[Depends] annotations - Using static caching for shared resources Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Enables parallel test execution for Transactions tests. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/e2e/Services/Webhooks/WebhooksCustomClientTest.php (1)
487-640:⚠️ Potential issue | 🟠 MajorFix stale name expectations after removing test dependencies.
Line 529, Line 580, and Line 631 still expect"New Name", but the new setup only creates"User Name". These assertions will fail once tests run independently. Prefer asserting the name from the setup data (or update the name within each test).💡 Suggested fix
- $this->assertEquals($webhook['data']['name'], 'New Name'); + $this->assertEquals($webhook['data']['name'], $data['name']);- $this->assertEquals($webhook['data']['name'], 'New Name'); + $this->assertEquals($webhook['data']['name'], $data['name']);- $this->assertEquals($webhook['data']['name'], 'New Name'); + $this->assertEquals($webhook['data']['name'], $data['name']);tests/e2e/Services/Webhooks/WebhooksBase.php (3)
422-544:⚠️ Potential issue | 🟠 MajorFix undefined
$datausage in attribute deletion.
Line 519 references$data['actorsId'], but$datais not defined in this method. This will raise a warning and can cause the delete call to target a null path. Use$actorsIdinstead.✅ Suggested fix
- $removed = $this->client->call(Client::METHOD_DELETE, '/databases/' . $databaseId . '/collections/' . $data['actorsId'] . '/attributes/' . $extra['body']['key'], array_merge([ + $removed = $this->client->call(Client::METHOD_DELETE, '/databases/' . $databaseId . '/collections/' . $actorsId . '/attributes/' . $extra['body']['key'], array_merge([
791-911:⚠️ Potential issue | 🟠 MajorFix undefined
$datausage in column deletion.
Line 886 references$data['actorsId'], but$datais not defined in this method. Use$actorsIdto avoid runtime warnings and an invalid URL.✅ Suggested fix
- $removed = $this->client->call(Client::METHOD_DELETE, '/tablesdb/' . $databaseId . '/tables/' . $data['actorsId'] . '/columns/' . $extra['body']['key'], array_merge([ + $removed = $this->client->call(Client::METHOD_DELETE, '/tablesdb/' . $databaseId . '/tables/' . $actorsId . '/columns/' . $extra['body']['key'], array_merge([
1343-1378:⚠️ Potential issue | 🟠 MajorUpdate expected bucket name for delete test.
Line 1375 expects"Test Bucket Updated", but this test now deletes a freshly created bucket fromsetupStorageBucket()(name"Test Bucket"). The assertion will fail without the prior update test.✅ Suggested fix
- $this->assertEquals('Test Bucket Updated', $webhook['data']['name']); + $this->assertEquals('Test Bucket', $webhook['data']['name']);
🤖 Fix all issues with AI agents
In `@tests/e2e/Services/Realtime/RealtimeConsoleClientTest.php`:
- Around line 139-146: The test uses the wrong payload key when creating a
TablesDB index: in the Client::call invocation that posts to '/tablesdb/' .
$data['databaseId'] . '/tables/' . $data['actorsId'] . '/indexes' replace the
'attributes' field with 'columns' (i.e., send 'columns' => ['name']) so the
request matches the TablesDB API and mirrors testIndexesTablesAPI(); ensure the
rest of the parameters (key, type) remain unchanged.
In `@tests/e2e/Services/Teams/TeamsBaseClient.php`:
- Around line 73-113: The method createPendingMembershipHelper has an unused
$teamName parameter (PHPMD flagged); remove $teamName from its PHPDoc and
signature and update every caller to stop passing a second argument so callers
only pass the team UID, then run tests/static analysis to ensure no remaining
call-site mismatches.
In `@tests/e2e/Services/Teams/TeamsBaseServer.php`:
- Around line 11-38: The helper method createServerMembershipHelper has an
unused parameter $teamName; remove it from the method signature and update all
call sites that pass that extra argument (the three tests calling
createServerMembershipHelper) so they only pass the team UID. Ensure the
function signature becomes createServerMembershipHelper(string $teamUid): array
and adjust callers to match (update any calls at the three locations referenced
in the review to drop the second argument) while keeping the method body and
returned array unchanged.
In `@tests/e2e/Services/Webhooks/WebhooksCustomClientTest.php`:
- Around line 540-545: Remove the unused local variables introduced by the
refactor: delete the unused $email variables and the unused $session variable in
the WebhooksCustomClientTest setup blocks (they are the locals assigned from
$data in the sequence with $data = $this->setupAccountWithSession(); $id =
$data['id']; $email = $data['email']; $password = $data['password']; $newEmail =
...; $session = $data['session'];). Keep assignments that are actually used
(e.g., $id, $password, $newEmail) and remove only the unused $email and $session
binds to eliminate PHPMD warnings and dead locals.
🧹 Nitpick comments (5)
tests/e2e/Services/Realtime/RealtimeConsoleClientTest.php (2)
25-67: Add response status assertions to prevent silent failures.The helper extracts IDs from response bodies without verifying the API calls succeeded. If a call fails, the test will fail later with a confusing error when trying to use an invalid ID.
🛡️ Proposed fix to add assertions
$database = $this->client->call(Client::METHOD_POST, '/databases', array_merge([ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], ], $this->getHeaders()), [ 'databaseId' => ID::unique(), 'name' => 'Actors DB', ]); + $this->assertEquals(201, $database['headers']['status-code']); $databaseId = $database['body']['$id']; $actors = $this->client->call(Client::METHOD_POST, '/databases/' . $databaseId . '/collections', array_merge([ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], ], $this->getHeaders()), [ 'collectionId' => ID::unique(), 'name' => 'Actors', 'permissions' => [ Permission::read(Role::any()), Permission::create(Role::any()), Permission::update(Role::any()), Permission::delete(Role::any()), ], ]); + $this->assertEquals(201, $actors['headers']['status-code']); $actorsId = $actors['body']['$id']; // Create attribute and wait for it to be available - $this->client->call(Client::METHOD_POST, '/databases/' . $databaseId . '/collections/' . $actorsId . '/attributes/string', array_merge([ + $attribute = $this->client->call(Client::METHOD_POST, '/databases/' . $databaseId . '/collections/' . $actorsId . '/attributes/string', array_merge([ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], ], $this->getHeaders()), [ 'key' => 'name', 'size' => 256, 'required' => true, ]); + $this->assertEquals(202, $attribute['headers']['status-code']);
72-108: Add response status assertions (same as collections helper).Same recommendation applies here—add assertions after each API call to fail fast with clear errors.
tests/e2e/Services/Databases/Transactions/TransactionPermissionsBase.php (3)
104-128: Transaction created but never cleaned up.The transaction created at lines 105-110 is left orphaned after the test completes. Even though the staging operation fails with 401, the transaction itself was successfully created and should be deleted or rolled back to maintain test isolation.
Consider adding cleanup in a
finallyblock or at the end of the test:Suggested cleanup pattern
// This should fail with 401 Unauthorized $this->assertEquals(401, $staged['headers']['status-code']); + + // Cleanup: delete the transaction + $this->client->call(Client::METHOD_DELETE, $this->getTransactionUrl($transaction['body']['$id']), array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders())); }
329-353: Successful staging tests leave transactions uncommitted.Both
testDocumentLevelUpdatePermissionGrantedandtestDocumentLevelDeletePermissionGrantedsuccessfully stage operations but never commit or rollback the transactions. While the tests verify the correct permission behavior, the orphaned transactions may affect subsequent tests or accumulate resources.Also applies to: 409-433
1145-1173: Consider cleaning up transactions created in the list test.The test creates 3 transactions in a loop (lines 1147-1156) to verify listing functionality, but none are cleaned up afterward. These orphaned transactions may affect other tests that rely on transaction counts.
Suggested cleanup
// Verify transaction structure foreach ($list['body']['transactions'] as $transaction) { $this->assertArrayHasKey('$id', $transaction); $this->assertArrayHasKey('$createdAt', $transaction); $this->assertArrayHasKey('$updatedAt', $transaction); $this->assertArrayHasKey('status', $transaction); $this->assertArrayHasKey('operations', $transaction); } + + // Cleanup created transactions + foreach ($transactionIds as $transactionId) { + $this->client->call(Client::METHOD_DELETE, $this->getTransactionUrl($transactionId), array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $userHeaders)); + } }
| $this->client->call(Client::METHOD_POST, '/tablesdb/' . $data['databaseId'] . '/tables/' . $data['actorsId'] . '/indexes', array_merge([ | ||
| 'content-type' => 'application/json', | ||
| 'x-appwrite-project' => $this->getProject()['$id'], | ||
| ], $this->getHeaders()), [ | ||
| 'key' => 'key_name', | ||
| 'type' => 'key', | ||
| 'attributes' => ['name'], | ||
| ]); |
There was a problem hiding this comment.
Use 'columns' instead of 'attributes' for TablesDB index creation.
The helper uses 'attributes' but the TablesDB API expects 'columns'. Compare with testIndexesTablesAPI() at line 594 which correctly uses 'columns' => ['name']. This inconsistency will likely cause the index creation to fail or behave unexpectedly.
🐛 Proposed fix
$this->client->call(Client::METHOD_POST, '/tablesdb/' . $data['databaseId'] . '/tables/' . $data['actorsId'] . '/indexes', array_merge([
'content-type' => 'application/json',
'x-appwrite-project' => $this->getProject()['$id'],
], $this->getHeaders()), [
'key' => 'key_name',
'type' => 'key',
- 'attributes' => ['name'],
+ 'columns' => ['name'],
]);Based on learnings: "In table-related endpoints...parameter descriptions should use 'table' and 'row' terminology instead of 'collection' and 'document' for clarity and consistency."
📝 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.
| $this->client->call(Client::METHOD_POST, '/tablesdb/' . $data['databaseId'] . '/tables/' . $data['actorsId'] . '/indexes', array_merge([ | |
| 'content-type' => 'application/json', | |
| 'x-appwrite-project' => $this->getProject()['$id'], | |
| ], $this->getHeaders()), [ | |
| 'key' => 'key_name', | |
| 'type' => 'key', | |
| 'attributes' => ['name'], | |
| ]); | |
| $this->client->call(Client::METHOD_POST, '/tablesdb/' . $data['databaseId'] . '/tables/' . $data['actorsId'] . '/indexes', array_merge([ | |
| 'content-type' => 'application/json', | |
| 'x-appwrite-project' => $this->getProject()['$id'], | |
| ], $this->getHeaders()), [ | |
| 'key' => 'key_name', | |
| 'type' => 'key', | |
| 'columns' => ['name'], | |
| ]); |
🤖 Prompt for AI Agents
In `@tests/e2e/Services/Realtime/RealtimeConsoleClientTest.php` around lines 139 -
146, The test uses the wrong payload key when creating a TablesDB index: in the
Client::call invocation that posts to '/tablesdb/' . $data['databaseId'] .
'/tables/' . $data['actorsId'] . '/indexes' replace the 'attributes' field with
'columns' (i.e., send 'columns' => ['name']) so the request matches the TablesDB
API and mirrors testIndexesTablesAPI(); ensure the rest of the parameters (key,
type) remain unchanged.
| /** | ||
| * Helper method to create a pending membership (not accepted). | ||
| * Returns membership data for tests that need unaccepted memberships. | ||
| * | ||
| * @param string $teamUid Team ID | ||
| * @param string $teamName Team name | ||
| * @return array{teamUid: string, teamName: string, secret: string, membershipUid: string, userUid: string, email: string, name: string} | ||
| */ | ||
| protected function createPendingMembershipHelper(string $teamUid, string $teamName): array | ||
| { | ||
| $email = uniqid() . 'friend@localhost.test'; | ||
| $name = 'Friend User'; | ||
|
|
||
| // Create membership invitation | ||
| $response = $this->client->call(Client::METHOD_POST, '/teams/' . $teamUid . '/memberships', array_merge([ | ||
| 'content-type' => 'application/json', | ||
| 'x-appwrite-project' => $this->getProject()['$id'], | ||
| ], $this->getHeaders()), [ | ||
| 'email' => $email, | ||
| 'name' => $name, | ||
| 'roles' => ['developer'], | ||
| 'url' => 'http://localhost:5000/join-us#title' | ||
| ]); | ||
|
|
||
| $this->assertEquals(201, $response['headers']['status-code']); | ||
|
|
||
| // Get invitation details from email | ||
| $lastEmail = $this->getLastEmailByAddress($email); | ||
| $this->assertNotEmpty($lastEmail, 'Email not found for address: ' . $email); | ||
|
|
||
| $tokens = $this->extractQueryParamsFromEmailLink($lastEmail['html']); | ||
|
|
||
| return [ | ||
| 'teamUid' => $tokens['teamId'], | ||
| 'teamName' => $tokens['teamName'], | ||
| 'secret' => $tokens['secret'], | ||
| 'membershipUid' => $tokens['membershipId'], | ||
| 'userUid' => $tokens['userId'], | ||
| 'email' => $email, | ||
| 'name' => $name, | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "createPendingMembershipHelper" --type=php -A 2Repository: appwrite/appwrite
Length of output: 2227
🏁 Script executed:
# Check line numbers around 438-440 and context
sed -n '435,450p' tests/e2e/Services/Teams/TeamsBaseClient.php | cat -n
# Also check TeamsConsoleClientTest.php context around line 169
sed -n '165,175p' tests/e2e/Services/Teams/TeamsConsoleClientTest.php | cat -nRepository: appwrite/appwrite
Length of output: 1431
🏁 Script executed:
# Check TeamsConsoleClientTest class definition
head -30 tests/e2e/Services/Teams/TeamsConsoleClientTest.php
# Also double-check for any other files that might use this method
rg "createPendingMembershipHelper" --type=php -lRepository: appwrite/appwrite
Length of output: 1056
Remove unused $teamName parameter from createPendingMembershipHelper method.
The parameter is not used in the method body and PHPMD flags it. Update all call sites:
- Line 242
- Lines 443–444 (note: not 438–440)
- Line 475
- Lines 792–793
- Line 169 in TeamsConsoleClientTest.php
🧹 Proposed fix
-protected function createPendingMembershipHelper(string $teamUid, string $teamName): array
+protected function createPendingMembershipHelper(string $teamUid): array🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 81-81: Avoid unused parameters such as '$teamName'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In `@tests/e2e/Services/Teams/TeamsBaseClient.php` around lines 73 - 113, The
method createPendingMembershipHelper has an unused $teamName parameter (PHPMD
flagged); remove $teamName from its PHPDoc and signature and update every caller
to stop passing a second argument so callers only pass the team UID, then run
tests/static analysis to ensure no remaining call-site mismatches.
| * Helper method to create a server-side team membership (auto-confirmed). | ||
| * Returns membership data for tests. | ||
| * | ||
| * @param string $teamUid Team ID | ||
| * @param string $teamName Team name | ||
| * @return array{teamUid: string, userUid: string, membershipUid: string} | ||
| */ | ||
| public function testGetTeamMemberships($data): array | ||
| protected function createServerMembershipHelper(string $teamUid, string $teamName): array | ||
| { | ||
| $id = $data['teamUid'] ?? ''; | ||
| $email = uniqid() . 'friend@localhost.test'; | ||
|
|
||
| $response = $this->client->call(Client::METHOD_POST, '/teams/' . $teamUid . '/memberships', array_merge([ | ||
| 'content-type' => 'application/json', | ||
| 'x-appwrite-project' => $this->getProject()['$id'], | ||
| ], $this->getHeaders()), [ | ||
| 'email' => $email, | ||
| 'name' => 'Friend User', | ||
| 'roles' => ['admin', 'editor'], | ||
| 'url' => 'http://localhost:5000/join-us#title' | ||
| ]); | ||
|
|
||
| $this->assertEquals(201, $response['headers']['status-code']); | ||
|
|
||
| return [ | ||
| 'teamUid' => $teamUid, | ||
| 'userUid' => $response['body']['userId'], | ||
| 'membershipUid' => $response['body']['$id'], | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all usages of createServerMembershipHelper in PHP files
rg -n "createServerMembershipHelper" --type=php -A2 -B2Repository: appwrite/appwrite
Length of output: 2067
🏁 Script executed:
#!/bin/bash
# Read the full method body to verify $teamName is not used
sed -n '18,37p' tests/e2e/Services/Teams/TeamsBaseServer.phpRepository: appwrite/appwrite
Length of output: 896
Remove unused $teamName parameter from helper method signature.
The parameter is never used within the method body. Update the method signature and all three call sites.
Proposed changes
-protected function createServerMembershipHelper(string $teamUid, string $teamName): array
+protected function createServerMembershipHelper(string $teamUid): arrayUpdate call sites at lines 65, 254, and 300:
-$membershipData = $this->createServerMembershipHelper($teamData['teamUid'], $teamData['teamName']);
+$membershipData = $this->createServerMembershipHelper($teamData['teamUid']);🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 18-18: Avoid unused parameters such as '$teamName'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In `@tests/e2e/Services/Teams/TeamsBaseServer.php` around lines 11 - 38, The
helper method createServerMembershipHelper has an unused parameter $teamName;
remove it from the method signature and update all call sites that pass that
extra argument (the three tests calling createServerMembershipHelper) so they
only pass the team UID. Ensure the function signature becomes
createServerMembershipHelper(string $teamUid): array and adjust callers to match
(update any calls at the three locations referenced in the review to drop the
second argument) while keeping the method body and returned array unchanged.
| $data = $this->setupAccountWithSession(); | ||
| $id = $data['id']; | ||
| $email = $data['email']; | ||
| $password = $data['password']; | ||
| $newEmail = uniqid() . 'new@localhost.test'; | ||
| $session = $data['session'] ?? ''; | ||
| $session = $data['session']; |
There was a problem hiding this comment.
Remove unused locals introduced by the refactor.
PHPMD flagged unused variables at Line 542 ($email), Line 647 ($email), Line 698 ($email), and Line 815 ($session). These can be safely removed to reduce noise.
🔧 Suggested cleanup
- $email = $data['email'];- $email = $data['email'];- $email = $data['email'];- $session = $data['session'];Also applies to: 647-648, 698-699, 815-815
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 542-542: Avoid unused local variables such as '$email'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In `@tests/e2e/Services/Webhooks/WebhooksCustomClientTest.php` around lines 540 -
545, Remove the unused local variables introduced by the refactor: delete the
unused $email variables and the unused $session variable in the
WebhooksCustomClientTest setup blocks (they are the locals assigned from $data
in the sequence with $data = $this->setupAccountWithSession(); $id =
$data['id']; $email = $data['email']; $password = $data['password']; $newEmail =
...; $session = $data['session'];). Keep assignments that are actually used
(e.g., $id, $password, $newEmail) and remove only the unused $email and $session
binds to eliminate PHPMD warnings and dead locals.
1 participant
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist