Repository navigation
Issue/479 command coverage - #485
Merged
armanist merged 6 commits intoApr 30, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds execution-path unit tests for multiple console command tests, exercising runtime behavior (CommandTester/HelperSet), filesystem effects, env updates, simulated interactive input, and teardown cleanup to isolate state. Changes
Sequence Diagram(s)(No sequence diagrams generated — changes are test additions and do not introduce new multi-component runtime control flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
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 60 minutes.Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #485 +/- ##
============================================
+ Coverage 85.28% 86.98% +1.69%
Complexity 2903 2903
============================================
Files 249 249
Lines 7651 7651
============================================
+ Hits 6525 6655 +130
+ Misses 1126 996 -130 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5fa5a6617
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/Unit/Console/Commands/MigrationGenerateCommandTest.php (1)
46-58: Assert the migration file side effect, not only output text.The command’s core behavior is file creation; asserting file count increase will make this test harder to false-pass.
Suggested enhancement
public function testExecCreatesMigrationFile(): void { + $before = $this->existingMigrationFiles; $tester = new CommandTester($this->command); $tester->execute([ 'action' => 'create', 'table' => 'tickets', @@ $output = $tester->getDisplay(); $this->assertStringContainsString('Migration file', $output); $this->assertStringContainsString('successfully created', $output); + + $files = glob(base_dir() . DS . 'migrations' . DS . '*.php'); + $after = is_array($files) ? $files : []; + $this->assertGreaterThan(count($before), count($after)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Console/Commands/MigrationGenerateCommandTest.php` around lines 46 - 58, Update testExecCreatesMigrationFile in MigrationGenerateCommandTest to assert the actual file-side effect instead of only output: before calling $tester->execute, record the list/count of migration files (e.g. glob or scandir for the migrations directory), then execute the command via CommandTester->execute (the same 'action' => 'create', 'table' => 'tickets' args), and after execution assert that the migration file count has increased or that a new migration filename matching the expected pattern (timestamp + table name) now exists; use the test helper methods in this class and reference testExecCreatesMigrationFile and CommandTester->execute to locate where to add the pre/post filesystem checks.
🤖 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/InstallToolkitCommandTest.php`:
- Around line 37-68: This test mutates env-backed auth values (BASIC_AUTH_NAME,
BASIC_AUTH_PWD) and must restore them: at the start of
testExecUpdatesEnvAndRunsModuleGeneration capture the originals (e.g. $origName
= env('BASIC_AUTH_NAME'); $origPwd = env('BASIC_AUTH_PWD')), then wrap the
exercise/assertions in a try/finally and in finally restore the originals (use
putenv and $_ENV/$_SERVER as appropriate — unset if original was null, or set
back to the saved value) so subsequent tests are not polluted; keep the existing
assertions and calledCommand/calledArguments checks intact.
In `@tests/Unit/Console/Commands/ModuleGenerateCommandTest.php`:
- Around line 13-14: Replace the fixed $moduleName property and unsafe cleanup
with a per-test unique module name and non-destructive teardown: set $moduleName
by generating a unique string (e.g., uniqid('Issue479Module_') or use PHPUnit's
temp directory) in setUp(), record any created paths, and in tearDown() only
remove files/directories if they exist and match the generated $moduleName
(avoid globbing or unconditional deletes). Update
ModuleGenerateCommandTest::$moduleName initialization, the setUp() that prepares
modulesConfigPath, and the tearDown() cleanup logic so it checks file existence
and ownership by the test-generated name before unlink/rmdir (or use an isolated
temp dir/vfsStream) to prevent removing pre-existing modules/configs.
In `@tests/Unit/Console/Commands/OpenApiCommandTest.php`:
- Around line 44-59: The test OpenApiCommandTest mutates the shared OpenAPI
asset (assets_dir() . DS . 'OpenApiUi' / 'index.css') and only best-effort
cleans up, risking state leakage and clobbering existing content; update the
test to detect and back up any existing index.css before writing the stub,
create the OpenApiUi directory if missing, run the CommandTester execution, and
then in a guaranteed cleanup block (try/finally or tearDown) restore the
original file if a backup was made or remove the created file and directory
otherwise; reference $openApiAssets, 'index.css', $this->fs and the
CommandTester usage to locate where to add the backup/restore and ensure cleanup
runs even on test failure.
In `@tests/Unit/Console/Commands/VersionCommandTest.php`:
- Around line 33-39: The current test in VersionCommandTest.php swallows
execution errors by catching any Exception and asserting on its message, which
masks real failures; remove the try/catch and either (A) let the command run
normally and assert the output contains '9.9.9-test' after $tester->execute([])
so any thrown exception fails the test, or (B) if the test genuinely expects a
specific failure, replace the catch with a focused expectation using
$this->expectExceptionMessage('slant.flf') (and/or $this->expectException(...)
for the specific exception class) before calling $tester->execute([]); update
the assertions around $tester->getDisplay() and assertStringContainsString
accordingly to reference the same code paths.
---
Nitpick comments:
In `@tests/Unit/Console/Commands/MigrationGenerateCommandTest.php`:
- Around line 46-58: Update testExecCreatesMigrationFile in
MigrationGenerateCommandTest to assert the actual file-side effect instead of
only output: before calling $tester->execute, record the list/count of migration
files (e.g. glob or scandir for the migrations directory), then execute the
command via CommandTester->execute (the same 'action' => 'create', 'table' =>
'tickets' args), and after execution assert that the migration file count has
increased or that a new migration filename matching the expected pattern
(timestamp + table name) now exists; use the test helper methods in this class
and reference testExecCreatesMigrationFile and CommandTester->execute to locate
where to add the pre/post filesystem checks.
🪄 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: 7c9d458a-0f97-4d14-bb69-c77d11b7fd02
📒 Files selected for processing (8)
tests/Unit/Console/Commands/InstallToolkitCommandTest.phptests/Unit/Console/Commands/MigrationGenerateCommandTest.phptests/Unit/Console/Commands/MigrationMigrateCommandTest.phptests/Unit/Console/Commands/ModuleGenerateCommandTest.phptests/Unit/Console/Commands/OpenApiCommandTest.phptests/Unit/Console/Commands/RouteListCommandTest.phptests/Unit/Console/Commands/ServeCommandTest.phptests/Unit/Console/Commands/VersionCommandTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/Unit/Console/Commands/VersionCommandTest.php (2)
34-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t let missing font assets count as a functional pass.
At Line 34, the fallback path can pass without validating the command’s success-path output, which weakens coverage confidence for this command. Skip when the asset is unavailable (or ensure the asset is present) so a pass always means behavior was actually asserted.
Proposed change
- if (!is_file($fontPath)) { - $this->expectException(\Exception::class); - $this->expectExceptionMessage('slant.flf'); - } + if (!is_file($fontPath)) { + $this->markTestSkipped('Missing figlet font: ' . $fontPath); + } - $tester->execute([]); + $exitCode = $tester->execute([]); $output = $tester->getDisplay(); + $this->assertSame(0, $exitCode); $this->assertStringContainsString('3.0.0', $output);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Console/Commands/VersionCommandTest.php` around lines 34 - 41, The test currently falls through when the font asset ($fontPath) is missing, letting the success-path assertions run despite not exercising real behavior; update the guard that checks is_file($fontPath) to call $this->markTestSkipped('Missing font asset: slant.flf') (or ensure the asset is installed before running) instead of setting expectException so the test is skipped when the asset is absent, leaving the subsequent $tester->execute([]) and the assertion that '3.0.0' appears in the output to only run when the font file is present.
29-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore global config after this test.
Line 29 mutates shared config state and does not restore it, which can make later tests order-dependent.
Proposed change
public function testExecPrintsFrameworkVersion(): void { - config()->set('app', ['version' => '3.0.0']); + $previousAppConfig = config()->get('app'); + config()->set('app', ['version' => '3.0.0']); - $tester = new CommandTester($this->command); - $fontPath = assets_dir() . DS . 'shared' . DS . 'fonts' . DS . 'figlet' . DS . 'slant.flf'; + try { + $tester = new CommandTester($this->command); + $fontPath = assets_dir() . DS . 'shared' . DS . 'fonts' . DS . 'figlet' . DS . 'slant.flf'; - if (!is_file($fontPath)) { - $this->expectException(\Exception::class); - $this->expectExceptionMessage('slant.flf'); - } + if (!is_file($fontPath)) { + $this->markTestSkipped('Missing figlet font: ' . $fontPath); + } - $tester->execute([]); - $output = $tester->getDisplay(); - $this->assertStringContainsString('3.0.0', $output); + $tester->execute([]); + $output = $tester->getDisplay(); + $this->assertStringContainsString('3.0.0', $output); + } finally { + config()->set('app', $previousAppConfig); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Console/Commands/VersionCommandTest.php` at line 29, The test mutates global config by calling config()->set('app', ['version' => '3.0.0']) and does not restore it; capture the original app config before setting (e.g. $originalApp = config('app')) and restore it after the test completes (either in the test itself or in VersionCommandTest::tearDown()) by calling config()->set('app', $originalApp) so other tests aren’t affected.
🧹 Nitpick comments (1)
tests/Unit/Console/Commands/ModuleGenerateCommandTest.php (1)
62-67: ⚡ Quick winStrengthen assertions for module config semantics.
This currently checks only entry presence. Add assertions for the created module config payload (e.g.,
prefixandenabled) so regressions in option/template wiring are caught.Suggested assertion upgrade
$moduleConfigs = $this->fs->require($this->modulesConfigPath); - if (is_array($moduleConfigs) && isset($moduleConfigs[$this->moduleName])) { - $this->moduleConfigCreatedByTest = true; - } + $this->assertIsArray($moduleConfigs); + $this->assertArrayHasKey($this->moduleName, $moduleConfigs); + $this->assertArrayHasKey('prefix', $moduleConfigs[$this->moduleName]); + $this->assertArrayHasKey('enabled', $moduleConfigs[$this->moduleName]); + $this->moduleConfigCreatedByTest = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Console/Commands/ModuleGenerateCommandTest.php` around lines 62 - 67, The test currently only verifies the module entry exists; update the assertions after loading $moduleConfigs from $this->modulesConfigPath (and after the existing $moduleConfigCreatedByTest logic) to assert the created module's config values explicitly: fetch $moduleConfigs[$this->moduleName] and assert it is an array, then assert its 'prefix' equals the expected prefix value (derived from $this->moduleName or the test's expected string) and assert its 'enabled' flag equals the expected boolean; keep the existing string assertion against $tester->getDisplay() but add these payload assertions to catch regressions in prefix/enabled wiring.
🤖 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/ModuleGenerateCommandTest.php`:
- Around line 70-93: The teardown currently skips cleanup when runtime flags
($this->moduleConfigCreatedByTest, $this->moduleCreatedByTest) were not set,
which can leave artifacts if a test fails before setting them; change tearDown()
to base cleanup solely on current state checks: for the modules config use
$this->fs->exists($this->modulesConfigPath), require the file, verify is_array
and isset($moduleConfigs[$this->moduleName]) and then unset and write back
(keeping the existing export and "<?php\n\nreturn " pattern); for module
directory removal remove the $this->moduleCreatedByTest check and instead use
$this->fs->isDirectory($this->modulePath), basename($this->modulePath) ===
$this->moduleName and strpos($this->moduleName, 'Issue479Module_') === 0 to
decide to delete the directory—this ensures cleanup depends only on
filesystem/config state not on test-set flags.
---
Duplicate comments:
In `@tests/Unit/Console/Commands/VersionCommandTest.php`:
- Around line 34-41: The test currently falls through when the font asset
($fontPath) is missing, letting the success-path assertions run despite not
exercising real behavior; update the guard that checks is_file($fontPath) to
call $this->markTestSkipped('Missing font asset: slant.flf') (or ensure the
asset is installed before running) instead of setting expectException so the
test is skipped when the asset is absent, leaving the subsequent
$tester->execute([]) and the assertion that '3.0.0' appears in the output to
only run when the font file is present.
- Line 29: The test mutates global config by calling config()->set('app',
['version' => '3.0.0']) and does not restore it; capture the original app config
before setting (e.g. $originalApp = config('app')) and restore it after the test
completes (either in the test itself or in VersionCommandTest::tearDown()) by
calling config()->set('app', $originalApp) so other tests aren’t affected.
---
Nitpick comments:
In `@tests/Unit/Console/Commands/ModuleGenerateCommandTest.php`:
- Around line 62-67: The test currently only verifies the module entry exists;
update the assertions after loading $moduleConfigs from $this->modulesConfigPath
(and after the existing $moduleConfigCreatedByTest logic) to assert the created
module's config values explicitly: fetch $moduleConfigs[$this->moduleName] and
assert it is an array, then assert its 'prefix' equals the expected prefix value
(derived from $this->moduleName or the test's expected string) and assert its
'enabled' flag equals the expected boolean; keep the existing string assertion
against $tester->getDisplay() but add these payload assertions to catch
regressions in prefix/enabled wiring.
🪄 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: bb8047cc-6b3c-4c9b-8e66-fabcb54027f1
📒 Files selected for processing (4)
tests/Unit/Console/Commands/InstallToolkitCommandTest.phptests/Unit/Console/Commands/ModuleGenerateCommandTest.phptests/Unit/Console/Commands/OpenApiCommandTest.phptests/Unit/Console/Commands/VersionCommandTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Unit/Console/Commands/InstallToolkitCommandTest.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #479
Summary by CodeRabbit