refactor: add Guzzle injection to AnalyzeCommand with tests#55
Open
jordanpartridge wants to merge 1 commit intomasterfrom
Open
refactor: add Guzzle injection to AnalyzeCommand with tests#55jordanpartridge wants to merge 1 commit intomasterfrom
jordanpartridge wants to merge 1 commit intomasterfrom
Conversation
…tern Make AnalyzeCommand testable by injecting HTTP client and mock values for repo/sha detection, following the same withMocks() pattern used by CertifyCommand. Add comprehensive test suite covering all branches: validation, API interaction, configuration, and error handling. Also applies pint style fixes across touched files. Closes #48
📝 WalkthroughWalkthroughThe changes refactor AnalyzeCommand to support dependency injection for testing by adding an injectable HTTP client and a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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. Comment |
📊 Coverage Report
Files Below Threshold
🏆 Synapse Sentinel Gate |
🔧 Synapse Sentinel: 2 checks need attentionThe following issues must be resolved before this PR can be merged: All tests passed.--- Security AuditReview the output and fix any issues.Quick Reference:
🤖 Generated by Synapse Sentinel - View Run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/Commands/AnalyzeCommand.php (1)
71-74: Injected client path bypasses resolved API URL/timeout configuration.When
withMocks()provides a client,--api-url/PREFRONTAL_API_URLand timeout settings are ignored, which creates a behavior gap between test and production paths. Consider injecting a client factory/handler instead, sohandle()still builds the request client from resolved config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Commands/AnalyzeCommand.php` around lines 71 - 74, The injected HTTP client ($this->httpClient) in AnalyzeCommand::handle bypasses resolved configuration (api URL and timeout) when withMocks() supplies a client; change the constructor/injection to accept a client factory or handler (e.g., a callable or HttpClientFactory) instead of a concrete Client so that handle() always obtains a Client built with the resolved $apiUrl and timeout before use; update uses of $this->httpClient to call the factory (e.g., $client = ($this->httpClientFactory)($apiUrl, $timeout) or a getClient($apiUrl, $timeout) method) and keep withMocks() providing a factory that returns a mock client to preserve test behavior while ensuring production path uses configured values.
🤖 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/Commands/AnalyzeCommandTest.php`:
- Around line 30-75: Add a new test in
tests/Unit/Commands/AnalyzeCommandTest.php under the validation describe block
that exercises AnalyzeCommand::handle()'s unreadable-file branch: create a
temporary failures file (use $this->failuresFile), write valid JSON into it,
change its permissions to be unreadable (e.g., chmod 000), run
$this->artisan('analyze', ['--api-token' => 'test-token', '--failures' =>
$this->failuresFile]) and assert it outputs the exact "Could not read failures
file" message and fails; finally restore permissions/delete the temp file in
teardown so CI isn’t left with a locked file.
- Around line 16-19: The createCommand closure currently forces defaults
'owner/repo' and 'abc123', preventing detection-path tests from exercising
AnalyzeCommand::detectRepo() and detectSha(); change the call to
$command->withMocks to pass through the provided values (e.g.,
$command->withMocks($httpClient, $repo, $sha) or explicitly $repo ?? null, $sha
?? null) so tests can invoke ($this->createCommand)(null, null) to trigger
detection logic in AnalyzeCommand (and keep the closure signature and
app()->singleton usage unchanged).
---
Nitpick comments:
In `@app/Commands/AnalyzeCommand.php`:
- Around line 71-74: The injected HTTP client ($this->httpClient) in
AnalyzeCommand::handle bypasses resolved configuration (api URL and timeout)
when withMocks() supplies a client; change the constructor/injection to accept a
client factory or handler (e.g., a callable or HttpClientFactory) instead of a
concrete Client so that handle() always obtains a Client built with the resolved
$apiUrl and timeout before use; update uses of $this->httpClient to call the
factory (e.g., $client = ($this->httpClientFactory)($apiUrl, $timeout) or a
getClient($apiUrl, $timeout) method) and keep withMocks() providing a factory
that returns a mock client to preserve test behavior while ensuring production
path uses configured values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05698d19-7722-4c42-9331-5593714ee130
📒 Files selected for processing (7)
app/Commands/AnalyzeCommand.phpapp/Commands/CertifyCommand.phpapp/GitHub/ChecksClient.phpapp/Services/PromptAssembler.phpapp/Transformers/PhpStanPromptTransformer.phpapp/Transformers/TestFailurePromptTransformer.phptests/Unit/Commands/AnalyzeCommandTest.php
Comment on lines
+16
to
+19
| $this->createCommand = function (?Client $httpClient = null, ?string $repo = null, ?string $sha = null) { | ||
| $command = new AnalyzeCommand; | ||
| $command->withMocks($httpClient, $repo ?? 'owner/repo', $sha ?? 'abc123'); | ||
| app()->singleton(AnalyzeCommand::class, fn () => $command); |
There was a problem hiding this comment.
createCommand currently blocks testing real repo/SHA detection paths.
Line 18 always substitutes 'owner/repo' and 'abc123', so tests cannot exercise detectRepo() / detectSha() fallback behavior (HTTPS/SSH/no-remote, git/no-git).
🔧 Proposed fix
- $this->createCommand = function (?Client $httpClient = null, ?string $repo = null, ?string $sha = null) {
+ $this->createCommand = function (
+ ?Client $httpClient = null,
+ ?string $repo = 'owner/repo',
+ ?string $sha = 'abc123'
+ ) {
$command = new AnalyzeCommand;
- $command->withMocks($httpClient, $repo ?? 'owner/repo', $sha ?? 'abc123');
+ $command->withMocks($httpClient, $repo, $sha);
app()->singleton(AnalyzeCommand::class, fn () => $command);
};Then detection-path tests can call:
($this->createCommand)($httpClient, null, null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Unit/Commands/AnalyzeCommandTest.php` around lines 16 - 19, The
createCommand closure currently forces defaults 'owner/repo' and 'abc123',
preventing detection-path tests from exercising AnalyzeCommand::detectRepo() and
detectSha(); change the call to $command->withMocks to pass through the provided
values (e.g., $command->withMocks($httpClient, $repo, $sha) or explicitly $repo
?? null, $sha ?? null) so tests can invoke ($this->createCommand)(null, null) to
trigger detection logic in AnalyzeCommand (and keep the closure signature and
app()->singleton usage unchanged).
Comment on lines
+30
to
+75
| describe('validation', function () { | ||
| it('fails when no API token is provided', function () { | ||
| putenv('PREFRONTAL_API_TOKEN'); | ||
|
|
||
| $this->artisan('analyze') | ||
| ->expectsOutputToContain('API token required') | ||
| ->assertFailed(); | ||
| }); | ||
|
|
||
| it('fails when no failures file is provided', function () { | ||
| $this->artisan('analyze', ['--api-token' => 'test-token']) | ||
| ->expectsOutputToContain('Failures file required') | ||
| ->assertFailed(); | ||
| }); | ||
|
|
||
| it('fails when failures file does not exist', function () { | ||
| $this->artisan('analyze', [ | ||
| '--api-token' => 'test-token', | ||
| '--failures' => '/tmp/nonexistent_file_'.uniqid().'.json', | ||
| ]) | ||
| ->expectsOutputToContain('Failures file required') | ||
| ->assertFailed(); | ||
| }); | ||
|
|
||
| it('fails when failures file contains invalid JSON', function () { | ||
| file_put_contents($this->failuresFile, 'not valid json'); | ||
|
|
||
| $this->artisan('analyze', [ | ||
| '--api-token' => 'test-token', | ||
| '--failures' => $this->failuresFile, | ||
| ]) | ||
| ->expectsOutputToContain('Invalid JSON') | ||
| ->assertFailed(); | ||
| }); | ||
|
|
||
| it('fails when failures file contains empty JSON array', function () { | ||
| file_put_contents($this->failuresFile, '[]'); | ||
|
|
||
| $this->artisan('analyze', [ | ||
| '--api-token' => 'test-token', | ||
| '--failures' => $this->failuresFile, | ||
| ]) | ||
| ->expectsOutputToContain('Invalid JSON') | ||
| ->assertFailed(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Unreadable failures-file branch is still untested.
AnalyzeCommand::handle() has a dedicated "Could not read failures file" path, but this validation block does not cover it.
🧪 Suggested test case
+ it('fails when failures path cannot be read', function () {
+ $this->artisan('analyze', [
+ '--api-token' => 'test-token',
+ '--failures' => sys_get_temp_dir(), // directory exists but is not a readable JSON file
+ ])
+ ->expectsOutputToContain('Could not read failures file')
+ ->assertFailed();
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Unit/Commands/AnalyzeCommandTest.php` around lines 30 - 75, Add a new
test in tests/Unit/Commands/AnalyzeCommandTest.php under the validation describe
block that exercises AnalyzeCommand::handle()'s unreadable-file branch: create a
temporary failures file (use $this->failuresFile), write valid JSON into it,
change its permissions to be unreadable (e.g., chmod 000), run
$this->artisan('analyze', ['--api-token' => 'test-token', '--failures' =>
$this->failuresFile]) and assert it outputs the exact "Could not read failures
file" message and fails; finally restore permissions/delete the temp file in
teardown so CI isn’t left with a locked file.
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
1 participant
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.
Summary
AnalyzeCommandto accept injected GuzzleClientviawithMocks()pattern (matchingCertifyCommand)mockRepoandmockShaproperties to bypassshell_execcalls in teststests/Unit/Commands/AnalyzeCommandTest.phpwith 15 tests covering all branches: validation (missing token, missing file, invalid JSON), API interaction (success with fixes, empty fixes, error responses, Guzzle exceptions), and configuration (env vars, option overrides)Test plan
Closes #48
Summary by CodeRabbit
Tests
Refactor