-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Consolidate repetitive model instantiations across tests #3416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6ecd527 to
aa3b3dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the test suite to reduce code duplication by introducing a factory-like helper method for creating test User instances. The changes replace multiple identical inline model instantiations with calls to a centralized makeUser() method, improving maintainability and consistency.
Key changes:
- Added
makeUser()helper method to eliminate repetitive User model instantiation - Added
setUp()method to properly initialize Carbon test time - Added missing return type declarations to test methods
Comments suppressed due to low confidence (1)
tests/ModelTest.php:1
- The screenshot filename references '2025-06-24', which is a future date (current date is December 8, 2025). This appears to be an incorrect timestamp in the screenshot filename.
<?php
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| public function testInsert(): void | ||
| private function makeUser(): User |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The helper method makeUser() has private visibility, which prevents subclasses from reusing this factory method. Consider changing it to protected to allow test class inheritance and reuse of the helper.
| private function makeUser(): User | |
| protected function makeUser(): User |
| $user->save(); | ||
|
|
||
| return $user; | ||
| } | ||
|
|
||
| public function testInsert(): void | ||
| { | ||
| $user = $this->makeUser(); |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The makeUser() method calls save() which performs a database operation. This tightly couples test data creation with persistence. Consider separating concerns by having the method only construct the user object and letting individual tests decide when to save, or alternatively rename to createUser() to better reflect that it persists data.
| $user->save(); | |
| return $user; | |
| } | |
| public function testInsert(): void | |
| { | |
| $user = $this->makeUser(); | |
| return $user; | |
| } | |
| public function testInsert(): void | |
| { | |
| $user = $this->makeUser(); | |
| $user->save(); |
|
Thank you @zigzagdev |
Description
This pull request refactors the test suite to eliminate redundant model instantiations. Several test cases repeatedly constructed identical
User,Item, andBookmodel objects inline, leading to code duplication and reduced maintainability.To address this:
This is a non-functional change (refactor only) and does not alter the behaviour or scope of any existing tests.
Checklist