testability #1
testability #1
Conversation
📝 WalkthroughWalkthroughAdds a testable CLI input abstraction: a new Changes
Sequence DiagramsequenceDiagram
autonumber
actor User
participant Command
participant IInputReader
participant StdinInputReader
participant Output
participant STDIN
participant TestInputReader
rect rgb(230,245,255)
Note over Command,StdinInputReader: Production flow (default reader)
User->>Command: execute()
Command->>Command: getInputReader()
Command->>IInputReader: prompt("...")
IInputReader->>StdinInputReader: prompt()
StdinInputReader->>Output: write(prompt)
StdinInputReader->>STDIN: read line
STDIN-->>StdinInputReader: input
StdinInputReader->>IInputReader: return trimmed input
IInputReader->>Command: return response
end
rect rgb(255,245,230)
Note over Command,TestInputReader: Test flow (injected TestInputReader)
Command->>Command: setInputReader(testReader)
TestInputReader->>TestInputReader: addResponses(["Alice"])
Command->>IInputReader: prompt("...")
IInputReader->>TestInputReader: prompt()
TestInputReader->>TestInputReader: record promptHistory, dequeue response
TestInputReader->>Command: return "Alice"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (2)tests/Cli/Commands/CommandTest.php (2)
src/Cli/IO/StdinInputReader.php (4)
🔇 Additional comments (16)
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 |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
![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 (4)
versionlog.md (1)
1-2: Consider adding a release date for consistency.Other version entries include dates (e.g.,
## 0.8.8 2025-11-28), but 0.8.9 is missing one. Consider adding the date when this version is released to maintain consistency in the changelog format.tests/Cli/IO/TestInputReaderTest.php (2)
41-47: Consider testing the full exception message.The exception message includes additional context about available responses and usage hints. You might want to verify the complete message or use
assertStringContainsStringfor the additional parts to ensure the error guidance is correct.
107-135: Consider adding a test for invalid choice input.The
TestInputReader::choice()method returns the raw response when it doesn't match any option (lines 194-196 in TestInputReader.php). Adding a test to verify this behavior would improve coverage:public function testChoiceReturnsRawResponseForInvalidInput(): void { $options = ['option1', 'option2']; $this->reader->addResponse('invalid'); $result = $this->reader->choice('Select:', $options); $this->assertEquals('invalid', $result); }src/Cli/IO/StdinInputReader.php (1)
54-74: Consider error handling for stty commands.The
system()calls don't check for failures. Ifsttyis unavailable or fails, the code continues without indication. Consider capturing the return value or usingshell_execwith error handling.🔎 Possible improvement
// Only hide input on Unix-like systems if( strtoupper( substr( PHP_OS, 0, 3 ) ) !== 'WIN' ) { // Disable terminal echo - system( 'stty -echo' ); + $result = @system( 'stty -echo 2>/dev/null', $retval ); $input = fgets( STDIN ); // Re-enable terminal echo - system( 'stty echo' ); + @system( 'stty echo 2>/dev/null' ); // Add newline since user's enter key wasn't echoed $this->output->writeln( '' ); } else {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
readme.mdsrc/Cli/Commands/Command.phpsrc/Cli/IO/IInputReader.phpsrc/Cli/IO/StdinInputReader.phpsrc/Cli/IO/TestInputReader.phptests/Cli/IO/TestInputReaderTest.phpversionlog.md
🔇 Additional comments (13)
tests/Cli/IO/TestInputReaderTest.php (2)
8-16: Good test structure with proper setup.The test class properly initializes a fresh
TestInputReaderinstance insetUp(), ensuring test isolation.
167-186: LGTM!The reset and fluent interface tests properly verify state clearing and method chaining behavior.
readme.md (3)
477-506: Clear documentation for non-interactive command testing.The example demonstrates proper setup for testing commands without user input, including mock input/output configuration.
508-578: Excellent documentation for interactive command testing.The examples clearly demonstrate how to use
TestInputReaderwith pre-programmed responses and how to verify prompt history. The cancellation test case is a valuable addition showing negative path testing.
580-638: Good feature documentation for TestInputReader.The feature list and code example effectively communicate the test double's capabilities including response queue, prompt history, and fluent interface.
src/Cli/IO/IInputReader.php (1)
14-59: Well-designed interface for input abstraction.The interface is clean, focused, and provides a good abstraction for CLI input operations. The method signatures are consistent and the PHPDoc documentation is thorough.
src/Cli/Commands/Command.php (3)
7-8: LGTM!Clean integration of the input reader dependency with proper imports and nullable property declaration.
Also applies to: 19-19
334-338: Good dependency injection pattern.The setter allows test doubles to be injected while maintaining the fluent interface pattern consistent with other setters in this class.
353-423: Well-documented convenience methods.The helper methods are well-documented with examples and properly delegate to the input reader. This creates a clean API for command authors.
src/Cli/IO/StdinInputReader.php (1)
20-32: LGTM!Clean constructor with dependency injection and simple prompt implementation.
src/Cli/IO/TestInputReader.php (3)
28-75: Well-designed test double with fluent interface.The queue-based response mechanism with method chaining provides a clean API for test setup.
128-141: Excellent error messaging.The exception provides actionable debugging information including the prompt index, message, and available response count. This will help developers quickly identify missing test setup.
172-197: Intentional behavioral difference from production is well-documented.The comment on lines 194-196 clearly explains why invalid choices return the raw response instead of re-prompting, allowing tests to verify behavior for edge cases. This is a sensible design decision for a test double.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Cli/IO/StdinInputReader.php (1)
115-120: Unbounded recursion on invalid input.If a user repeatedly enters invalid choices, this recursive call could theoretically cause a stack overflow. Consider converting to an iterative loop or adding a maximum retry limit.
🔎 Iterative alternative
public function choice( string $message, array $options, ?string $default = null ): string { + while( true ) { // Display the prompt message $this->output->writeln( $message ); $this->output->writeln( '' ); // Display options with index numbers foreach( $options as $index => $option ) { $marker = ($default === $option) ? '*' : ' '; $this->output->writeln( " [{$marker}] {$index}. {$option}" ); } $this->output->writeln( '' ); // Prompt for selection $prompt = $default !== null ? "Choice [{$default}]: " : "Choice: "; $response = $this->prompt( $prompt ); // If user just presses enter and there's a default, use it if( $response === '' && $default !== null ) { return $default; } // Check if response is a numeric index if( is_numeric( $response ) ) { $index = (int)$response; if( isset( $options[$index] ) ) { return $options[$index]; } } // Check if response matches an option exactly if( in_array( $response, $options, true ) ) { return $response; } // Invalid choice - ask again $this->output->error( "Invalid choice. Please try again." ); $this->output->writeln( '' ); - - return $this->choice( $message, $options, $default ); + } }
🧹 Nitpick comments (1)
tests/Cli/IO/StdinInputReaderTest.php (1)
44-90: Consider using PHPUnit data providers for parameterized tests.The current approach of recreating mocks within loops works correctly but is unconventional. A data provider would be more idiomatic, efficient, and clearer.
🔎 Data provider approach
+ /** + * @dataProvider confirmYesResponseProvider + */ + public function testConfirmAcceptsYesResponse( string $response ): void + { + $mock = $this->getMockBuilder( StdinInputReader::class ) + ->setConstructorArgs( [$this->output] ) + ->onlyMethods( ['prompt'] ) + ->getMock(); + + $mock->expects( $this->once() ) + ->method( 'prompt' ) + ->willReturn( $response ); + + $result = $mock->confirm( 'Continue?', false ); + + $this->assertTrue( $result, "Response '{$response}' should be treated as positive" ); + } + + public function confirmYesResponseProvider(): array + { + return [ + 'lowercase y' => ['y'], + 'lowercase yes' => ['yes'], + 'uppercase YES' => ['YES'], + 'mixed case Yes' => ['Yes'], + 'lowercase true' => ['true'], + 'uppercase TRUE' => ['TRUE'], + 'numeric 1' => ['1'], + ]; + }Apply the same pattern for
testConfirmRejectsNoResponse.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Cli/IO/StdinInputReader.phpsrc/Cli/IO/TestInputReader.phptests/Cli/Commands/CommandTest.phptests/Cli/IO/StdinInputReaderTest.phptests/Cli/IO/TestInputReaderTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Cli/IO/TestInputReaderTest.php
- src/Cli/IO/TestInputReader.php
🧰 Additional context used
🧬 Code graph analysis (3)
tests/Cli/Commands/CommandTest.php (1)
src/Cli/IO/TestInputReader.php (7)
TestInputReader(28-198)addResponse(59-63)getPromptHistory(84-87)prompt(128-141)confirm(146-157)secret(162-167)choice(172-197)
tests/Cli/IO/StdinInputReaderTest.php (2)
src/Cli/IO/StdinInputReader.php (3)
StdinInputReader(15-121)confirm(37-49)choice(79-120)src/Cli/IO/TestInputReader.php (2)
confirm(146-157)choice(172-197)
src/Cli/IO/StdinInputReader.php (4)
src/Cli/Console/Output.php (3)
Output(9-403)writeln(123-131)error(202-205)src/Cli/IO/TestInputReader.php (4)
prompt(128-141)confirm(146-157)secret(162-167)choice(172-197)src/Cli/IO/IInputReader.php (4)
prompt(22-22)confirm(34-34)secret(45-45)choice(58-58)src/Cli/Commands/Command.php (4)
prompt(353-356)confirm(375-378)secret(394-397)choice(420-423)
🔇 Additional comments (13)
tests/Cli/Commands/CommandTest.php (4)
8-9: LGTM!The new imports support the input reader integration tests added in this file.
174-218: LGTM!Comprehensive test coverage of the input reader getter/setter functionality with appropriate use of reflection to verify protected members.
220-332: LGTM!Comprehensive test coverage of the delegation pattern for interactive methods. Tests verify correct delegation to the input reader, response handling, and edge cases including defaults and numeric indices.
365-405: LGTM!Clean test helper implementation that exposes protected interactive methods for testing. This approach is more maintainable than using reflection in each test method.
src/Cli/IO/StdinInputReader.php (4)
20-22: LGTM!Clean constructor using PHP 8+ promoted property syntax. The Output dependency is appropriately required for prompt display.
27-32: LGTM!Correct implementation with proper error handling for
fgets()returning false and appropriate trimming of user input.
37-49: LGTM!Good user experience with clear default indicators and case-insensitive acceptance of common truthy values (y, yes, true, 1).
54-74: LGTM!The platform-specific echo hiding is implemented securely with hardcoded
system()arguments (no injection risk). The Windows fallback is appropriately documented.tests/Cli/IO/StdinInputReaderTest.php (5)
14-25: LGTM!Standard test setup and constructor verification.
27-42: LGTM!Appropriate mocking strategy to test default value handling without STDIN interaction.
92-108: LGTM!Appropriate verification of the default-based suffix using PHPUnit's
stringContainsmatcher.
110-177: LGTM!Comprehensive test coverage of choice functionality including default handling, numeric index selection, exact matches, and retry behavior. Good use of
willReturnOnConsecutiveCallsfor testing the retry flow.
179-192: LGTM!Limited test scope is appropriate given the challenge of testing terminal echo behavior in unit tests. The comments clearly explain the limitation.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.