Clean up incomplete tests and fix factory patterns#470
Merged
Conversation
Without module.json, nwidart/laravel-modules never discovers the Invoices module, so InvoicesServiceProvider is never registered, its migrations are never loaded, and the `invoices` table does not exist when the `payments` migration tries to add a FK to it — causing errno 150. https://claude.ai/code/session_015zna7fenkZhhJgqWcpt53W
When a factory is created with ->state(['company_id' => ...]), the state
values are not yet visible inside definition() — so resolveCompanyId()
always returns null. With APP_ENV=testing, the runningUnitTests() guard
was true and resolveForeignKey executed User::where('company_id', null)
which fails because the users table has no company_id column (user-company
is a pivot relationship).
Fix: skip the DB lookup when companyId is null and fall back to
$relatedClass::factory() so standalone factory usage still works. When
the caller provides an explicit id via state (as all seeders do), Laravel
ignores the factory reference and uses the state value instead.
https://claude.ai/code/session_015zna7fenkZhhJgqWcpt53W
Company::factory()->create() in definition() fired on every Relation factory call because resolveCompanyId() always returns null when company_id is supplied via ->state() (state runs after definition). With 11 companies × 25 relations each, 275 extra companies were created, blowing the DatabaseSeeder company-count assertion. Replace with a lazy Company::factory() reference so the company is only created if the state does not override company_id. https://claude.ai/code/session_015zna7fenkZhhJgqWcpt53W
…rrent_tenant
The method body was never closed before the #endregion comment, causing
PHPUnit to report "Unclosed '{' on line 21" and fail to load the test suite.
https://claude.ai/code/session_015zna7fenkZhhJgqWcpt53W
Two stale/incomplete methods were duplicated causing PHP fatal errors: - it_fails_to_create_project_without_required_project_name (markTestIncomplete) - it_updates_a_project (markTestIncomplete + old field names) https://claude.ai/code/session_015zna7fenkZhhJgqWcpt53W
resolveCompany() in definition() returns null before state() values are applied, causing a crash on ->id. Use lazy factory reference instead. https://claude.ai/code/session_015zna7fenkZhhJgqWcpt53W
…r methods PHPUnit exits with code 1 when a data provider supplies more arguments than the test method accepts. Add optional $format parameter to the 8 affected methods so the provider matches the signature. https://claude.ai/code/session_015zna7fenkZhhJgqWcpt53W
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Modules/Clients/Tests/Feature/CustomersTest.php (1)
439-458: ⚡ Quick winRemove duplicated fixture creation in tenant-listing test.
$customerA/$customerBare each created twice, and the first pair is overwritten. This adds unnecessary DB writes and makes the test intent harder to follow.Proposed cleanup
- $customerA = Relation::factory()->for($this->company)->customer()->create(['company_name' => 'Visible Customer']); - $customerB = Relation::factory()->for($companyB)->customer()->create(['company_name' => 'Hidden Customer']); - $customerA = Relation::factory()->for($this->company)->customer()->create(['company_name' => 'Visible Customer']); - $customerB = Relation::factory()->for($companyB)->customer()->create(['company_name' => 'Hidden Customer']); + $customerA = Relation::factory()->for($this->company)->customer()->create(['company_name' => 'Visible Customer']); + $customerB = Relation::factory()->for($companyB)->customer()->create(['company_name' => 'Hidden Customer']);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Modules/Clients/Tests/Feature/CustomersTest.php` around lines 439 - 458, In the it_only_lists_customers_for_the_current_tenant test remove the duplicate fixture creation lines so each customer is created only once: keep a single Relation::factory()->for($this->company)->customer()->create([...]) for $customerA and a single Relation::factory()->for($companyB)->customer()->create([...]) for $customerB, then leave the Livewire::actingAs(...)->test(ListRelations::class, ...) and assertions unchanged (ensure $customerB->id still references the created hidden customer).Modules/Invoices/Tests/Unit/Peppol/FormatHandlers/FormatHandlersTest.php (1)
44-44: ⚡ Quick winAvoid nullable unused provider args in these test signatures.
These methods now accept
$format = null, but they never use it (matching the PHPMD unused-parameter warnings). Either use$formatin assertions or split to a provider that yields only$handlerClassfor these tests.Also applies to: 55-55, 66-66, 80-80, 94-94, 115-115, 130-130, 145-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Modules/Invoices/Tests/Unit/Peppol/FormatHandlers/FormatHandlersTest.php` at line 44, The test method it_returns_correct_mime_type($handlerClass, $format = null) (and the other similar test methods flagged) declare an unused nullable $format parameter which triggers PHPMD warnings; remove the unused parameter from the test signatures (e.g., change it_returns_correct_mime_type($handlerClass): void) and update the data provider(s) to yield only $handlerClass, or alternately use $format in the body/assertions if that value is intended; apply the same change to the other affected test methods in FormatHandlersTest.php so signatures and providers align and the unused-parameter warnings are eliminated.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Modules/Clients/Tests/Feature/CustomersTest.php`:
- Around line 439-458: In the it_only_lists_customers_for_the_current_tenant
test remove the duplicate fixture creation lines so each customer is created
only once: keep a single
Relation::factory()->for($this->company)->customer()->create([...]) for
$customerA and a single
Relation::factory()->for($companyB)->customer()->create([...]) for $customerB,
then leave the Livewire::actingAs(...)->test(ListRelations::class, ...) and
assertions unchanged (ensure $customerB->id still references the created hidden
customer).
In `@Modules/Invoices/Tests/Unit/Peppol/FormatHandlers/FormatHandlersTest.php`:
- Line 44: The test method it_returns_correct_mime_type($handlerClass, $format =
null) (and the other similar test methods flagged) declare an unused nullable
$format parameter which triggers PHPMD warnings; remove the unused parameter
from the test signatures (e.g., change
it_returns_correct_mime_type($handlerClass): void) and update the data
provider(s) to yield only $handlerClass, or alternately use $format in the
body/assertions if that value is intended; apply the same change to the other
affected test methods in FormatHandlersTest.php so signatures and providers
align and the unused-parameter warnings are eliminated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55507f9a-2cde-488b-94fb-848c72973515
📒 Files selected for processing (7)
Modules/Clients/Database/Factories/RelationFactory.phpModules/Clients/Tests/Feature/CustomersTest.phpModules/Core/Database/Factories/AbstractFactory.phpModules/Core/Database/Factories/EmailTemplateFactory.phpModules/Invoices/Tests/Unit/Peppol/FormatHandlers/FormatHandlersTest.phpModules/Invoices/module.jsonModules/Projects/Tests/Feature/ProjectsTest.php
💤 Files with no reviewable changes (1)
- Modules/Projects/Tests/Feature/ProjectsTest.php
Collaborator
Author
|
@coderabbitai autofix |
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
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
This PR removes incomplete/skipped tests, fixes factory patterns to properly handle company resolution, and adds missing module configuration. The changes improve test reliability and factory consistency across the codebase.
Key Changes
Removed incomplete tests from
ProjectsTest.php:it_fails_to_create_project_without_required_project_name()testit_updates_a_project()test marked withmarkTestIncomplete()andmarkTestSkipped()Fixed factory patterns to properly handle company resolution:
RelationFactoryto use lazy factory pattern (Company::factory()) instead of eagerly creating companies whencompanyIdis nullEmailTemplateFactoryto use lazy factory pattern for company resolutionAbstractFactory::resolveForeignKey()to only query existing records when$companyIdis explicitly provided, otherwise return a lazy factory instanceAdded missing module configuration:
Modules/Invoices/module.jsonwith proper module metadata and service provider configurationFixed test method signatures in
FormatHandlersTest.php:$format = nullparameter to all data provider test methods to match the provider's data structureFixed syntax error in
CustomersTest.php:it_only_lists_customers_for_the_current_tenant()methodImplementation Details
The factory pattern changes follow Laravel's lazy factory approach, deferring factory instantiation until needed. This prevents unnecessary database operations during test setup and allows factories to be properly resolved within the context where they're used.
https://claude.ai/code/session_015zna7fenkZhhJgqWcpt53W
Summary by CodeRabbit
Bug Fixes
Tests
Chores