Repository navigation
Consolidate shared test fixtures and cover nested deferred callbacks - #624
Conversation
Move common database models, routing controllers, enums and route files into their shared fixture locations, and extract mail, notification, queue and model-inspector helpers. Update consumers without changing runtime behavior or dropping Hypervel-specific assertions and isolation adaptations. Reuse the shared models for ignoring-touch assertions while keeping the richer relationship fixtures local. Preserve native typing, immutable dates and the existing timestamp-mutator correction. Add the upstream HTTP regression for callbacks deferred within another callback; the recursive runtime drain is already present. Port Laravel framework laravel/framework#61422 and the regression from laravel/framework#61431 against master 7068848dfe48fc3a433598e09ce798799d442a52. Validated affected test files individually, MySQL and MariaDB timestamp casts, MySQL after-commit transaction coverage, the full parallel suite, formatting and both PHPStan configurations.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: hypervel/components/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (77)
💤 Files with no reviewable changes (5)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes reorganize database, routing, mail, notification, and queue test fixtures for reuse. Integration tests update fixture imports and add type declarations. The deferred-callback tests add coverage for callbacks deferred from within another callback. ChangesDatabase integration tests
Routing integration tests
Mail, notification, and deferred-callback tests
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Other Merge Risk: ⚪ Minimal · up to The fixture moves and new regression test present no identified merge-blocking issue; merge after normal checks. 🚥 Pre-merge checks | ✅ 3 | ❌ 1 | ❓ 1❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 50 files. (21 skipped: 21 over the file limit.) Full details: Description checkExplanation The description explains the fixture consolidation and regression test, but it does not follow the required template. It omits the contribution type, explicit problem-and-change section, supporting evidence with regression results, verification commands and results, and the required submission checklist. Resolution Complete the required template sections. Select the contribution type, describe the problem and resulting behavior, identify the regression test and its before-and-after results, list each verification command with its result, and complete the before-submitting checklist.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 review |
✅ Action performedReview finished.
|
|
@cubic-dev-ai review |
@binaryfire I have started the AI code review. It will take a few minutes to complete. |
PR Summary by QodoConsolidate shared test fixtures and cover nested deferred callbacks
AI Description
Diagram
High-Level Assessment
Files changed (72)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| use Hypervel\Notifications\Notification; | ||
| use Hypervel\Queue\Attributes\DeleteWhenMissingModels; | ||
| use Hypervel\Queue\SerializesModels; | ||
| use Hypervel\Tests\Integration\Queue\DeleteNotificationWhenMissingModelTest\DeleteNotificationTestModel; |
There was a problem hiding this comment.
This shared notification depends on DeleteNotificationTestModel, which is declared inside DeleteNotificationWhenMissingModelTest.php rather than in its own autoloadable file. The current test loads both classes in one process, but a separate queue worker cannot load the model when restoring the notification. Moving the model into its own fixture would let the notification work independently.
Knowledge Base Used: Testing, debugging, and runtime inspection
There was a problem hiding this comment.
This notification is used only by DeleteNotificationWhenMissingModelTest. Its runQueueWorkerCommand() calls queue:work through the in-process Artisan test helper, after the model class has been loaded. No separate-process worker or other test consumes this fixture. Keeping the model local matches upstream and our rule for single-test helpers; extracting it would support a hypothetical consumer rather than fix a failing path.
There was a problem hiding this comment.
You're right. runQueueWorkerCommand(['--once' => '1']) executes the worker through the in-process Artisan test helper, and DeleteNotificationWhenMissingModelTest.php has already been loaded before the notification is queued and restored. Since this fixture is only consumed by that test, the model is available throughout the path; a separate worker process is not involved.
Keeping the model local is consistent with the upstream arrangement and the repository's single-test helper convention. This finding does not apply, so no change is needed.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 77 files
Confidence score: 4/5
tests/Notifications/Fixtures/DeleteWhenMissingNotification.phppins its constructor toDeleteNotificationTestModel, which is defined inline in a test; that coupling may limit reuse of the shared fixture. Consider moving the model into a shared fixture or making it injectable.tests/Integration/Routing/ImplicitBackedEnumRouteBindingTest.phpimports a duplicateCategoryBackedEnum, which could drift from the shared enum; consolidate the fixture and itsfromCode()helper.tests/Routing/Fixtures/ApiResourceTestController.phpand the related fixture methods and assertions use a backtick in place of the apostrophe, so tests can pass while checking the typo; correct the returned strings and assertions.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/Notifications/Fixtures/DeleteWhenMissingNotification.php">
<violation number="1" location="tests/Notifications/Fixtures/DeleteWhenMissingNotification.php:13">
P3: `DeleteWhenMissingNotification` is a shared fixture (the PR's point is reuse), but its constructor pins `DeleteNotificationTestModel`, a class defined inline in `tests/Integration/Queue/DeleteNotificationWhenMissingModelTest.php`. That class is not PSR-4 autoloadable — `Hypervel\Tests\` maps to `tests/`, which resolves to `tests/Integration/Queue/DeleteNotificationWhenMissingModelTest/DeleteNotificationTestModel.php`, a file that does not exist. The current test only works because `queue:work` runs in-process via `$this->artisan()` after the test file has already been loaded; reusing the fixture from another test or from a separate-process worker fails with class-not-found. Follow the existing `tests/Notifications/Fixtures/Models/NotifiableUser.php` pattern and move the model to an autoloadable fixtures path (e.g. `tests/Notifications/Fixtures/Models/DeleteNotificationTestModel.php`) or define the fixture next to the model.</violation>
</file>
<file name="tests/Integration/Routing/ImplicitBackedEnumRouteBindingTest.php">
<violation number="1" location="tests/Integration/Routing/ImplicitBackedEnumRouteBindingTest.php:9">
P3: The import pulls in a second copy of `CategoryBackedEnum` (`Fixtures\Integration\...`) that duplicates the shared `tests/Routing/Fixtures/CategoryBackedEnum.php` enum, differing only by the added `fromCode()` helper. Since this PR is consolidating shared fixtures, move `fromCode()` into the shared base enum and drop the `Integration/` copy so there is a single definition.</violation>
</file>
<file name="tests/Routing/Fixtures/ApiResourceTestController.php">
<violation number="1" location="tests/Routing/Fixtures/ApiResourceTestController.php:16">
P3: These fixture methods return `I\`m index` with a stray backtick instead of an apostrophe (`I'm`). The typo is replicated in `ApiResourceTaskController` and in the `assertSame('I\`m …', …)` assertions added in `tests/Integration/Routing/RouteApiResourceTest.php`, so the suite passes either way, but the rendered response text is grammatically wrong. Fix the backtick in the fixtures and update the matching assertions together.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| use Hypervel\Notifications\Notification; | ||
| use Hypervel\Queue\Attributes\DeleteWhenMissingModels; | ||
| use Hypervel\Queue\SerializesModels; | ||
| use Hypervel\Tests\Integration\Queue\DeleteNotificationWhenMissingModelTest\DeleteNotificationTestModel; |
There was a problem hiding this comment.
P3: DeleteWhenMissingNotification is a shared fixture (the PR's point is reuse), but its constructor pins DeleteNotificationTestModel, a class defined inline in tests/Integration/Queue/DeleteNotificationWhenMissingModelTest.php. That class is not PSR-4 autoloadable — Hypervel\Tests\ maps to tests/, which resolves to tests/Integration/Queue/DeleteNotificationWhenMissingModelTest/DeleteNotificationTestModel.php, a file that does not exist. The current test only works because queue:work runs in-process via $this->artisan() after the test file has already been loaded; reusing the fixture from another test or from a separate-process worker fails with class-not-found. Follow the existing tests/Notifications/Fixtures/Models/NotifiableUser.php pattern and move the model to an autoloadable fixtures path (e.g. tests/Notifications/Fixtures/Models/DeleteNotificationTestModel.php) or define the fixture next to the model.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/Notifications/Fixtures/DeleteWhenMissingNotification.php, line 13:
<comment>`DeleteWhenMissingNotification` is a shared fixture (the PR's point is reuse), but its constructor pins `DeleteNotificationTestModel`, a class defined inline in `tests/Integration/Queue/DeleteNotificationWhenMissingModelTest.php`. That class is not PSR-4 autoloadable — `Hypervel\Tests\` maps to `tests/`, which resolves to `tests/Integration/Queue/DeleteNotificationWhenMissingModelTest/DeleteNotificationTestModel.php`, a file that does not exist. The current test only works because `queue:work` runs in-process via `$this->artisan()` after the test file has already been loaded; reusing the fixture from another test or from a separate-process worker fails with class-not-found. Follow the existing `tests/Notifications/Fixtures/Models/NotifiableUser.php` pattern and move the model to an autoloadable fixtures path (e.g. `tests/Notifications/Fixtures/Models/DeleteNotificationTestModel.php`) or define the fixture next to the model.</comment>
<file context>
@@ -0,0 +1,47 @@
+use Hypervel\Notifications\Notification;
+use Hypervel\Queue\Attributes\DeleteWhenMissingModels;
+use Hypervel\Queue\SerializesModels;
+use Hypervel\Tests\Integration\Queue\DeleteNotificationWhenMissingModelTest\DeleteNotificationTestModel;
+
+#[DeleteWhenMissingModels]
</file context>
There was a problem hiding this comment.
The only consumer is DeleteNotificationWhenMissingModelTest, which runs queue:work in-process through the Artisan test helper. The model is loaded before dispatch and restoration. This matches the upstream fixture arrangement. There is no independent consumer or separate-process worker here that needs model autoloading, so I am keeping the single-test model local.
| use Hypervel\Contracts\Foundation\Application as ApplicationContract; | ||
| use Hypervel\Support\Facades\Route; | ||
| use Hypervel\Tests\Integration\Routing\Fixtures\CategoryBackedEnum; | ||
| use Hypervel\Tests\Routing\Fixtures\Integration\CategoryBackedEnum; |
There was a problem hiding this comment.
P3: The import pulls in a second copy of CategoryBackedEnum (Fixtures\Integration\...) that duplicates the shared tests/Routing/Fixtures/CategoryBackedEnum.php enum, differing only by the added fromCode() helper. Since this PR is consolidating shared fixtures, move fromCode() into the shared base enum and drop the Integration/ copy so there is a single definition.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/Integration/Routing/ImplicitBackedEnumRouteBindingTest.php, line 9:
<comment>The import pulls in a second copy of `CategoryBackedEnum` (`Fixtures\Integration\...`) that duplicates the shared `tests/Routing/Fixtures/CategoryBackedEnum.php` enum, differing only by the added `fromCode()` helper. Since this PR is consolidating shared fixtures, move `fromCode()` into the shared base enum and drop the `Integration/` copy so there is a single definition.</comment>
<file context>
@@ -6,27 +6,30 @@
use Hypervel\Contracts\Foundation\Application as ApplicationContract;
use Hypervel\Support\Facades\Route;
-use Hypervel\Tests\Integration\Routing\Fixtures\CategoryBackedEnum;
+use Hypervel\Tests\Routing\Fixtures\Integration\CategoryBackedEnum;
class ImplicitBackedEnumRouteBindingTest extends RoutingTestCase
</file context>
There was a problem hiding this comment.
The integration fixture includes fromCode() for explicit route binding; the other enum is the plain backed-enum fixture used by the routing unit tests. Both definitions and their separate placement match upstream. Consolidating them further would add a separate refactor without correcting a test failure or coverage gap.
| */ | ||
| public function index(): string | ||
| { | ||
| return 'I`m index'; |
There was a problem hiding this comment.
P3: These fixture methods return I\m index with a stray backtick instead of an apostrophe (I'm). The typo is replicated in ApiResourceTaskControllerand in theassertSame('I`m …', …)assertions added intests/Integration/Routing/RouteApiResourceTest.php`, so the suite passes either way, but the rendered response text is grammatically wrong. Fix the backtick in the fixtures and update the matching assertions together.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/Routing/Fixtures/ApiResourceTestController.php, line 16:
<comment>These fixture methods return `I\`m index` with a stray backtick instead of an apostrophe (`I'm`). The typo is replicated in `ApiResourceTaskController` and in the `assertSame('I\`m …', …)` assertions added in `tests/Integration/Routing/RouteApiResourceTest.php`, so the suite passes either way, but the rendered response text is grammatically wrong. Fix the backtick in the fixtures and update the matching assertions together.</comment>
<file context>
@@ -0,0 +1,50 @@
+ */
+ public function index(): string
+ {
+ return 'I`m index';
+ }
+
</file context>
There was a problem hiding this comment.
These are arbitrary marker strings in test-only controllers, matched by the routing assertions. They are unchanged from upstream and are not application response text. Rewriting the strings and assertions together would not improve what the tests verify, so I am keeping them aligned with upstream.
1 participant
Laravel Updates
Additional Hypervel Fixes
This changes tests and fixtures only. The full parallel suite, affected MySQL and MariaDB tests, formatting, and source and type analysis pass.
Summary by CodeRabbit