test: add regression tests for null name in Users.CreateUser endpoints#11618
test: add regression tests for null name in Users.CreateUser endpoints#11618abishekgiri wants to merge 2 commits intoappwrite:1.9.xfrom
Conversation
📝 WalkthroughWalkthroughThe pull request adds comprehensive test coverage for user creation with null name values. A new static data provider supplies test variants for eight password hashing endpoints ( Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi! This is my first contribution. Happy to make any changes if needed. Thanks! |
Greptile SummaryThis PR adds a Key observations:
Confidence Score: 4/5
Important Files Changed
Reviews (1): Last reviewed commit: "Merge branch '1.9.x' into test-8785-null..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #[DataProvider('createUserNullNameProvider')] | ||
| public function testCreateUserWithNullName(string $path, array $payload): void | ||
| { | ||
| $userId = 'null-name-' . ID::unique(); | ||
| $email = $userId . '@example.test'; | ||
|
|
||
| $user = $this->client->call(Client::METHOD_POST, $path, array_merge([ | ||
| 'content-type' => 'application/json', | ||
| 'x-appwrite-project' => $this->getProject()['$id'], | ||
| ], $this->getHeaders()), array_merge([ | ||
| 'userId' => $userId, | ||
| 'email' => $email, | ||
| 'name' => null, | ||
| ], $payload)); | ||
|
|
||
| $this->assertEquals(Response::STATUS_CODE_CREATED, $user['headers']['status-code']); | ||
| $this->assertEquals($userId, $user['body']['$id']); | ||
| $this->assertEquals($email, $user['body']['email']); | ||
| $this->assertSame('', $user['body']['name']); | ||
| } |
There was a problem hiding this comment.
The test creates up to 8 real users (one per provider entry) but never deletes them. While ID::unique() prevents collisions on repeated runs, the test database will accumulate stale null-name-* users over time.
Consider either adding a teardown step inside the test or deleting each user after asserting the result:
// After your existing assertions:
$this->client->call(Client::METHOD_DELETE, '/users/' . $userId, array_merge([
'content-type' => 'application/json',
'x-appwrite-project' => $this->getProject()['$id'],
], $this->getHeaders()));This keeps the test database clean without affecting the regression coverage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/e2e/Services/Users/UsersBase.php (2)
313-370: Reduce fixture duplication to avoid drift between test helpers.The hashed payloads here duplicate values already defined in
setupHashedPasswordUsers()(Line 97-181). Consider centralizing these fixtures in one shared map/helper and reusing it in both places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Services/Users/UsersBase.php` around lines 313 - 370, The tests duplicate hashed-password fixtures between the return dataset and setupHashedPasswordUsers(); extract the common map into a shared constant or helper (e.g., HASHED_PASSWORD_FIXTURES) and have both the data provider (the returned array with keys 'plaintext','md5','bcrypt','argon2','sha','phpass','scrypt','scrypt-modified') and the setupHashedPasswordUsers() function reference that single source; update UsersBase.php to remove the inline duplicate arrays and import/consume the shared map so all fixtures are maintained in one place.
311-312: Add PHPDoc for the new data provider and test method.Both newly added public methods are missing PHPDoc comments required by repository standards.
✍️ Suggested update
+ /** + * Provides create-user endpoint payload variants for null-name regression coverage. + * + * `@return` array<string, array{0:string, 1:array<string, mixed>}> + */ public static function createUserNullNameProvider(): array { return [ // ... ]; } + /** + * Ensures create-user endpoints accept `name: null` and normalize it to an empty string. + */ #[DataProvider('createUserNullNameProvider')] public function testCreateUserWithNullName(string $path, array $payload): void { // ... }As per coding guidelines, "Include comprehensive PHPDoc comments".
Also applies to: 374-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Services/Users/UsersBase.php` around lines 311 - 312, Add comprehensive PHPDoc comments for the new public data provider createUserNullNameProvider and the corresponding public test method: include a one-line description, the `@return` array annotation for the provider, and for the test method include a description plus `@dataProvider` annotation reference and `@param` type annotations as required by repository standards so both methods have full PHPDoc blocks matching existing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/Services/Users/UsersBase.php`:
- Around line 313-370: The tests duplicate hashed-password fixtures between the
return dataset and setupHashedPasswordUsers(); extract the common map into a
shared constant or helper (e.g., HASHED_PASSWORD_FIXTURES) and have both the
data provider (the returned array with keys
'plaintext','md5','bcrypt','argon2','sha','phpass','scrypt','scrypt-modified')
and the setupHashedPasswordUsers() function reference that single source; update
UsersBase.php to remove the inline duplicate arrays and import/consume the
shared map so all fixtures are maintained in one place.
- Around line 311-312: Add comprehensive PHPDoc comments for the new public data
provider createUserNullNameProvider and the corresponding public test method:
include a one-line description, the `@return` array annotation for the provider,
and for the test method include a description plus `@dataProvider` annotation
reference and `@param` type annotations as required by repository standards so
both methods have full PHPDoc blocks matching existing tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17604927-7fb7-4eb2-b45a-b60b4cc911a9
📒 Files selected for processing (1)
tests/e2e/Services/Users/UsersBase.php
1 participant
Fixes #8785
Summary
The issue describes that
Users.CreateUserendpoints return a vague server error whennameis null.The core fix already exists in
app/controllers/api/users.php, where null names are normalized to an empty string.This PR adds regression test coverage to ensure this behavior is preserved.
Changes
name: nullTesting
docker compose exec appwrite test /usr/src/code/tests/e2e/Services/Users --filter=testCreateUserWithNullName
All tests passed: