Add Playwright e2e suite and reorganize PHPUnit tests - #460
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
d40b12c to
1b7789c
Compare
243edef to
0b36b06
Compare
…p-code changes feature/laravel12-refactor (Bottelet#459) is the "app-code only" half of a PR split, so its own diff never touches tests/ - but several of the app-code changes on this branch changed real behavior, and the inherited (pre-split) copies of the tests exercising that behavior were never updated to match. Running the full suite standalone on this branch (rather than only via Bottelet#460, which already carries fixed copies of these files) surfaced 3 errors + 18 failures, all traced to one of four root causes: - AbsenceReason enum values changed from freeform strings ('Sick') to a fixed set ('sick_leave' etc.) - AbsenceControllerTest still sent the old value, so every absence-creation request now fails validation before reaching the code path under test. (tests/Feature/Absenses/AbsenceControllerTest.php) - StoreUserRequest's role/department validation keys were fixed from the never-actually-sent plural 'roles'/'departments' to the singular 'role'/'department' that the form actually submits (see the fix's own comment in the class) - UsersControllerTest's payload helper still built the old plural keys, so user-creation requests failed validation before ever reaching the exception path the two affected tests exist to exercise. Fixing the payload also resolved the "did not remove its own error/exception handlers" risky-test warnings on both, which were a downstream symptom of the request never reaching the code that installs and removes them. (tests/Feature/Users/UsersControllerTest.php) - Status-update validation moved from a manual check + custom 400/session-flash response to a proper FormRequest, which Laravel turns into an automatic 422 (JSON) or redirect+assertSessionHasErrors (web) response - four tests across two controllers still asserted the old 400/flash_message_warning shape. (tests/Feature/Projects/ProjectSecurityTest.php, tests/Feature/Tasks/TaskSecurityTest.php) - The exception Handler now converts a FormRequest's authorize() failure into a flash+redirect-back for non-JSON requests (matching the rest of the app's permission-denial pattern) instead of Laravel's generic 403 page - one test still asserted the old assertForbidden() shape. (tests/Feature/Clients/ClientAuthorizationTest.php) Two more failures were pre-existing test bugs unrelated to any behavior change: - ClientServiceTest::it_gets_{tasks,projects,leads}_with_relations called assertCount() directly on the HasMany relation builder returned by ClientService (never resolved with ->get()), which throws a TypeError under this PHPUnit/Laravel version rather than counting the underlying query. - DepartmentsControllerTest::it_can_create_department and OfferAuthorizationTest's won/lost tests sent requests via $this->json(), which sets an Accept: application/json header and made the controllers take their JSON-response branch (200/201) instead of the redirect branch (302) the tests assert - switched to plain $this->post(). Also fixed, unrelated to any of the above: ProjectFilesConfigurationTest asserted .env.ci's CACHE_STORE must be "array", but .env.ci is intentionally "database" (it drives real HTTP requests in the Playwright suite, and "array" never serializes cached values - see the updated test's own message for the specific failure mode this guards against). The test's expectation was stale, not the config; flipped the assertion and renamed the test to match. Brittleness note for the record: none of these are hardcoded-count style brittle tests (that pattern exists elsewhere, e.g. CountryTest's hardcoded country count - flagged separately, not touched here) - all nine were behavior assertions that fell out of sync with intentional app-code changes because the test-fixing work for this refactor landed exclusively on the stacked tests PR instead of also being back-ported here. Verified: full suite standalone on this branch, 895 tests, 2191 assertions, 0 errors, 0 failures (9 pre-existing notices/8 incomplete unrelated and unchanged).
1d04a18 to
90f0396
Compare
- Add a Playwright end-to-end test suite covering major domain flows.
- Reorganize and rename PHPUnit tests to consistent AAA structure and
it_-prefixed, grammatically-sentenced method names; tighten every
bare assertRedirect() to assert an actual target.
- Add a generic validity test for all resources/lang/*.json files.
- Remove brittle exact-count assertions over growable enum/lookup
collections (Country, AbsenceReason); replace with membership
checks that don't break on legitimate additions.
- Fix AbsenceReason::values() constructing its TIME_OFF_IN_LIEU entry
with the wrong reason constant, making it indistinguishable from
TIME_OFF and breaking fromStatus('time_off_in_lieu'); an existing
test had codified the bug as expected behavior and is corrected.
- Replace no-op assertTrue(true) assertions with
expectNotToPerformAssertions() or a real return-value check.
- Strengthen 26 status-only assertions (bare assertOk()/assertStatus(200))
with real content, database-state, or response-header checks across
document view/download, create-page, and settings/offer/lead tests.
- Close FormRequest validation-failure and authorization-denial test
gaps across 31 FormRequest classes, including three previously
zero-coverage routes (UpdateTaskRequest, UpdateProjectRequest,
AddInvoiceLine) and an unverified denial path on roles.store, which
sits outside the admin middleware group and relies solely on
StoreRoleRequest::authorize().
90f0396 to
06516b7
Compare
8d4926b to
0d56dca
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Schema-aware test setup and Dusk organization tests/AbstractTestCase.php, tests/Browser/* |
Test database initialization detects RefreshDatabase, adds a JSON GET helper, and browser tests receive renamed methods and separated Act/Assert sections. |
Feature coverage
| Layer / File(s) | Summary |
|---|---|
Feature request and authorization coverage tests/Feature/Controllers/*, tests/Feature/Settings/*, tests/Feature/Tasks/*, tests/Feature/Projects/* |
HTTP requests use verb-specific helpers and explicit JSON headers, while validation, redirects, permissions, and persistence assertions are expanded. |
Domain workflow coverage tests/Feature/Documents/*, tests/Feature/Leads/*, tests/Feature/Offers/*, tests/Feature/Payments/* |
Document, lead, offer, appointment, and payment workflows receive consolidated authorization, storage, lifecycle, and error-path tests. |
Unit coverage and support
| Layer / File(s) | Summary |
|---|---|
Service, model, enum, and environment tests tests/Unit/* |
Unit tests add assertions for services, repositories, enums, models, observers, cache serialization, environment files, language files, and relationship behavior. |
Storage and integration doubles tests/Unit/Services/Storage/*, tests/Unit/Integration/* |
Dropbox tests use injected stubs and deterministic cleanup; integration tests use standardized section markers and stronger response assertions. |
Estimated code review effort: 5 (Critical) | ~120 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 5.29% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly matches the PR’s main changes: adding Playwright e2e coverage and reorganizing PHPUnit tests. |
| Description check | ✅ Passed | The description is detailed and directly describes the test-suite changes, related fixes, and validation for this PR. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
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 help to get the list of available commands.
There was a problem hiding this comment.
Actionable comments posted: 30
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
tests/Feature/Integrations/IntegrationsTest.php (1)
18-41: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAdd a non-admin denial case for integration writes.
IntegrationsControlleris protected byuser.is.admin, but this covers only the allowed path. Post the same payload as a non-admin and assert denial plus no persisted integration. As per path instructions, “tests must not … assert only the happy path for something security-relevant.”🤖 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 `@tests/Feature/Integrations/IntegrationsTest.php` around lines 18 - 41, Add a non-admin test alongside it/around it for the integration store flow, using the same payload as it_persists_only_validated_fields_and_returns_ok_when_storing. Authenticate as a non-admin, post to integrations.store, assert the request is denied, and verify no integration record was persisted.Source: Path instructions
tests/Feature/Controllers/Search/SearchControllerSecurityTest.php (1)
38-167: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename the
it_search...tests as sentences.Names such as
it_search_with_valid_type_client_returns_resultsare not grammatical. Use forms such asit_returns_results_for_a_valid_client_searchconsistently. As per path instructions, each test method must “read as a sentence.”🤖 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 `@tests/Feature/Controllers/Search/SearchControllerSecurityTest.php` around lines 38 - 167, Rename the affected it_search... test methods in SearchControllerSecurityTest so each reads as a grammatical sentence, following the pattern it_returns_results_for_a_valid_client_search. Apply this consistently to the valid-type, invalid-type, case-insensitivity, and injection-related tests without changing their behavior.Source: Path instructions
tests/Unit/Projects/ProjectServiceTest.php (1)
116-136: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest name doesn't match what it verifies; the real "missing" branch is untested.
ProjectService::createthrowsInvalidArgumentExceptionwhenclient_external_idis empty/missing, and only returnsnullwhen a non-emptyclient_external_iddoesn't match a client. This test passes'client_external_id' => 'missing'(a non-empty string), so it actually exercises the "client not found" branch — identical toit_returns_null_when_client_not_foundright below it — not the "missing key" scenario its name implies. TheInvalidArgumentExceptionpath is left uncovered.✏️ Suggested fix
- public function it_returns_null_when_client_external_id_missing(): void - { - /* Arrange */ - $service = $this->app->make(ProjectService::class); - $user = User::factory()->create(); - $status = Status::factory()->create(['source_type' => Project::class]); - - /* Act */ - $result = $service->create([ - 'client_external_id' => 'missing', - 'title' => 'x', - 'description' => 'x', - 'user_assigned_id' => $user->id, - 'deadline' => '2026-01-01', - 'status_id' => $status->id, - ], $user->id); - - /* Assert */ - $this->assertNull($result); - } + public function it_throws_when_client_external_id_is_missing(): void + { + /* Arrange */ + $service = $this->app->make(ProjectService::class); + $user = User::factory()->create(); + $status = Status::factory()->create(['source_type' => Project::class]); + + /* Act & Assert */ + $this->expectException(\InvalidArgumentException::class); + $service->create([ + 'title' => 'x', + 'description' => 'x', + 'user_assigned_id' => $user->id, + 'deadline' => '2026-01-01', + 'status_id' => $status->id, + ], $user->id); + }🤖 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 `@tests/Unit/Projects/ProjectServiceTest.php` around lines 116 - 136, Update it_returns_null_when_client_external_id_missing to omit client_external_id from the create payload and assert the InvalidArgumentException thrown by ProjectService::create. Keep the existing non-empty “missing” value and null assertion in it_returns_null_when_client_not_found, preserving separate coverage for both branches.tests/Feature/Payments/PaymentsTest.php (1)
84-101: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the parsed amount, not just HTTP 201.
These four tests specifically exercise locale decimal separators and negative amounts, but none of them assert the actual stored/parsed
amount— only status 201 and non-empty payments.it_can_add_payment_with_minus_amount(Line 141) already shows the pattern (assertEquals(-5000, ...->first()->amount)); without a similar assertion here, a parsing regression (e.g., comma vs dot mis-parsed, or wrong sign handling) would go undetected.✅ Example fix for one case
$this->assertTrue($isEmpty); $response->assertStatus(201); $this->assertFalse($this->invoice->refresh()->payments->isEmpty()); + $this->assertEquals(5023, $this->invoice->refresh()->payments->first()->amount);Also applies to: 104-121, 145-162, 165-182
🤖 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 `@tests/Feature/Payments/PaymentsTest.php` around lines 84 - 101, Update the payment decimal-separator and negative-amount tests, including it_can_add_payment_with_decimals_dot_separator and the cases around the referenced methods, to assert the persisted payment amount value after refreshing the invoice. Follow the existing assertion pattern in it_can_add_payment_with_minus_amount and verify each input’s expected parsed amount, while retaining the current status and non-empty payment assertions.tests/Feature/Projects/ProjectAssignmentAuthorizationTest.php (2)
63-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate authorization coverage across files.
This scenario (authorized project reassignment) is also covered in
ProjectAuthorizationTest.phpandProjectSecurityTest.php. See consolidated comment for full details.🤖 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 `@tests/Feature/Projects/ProjectAssignmentAuthorizationTest.php` around lines 63 - 88, Remove the duplicate test method it_authorized_user_can_reassign_project from ProjectAssignmentAuthorizationTest, keeping the existing authorized reassignment coverage in ProjectAuthorizationTest and ProjectSecurityTest.
90-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate authorization coverage across files.
This scenario (unauthorized project reassignment) is also covered in
ProjectAuthorizationTest.phpandProjectSecurityTest.php. See consolidated comment for full details.🤖 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 `@tests/Feature/Projects/ProjectAssignmentAuthorizationTest.php` around lines 90 - 114, Remove the duplicate test method it_unauthorized_user_cannot_reassign_project from ProjectAssignmentAuthorizationTest, retaining the consolidated unauthorized reassignment coverage in ProjectAuthorizationTest.php and ProjectSecurityTest.php.tests/Feature/Projects/ProjectAuthorizationTest.php (1)
34-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate authorization coverage across files.
Same scenario also covered in
ProjectAssignmentAuthorizationTest.phpandProjectSecurityTest.php. See consolidated comment.🤖 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 `@tests/Feature/Projects/ProjectAuthorizationTest.php` around lines 34 - 49, Remove the duplicate test method it_updates_project_assignment_when_user_has_permission from ProjectAuthorizationTest, retaining the equivalent coverage in ProjectAssignmentAuthorizationTest and ProjectSecurityTest.
🤖 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.
Inline comments:
In `@tests/AbstractTestCase.php`:
- Around line 161-164: Remove the redundant getJsonRequest() wrapper from
AbstractTestCase and update its callers to use Laravel’s built-in getJson()
method directly, preserving the existing URL and request behavior.
- Around line 35-40: In setUp(), simplify the $usesRefreshDatabase check by
calling class_uses_recursive($this) directly inside array_keys, removing the
immediately invoked closure and its ->call($this) rebinding while preserving the
existing RefreshDatabase detection.
- Around line 42-53: Update the RefreshDatabase test setup in AbstractTestCase
and/or the affected TypeOfStatusTest so seeder-backed Status::typeOf*()
reference rows are created before assertions run. Prefer the existing test
seeding mechanism, ensuring RefreshDatabase tests invoke the appropriate seeder
while preserving the current migrate:fresh path for other tests.
In `@tests/Browser/LeadTest.php`:
- Around line 61-64: Correct the method casing from assertsee to assertSee in
the LeadTest created_at assertion and the corresponding TaskTest created_at
assertion, preserving their existing arguments and assertion behavior.
- Around line 152-170: Update the browser test method
it_can_go_to_create_new_client_in_dropdown_if_no_clients_exists_from_lead to
visit /leads/create instead of /projects/create, preserving the existing client
cleanup, user selection, dropdown interaction, and expected /clients/create
assertion.
In `@tests/Browser/ProjectTest.php`:
- Around line 131-146: Add a Client::query()->forceDelete() precondition at the
start of
it_can_go_to_create_new_client_in_dropdown_if_no_clients_exists_from_project,
before creating the user or browsing, so the test reliably verifies behavior
when no clients exist.
In `@tests/Feature/Commands/ClearEntrustCacheCommandTest.php`:
- Around line 16-36: Update it_displays_success_message and
it_shows_details_with_verbose_option in ClearEntrustCacheCommandTest to assert
the respective promised console output in addition to the successful exit code.
Verify the normal command’s success message and the verbose command’s detail
output; otherwise rename the methods to reflect execution-success coverage.
In `@tests/Feature/Commands/UpgradeCommandTest.php`:
- Around line 19-36: Rename the UpgradeCommand test methods, including
it_command_does_not_delete_existing_permissions and the tests through the
owner/administrator permission cases, so each reads as a sentence describing the
expected behavior. Use names such as
it_preserves_existing_permissions_when_upgrade_runs,
it_preserves_existing_role_assignments_when_upgrade_runs,
it_assigns_all_permissions_to_the_owner_role, and
it_assigns_all_permissions_to_the_administrator_role.
In `@tests/Feature/Controllers/ClientCreatePermissionCacheTest.php`:
- Around line 57-69: Create the owner role, required permissions, and
authenticated user explicitly within
it_allows_owner_to_access_clients_create_with_cached_permissions instead of
retrieving the role from shared seeded state. Use the test’s existing
factory/authentication patterns, then call cachedPermissions() and perform the
clients.create request against that self-contained fixture.
In `@tests/Feature/Controllers/CommentControllerTest.php`:
- Line 102: Update the assertDatabaseMissing checks in
tests/Feature/Controllers/CommentControllerTest.php at lines 102-102 and 119-119
to use the persisted sanitized description <p>Should fail</p>, and at lines
185-185 to use <p>Should be blocked</p> (or an equivalent source-specific
record), ensuring invalid or unauthenticated requests cannot satisfy the
assertions through raw-text mismatches.
In `@tests/Feature/Documents/DocumentsTest.php`:
- Around line 673-735: Strengthen the assertions in
it_allows_file_upload_to_task_with_permission,
it_forbids_file_upload_to_task_without_permission,
it_allows_file_upload_to_project_with_permission, and
it_forbids_file_upload_to_project_without_permission to match the detailed
expectations of their later sibling tests. Verify successful uploads with the
expected response and document creation, and verify denied uploads include the
correct redirect target, flash message, and no created document.
In `@tests/Feature/Invoices/AddInvoiceLineTest.php`:
- Around line 28-50: Add an unauthorized-user test alongside
it_creates_invoice_line_with_valid_payload that attempts the same invoice-line
creation without modify-invoice-lines permission, then assert the expected
redirect and warning response and verify no invoice_lines record was created for
the invoice. Keep the existing valid-payload test unchanged.
In `@tests/Feature/Invoices/InvoiceLinesTest.php`:
- Around line 75-80: Rename the `$r` variable in the invoice line deletion test
to `$response`, updating the subsequent status assertion to use the new name and
matching the sibling test’s naming convention.
In `@tests/Feature/Leads/LeadsTest.php`:
- Around line 63-82: Remove the earlier client and lead creation block in
setUp(), including the local $client and the Lead::factory() assignment tied to
$this->authorizedUser. Keep the later $this->lead creation and all unrelated
user setup unchanged.
- Around line 437-450: In it_deletes_lead_when_user_has_permission, explicitly
re-authenticate with the assigned userWithPermission after setting $this->user
by calling actingAs before issuing the delete request. Preserve the existing
permission setup and assertions while ensuring the request runs under
userWithPermission rather than the setUp() user.
In `@tests/Feature/Offers/AppointmentsTest.php`:
- Around line 99-111: Remove the redundant test
it_verifies_appointments_controller_does_not_have_create_request_dependency,
since CreateAppointmentCalendarRequest usage is covered by
it_creates_appointment_calendar_request_class_no_longer_used_by_controller and
store absence is covered by
it_verifies_appointments_controller_does_not_have_store_method. Preserve the
dedicated store-method absence test.
In `@tests/Feature/Projects/ProjectAuthorizationTest.php`:
- Around line 51-73: Remove the duplicate test method
it_rejects_project_assignment_update_when_user_lacks_permission from
ProjectAuthorizationTest, retaining the consolidated coverage in
ProjectAssignmentAuthorizationTest.php and ProjectSecurityTest.php.
In `@tests/Feature/Projects/ProjectSecurityTest.php`:
- Around line 183-206: Remove the duplicate unauthorized-assignee update test
method it_unauthorized_user_cannot_update_assign from ProjectSecurityTest,
retaining the consolidated coverage in ProjectAssignmentAuthorizationTest.php
and ProjectAuthorizationTest.php.
- Around line 165-181: Remove the duplicate authorized-assignment scenario from
it_authorized_user_can_update_assign in ProjectSecurityTest, keeping the
canonical coverage in ProjectAssignmentAuthorizationTest and
ProjectAuthorizationTest.
In `@tests/Feature/Roles/RoleTest.php`:
- Around line 102-108: Update the flash message assertion in the relevant
RoleTest case to reuse the translated text from RedirectIfNotAdmin via __('Only
Allowed for admins'), matching the existing assertion near line 42 instead of
the differently cased literal.
In `@tests/Feature/Url/UrlGenerationEdgeCasesTest.php`:
- Around line 236-246: Move the `$jsUrl` construction from the Arrange section
into the Act section of `it_generates_javascript_url_matching_php_url_helper()`,
leaving setup values such as `setAppUrl()`, `$phpUrl`, and `$jsBaseUrl` in
Arrange and preserving the existing assertion flow.
In `@tests/Unit/Absences/AbsenceServiceTest.php`:
- Around line 34-47: Rename the test method
it_delete_absence_deletes_absence_record to it_deletes_an_absence_record,
leaving its implementation and assertions unchanged.
In `@tests/Unit/Invoices/DueAtTest.php`:
- Around line 85-98: Remove the pre-condition assertion from the Arrange block
in tests/Unit/Invoices/DueAtTest.php lines 85-98, either dropping it or
replacing it with an explicit precondition comment before mutating
secondInvoice; in tests/Unit/Invoices/GenerateInvoiceStatusTest.php lines 76-88,
likewise remove or comment the assertion before createStatus(). Preserve the
intended Act and Assert phases in both tests.
In `@tests/Unit/Models/ActivityModelRelationshipsTest.php`:
- Around line 168-183: Remove
it_verifies_all_activity_relationship_methods_exist because its method_exists
assertions are tautological and duplicate coverage from the preceding
relationship tests. Do not replace it unless adding coverage for a genuinely
untested Activity relationship behavior.
In `@tests/Unit/Models/PaymentModelTest.php`:
- Around line 125-135: Restore explicit Arrange/Act/Assert phases in both tests:
in tests/Unit/Models/PaymentModelTest.php lines 125-135, assign the
method_exists result in the Act phase and assert that result in Assert; in
tests/Unit/Models/SettingCacheTest.php lines 49-58, retrieve
cache.serializable_classes during Act and keep its validations in Assert.
In `@tests/Unit/Offers/OffersStatusEnumTest.php`:
- Around line 76-97: Rename the two test methods to sentence-style names that
clearly describe their assertions: the test verifying fromStatus returns an
OfferStatus instance and the test verifying the lost status returns “lost.” Keep
the test bodies and behavior unchanged.
In `@tests/Unit/Payments/PaymentServiceTest.php`:
- Around line 118-135: Update the exception test around
PaymentServiceTest::it_throws_exception_for_invalid_payment_source so
expectException() and addPayment() are grouped in an explicit /* Act & Assert */
section, removing the empty /* Assert */ marker. Apply the same
Arrange/Act/Assert adjustment to the other exception test in this diff while
preserving its expected exception behavior.
In `@tests/Unit/Payments/PaymentSourceEnumTest.php`:
- Around line 52-64: Replace the tautological property_exists assertions in
it_verifies_payment_source_contains_both_display_and_source_value with
assertions that validate the created PaymentSource instance’s source and
displayValue values against the expected values from $this->paymentSource. Keep
the test focused on the result of PaymentSource::fromSource.
In `@tests/Unit/Services/Storage/DropboxTest.php`:
- Around line 113-136: The test’s AAA labels are reversed around the exception
assertions. In the upload exception test, move the `/* Act */` label before the
`try` block and place `/* Assert */` immediately before the `expectException`
and `expectExceptionMessage` calls, matching
`it_throws_exception_when_integration_not_configured`.
In `@tests/Unit/ViewComposers/ViewComposerNullSafetyTest.php`:
- Around line 47-95: Remove the trailing empty /* Assert */ markers from
it_handles_missing_task_in_view_data, it_handles_missing_lead_in_view_data, and
it_handles_missing_invoice_in_view_data, leaving the descriptive assertion
comments and assertions as the single Assert sections consistent with sibling
tests.
---
Outside diff comments:
In `@tests/Feature/Controllers/Search/SearchControllerSecurityTest.php`:
- Around line 38-167: Rename the affected it_search... test methods in
SearchControllerSecurityTest so each reads as a grammatical sentence, following
the pattern it_returns_results_for_a_valid_client_search. Apply this
consistently to the valid-type, invalid-type, case-insensitivity, and
injection-related tests without changing their behavior.
In `@tests/Feature/Integrations/IntegrationsTest.php`:
- Around line 18-41: Add a non-admin test alongside it/around it for the
integration store flow, using the same payload as
it_persists_only_validated_fields_and_returns_ok_when_storing. Authenticate as a
non-admin, post to integrations.store, assert the request is denied, and verify
no integration record was persisted.
In `@tests/Feature/Payments/PaymentsTest.php`:
- Around line 84-101: Update the payment decimal-separator and negative-amount
tests, including it_can_add_payment_with_decimals_dot_separator and the cases
around the referenced methods, to assert the persisted payment amount value
after refreshing the invoice. Follow the existing assertion pattern in
it_can_add_payment_with_minus_amount and verify each input’s expected parsed
amount, while retaining the current status and non-empty payment assertions.
In `@tests/Feature/Projects/ProjectAssignmentAuthorizationTest.php`:
- Around line 63-88: Remove the duplicate test method
it_authorized_user_can_reassign_project from ProjectAssignmentAuthorizationTest,
keeping the existing authorized reassignment coverage in
ProjectAuthorizationTest and ProjectSecurityTest.
- Around line 90-114: Remove the duplicate test method
it_unauthorized_user_cannot_reassign_project from
ProjectAssignmentAuthorizationTest, retaining the consolidated unauthorized
reassignment coverage in ProjectAuthorizationTest.php and
ProjectSecurityTest.php.
In `@tests/Feature/Projects/ProjectAuthorizationTest.php`:
- Around line 34-49: Remove the duplicate test method
it_updates_project_assignment_when_user_has_permission from
ProjectAuthorizationTest, retaining the equivalent coverage in
ProjectAssignmentAuthorizationTest and ProjectSecurityTest.
In `@tests/Unit/Projects/ProjectServiceTest.php`:
- Around line 116-136: Update it_returns_null_when_client_external_id_missing to
omit client_external_id from the create payload and assert the
InvalidArgumentException thrown by ProjectService::create. Keep the existing
non-empty “missing” value and null assertion in
it_returns_null_when_client_not_found, preserving separate coverage for both
branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8fe9be92-22d6-4fdd-a55e-65bbd9e8c844
⛔ Files ignored due to path filters (38)
CHANGELOG.mdis excluded by!*.md,!**/*.mdand included by noneREADME.mdis excluded by!*.md,!**/*.mdand included by noneplaywright.config.tsis excluded by none and included by nonetests/e2e/absence/absence.spec.jsis excluded by none and included by nonetests/e2e/appointments/appointments.spec.jsis excluded by none and included by nonetests/e2e/auth/auth.spec.jsis excluded by none and included by nonetests/e2e/clients/clients.spec.jsis excluded by none and included by nonetests/e2e/clients/create_button.spec.jsis excluded by none and included by nonetests/e2e/departments/departments.spec.jsis excluded by none and included by nonetests/e2e/documents/documents.spec.jsis excluded by none and included by nonetests/e2e/helpers/coverage-fixtures.tsis excluded by none and included by nonetests/e2e/helpers/plain-e2e.jsis excluded by none and included by nonetests/e2e/helpers/route-cases.tsis excluded by none and included by nonetests/e2e/helpers/route-paths.tsis excluded by none and included by nonetests/e2e/helpers/session-context.tsis excluded by none and included by nonetests/e2e/helpers/user-auth.tsis excluded by none and included by nonetests/e2e/journeys/journeys.spec.jsis excluded by none and included by nonetests/e2e/journeys/route-exposure.spec.jsis excluded by none and included by nonetests/e2e/leads/create_button.spec.jsis excluded by none and included by nonetests/e2e/leads/leads.spec.jsis excluded by none and included by nonetests/e2e/notifications/notifications.spec.jsis excluded by none and included by nonetests/e2e/products/products.spec.jsis excluded by none and included by nonetests/e2e/projects/create_button.spec.jsis excluded by none and included by nonetests/e2e/projects/projects.spec.jsis excluded by none and included by nonetests/e2e/roles/roles.spec.jsis excluded by none and included by nonetests/e2e/search/search.spec.jsis excluded by none and included by nonetests/e2e/settings/settings.spec.jsis excluded by none and included by nonetests/e2e/setup/.gitkeepis excluded by none and included by nonetests/e2e/setup/global-setup.jsis excluded by none and included by nonetests/e2e/tasks/create_button.spec.jsis excluded by none and included by nonetests/e2e/tasks/tasks.spec.jsis excluded by none and included by nonetests/e2e/users/create_button.spec.jsis excluded by none and included by nonetests/e2e/users/users.spec.jsis excluded by none and included by nonetests/helpers/admin-auth.tsis excluded by none and included by nonetests/helpers/config.tsis excluded by none and included by nonetests/helpers/csrf.tsis excluded by none and included by nonetests/helpers/feature-domain.tsis excluded by none and included by nonetests/helpers/fixtures.tsis excluded by none and included by none
📒 Files selected for processing (126)
app/Enums/AbsenceReason.phptests/AbstractTestCase.phptests/Browser/AppointmentTest.phptests/Browser/ClientTest.phptests/Browser/LeadTest.phptests/Browser/LoginTest.phptests/Browser/ProjectTest.phptests/Browser/TaskTest.phptests/Browser/UserTest.phptests/Feature/Absenses/AbsenceControllerTest.phptests/Feature/Clients/ClientAuthorizationTest.phptests/Feature/Clients/ClientPerformanceTest.phptests/Feature/Clients/ClientsControllerTest.phptests/Feature/Commands/ClearEntrustCacheCommandTest.phptests/Feature/Commands/UpgradeCommandTest.phptests/Feature/Controllers/ClientCreatePermissionCacheTest.phptests/Feature/Controllers/CommentControllerTest.phptests/Feature/Controllers/CreateRouteAuthorizationTest.phptests/Feature/Controllers/Search/SearchControllerSecurityTest.phptests/Feature/Departments/DepartmentsControllerTest.phptests/Feature/Departments/DepartmentsTest.phptests/Feature/Documents/DocumentAccessHelperTest.phptests/Feature/Documents/DocumentAuthorizationTest.phptests/Feature/Documents/DocumentSecurityTest.phptests/Feature/Documents/DocumentUploadModalTest.phptests/Feature/Documents/DocumentsControllerAuthorizationTest.phptests/Feature/Documents/DocumentsTest.phptests/Feature/Integrations/IntegrationsTest.phptests/Feature/Invoices/AddInvoiceLineTest.phptests/Feature/Invoices/InvoiceLinesTest.phptests/Feature/Leads/DeleteLeadControllerTest.phptests/Feature/Leads/LeadAssignmentAuthorizationTest.phptests/Feature/Leads/LeadAuthorizationTest.phptests/Feature/Leads/LeadSecurityTest.phptests/Feature/Leads/LeadsControllerTest.phptests/Feature/Leads/LeadsIndexAndShowTest.phptests/Feature/Leads/LeadsTest.phptests/Feature/Offers/AppointmentSecurityTest.phptests/Feature/Offers/AppointmentsStoreRemovedTest.phptests/Feature/Offers/AppointmentsTest.phptests/Feature/Offers/OfferAuthorizationTest.phptests/Feature/Offers/OffersControllerTest.phptests/Feature/Offers/OffersTest.phptests/Feature/Payments/PaymentsControllerTest.phptests/Feature/Payments/PaymentsTest.phptests/Feature/Projects/DeleteProjectControllerTest.phptests/Feature/Projects/ProjectAssignmentAuthorizationTest.phptests/Feature/Projects/ProjectAuthorizationTest.phptests/Feature/Projects/ProjectSecurityTest.phptests/Feature/Projects/ProjectsIndexAndShowTest.phptests/Feature/Projects/ProjectsTest.phptests/Feature/Roles/RoleTest.phptests/Feature/Settings/SettingsAuthorizationTest.phptests/Feature/Settings/SettingsSecurityTest.phptests/Feature/Settings/SettingsValidationTest.phptests/Feature/Storage/StorageAdapterIsolationTest.phptests/Feature/Tasks/CreateTaskFromProjectTest.phptests/Feature/Tasks/DeleteTaskControllerTest.phptests/Feature/Tasks/TaskAssignmentAuthorizationTest.phptests/Feature/Tasks/TaskAuthorizationTest.phptests/Feature/Tasks/TaskIndexStatusDuplicatesTest.phptests/Feature/Tasks/TaskSecurityTest.phptests/Feature/Tasks/TaskStatusDuplicatesTest.phptests/Feature/Tasks/TasksTest.phptests/Feature/Url/SubdirectoryUrlGenerationTest.phptests/Feature/Url/UrlGenerationEdgeCasesTest.phptests/Feature/Users/UserAuthorizationTest.phptests/Feature/Users/UserRestoreTest.phptests/Feature/Users/UserSecurityTest.phptests/Feature/Users/UsersControllerCalendarTest.phptests/Feature/Users/UsersTest.phptests/Unit/Absences/AbsenceServiceTest.phptests/Unit/Api/ApiControllerTest.phptests/Unit/Clients/ClientServiceTest.phptests/Unit/Comments/CommentServiceTest.phptests/Unit/Deadlines/DeadlineTest.phptests/Unit/DemoEnvironment/CanNotAccessTest.phptests/Unit/Entrust/EntrustUserTraitTest.phptests/Unit/Enums/AbsenceReasonTest.phptests/Unit/Enums/CountryTest.phptests/Unit/Environment/EnvironmentConfigurationTest.phptests/Unit/Environment/LanguageFilesTest.phptests/Unit/Environment/ProjectFilesConfigurationTest.phptests/Unit/Events/ClientActionTest.phptests/Unit/Events/LeadActionTest.phptests/Unit/Events/ProjectActionTest.phptests/Unit/Events/TaskActionTest.phptests/Unit/Exceptions/HandlerTest.phptests/Unit/Integration/IntegrationRegistryIsolationTest.phptests/Unit/Integration/IntegrationServiceTest.phptests/Unit/Invoices/CanUpdateInvoiceTest.phptests/Unit/Invoices/DueAtTest.phptests/Unit/Invoices/GenerateInvoiceStatusTest.phptests/Unit/Invoices/InvoiceCalculatorTest.phptests/Unit/Invoices/InvoiceLineServiceTest.phptests/Unit/Invoices/InvoiceStatusEnumTest.phptests/Unit/Leads/LeadObserverDeleteTest.phptests/Unit/Leads/LeadServiceTest.phptests/Unit/Models/ActivityModelBootTest.phptests/Unit/Models/ActivityModelRelationshipsTest.phptests/Unit/Models/AppointmentModelBootTest.phptests/Unit/Models/ClientModelTest.phptests/Unit/Models/DocumentModelBootTest.phptests/Unit/Models/InvoiceLineModelBootTest.phptests/Unit/Models/ModelRelationshipOrganizationTest.phptests/Unit/Models/PaymentModelTest.phptests/Unit/Models/SettingCacheTest.phptests/Unit/Offers/AppointmentServiceTest.phptests/Unit/Offers/OfferServiceTest.phptests/Unit/Offers/OffersStatusEnumTest.phptests/Unit/Payments/PaymentServiceRefactoredTest.phptests/Unit/Payments/PaymentServiceTest.phptests/Unit/Payments/PaymentSourceEnumTest.phptests/Unit/Projects/ProjectObserverDeleteTest.phptests/Unit/Projects/ProjectServiceTest.phptests/Unit/Repositories/CurrencyTest.phptests/Unit/Repositories/RoleRepositoryTest.phptests/Unit/Roles/RoleServiceTest.phptests/Unit/Services/Storage/Authentication/DropboxAuthenticatorTest.phptests/Unit/Services/Storage/DropboxTest.phptests/Unit/Tasks/TaskObserverDeleteTest.phptests/Unit/Tasks/TaskServiceTest.phptests/Unit/User/GetAttributesTest.phptests/Unit/User/UserRoleTest.phptests/Unit/User/UserServiceTest.phptests/Unit/ViewComposers/ViewComposerNullSafetyTest.php
💤 Files with no reviewable changes (15)
- tests/Feature/Payments/PaymentsControllerTest.php
- tests/Feature/Departments/DepartmentsControllerTest.php
- tests/Feature/Documents/DocumentSecurityTest.php
- tests/Feature/Leads/LeadsControllerTest.php
- tests/Feature/Leads/DeleteLeadControllerTest.php
- tests/Feature/Offers/OffersControllerTest.php
- tests/Feature/Offers/AppointmentsStoreRemovedTest.php
- tests/Feature/Offers/AppointmentSecurityTest.php
- tests/Feature/Leads/LeadAssignmentAuthorizationTest.php
- tests/Feature/Leads/LeadSecurityTest.php
- tests/Feature/Documents/DocumentAccessHelperTest.php
- tests/Feature/Offers/OfferAuthorizationTest.php
- tests/Feature/Documents/DocumentAuthorizationTest.php
- tests/Feature/Leads/LeadAuthorizationTest.php
- tests/Feature/Documents/DocumentsControllerAuthorizationTest.php
| $usesRefreshDatabase = in_array( | ||
| \Illuminate\Foundation\Testing\RefreshDatabase::class, | ||
| array_keys((function () { | ||
| return class_uses_recursive($this); | ||
| })->call($this)) | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Drop the unnecessary closure indirection.
class_uses_recursive($this) already works directly inside setUp() — it's a plain global helper, not a method needing rebinding. The immediately-invoked closure with ->call($this) adds no behavior, just noise.
♻️ Simplify
- $usesRefreshDatabase = in_array(
- \Illuminate\Foundation\Testing\RefreshDatabase::class,
- array_keys((function () {
- return class_uses_recursive($this);
- })->call($this))
- );
+ $usesRefreshDatabase = in_array(
+ \Illuminate\Foundation\Testing\RefreshDatabase::class,
+ class_uses_recursive($this)
+ );📝 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.
| $usesRefreshDatabase = in_array( | |
| \Illuminate\Foundation\Testing\RefreshDatabase::class, | |
| array_keys((function () { | |
| return class_uses_recursive($this); | |
| })->call($this)) | |
| ); | |
| $usesRefreshDatabase = in_array( | |
| \Illuminate\Foundation\Testing\RefreshDatabase::class, | |
| class_uses_recursive($this) | |
| ); |
🤖 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 `@tests/AbstractTestCase.php` around lines 35 - 40, In setUp(), simplify the
$usesRefreshDatabase check by calling class_uses_recursive($this) directly
inside array_keys, removing the immediately invoked closure and its
->call($this) rebinding while preserving the existing RefreshDatabase detection.
| if ( ! $usesRefreshDatabase && ! Schema::hasTable('users')) { | ||
| Artisan::call('migrate:fresh', ['--seed' => true]); | ||
| static::$schemaIsUpToDate = true; | ||
| } | ||
|
|
||
| // Use a guaranteed unique email for the test user | ||
| $uniqueEmail = 'testuser_' . uniqid('', true) . '@example.org'; | ||
| if ( ! Schema::hasTable('users')) { | ||
| throw new RuntimeException( | ||
| 'The `users` table does not exist after test database setup. ' | ||
| . ($usesRefreshDatabase | ||
| ? 'This test uses RefreshDatabase, which should have migrated it - check that trait\'s setup.' | ||
| : 'migrate:fresh --seed just ran and should have created it - check the migration/seeder output.') | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find RefreshDatabase-based tests that reference seeded Status/reference data without recreating it via factory/firstOrCreate.
rg -nP --type=php -l 'use RefreshDatabase;' tests | xargs -I{} sh -c 'echo "== {} =="; rg -n "typeOfLead|typeOfTask|typeOfProject|Status::query" {}'
echo "---- Confirm RefreshDatabase default seeding behavior ----"
fd -i "RefreshDatabase.php" -p vendor/laravel/framework --exec cat -n {}Repository: Bottelet/DaybydayCRM
Length of output: 6006
🏁 Script executed:
#!/bin/bash
set -u
echo "== matching RefreshDatabase tests with relevant seed-like references =="
rg --type=php -l 'use RefreshDatabase;' tests | while read -r f; do
v="$(rg -n 'typeOf(Lead|Task|Project)|Status::query|RefreshDatabase\(' "$f" || true)"
if [ -n "$v" ]; then
echo "== $f =="
printf '%s\n' "$v"
fi
done
echo
echo "== AbstractTestCase relevant section =="
cat -n tests/AbstractTestCase.php | sed -n '1,90p'
echo
echo "== seeded seeder data around status factories/seeders =="
fd -i 'Status' database --exec sh -c 'echo "== $1 =="; cat -n "$1" | sed -n "1,220p"' sh {}
fd -i '*Seeder*.php' database --exec sh -c 'echo "== $1 =="; rg -n "Status::|typeOf|Hello|roles|permissions|Status" "$1" || true' sh {}
echo
echo "== tests containing Status::typeOf / fresh seeded-like row expectations =="
rg --type=php -n 'Status::typeOf|typeOf(Lead|Task|Project)|\'Hello\'|\'Pending\'|\'Completed\'|\'Cancelled\'|\'Active\'' tests databaseRepository: Bottelet/DaybydayCRM
Length of output: 10628
🏁 Script executed:
#!/bin/bash
set -u
echo "== composer frameworks =="
for f in composer.json composer.lock; do
[ -f "$f" ] && { echo "-- $f --"; rg -n '"laravel/framework"|"refresh-database"|refresh-database|10\.|11\.|12\.' "$f" || true; }
done
echo
echo "== AbstractTestCase setUp with test status =="
sed -n '24,53p' tests/AbstractTestCase.php | nl -ba -v24 | sed 's/^//'
echo
echo "== RefreshDatabase-based test referencing seeded Status rows =="
echo "== tests/Unit/Status/TypeOfStatusTest.php =="
sed -n '1,80p' tests/Unit/Status/TypeOfStatusTest.php | nl -ba -v1
echo
echo "== RefreshDatabase trait availability/version if present =="
if [ -f vendor/laravel/framework/src/Illuminate/Foundation/Testing/RefreshDatabase.php ]; then
rg -n "seeding|seeder|Migrates|RefreshDatabase" vendor/laravel/framework/src/Illuminate/Foundation/Testing/RefreshDatabase.php || true
else
echo "vendor/laravel/framework not present"
fiRepository: Bottelet/DaybydayCRM
Length of output: 9134
🌐 Web query:
Laravel 12 RefreshDatabase trait seeds database by default
💡 Result:
No, the RefreshDatabase trait in Laravel 12 does not seed the database by default [1][2]. By default, using the RefreshDatabase trait simply ensures your database is migrated (if necessary) and wrapped in a database transaction to reset the state between tests, but it does not execute any seeders [1][2]. If you wish to seed your database while using this trait, you have two primary options: 1. Manual Seeding: You can invoke the $this->seed method directly within your test methods [1][2]. This allows you to call the default DatabaseSeeder or specify a particular seeder class [1][2]. 2. Automatic Seeding: You can instruct Laravel to automatically seed the database for tests using the RefreshDatabase trait by defining a $seed property on your base test class [1][2]. When you set protected $seed = true; on your test class, Laravel will execute the Database\Seeders\DatabaseSeeder before each test [1][2]. You can also specify a custom seeder class for a specific test class by defining a $seeder property [1][2].
Citations:
- 1: https://github.com/laravel/docs/blob/12.x/database-testing.md
- 2: https://readouble.com/laravel/12.x/en/database-testing.html
Seed ReferenceDatabase-built test tables for RefreshDatabase tests.
tests/Unit/Status/TypeOfStatusTest.php uses RefreshDatabase and reads the seeder-backed Status::typeOf*() rows, but Laravel 12’s RefreshDatabase only migrates/truncates and does not run seeders by default. Add test-level seeding ($this->seed() / $this->seeder(...)), make the base AbstractTestCase seed when RefreshDatabase is active, or use factories/firstOrCreate() so seeded reference data is not silently missing.
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 42-42: Avoid using static access to class '\Illuminate\Support\Facades\Schema' in method 'setUp'. (undefined)
(StaticAccess)
[error] 43-43: Avoid using static access to class '\Illuminate\Support\Facades\Artisan' in method 'setUp'. (undefined)
(StaticAccess)
[error] 46-46: Avoid using static access to class '\Illuminate\Support\Facades\Schema' in method 'setUp'. (undefined)
(StaticAccess)
🤖 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 `@tests/AbstractTestCase.php` around lines 42 - 53, Update the RefreshDatabase
test setup in AbstractTestCase and/or the affected TypeOfStatusTest so
seeder-backed Status::typeOf*() reference rows are created before assertions
run. Prefer the existing test seeding mechanism, ensuring RefreshDatabase tests
invoke the appropriate seeder while preserving the current migrate:fresh path
for other tests.
| protected function getJsonRequest(string $url) | ||
| { | ||
| return $this->get($url, ['Accept' => 'application/json']); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -i "MakesHttpRequests.php" -p vendor/laravel/framework --exec grep -n -A5 "function getJson" {}
rg -n 'getJsonRequest\(' testsRepository: Bottelet/DaybydayCRM
Length of output: 870
🌐 Web query:
Laravel framework MakesHttpRequests getJson source
💡 Result:
The getJson method is a helper defined within the MakesHttpRequests trait in the Laravel framework [1][2]. This trait is typically used in test classes to facilitate HTTP requests during application testing [3][4]. The source code for the getJson method, located in src/Illuminate/Foundation/Testing/Concerns/MakesHttpRequests.php, acts as a wrapper for the more generic json method [1]. Its implementation is as follows: public function getJson($uri, array $headers = [], $options = 0) { return $this->json('GET', $uri, [], $headers, $options); } By calling the json method with the 'GET' verb, getJson automatically sets the necessary headers (such as Accept: application/json) to ensure the application treats the request as a JSON API call [1]. The method returns an instance of Illuminate\Testing\TestResponse, which provides various fluent assertions for inspecting the JSON response [3][4].
Citations:
- 1: https://github.com/laravel/framework/blob/master/src/Illuminate/Foundation/Testing/Concerns/MakesHttpRequests.php
- 2: https://api.laravel.com/docs/13.x/Illuminate/Foundation/Testing/Concerns/MakesHttpRequests.html
- 3: https://github.com/laravel/docs/blob/9c42f8596c5ceadc3496f20044b7a38f620419a2/http-tests.md
- 4: https://laravel.com/docs/13.x/http-tests
🌐 Web query:
Laravel json method Accept Content-Type application/json
💡 Result:
In Laravel, the relationship between JSON methods and the Accept header varies depending on whether you are working with testing utilities or application responses. Testing Methods When using Laravel's built-in testing helpers such as json, getJson, postJson, etc., the framework automatically sets the Accept header to application/json [1]. For instance, calling $this->postJson(...) will send a request with the following headers by default [1]: - Content-Type: application/json - Accept: application/json Application Responses When returning a JSON response in your application code, using the response->json method automatically sets the Content-Type header of that response to application/json [2]. Handling Incoming Requests Laravel determines if an incoming request expects a JSON response by checking the Accept header [3]. If you need to force your application to treat all requests as JSON (for example, to ensure API error messages are returned as JSON instead of HTML/redirects when the client does not send an Accept header), you can use middleware [4][5][6]. A common approach is to create a middleware that forces the header [4][5]: public function handle(Request $request, Closure $next) { $request->headers->set('Accept', 'application/json'); return $next($request); } Additionally, in newer versions of Laravel (e.g., 13.x), you can configure the application to prefer JSON responses via the application builder [7]. This allows you to globally treat broad Accept headers (like /) as a preference for JSON without needing custom middleware for every route [7].
Citations:
- 1: https://github.com/laravel/framework/blob/11.x/src/Illuminate/Foundation/Testing/Concerns/MakesHttpRequests.php
- 2: https://laravel.com/docs/13.x/responses
- 3: https://github.com/laravel/framework/blob/12.x/src/Illuminate/Http/Concerns/InteractsWithContentTypes.php
- 4: https://www.codegenes.net/blog/how-do-you-force-a-json-response-on-every-response-in-laravel/
- 5: https://laraveldaily.com/tip/force-json-response-for-api-requests
- 6: https://stackoverflow.com/questions/36366727/how-do-you-force-a-json-response-on-every-response-in-laravel
- 7: [13.x] Add prefersJsonResponses() to the application builder laravel/framework#59753
Use Laravel’s built-in getJson() instead of a test-only wrapper.
$this->getJson() issues a GET JSON request with the needed Accept: application/json headers, so getJsonRequest() is unnecessary duplication.
🤖 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 `@tests/AbstractTestCase.php` around lines 161 - 164, Remove the redundant
getJsonRequest() wrapper from AbstractTestCase and update its callers to use
Laravel’s built-in getJson() method directly, preserving the existing URL and
request behavior.
| $browser->assertSee($lead->description) | ||
| ->assertsee(date(carbonFullDateWithText(), strtotime($lead->created_at))) | ||
| ->assertSee(date(carbonFullDateWithText(), strtotime($lead->deadline))) | ||
| ->assertSee($lead->status->title); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
assertsee typo copy-pasted across two Dusk tests. Both call ->assertsee(...) instead of ->assertSee(...); PHP's case-insensitive method resolution makes it work, but it should be corrected for consistency.
tests/Browser/LeadTest.php#L61-L64: change->assertsee(date(carbonFullDateWithText(), strtotime($lead->created_at)))to->assertSee(...).tests/Browser/TaskTest.php#L62-L66: change->assertsee(date(carbonDateWithText(), strtotime($task->created_at)))to->assertSee(...).
📍 Affects 2 files
tests/Browser/LeadTest.php#L61-L64(this comment)tests/Browser/TaskTest.php#L62-L66
🤖 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 `@tests/Browser/LeadTest.php` around lines 61 - 64, Correct the method casing
from assertsee to assertSee in the LeadTest created_at assertion and the
corresponding TaskTest created_at assertion, preserving their existing arguments
and assertion behavior.
| #[Test] | ||
| public function it_i_can_create_a_new_lead() | ||
| public function it_can_go_to_create_new_client_in_dropdown_if_no_clients_exists_from_lead() | ||
| { | ||
| /* Arrange */ | ||
| $client = Client::factory()->create(); | ||
| $contact = $client->primary_contact; | ||
| $user = User::factory()->create(); | ||
| Client::query()->forceDelete(); | ||
|
|
||
| /* Act & Assert */ | ||
| $this->browse(function (Browser $browser) use ($user, $client, $contact) { | ||
| $user = User::factory()->create(); | ||
|
|
||
| /* Act */ | ||
| $this->browse(function (Browser $browser) use ($user) { | ||
| $browser->loginAs(User::whereEmail('admin@admin.com')->first()) | ||
| ->visit('/leads/create') | ||
| ->type('title', 'This is a test lead title') | ||
| ->type('.note-editable', 'This is a short comment about the lead') | ||
| ->visit('/projects/create') | ||
| ->select('user_assigned_id', $user->id) | ||
| ->select('client_external_id', $client->external_id) | ||
| ->press('Create lead') | ||
| ->assertSee($user->name) | ||
| ->assertSee($contact->name) | ||
| ->assertSee('This is a test lead title'); | ||
| ->select('client_external_id', 'new_client'); | ||
|
|
||
| /* Assert */ | ||
| $browser->assertPathIs('/clients/create'); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wrong route tested — this exercises the project flow, not the lead flow.
it_can_go_to_create_new_client_in_dropdown_if_no_clients_exists_from_lead visits /projects/create (line 163), identical to ProjectTest::it_can_go_to_create_new_client_in_dropdown_if_no_clients_exists_from_project. This looks like a copy/paste that missed updating the target route — the "no clients exist" flow originating from lead creation (/leads/create) is never actually tested, while /projects/create gets duplicate coverage.
🐛 Proposed fix
$this->browse(function (Browser $browser) use ($user) {
$browser->loginAs(User::whereEmail('admin@admin.com')->first())
- ->visit('/projects/create')
+ ->visit('/leads/create')
->select('user_assigned_id', $user->id)
->select('client_external_id', 'new_client');📝 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.
| #[Test] | |
| public function it_i_can_create_a_new_lead() | |
| public function it_can_go_to_create_new_client_in_dropdown_if_no_clients_exists_from_lead() | |
| { | |
| /* Arrange */ | |
| $client = Client::factory()->create(); | |
| $contact = $client->primary_contact; | |
| $user = User::factory()->create(); | |
| Client::query()->forceDelete(); | |
| /* Act & Assert */ | |
| $this->browse(function (Browser $browser) use ($user, $client, $contact) { | |
| $user = User::factory()->create(); | |
| /* Act */ | |
| $this->browse(function (Browser $browser) use ($user) { | |
| $browser->loginAs(User::whereEmail('admin@admin.com')->first()) | |
| ->visit('/leads/create') | |
| ->type('title', 'This is a test lead title') | |
| ->type('.note-editable', 'This is a short comment about the lead') | |
| ->visit('/projects/create') | |
| ->select('user_assigned_id', $user->id) | |
| ->select('client_external_id', $client->external_id) | |
| ->press('Create lead') | |
| ->assertSee($user->name) | |
| ->assertSee($contact->name) | |
| ->assertSee('This is a test lead title'); | |
| ->select('client_external_id', 'new_client'); | |
| /* Assert */ | |
| $browser->assertPathIs('/clients/create'); | |
| }); | |
| } | |
| #[Test] | |
| public function it_can_go_to_create_new_client_in_dropdown_if_no_clients_exists_from_lead() | |
| { | |
| /* Arrange */ | |
| Client::query()->forceDelete(); | |
| $user = User::factory()->create(); | |
| /* Act */ | |
| $this->browse(function (Browser $browser) use ($user) { | |
| $browser->loginAs(User::whereEmail('admin@admin.com')->first()) | |
| ->visit('/leads/create') | |
| ->select('user_assigned_id', $user->id) | |
| ->select('client_external_id', 'new_client'); | |
| /* Assert */ | |
| $browser->assertPathIs('/clients/create'); | |
| }); | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 153-170: The method it_can_go_to_create_new_client_in_dropdown_if_no_clients_exists_from_lead is not named in camelCase. (undefined)
(CamelCaseMethodName)
🪛 PHPStan (2.2.5)
[warning] 156-156: Call to an undefined static method App\Models\Client::query().
(staticMethod.notFound)
[warning] 158-158: Call to an undefined static method App\Models\User::factory().
(staticMethod.notFound)
[warning] 162-162: Call to an undefined static method App\Models\User::whereEmail().
(staticMethod.notFound)
🤖 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 `@tests/Browser/LeadTest.php` around lines 152 - 170, Update the browser test
method it_can_go_to_create_new_client_in_dropdown_if_no_clients_exists_from_lead
to visit /leads/create instead of /projects/create, preserving the existing
client cleanup, user selection, dropdown interaction, and expected
/clients/create assertion.
| public function it_getting_source_returns_instance_of_offer_status() | ||
| { | ||
| /* Arrange */ | ||
|
|
||
| /* Act */ | ||
| $status = OfferStatus::lost()->getStatus(); | ||
| $result = OfferStatus::fromStatus($this->offerStatus); | ||
|
|
||
| /* Assert */ | ||
| $this->assertEquals('lost', $status); | ||
| $this->assertInstanceOf(OfferStatus::class, $result); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function it_gets_status_from_display_value() | ||
| public function it_source_returns_correct_source_in_instance() | ||
| { | ||
| /* Arrange */ | ||
|
|
||
| /* Act */ | ||
| $status = OfferStatus::fromDisplayValue('Won'); | ||
| $status = OfferStatus::lost()->getStatus(); | ||
|
|
||
| /* Assert */ | ||
| $this->assertEquals(OfferStatus::won()->getStatus(), $status); | ||
| $this->assertEquals('lost', $status); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use sentence-style test names.
it_getting_source_returns_instance_of_offer_status and it_source_returns_correct_source_in_instance do not read as sentences.
Proposed fix
- public function it_getting_source_returns_instance_of_offer_status()
+ public function it_returns_an_offer_status_instance_for_a_source()
...
- public function it_source_returns_correct_source_in_instance()
+ public function it_returns_the_correct_source_value()📝 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.
| public function it_getting_source_returns_instance_of_offer_status() | |
| { | |
| /* Arrange */ | |
| /* Act */ | |
| $status = OfferStatus::lost()->getStatus(); | |
| $result = OfferStatus::fromStatus($this->offerStatus); | |
| /* Assert */ | |
| $this->assertEquals('lost', $status); | |
| $this->assertInstanceOf(OfferStatus::class, $result); | |
| } | |
| #[Test] | |
| public function it_gets_status_from_display_value() | |
| public function it_source_returns_correct_source_in_instance() | |
| { | |
| /* Arrange */ | |
| /* Act */ | |
| $status = OfferStatus::fromDisplayValue('Won'); | |
| $status = OfferStatus::lost()->getStatus(); | |
| /* Assert */ | |
| $this->assertEquals(OfferStatus::won()->getStatus(), $status); | |
| $this->assertEquals('lost', $status); | |
| } | |
| public function it_returns_an_offer_status_instance_for_a_source() | |
| { | |
| /* Arrange */ | |
| /* Act */ | |
| $result = OfferStatus::fromStatus($this->offerStatus); | |
| /* Assert */ | |
| $this->assertInstanceOf(OfferStatus::class, $result); | |
| } | |
| #[Test] | |
| public function it_returns_the_correct_source_value() | |
| { | |
| /* Arrange */ | |
| /* Act */ | |
| $status = OfferStatus::lost()->getStatus(); | |
| /* Assert */ | |
| $this->assertEquals('lost', $status); | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 76-85: The method it_getting_source_returns_instance_of_offer_status is not named in camelCase. (undefined)
(CamelCaseMethodName)
[error] 81-81: Avoid using static access to class '\App\Enums\OfferStatus' in method 'it_getting_source_returns_instance_of_offer_status'. (undefined)
(StaticAccess)
[error] 88-97: The method it_source_returns_correct_source_in_instance is not named in camelCase. (undefined)
(CamelCaseMethodName)
🤖 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 `@tests/Unit/Offers/OffersStatusEnumTest.php` around lines 76 - 97, Rename the
two test methods to sentence-style names that clearly describe their assertions:
the test verifying fromStatus returns an OfferStatus instance and the test
verifying the lost status returns “lost.” Keep the test bodies and behavior
unchanged.
Source: Path instructions
|
|
||
| /* Act */ | ||
| $this->service->addPayment($invoice, 100, '2024-01-15', 'cash'); | ||
|
|
||
| /* Assert */ | ||
| } | ||
|
|
||
| #[Test] | ||
| public function it_throws_exception_for_invalid_payment_source() | ||
| { | ||
| /* Arrange */ | ||
| $invoice = Invoice::factory()->create(['sent_at' => now()]); | ||
|
|
||
| /* Act & Assert */ | ||
| $this->expectException(InvalidArgumentException::class); | ||
|
|
||
| /* Act */ | ||
| $this->service->addPayment($invoice, 100, '2024-01-15', 'invalid_source'); | ||
|
|
||
| /* Assert */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep exception expectations in an explicit Act/Assert section.
Both exception tests leave /* Assert */ empty while expectException() is registered before /* Act */. Use a single /* Act & Assert */ block for the expectation and invocation, or otherwise make the assertion section meaningful.
As per path instructions: tests under tests/**/*.php must follow Arrange/Act/Assert structure.
🧰 Tools
🪛 PHPStan (2.2.5)
[warning] 129-129: Call to an undefined static method App\Models\Invoice::factory().
(staticMethod.notFound)
🤖 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 `@tests/Unit/Payments/PaymentServiceTest.php` around lines 118 - 135, Update
the exception test around
PaymentServiceTest::it_throws_exception_for_invalid_payment_source so
expectException() and addPayment() are grouped in an explicit /* Act & Assert */
section, removing the empty /* Assert */ marker. Apply the same
Arrange/Act/Assert adjustment to the other exception test in this diff while
preserving its expected exception behavior.
Source: Path instructions
| #[Test] | ||
| #[Group('junie_repaired')] | ||
| public function it_gets_display_value_from_source() | ||
| public function it_verifies_payment_source_contains_both_display_and_source_value() | ||
| { | ||
| /* Arrange */ | ||
|
|
||
| /* Act */ | ||
| $displayValue = PaymentSource::fromSource($this->paymentSource)->getDisplayValue(); | ||
| $paymentSource = PaymentSource::fromSource($this->paymentSource); | ||
|
|
||
| /* Assert */ | ||
| $this->assertEquals('Bank', $displayValue); | ||
| $this->assertTrue(property_exists($paymentSource, 'source')); | ||
| $this->assertTrue(property_exists($paymentSource, 'displayValue')); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tautological property_exists assertions provide no runtime coverage.
PHPStan confirms both property_exists checks always evaluate to true (compile-time fact from the class declaration), so this test doesn't actually verify anything about the created instance's values.
✏️ Suggested fix
/* Act */
$paymentSource = PaymentSource::fromSource($this->paymentSource);
/* Assert */
- $this->assertTrue(property_exists($paymentSource, 'source'));
- $this->assertTrue(property_exists($paymentSource, 'displayValue'));
+ $this->assertSame($this->paymentSource, $paymentSource->getSource());
+ $this->assertSame('Bank', $paymentSource->getDisplayValue());🧰 Tools
🪛 PHPMD (2.15.0)
[error] 54-64: The method it_verifies_payment_source_contains_both_display_and_source_value is not named in camelCase. (undefined)
(CamelCaseMethodName)
[error] 59-59: Avoid using static access to class '\App\Enums\PaymentSource' in method 'it_verifies_payment_source_contains_both_display_and_source_value'. (undefined)
(StaticAccess)
🪛 PHPStan (2.2.5)
[warning] 62-62: Call to function property_exists() with App\Enums\PaymentSource and 'source' will always evaluate to true.
(function.alreadyNarrowedType)
[warning] 63-63: Call to function property_exists() with App\Enums\PaymentSource and 'displayValue' will always evaluate to true.
(function.alreadyNarrowedType)
🤖 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 `@tests/Unit/Payments/PaymentSourceEnumTest.php` around lines 52 - 64, Replace
the tautological property_exists assertions in
it_verifies_payment_source_contains_both_display_and_source_value with
assertions that validate the created PaymentSource instance’s source and
displayValue values against the expected values from $this->paymentSource. Keep
the test focused on the result of PaymentSource::fromSource.
Source: Linters/SAST tools
| $folder = 'client-123'; | ||
| $filePath = '/path/to/test.pdf'; | ||
| $filePath = sys_get_temp_dir() . '/test.pdf'; | ||
|
|
||
| file_put_contents($filePath, 'test file content'); | ||
|
|
||
| $this->mockClient->expects($this->once()) | ||
| ->method('upload') | ||
| $this->mockClient->method('upload') | ||
| ->willThrowException(new Exception('Upload failed')); | ||
|
|
||
| $this->app->instance('Spatie\Dropbox\Client', $this->mockClient); | ||
|
|
||
| $dropbox = new Dropbox(); | ||
| $dropbox = new Dropbox($this->mockClient); | ||
|
|
||
| /* Assert */ | ||
| $this->expectException(RuntimeException::class); | ||
| $this->expectExceptionMessage('Failed to upload file to Dropbox'); | ||
|
|
||
| /* Act */ | ||
| $dropbox->upload($folder, $filename, $filePath); | ||
|
|
||
| // Clean up | ||
| unlink($filePath); | ||
| try { | ||
| $dropbox->upload($folder, $filename, $filePath); | ||
| } finally { | ||
| // Clean up even when exception is thrown | ||
| if (file_exists($filePath)) { | ||
| unlink($filePath); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
AAA labels reversed here too — same pattern already fixed in it_throws_exception_when_integration_not_configured below.
expectException/expectExceptionMessage are placed under /* Assert */ (122-125) ahead of the actual /* Act */ call (126-129). Mirror the fix already applied to it_throws_exception_when_integration_not_configured (223-234) in this same file.
✏️ Suggested fix
file_put_contents($filePath, 'test file content');
$this->mockClient->method('upload')
->willThrowException(new Exception('Upload failed'));
$dropbox = new Dropbox($this->mockClient);
-
- /* Assert */
$this->expectException(RuntimeException::class);
$this->expectExceptionMessage('Failed to upload file to Dropbox');
/* Act */
try {
$dropbox->upload($folder, $filename, $filePath);
} finally {
// Clean up even when exception is thrown
if (file_exists($filePath)) {
unlink($filePath);
}
}
+
+ /* Assert */
}🧰 Tools
🪛 ast-grep (0.44.1)
[info] 132-132: Avoid unsafe call to unlink
Context: unlink($filePath)
Note: [CWE-73] External Control of File Name or Path.
(avoid-unlink)
🪛 PHPMD (2.15.0)
[error] 109-136: The method it_handles_upload_errors_gracefully is not named in camelCase. (undefined)
(CamelCaseMethodName)
🤖 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 `@tests/Unit/Services/Storage/DropboxTest.php` around lines 113 - 136, The
test’s AAA labels are reversed around the exception assertions. In the upload
exception test, move the `/* Act */` label before the `try` block and place `/*
Assert */` immediately before the `expectException` and `expectExceptionMessage`
calls, matching `it_throws_exception_when_integration_not_configured`.
Source: Path instructions
| #[Test] | ||
| public function it_handles_missing_task_in_view_data() | ||
| { | ||
| /* Arrange */ | ||
| $view = new FakeView([]); | ||
|
|
||
| /* Act */ | ||
| (new TaskHeaderComposer())->compose($view); | ||
|
|
||
| /* Assert all three keys are pushed, all null */ | ||
| $this->assertNull($view->getShared('contact')); | ||
| $this->assertNull($view->getShared('client')); | ||
| $this->assertNull($view->getShared('contact_info')); | ||
|
|
||
| /* Assert */ | ||
| } | ||
|
|
||
| #[Test] | ||
| public function it_handles_missing_lead_in_view_data() | ||
| { | ||
| /* Arrange */ | ||
| $view = new FakeView([]); | ||
|
|
||
| /* Act */ | ||
| (new LeadHeaderComposer())->compose($view); | ||
|
|
||
| /* Assert all three keys present, all null */ | ||
| $this->assertNull($view->getShared('contact')); | ||
| $this->assertNull($view->getShared('client')); | ||
| $this->assertNull($view->getShared('contact_info')); | ||
|
|
||
| /* Assert */ | ||
| } | ||
|
|
||
| #[Test] | ||
| public function it_handles_missing_invoice_in_view_data() | ||
| { | ||
| /* Arrange */ | ||
| $view = new FakeView([]); | ||
|
|
||
| /* Act */ | ||
| (new InvoiceHeaderComposer())->compose($view); | ||
|
|
||
| /* Assert both keys present and null */ | ||
| $this->assertNull($view->getShared('client')); | ||
| $this->assertNull($view->getShared('contact_info')); | ||
|
|
||
| /* Assert */ | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Dangling, empty /* Assert */ markers after the real assertions.
In these three new/reformatted tests, the actual assertions sit under a descriptive comment (e.g. /* Assert all three keys are pushed, all null */), and the canonical /* Assert */ marker is left empty at the end. This is inconsistent with the AAA convention used by the unchanged sibling tests later in this same file (e.g. it_handles_lead_without_client, it_handles_invoice_without_client), which place /* Assert */ directly before their assertions with no trailing duplicate.
✏️ Suggested fix (applies to all three affected tests)
/* Act */
(new TaskHeaderComposer())->compose($view);
- /* Assert all three keys are pushed, all null */
+ /* Assert */
+ // all three keys are pushed, all null
$this->assertNull($view->getShared('contact'));
$this->assertNull($view->getShared('client'));
$this->assertNull($view->getShared('contact_info'));
-
- /* Assert */
}Also applies to: 107-115, 159-161
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 48-62: The method it_handles_missing_task_in_view_data is not named in camelCase. (undefined)
(CamelCaseMethodName)
[error] 65-79: The method it_handles_missing_lead_in_view_data is not named in camelCase. (undefined)
(CamelCaseMethodName)
[error] 82-95: The method it_handles_missing_invoice_in_view_data is not named in camelCase. (undefined)
(CamelCaseMethodName)
🪛 PHPStan (2.2.5)
[warning] 54-54: Parameter #1 $view of method App\Http\ViewComposers\TaskHeaderComposer::compose() expects Illuminate\Contracts\View\View, Tests\Support\FakeView given.
(argument.type)
[warning] 71-71: Parameter #1 $view of method App\Http\ViewComposers\LeadHeaderComposer::compose() expects Illuminate\Contracts\View\View, Tests\Support\FakeView given.
(argument.type)
[warning] 88-88: Parameter #1 $view of method App\Http\ViewComposers\InvoiceHeaderComposer::compose() expects Illuminate\Contracts\View\View, Tests\Support\FakeView given.
(argument.type)
🤖 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 `@tests/Unit/ViewComposers/ViewComposerNullSafetyTest.php` around lines 47 -
95, Remove the trailing empty /* Assert */ markers from
it_handles_missing_task_in_view_data, it_handles_missing_lead_in_view_data, and
it_handles_missing_invoice_in_view_data, leaving the descriptive assertion
comments and assertions as the single Assert sections consistent with sibling
tests.
Source: Path instructions
…al controllers
- Absence: redirect to dashboard instead of the view-gated absence
index when the acting user can create but not view absences, so the
success flow doesn't immediately bounce through a second permission
check.
- Clients: extend the client-view middleware to cover the DataTable
AJAX endpoints (anyData, taskDataTable, projectDataTable,
leadDataTable, invoiceDataTable), which were previously reachable
without permission.
- Roles: give roles.update a proper name and route it through
Route::match(['put', 'patch'], 'roles/{role}', ...) instead of an
unnamed, unguarded '/roles/update/{external_id}' route; extend the
admin-only middleware to indexData as well. Updated the blade view
and the Playwright spec to use the named route.
- Documents: extract the repeated file-size-in-MB calculation into a
helper, and return the JSON response client uploads were missing
(uploadToTask/uploadToProject already returned it; upload() fell
through with no response, which Laravel can't turn into a valid
HTTP response).
- Invoices: wrap invoice line creation in a DB transaction so a
mid-loop validation failure rolls back the whole batch instead of
leaving partial lines; use firstOrFail() for lookups that were
crashing instead of 404ing on a bad external_id.
- Offers: wrap invoice line replacement (delete + recreate) in a DB
transaction for the same reason.
- Users: guard Setting::cached() being null (no settings row yet, e.g.
fresh installs/tests) instead of crashing on ->company/->max_users.
- Migration: add the invoice_lines/offers foreign keys that were
dropped during the SQLite table-rebuild workaround.
Verified against the full suite: 988 tests, 2616 assertions, all
passing (8 new tests added alongside the fixes above).
1 participant
Summary
Test-infrastructure portion of #455, stacked on #459 (merge that one first) and split out into its own PR so it can be reviewed as "does the suite pass and does it actually test something" rather than line-by-line. Base will retarget to
developautomatically once #459 merges.164 files changed (+8,811 / -6,675): 160 are under
tests/(62 Feature, 55 Unit, 30 new Playwright e2e specs, 7 existing Dusk browser tests reorganized, 5 helpers,AbstractTestCase.php), plusplaywright.config.ts,CHANGELOG.md,README.md, and one app file (see bug fix below).Why stacked on #459 and not
developdirectly: a portion of these tests exercise app-code fixes that only exist on that branch (Setting::cached(), theCurrencyUSD-separator fix, the new task/lead/project edit routes, theRolesControllermiddleware fix, etc). Runningtests/*alone againstdevelop's current app code fails 34/1070 tests for exactly this reason.What's in here
it_-prefixed grammatically-sentenced method names, every bareassertRedirect()tightened to assert an actual target.Country,AbsenceReason) that would break on any legitimate addition; replaced with membership checks.UpdateTaskRequest,UpdateProjectRequest,AddInvoiceLine).assertOk()/assertStatus(200)) with real content, database-state, or response-header checks.assertTrue(true)placeholders withexpectNotToPerformAssertions()or a real return-value check.roles.storedenial-path gap and updated its expected assertion to match [Merge first!]: Laravel and refactor to thin Controllers (app code only) #459's middleware fix (rejection now comes fromRedirectIfNotAdmin, notStoreRoleRequest::authorize()).AbsenceReason::values()constructed itsTIME_OFF_IN_LIEUentry with the wrong reason constant, making it indistinguishable fromTIME_OFFand silently breakingfromStatus('time_off_in_lieu'). An existing test had codified the bug as expected behavior; both the code and the test are corrected.resources/lang/*.jsonfile is well-formed and a flat string map, covering the three new locales from [Merge first!]: Laravel and refactor to thin Controllers (app code only) #459 automatically with no per-locale edits needed.Test plan