Bootstrap the application when tests refresh it#423
Conversation
refreshApplication() only created the application. The bootstrappers (LoadEnvironmentVariables, LoadConfiguration, RegisterFacades, RegisterProviders, GenerateProxies, BootProviders) only run on the HTTP and console boot paths, so on the test path the facade root was never bound and the first facade call in setUp() failed with "A facade root has not been set" — the default ExampleTest of a fresh application fails out of the box. Bootstrap the freshly-created application in refreshApplication(). HandleExceptions is deliberately omitted so PHPUnit reports failures instead of the Swoole error handler.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA shared default bootstrapper list was added to ChangesShared bootstrap sequence
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/foundation/src/Testing/TestCase.php (1)
76-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the bootstrapper list with the app kernels.
src/foundation/src/Testing/TestCase.phpduplicates the same bootstrapper sequence already defined insrc/foundation/src/Http/Kernel.phpandsrc/foundation/src/Console/Kernel.php, with onlyHandleExceptionsintentionally omitted here. A shared bootstrapper source or helper would keep the test path from drifting when the app boot sequence changes.🤖 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 `@src/foundation/src/Testing/TestCase.php` around lines 76 - 85, The bootstrapper sequence in TestCase::createApplication is duplicated from the app kernels and can drift over time; extract the shared bootstrapper list into a common source used by Http\Kernel and Console\Kernel, then reuse it in Testing\TestCase while keeping HandleExceptions excluded there. Update the bootstrapper setup so all three paths reference the same shared sequence rather than hardcoding the list in each place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/foundation/src/Testing/TestCase.php`:
- Around line 76-85: The bootstrapper sequence in TestCase::createApplication is
duplicated from the app kernels and can drift over time; extract the shared
bootstrapper list into a common source used by Http\Kernel and Console\Kernel,
then reuse it in Testing\TestCase while keeping HandleExceptions excluded there.
Update the bootstrapper setup so all three paths reference the same shared
sequence rather than hardcoding the list in each place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: be40302a-049e-42e1-bc07-edc6a6600fc0
📒 Files selected for processing (1)
src/foundation/src/Testing/TestCase.php
Greptile SummaryThis PR fixes a
Confidence Score: 5/5Safe to merge; the bootstrapping logic is correct and well-guarded, and the one nit is purely stylistic. The fix is straightforward: the No files require special attention beyond the single FQCN style nit in Important Files Changed
Reviews (2): Last reviewed commit: "Share the default bootstrapper sequence ..." | Re-trigger Greptile |
The HTTP kernel, the console kernel, and the test path each hardcoded the same bootstrapper list. Move it to Application::DEFAULT_BOOTSTRAPPERS so the three paths cannot drift; the test path keeps excluding HandleExceptions.
|
Addressed in b141d53 — the sequence now lives in |
|
Thanks for the PR. This bug needed some bigger refactoring. Fixed in #426 |
The default
ExampleTestof a freshly scaffolded 0.4 application fails out of the box:refreshApplication()only creates the application — the bootstrappers (LoadConfiguration,RegisterFacades,RegisterProviders,BootProviders, …) run on the HTTP and console boot paths but never on the test path, so the facade root is never bound and the first facade call insetUp()throws. The components suite doesn't catch this because unit tests don't boot a container and Testbench has its own bootstrap path; only real applications extendFoundation\Testing\TestCasedirectly.This bootstraps the freshly-created application in
refreshApplication().HandleExceptionsis deliberately omitted so PHPUnit reports failures instead of the app's Swoole error handler.Verified on a fresh skeleton app:
Summary by CodeRabbit
Bug Fixes
Improvements