[#488] Raise console command coverage and silence serve test logs#489
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request expands unit test coverage for five console command classes to meet a >=70% coverage threshold. A minor refactoring extracts proc_open descriptor construction in ServeCommand into a dedicated method. The changes add behavior-oriented tests for argument parsing, exception handling, private method invocation via reflection, and end-to-end command execution scenarios across DebugBarCommand, MigrationMigrateCommand, OpenApiCommand, ResourceCacheClearCommand, and ServeCommand. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 26 minutes and 14 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/Unit/Console/Commands/ResourceCacheClearCommandTest.php (1)
83-92: Use a seeded temporary module directory instead of mocking the private$modulesproperty.The test sets
modulesto['blog', 'shop'], butinitModule()callsimportModules()at line 150 before validation, which overwrites this fixture with actual modules from disk at line 124. This makes the test fragile and dependent on what modules exist on the filesystem. Seed a temporary module directory in the config instead to ensure test isolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Console/Commands/ResourceCacheClearCommandTest.php` around lines 83 - 92, Replace the fragile fixture that directly sets the private $modules property in testInitModuleAcceptsKnownModule with a seeded temporary modules directory so importModules() returns predictable modules: create a temp directory containing 'blog' and 'shop' module folders, update the configuration the command uses (so importModules() reads that temp path), call initModule('BLOG') via ReflectionMethod as before, then assert the private 'module' property equals 'blog'; ensure cleanup of the temp directory after the test. Use the test method name testInitModuleAcceptsKnownModule and target initModule/importModules/modules to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Unit/Console/Commands/MigrationMigrateCommandTest.php`:
- Around line 52-60: Update the
testExecWithInvalidDirectionShowsMigrationMessage test to assert the specific
migration-direction error text instead of just non-empty output: when invoking
CommandTester->execute(['direction' => 'invalid']) capture
CommandTester->getDisplay() and assert it contains the exact MigrationException
message emitted by the migration command (the migration-direction error string
used in the command's exception/throw path), e.g. assertStringContainsString or
equivalent comparing against that specific error constant/text from the
migration command or MigrationException so the test validates the new
invalid-direction branch.
In `@tests/Unit/Console/Commands/OpenApiCommandTest.php`:
- Around line 83-93: The test testOpenapiRoutesContainsModuleSpecPath assumes
backslashes in the path, which breaks on Linux; update the assertion to use the
platform separator when checking the module spec path returned by openapiRoutes
(located via ReflectionMethod in the test), e.g. build the expected substring
using DIRECTORY_SEPARATOR (or the DS constant used by the production code) like
'resources' . DIRECTORY_SEPARATOR . 'openapi' . DIRECTORY_SEPARATOR .
'spec.json' and assert that against the $routes from
$method->invoke($this->command, 'Blog').
- Around line 123-158: Wrap the temporary module/file setup and the invocation
of generateOpenapiSpecification in a try/finally block inside
testGenerateOpenapiSpecificationCreatesSpecFile so cleanup always runs; move the
ReflectionMethod creation/invoke and assertions into the try, and keep the
filesystem teardown (unlink/rmdir calls) in the finally to ensure modules_dir
artifacts are removed even if generateOpenapiSpecification throws.
In `@tests/Unit/Console/Commands/ServeCommandTest.php`:
- Around line 298-323: The test mutates the repository public/index.php; instead
create an isolated temporary project dir, write public/index.php there, chdir
into that temp dir before calling exposeStartPhpServer('127.0.0.1', $port) and
run exposeWaitUntilServerIsReady/exposeCleanupProcess against the process, then
restore the original cwd and remove the temp files in finally. Concretely:
create a unique temp directory (sys_get_temp_dir()/tempnam or similar), mkdir
$tmp . '/public', write the index file into $tmp/public/index.php, save getcwd()
to $originalCwd, chdir($tmp) before calling
exposeStartPhpServer/exposeWaitUntilServerIsReady/exposeCleanupProcess, and in
the finally block chdir($originalCwd) and delete the temp index and directory;
reference the test helpers exposeStartPhpServer, exposeWaitUntilServerIsReady,
and exposeCleanupProcess when locating changes.
- Around line 165-180: The test's mock returns a stream resource from
startPhpServer() but cleanupProcess() calls parent::cleanupProcess($process)
which ultimately calls proc_close() and triggers a TypeError; fix by either
having startPhpServer() return a real process resource created with proc_open()
(so parent::cleanupProcess can safely call proc_close), or add a type-guard
inside the test's cleanupProcess() to check the resource type (e.g.,
is_resource() && get_resource_type($process) === 'process') and only call
parent::cleanupProcess($process) for actual process resources, otherwise perform
appropriate stream cleanup.
---
Nitpick comments:
In `@tests/Unit/Console/Commands/ResourceCacheClearCommandTest.php`:
- Around line 83-92: Replace the fragile fixture that directly sets the private
$modules property in testInitModuleAcceptsKnownModule with a seeded temporary
modules directory so importModules() returns predictable modules: create a temp
directory containing 'blog' and 'shop' module folders, update the configuration
the command uses (so importModules() reads that temp path), call
initModule('BLOG') via ReflectionMethod as before, then assert the private
'module' property equals 'blog'; ensure cleanup of the temp directory after the
test. Use the test method name testInitModuleAcceptsKnownModule and target
initModule/importModules/modules to locate the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1741130-37bc-42b5-a262-b9b83a5595c1
📒 Files selected for processing (6)
src/Console/Commands/ServeCommand.phptests/Unit/Console/Commands/DebugBarCommandTest.phptests/Unit/Console/Commands/MigrationMigrateCommandTest.phptests/Unit/Console/Commands/OpenApiCommandTest.phptests/Unit/Console/Commands/ResourceCacheClearCommandTest.phptests/Unit/Console/Commands/ServeCommandTest.php
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #489 +/- ##
============================================
+ Coverage 89.10% 90.71% +1.60%
- Complexity 2904 2905 +1
============================================
Files 249 249
Lines 7653 7655 +2
============================================
+ Hits 6819 6944 +125
+ Misses 834 711 -123 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Closes #488
Summary by CodeRabbit
Tests
Refactor