Upgrade to v3.0, update namespaces/types, adjust workflow PHP versions, and reset DI container before tests#173
Merged
armanist merged 10 commits intosoftberg:masterfrom Mar 16, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMass namespace moves from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable poems in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
shared/Commands/DemoCommand.php (1)
355-358:⚠️ Potential issue | 🟠 MajorThe
imageUrlmethod parameters do not match the library's signature.The actual signature for
bluemmb/faker-picsum-photos-provider'simageUrlmethod is:imageUrl($width = 640, $height = 480, $specificOrSeed = null, $grayscale = null, $blur = null, $file_ending = null)The current call
imageUrl(640, 480, true, 0)incorrectly passes:
trueto the$specificOrSeedparameter (expects null)0to the$grayscaleparameter (expects null)Either remove these trailing arguments or pass the correct parameters if specific seed/grayscale values are intended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/Commands/DemoCommand.php` around lines 355 - 358, The call to $this->faker->imageUrl in DemoCommand (used to generate the $imageName for save_remote_image) passes incorrect trailing arguments that don't match bluemmb/faker-picsum-photos-provider's signature; update the invocation in DemoCommand.php so imageUrl(...) uses only the intended width and height (e.g., $this->faker->imageUrl(640, 480)) or, if you need a specific seed/grayscale/blur/file ending, pass them in the correct order according to imageUrl($width, $height, $specificOrSeed = null, $grayscale = null, $blur = null, $file_ending = null) before calling save_remote_image($url, $user['uuid'], $title).shared/Commands/PostCreateCommand.php (1)
50-62:⚠️ Potential issue | 🟡 MinorFix CLI help text to match actual arguments.
Line 50 still documents
[Image] [Author], but this command now requiresuser_uuidand also supports optionaluuid.Proposed fix
- protected ?string $help = 'Use the following format to create a post record:' . PHP_EOL . 'php qt post:create `Title` `Description` `[Image]` `[Author]`'; + protected ?string $help = 'Use the following format to create a post record:' . PHP_EOL . 'php qt post:create `Title` `Description` `Author UUID` `[Post UUID]` `[Image]`';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/Commands/PostCreateCommand.php` around lines 50 - 62, Update the help string in the PostCreateCommand class to reflect the actual argument list (title, description, user_uuid) and the optional uuid and image parameters; specifically change the protected ?string $help value so it shows: php qt post:create `Title` `Description` `UserUUID` `[UUID]` `[Image]` (or similar ordering that matches the protected array $args), ensuring the displayed argument names match the keys 'title', 'description', 'user_uuid', 'uuid', and 'image'.shared/Commands/UserCreateCommand.php (1)
94-102:⚠️ Potential issue | 🟠 MajorDo not persist
imageuntil the users table supports it.Passing
imageintoAuthService::add()can break user creation if the users schema still has noimagecolumn (pershared/Models/User.php+ migration context).Proposed fix
service(AuthService::class)->add([ 'uuid' => $this->getArgument('uuid'), 'firstname' => $this->getArgument('firstname'), 'lastname' => $this->getArgument('lastname'), 'role' => $this->getArgument('role'), 'email' => $this->getArgument('email'), 'password' => (new Hasher())->hash($this->getArgument('password')), - 'image' => $this->getArgument('image'), ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/Commands/UserCreateCommand.php` around lines 94 - 102, The call to service(AuthService::class)->add(...) in UserCreateCommand.php is sending an 'image' field which will fail if the users table/model lacks that column; remove 'image' from the payload passed to AuthService::add() (or only include it conditionally after checking the User model/migration supports it) so the array contains uuid, firstname, lastname, role, email, and hashed password only; update references to AuthService::add and any related validation to account for the removed/conditional image field.tests/Unit/shared/Services/PostServiceTest.php (1)
118-129:⚠️ Potential issue | 🟠 MajorMake
testPostServiceGetMyPostsdeterministic (currently failing CI).Line 128 is failing in CI because the test assumes seeded state (
10posts). Create/arrange data in-test and assert against that known fixture instead of a hard-coded global count.Proposed fix
public function testPostServiceGetMyPosts() { $users = $this->authService->getAll(); $userUuid = $users->first()->uuid; - $myPosts = $this->postService->getMyPosts($userUuid); - - $this->assertInstanceOf(ModelCollection::class, $myPosts); - - $this->assertCount(10, $myPosts); + $newPost = $this->postService->addPost([ + 'user_uuid' => $userUuid, + 'title' => 'Deterministic post title', + 'content' => 'Deterministic post content', + 'image' => '', + ]); + + $myPosts = $this->postService->getMyPosts($userUuid); + $this->assertInstanceOf(ModelCollection::class, $myPosts); + + $uuids = array_map(static fn($post) => $post->uuid, $myPosts->all()); + $this->assertContains($newPost->uuid, $uuids); + + $this->postService->deletePost($newPost->uuid); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/shared/Services/PostServiceTest.php` around lines 118 - 129, The test testPostServiceGetMyPosts relies on global seeded data (causing CI flakiness); update it to arrange its own fixtures by creating a user and known posts within the test, then call postService->getMyPosts with that user's uuid and assert the returned ModelCollection contains exactly the number of posts you created. Specifically, replace the authService->getAll() usage with creating a new user (or obtaining a freshly created user) and use factories to create N posts tied to that user's uuid, then assertInstanceOf(ModelCollection::class, $myPosts) and assertCount(N, $myPosts) to make the test deterministic.tests/Unit/shared/Services/AuthServiceTest.php (1)
24-43:⚠️ Potential issue | 🟠 MajorMake these tests deterministic (current assertions are order-dependent and failing in CI).
Line 32 and Line 43 assume
getAll()->first()is always the default user, but pipeline output shows a different first record. Resolve the default user explicitly before asserting.💡 Suggested fix
public function testAuthServiceGetAll() { $users = $this->authService->getAll(); $this->assertInstanceOf(ModelCollection::class, $users); $this->assertInstanceOf(User::class, $users->first()); - $this->assertEquals('default@quantumphp.io', $users->first()->email); + $defaultUser = $this->authService->get('email', 'default@quantumphp.io'); + $this->assertNotNull($defaultUser); } public function testAuthServiceGetUserByUuid() { - $users = $this->authService->getAll(); - $user = $this->authService->getUserByUuid($users->first()->uuid); + $defaultUser = $this->authService->get('email', 'default@quantumphp.io'); + $this->assertNotNull($defaultUser); + $user = $this->authService->getUserByUuid($defaultUser->getFieldValue('uuid')); $this->assertInstanceOf(User::class, $user); $this->assertEquals('default@quantumphp.io', $user->email); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/shared/Services/AuthServiceTest.php` around lines 24 - 43, The tests assume getAll()->first() is the default user which is non-deterministic; update both testAuthServiceGetAll and testAuthServiceGetUserByUuid to locate the default user explicitly by filtering the collection for email 'default@quantumphp.io' (e.g., $users->filter(fn($u) => $u->email === 'default@quantumphp.io')->first()) and then assert on that user (use that user's uuid when calling getUserByUuid); ensure you still assert the collection type (ModelCollection) and that the found item is an instance of User before checking the email.
🧹 Nitpick comments (4)
shared/Commands/DemoCommand.php (2)
376-384: Consider adding a defensive check for empty candidates array.If
$candidatesbecomes empty after filtering (e.g., if there's only one user),array_rand()will returnnull, causing an undefined offset error on line 383. While the currentCOUNTS['users'] = 3makes this unlikely, adding a guard would improve robustness.🛡️ Proposed defensive check
private function getRandomUserExcept(array $users, string $excludeUuid): array { $candidates = array_filter($users, function($u) use ($excludeUuid) { return $u['uuid'] !== $excludeUuid; }); $candidates = array_values($candidates); + if (empty($candidates)) { + throw new \InvalidArgumentException('No candidate users available for comment generation'); + } return $candidates[array_rand($candidates)]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/Commands/DemoCommand.php` around lines 376 - 384, The getRandomUserExcept function can call array_rand on an empty $candidates array; add a defensive check after building $candidates in getRandomUserExcept to handle the empty case (e.g., if (empty($candidates)) throw new InvalidArgumentException("No available users after excluding {$excludeUuid}");), otherwise proceed to array_rand as before; this prevents undefined offset when array_rand would fail.
458-472: Add missing return type declarations for consistency.Both
runCommandInternallyandrunCommandExternallymethods lack return type declarations, while other similar methods in the class (e.g.,createModule,generateComments,updateProgress) explicitly declare: void.♻️ Proposed fix to add void return types
- private function runCommandInternally(string $commandName, array $arguments = []) + private function runCommandInternally(string $commandName, array $arguments = []): void { $command = $this->getApplication()->find($commandName); $command->run(new ArrayInput($arguments), new NullOutput); } - private function runCommandExternally(string $commandName, array $arguments = []) + private function runCommandExternally(string $commandName, array $arguments = []): void { $command = $this->buildCommand($commandName, $arguments); $this->runExternalProcess($command); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/Commands/DemoCommand.php` around lines 458 - 472, Add explicit void return types to the two methods to match the class conventions: change the signatures of runCommandInternally(string $commandName, array $arguments = []) and runCommandExternally(string $commandName, array $arguments = []) to include ": void". Update both method declarations (and adjust any docblocks if present) so they read runCommandInternally(...): void and runCommandExternally(...): void to maintain consistency with methods like createModule, generateComments, and updateProgress.shared/Commands/CommandValidationTrait.php (1)
29-49: Consider lazy initialization for safer trait design.The uninitialized
$validatorproperty requires consumers to explicitly callinitValidator()before usingvalidate()orfirstError(). While all current command implementations safely initialize before validation, the trait would be more defensive if it self-initialized on first use, eliminating the risk of runtime errors from accidental misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/Commands/CommandValidationTrait.php` around lines 29 - 49, The trait currently requires initValidator() to be called before validate() or firstError(), risking uninitialized property errors; make $validator lazy-initialize by updating the protected property to allow null (e.g., ?Validator) and adding a private ensureValidator() helper or inline checks in validate() and firstError() that create a new Validator() if $this->validator is null, then proceed to call setRules(), isValid(), and firstError() as before; reference the Validator class, the initValidator() method (which can remain for explicit init), and the validate() and firstError() methods when applying this change.tests/Unit/shared/Services/PostServiceTest.php (1)
217-222: Use a unique tmp file and guard directory creation errors.The fixed filename (
php8fe1.tmp) can collide across runs/parallel jobs, andmkdirfailure is not handled.Proposed fix
- if (!is_dir(base_dir() . DS . 'tmp')) { - mkdir(base_dir() . DS . 'tmp'); - } - - file_put_contents($this->fileMeta['tmp_name'], hex2bin('FFD8FFE0')); + $tmpDir = base_dir() . DS . 'tmp'; + if (!is_dir($tmpDir) && !mkdir($tmpDir, 0777, true) && !is_dir($tmpDir)) { + $this->fail('Failed to create tmp directory for upload test'); + } + + $tmpFile = tempnam($tmpDir, 'upload_'); + $this->assertNotFalse($tmpFile); + $this->fileMeta['tmp_name'] = $tmpFile; + file_put_contents($tmpFile, hex2bin('FFD8FFE0'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/shared/Services/PostServiceTest.php` around lines 217 - 222, The test uses a fixed tmp filename and does not check mkdir errors; change to create a unique temp file (use tempnam(sys_get_temp_dir(), ...) or otherwise generate a unique name) and update $this->fileMeta['tmp_name'] accordingly, ensure the tmp directory creation for base_dir() . DS . 'tmp' checks the return value (or existence) and fails the test or throws an exception if mkdir returns false, and use the new unique path in the file_put_contents call so parallel runs cannot collide (update references to $this->fileMeta['tmp_name'] and the mkdir/file_put_contents usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Line 8: Replace the unstable moving dependency constraint "quantum/framework":
"dev-master" in composer.json with a stable 2.x constraint (e.g.
"quantum/framework": "^2.9") so release-track builds use a reproducible, tagged
release; update composer.lock by running composer update if needed after
changing the version.
In `@shared/Commands/PostUpdateCommand.php`:
- Around line 50-68: The help text in the PostUpdateCommand class (protected
?string $help) incorrectly advertises an unsupported -a/author option; update
the $help string to remove the `[-a \`Author\`]` portion so it matches the
actual options defined in the protected array $options (title, description,
image), ensuring $help and $options remain consistent.
In `@shared/Commands/UserDeleteCommand.php`:
- Around line 41-42: Update the command description string in the
UserDeleteCommand class (protected ?string $description) to accurately reflect
its action: it deletes a user by UUID or clears the entire users table with
confirmation; replace the incorrect phrase "entire posts table" with "entire
users table" or "all users" so the description matches the command's behavior.
In `@shared/Services/AuthService.php`:
- Around line 100-104: The add() implementation is returning the raw input $data
instead of the persisted record; update it to return the saved/persisted user
data by using the created model instance ($user) after save (e.g.,
$user->refresh() or $user->toArray()) and pass that persisted representation
into AuthUser->setData so generated fields and any DB-normalized values are
returned rather than the original $data.
In `@shared/Services/PostService.php`:
- Around line 179-183: updatePost currently calls $post->fill($data) which
trusts Post::$fillable and allows mutable fields like user_uuid to be changed;
filter the incoming $data before calling fill by whitelisting only true
updatable post attributes (e.g. title, body, status, tags — whatever your domain
allows) or explicitly removing sensitive keys like user_uuid, then call
$post->fill($sanitizedData) and save; update the updatePost method to perform
this sanitization (reference: updatePost, $post->fill, Post::$fillable) so
ownership cannot be changed via the service.
In `@tests/Feature/modules/Api/AuthControllerTest.php`:
- Line 186: In AuthControllerTest remove the unused local variable assignment by
either deleting the `$user =` part or replacing it with a plain call to
`createUser([...])` so the test no longer assigns an unused `$user`; locate the
statement using the `createUser` call on line with `$user` and remove the
variable binding to keep the test intent clean.
---
Outside diff comments:
In `@shared/Commands/DemoCommand.php`:
- Around line 355-358: The call to $this->faker->imageUrl in DemoCommand (used
to generate the $imageName for save_remote_image) passes incorrect trailing
arguments that don't match bluemmb/faker-picsum-photos-provider's signature;
update the invocation in DemoCommand.php so imageUrl(...) uses only the intended
width and height (e.g., $this->faker->imageUrl(640, 480)) or, if you need a
specific seed/grayscale/blur/file ending, pass them in the correct order
according to imageUrl($width, $height, $specificOrSeed = null, $grayscale =
null, $blur = null, $file_ending = null) before calling save_remote_image($url,
$user['uuid'], $title).
In `@shared/Commands/PostCreateCommand.php`:
- Around line 50-62: Update the help string in the PostCreateCommand class to
reflect the actual argument list (title, description, user_uuid) and the
optional uuid and image parameters; specifically change the protected ?string
$help value so it shows: php qt post:create `Title` `Description` `UserUUID`
`[UUID]` `[Image]` (or similar ordering that matches the protected array $args),
ensuring the displayed argument names match the keys 'title', 'description',
'user_uuid', 'uuid', and 'image'.
In `@shared/Commands/UserCreateCommand.php`:
- Around line 94-102: The call to service(AuthService::class)->add(...) in
UserCreateCommand.php is sending an 'image' field which will fail if the users
table/model lacks that column; remove 'image' from the payload passed to
AuthService::add() (or only include it conditionally after checking the User
model/migration supports it) so the array contains uuid, firstname, lastname,
role, email, and hashed password only; update references to AuthService::add and
any related validation to account for the removed/conditional image field.
In `@tests/Unit/shared/Services/AuthServiceTest.php`:
- Around line 24-43: The tests assume getAll()->first() is the default user
which is non-deterministic; update both testAuthServiceGetAll and
testAuthServiceGetUserByUuid to locate the default user explicitly by filtering
the collection for email 'default@quantumphp.io' (e.g., $users->filter(fn($u) =>
$u->email === 'default@quantumphp.io')->first()) and then assert on that user
(use that user's uuid when calling getUserByUuid); ensure you still assert the
collection type (ModelCollection) and that the found item is an instance of User
before checking the email.
In `@tests/Unit/shared/Services/PostServiceTest.php`:
- Around line 118-129: The test testPostServiceGetMyPosts relies on global
seeded data (causing CI flakiness); update it to arrange its own fixtures by
creating a user and known posts within the test, then call
postService->getMyPosts with that user's uuid and assert the returned
ModelCollection contains exactly the number of posts you created. Specifically,
replace the authService->getAll() usage with creating a new user (or obtaining a
freshly created user) and use factories to create N posts tied to that user's
uuid, then assertInstanceOf(ModelCollection::class, $myPosts) and assertCount(N,
$myPosts) to make the test deterministic.
---
Nitpick comments:
In `@shared/Commands/CommandValidationTrait.php`:
- Around line 29-49: The trait currently requires initValidator() to be called
before validate() or firstError(), risking uninitialized property errors; make
$validator lazy-initialize by updating the protected property to allow null
(e.g., ?Validator) and adding a private ensureValidator() helper or inline
checks in validate() and firstError() that create a new Validator() if
$this->validator is null, then proceed to call setRules(), isValid(), and
firstError() as before; reference the Validator class, the initValidator()
method (which can remain for explicit init), and the validate() and firstError()
methods when applying this change.
In `@shared/Commands/DemoCommand.php`:
- Around line 376-384: The getRandomUserExcept function can call array_rand on
an empty $candidates array; add a defensive check after building $candidates in
getRandomUserExcept to handle the empty case (e.g., if (empty($candidates))
throw new InvalidArgumentException("No available users after excluding
{$excludeUuid}");), otherwise proceed to array_rand as before; this prevents
undefined offset when array_rand would fail.
- Around line 458-472: Add explicit void return types to the two methods to
match the class conventions: change the signatures of
runCommandInternally(string $commandName, array $arguments = []) and
runCommandExternally(string $commandName, array $arguments = []) to include ":
void". Update both method declarations (and adjust any docblocks if present) so
they read runCommandInternally(...): void and runCommandExternally(...): void to
maintain consistency with methods like createModule, generateComments, and
updateProgress.
In `@tests/Unit/shared/Services/PostServiceTest.php`:
- Around line 217-222: The test uses a fixed tmp filename and does not check
mkdir errors; change to create a unique temp file (use
tempnam(sys_get_temp_dir(), ...) or otherwise generate a unique name) and update
$this->fileMeta['tmp_name'] accordingly, ensure the tmp directory creation for
base_dir() . DS . 'tmp' checks the return value (or existence) and fails the
test or throws an exception if mkdir returns false, and use the new unique path
in the file_put_contents call so parallel runs cannot collide (update references
to $this->fileMeta['tmp_name'] and the mkdir/file_put_contents usage).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2865745-6c26-43ac-99a7-55d841f66501
⛔ Files ignored due to path filters (1)
tests/_root/logs/app.logis excluded by!**/*.log
📒 Files selected for processing (33)
.github/workflows/php.ymlcomposer.jsonhelpers/functions.phpmigrations/create_table_comments_1698145440.phpmigrations/create_table_posts_1669639752.phpmigrations/create_table_users_1669639740.phpshared/Commands/CommandValidationTrait.phpshared/Commands/CommentCreateCommand.phpshared/Commands/CommentDeleteCommand.phpshared/Commands/DemoCommand.phpshared/Commands/PostCreateCommand.phpshared/Commands/PostDeleteCommand.phpshared/Commands/PostShowCommand.phpshared/Commands/PostUpdateCommand.phpshared/Commands/UserCreateCommand.phpshared/Commands/UserDeleteCommand.phpshared/Commands/UserShowCommand.phpshared/Models/Comment.phpshared/Models/Post.phpshared/Models/User.phpshared/Services/AuthService.phpshared/Services/CommentService.phpshared/Services/PostService.phpshared/Transformers/CommentTransformer.phpshared/Transformers/PostTransformer.phptests/Feature/AppTestCase.phptests/Feature/modules/Api/AuthControllerTest.phptests/Feature/modules/Api/CommentControllerTest.phptests/Helpers/functions.phptests/Unit/shared/Services/AuthServiceTest.phptests/Unit/shared/Services/PostServiceTest.phptests/_root/helpers/functions.phptests/_root/tmp/php8fe1.tmp
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/Unit/shared/Services/AuthServiceTest.php (1)
29-39:⚠️ Potential issue | 🟠 MajorRecreating
AuthServicehere is redundant and does not fix flaky assertions.Line 31 duplicates setup initialization and still leaves
first()-based assertions nondeterministic (as shown by the pipeline failures at Line 38 and Line 49). The test should assert presence/lookup of the expected user, not collection order.More stable test approach
public function testAuthServiceGetAll() { - $this->authService = ServiceFactory::create(AuthService::class); $users = $this->authService->getAll(); $this->assertInstanceOf(ModelCollection::class, $users); $this->assertInstanceOf(User::class, $users->first()); - $this->assertEquals('default@quantumphp.io', $users->first()->email); + $emails = array_map(static fn ($u) => $u->email, $users->all()); + $this->assertContains('default@quantumphp.io', $emails); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/shared/Services/AuthServiceTest.php` around lines 29 - 39, testAuthServiceGetAll recreates AuthService redundantly and uses nondeterministic first() assertions; remove the ServiceFactory::create(AuthService::class) call and rely on the setUp-initialized $this->authService, call getAll() on that instance, assert the return is a ModelCollection and then assert that the collection contains a User with email 'default@quantumphp.io' (rather than using $users->first()), e.g. by searching/filtering the ModelCollection for a User with that email or asserting a lookup method, keeping references to AuthService, ServiceFactory, getAll, ModelCollection, User and first() as guides.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Unit/shared/Services/AuthServiceTest.php`:
- Around line 24-27: tearDown() in AuthServiceTest is a no-op that only calls
parent::tearDown(), so it must actively reset DI/container state to prevent
cross-test leakage; update the AuthServiceTest::tearDown() implementation to
clear and reset the application/container and any singletons/mocked services
used by the tests (e.g., clear service entries, unset static singletons, restore
original bindings or call the framework's container reset/flush method), ensure
any created test doubles are torn down, then call parent::tearDown() at the end.
---
Outside diff comments:
In `@tests/Unit/shared/Services/AuthServiceTest.php`:
- Around line 29-39: testAuthServiceGetAll recreates AuthService redundantly and
uses nondeterministic first() assertions; remove the
ServiceFactory::create(AuthService::class) call and rely on the
setUp-initialized $this->authService, call getAll() on that instance, assert the
return is a ModelCollection and then assert that the collection contains a User
with email 'default@quantumphp.io' (rather than using $users->first()), e.g. by
searching/filtering the ModelCollection for a User with that email or asserting
a lookup method, keeping references to AuthService, ServiceFactory, getAll,
ModelCollection, User and first() as guides.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2f7129c-33be-40ff-8f30-d6c68400d0a4
📒 Files selected for processing (2)
tests/Unit/shared/Services/AuthServiceTest.phptests/bootstrap.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/_root/.env.example`:
- Line 1: Replace the secret-like APP_KEY value in the .env example with a
non-sensitive placeholder (e.g., "REPLACE_WITH_APP_KEY" or leave blank) so the
file no longer contains a real-looking key; update any project docs or scripts
to generate or inject a real APP_KEY at runtime/setup rather than storing it in
the example file.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Feature/modules/Api/CommentControllerTest.php`:
- Around line 71-79: The test uses the create response's 'data.uuid' directly
for the delete call without first asserting the create succeeded; update the
test around the create call (the first $response, $comment variables returned by
$this->request()) to assert the response indicates success and that 'data' and
'data.uuid' are present (e.g., status code and/or
assertArrayHasKey/assertNotEmpty on $comment['uuid']) before constructing the
DELETE request to /api/comments/delete/ and using $comment['uuid']; this ensures
clear test failure if creation failed.
- Line 61: The test calls ModelFactory::get(Comment::class)->findOneBy('uuid',
$comment['uuid'])->delete() which will fatal if findOneBy returns null; update
the teardown to first assign the result of
ModelFactory::get(Comment::class)->findOneBy('uuid', $comment['uuid']) to a
variable, assert that this variable is not null (e.g. assertNotNull) and only
then call ->delete() on it so failures are explicit and non-fatal; reference the
findOneBy result variable when calling delete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48aa07a4-5e1b-4409-99c0-f46028cefecd
⛔ Files ignored due to path filters (1)
logs/app.logis excluded by!**/*.log
📒 Files selected for processing (2)
tests/Feature/modules/Api/CommentControllerTest.phptests/_root/logs/.gitkeep
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/Unit/shared/Services/CommentServiceTest.php (1)
60-61:⚠️ Potential issue | 🟠 MajorReplace hard-coded seeded counts with baseline-relative assertions.
Line 61 and Line 119 are failing because these tests depend on fixture totals (
3/4) that are no longer stable. Arrange data in-test and assert relative changes (before + 1, then back tobefore) instead of absolute counts.Proposed deterministic pattern
@@ public function testCommentServiceGetCommentsByPost() - $comments = $this->commentService->getCommentsByPost($post->uuid); - - $this->assertInstanceOf(ModelCollection::class, $comments); - $this->assertCount(3, $comments); + $before = count($this->commentService->getCommentsByPost($post->uuid)); + $user = $this->authService->getAll()->first(); + $created = $this->commentService->addComment([ + 'user_uuid' => $user->uuid, + 'post_uuid' => $post->uuid, + 'content' => 'count probe', + ]); + + $comments = $this->commentService->getCommentsByPost($post->uuid); + $this->assertInstanceOf(ModelCollection::class, $comments); + $this->assertCount($before + 1, $comments); + + $this->commentService->deleteComment($created['uuid']); @@ public function testCommentServiceDeleteComment() - $data = $this->commentService->addComment([ + $before = count($this->commentService->getCommentsByPost($post->uuid)); + $data = $this->commentService->addComment([ 'user_uuid' => $user->uuid, 'post_uuid' => $post->uuid, 'content' => 'To be deleted' ]); @@ - $this->assertCount( - 4, - $this->commentService->getCommentsByPost($post->uuid) - ); + $this->assertCount($before + 1, $this->commentService->getCommentsByPost($post->uuid)); @@ - $this->assertCount( - 3, - $this->commentService->getCommentsByPost($post->uuid) - ); + $this->assertCount($before, $this->commentService->getCommentsByPost($post->uuid));Also applies to: 117-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/shared/Services/CommentServiceTest.php` around lines 60 - 61, The test currently asserts hard-coded totals (e.g. $this->assertCount(3, $comments)) which is flaky; instead capture the current baseline count from the service, perform the action, and assert relative changes (e.g. $before + 1) then revert and assert back to $before. Update places in CommentServiceTest that reference ModelCollection/$comments and the failing assertions (lines around the assertCount calls and the create/delete tests in 117-127) to fetch the initial count via the same method used in the test (the service call that returns $comments), assertCount($before + 1, $commentsAfterCreate), then after deletion assertCount($before, $commentsAfterDelete). Ensure you use the same identifiers ($comments, ModelCollection, and the CommentService methods used to list/create/delete) so the assertions are baseline-relative rather than absolute.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Unit/shared/Services/CommentServiceTest.php`:
- Around line 5-7: Remove the committed diagnostic block (STDERR logging and raw
queries) that causes a null-deref; instead ensure the test obtains a valid user
before reading ->uuid by either asserting the first user exists (e.g.
$this->assertNotNull($user = $this->authService->getAll()->first()) or creating
one via the factory) and then use $user->uuid, and delete the raw query/STDERR
lines so the test uses normal service/factory flows (references:
$this->authService->getAll()->first(), $user->uuid, STDERR, raw query usage).
---
Outside diff comments:
In `@tests/Unit/shared/Services/CommentServiceTest.php`:
- Around line 60-61: The test currently asserts hard-coded totals (e.g.
$this->assertCount(3, $comments)) which is flaky; instead capture the current
baseline count from the service, perform the action, and assert relative changes
(e.g. $before + 1) then revert and assert back to $before. Update places in
CommentServiceTest that reference ModelCollection/$comments and the failing
assertions (lines around the assertCount calls and the create/delete tests in
117-127) to fetch the initial count via the same method used in the test (the
service call that returns $comments), assertCount($before + 1,
$commentsAfterCreate), then after deletion assertCount($before,
$commentsAfterDelete). Ensure you use the same identifiers ($comments,
ModelCollection, and the CommentService methods used to list/create/delete) so
the assertions are baseline-relative rather than absolute.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc69f65a-49bd-4dcd-9a70-a6300081dcfe
📒 Files selected for processing (2)
tests/Feature/modules/Api/AuthControllerTest.phptests/Unit/shared/Services/CommentServiceTest.php
andrey-smaelov
approved these changes
Mar 16, 2026
This was referenced Apr 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Chores