test: remove redundant tests - #34
Conversation
Remove 'loads existing servers from inventory' test as it duplicates functionality already covered by the comprehensive CRUD lifecycle test. The existing test already verifies server loading and findByName operations.
- Remove 'saves server when confirmation is given' (duplicate of SSH test) - Remove 'shows default SSH key path when not provided' (covered by minimal options test) - Consolidate 'persists server data' and 'displays complete server info' into single comprehensive test - Maintain full coverage while reducing test redundancy by 75 lines
- Remove duplicate 'deletes server when confirmation is provided' test - Consolidate two display verification tests into single dataset-driven test - Use Pest's ->with() pattern to test both custom and default SSH key scenarios - Reduce redundancy by 41 lines while maintaining comprehensive coverage
Remove 'displays all server fields correctly' test as it duplicates functionality already covered by 'lists single server with complete details'. The existing test already verifies all server field display behavior.
WalkthroughConsolidates and removes several integration and unit tests around server console commands and repository loading. Adds a parameterized deletion-display test, merges add-command display and persistence into one test, removes a comprehensive list-display test, and drops a repository load-from-inventory test. No production code or public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as Console: server:add
participant Inv as Inventory
User->>CLI: Provide Name, Host, Port, User, Key
CLI->>CLI: Display server details (preview)
User-->>CLI: Confirm add
CLI->>Inv: Save server entry
Inv-->>CLI: Persisted
CLI-->>User: Success message and exit SUCCESS
sequenceDiagram
autonumber
actor User
participant CLI as Console: server:delete
participant Inv as Inventory
User->>CLI: --name=<server>
CLI->>Inv: Find server by name
Inv-->>CLI: ServerDTO
CLI->>CLI: Display server info (pre-delete)
alt Confirmed/Non-interactive
CLI->>Inv: Delete server
Inv-->>CLI: Deleted
CLI-->>User: Success/exit code
else Not confirmed
CLI-->>User: Aborted/exit code
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/Integration/Console/Server/ServerAddCommandTest.php(1 hunks)tests/Integration/Console/Server/ServerDeleteCommandTest.php(1 hunks)tests/Integration/Console/Server/ServerListCommandTest.php(0 hunks)tests/Unit/Repositories/ServerRepositoryTest.php(0 hunks)
💤 Files with no reviewable changes (2)
- tests/Unit/Repositories/ServerRepositoryTest.php
- tests/Integration/Console/Server/ServerListCommandTest.php
🧰 Additional context used
📓 Path-based instructions (3)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
**/*.php: Eliminate single-use private methods by inlining them directly
Cache expensive computed values by initializing them in the constructor instead of recomputing
Prefer direct property access over method calls when appropriate to avoid call overhead
Organize code into comment-separated sections; prefer alphabetical ordering when it does not conflict with logical grouping
**/*.php: Adhere to PSR-12 coding style in all PHP files
Declare strict_types=1 at the top of every PHP file
Prefer PHP 8.x features (union types, match, attributes, readonly) where appropriate
Always import classes with use statements; avoid fully qualified class names in code bodies
All methods must declare explicit return types; use proper generics in types/docblocks (e.g., Collection<int, User>)
Use dependency injection instead of manually resolving or instantiating classes
Prefer Symfony component classes (e.g., Filesystem, Process) over native PHP functions for testability
All object creation must use $container->build(ClassName::class) instead of new, except for value objects/DTOs/pure data structures
In production code, access the Container via constructor injection, not via static/global access
Add minimal DocBlock comments with descriptions, parameters, and return types for classes and functions
Use comments as visual separators for sections/subsections with a single newline between header, subheader, and paragraph; avoid obvious or stale comments
Run rector on changed PHP files before completing a task
Run pint on changed PHP files to fix code style before completing a task
Files:
tests/Integration/Console/Server/ServerDeleteCommandTest.phptests/Integration/Console/Server/ServerAddCommandTest.php
{tests/**,test/**,**/*@(Test|Spec).php}
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Do not run or edit tests unless explicitly instructed
Files:
tests/Integration/Console/Server/ServerDeleteCommandTest.phptests/Integration/Console/Server/ServerAddCommandTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
tests/**/*.php: In tests, direct Container instantiation and bind() for mocks is allowed and encouraged for isolation
Do not run PHPStan on test files; tests are excluded from static analysis
tests/**/*.php: Unit tests must instantiate services manually (no DI container)
Command/integration tests must use mockCommandContainer() for building commands and overriding services
Only use container auto-wiring in tests to verify DI configuration or multi-service integration (edge cases)
Keep test files under 1.8x the size of the source they test (without sacrificing readability)
Test core business logic; avoid testing the framework itself
Prefer dataset-driven testing using ->with([...]) for multiple scenarios
Consolidate related assertions (e.g., expect($x)->toBe(...)->and($y)->toBe(...))
Mock only external dependencies; keep unit tests isolated from filesystem/HTTP/processes
Avoid performance tests unless performance is the primary concern
Use the AAA pattern in tests (Arrange, Act, Assert; optional Cleanup)
In exception tests, use a combined // ACT & ASSERT step when the act triggers the assertion
Organize tests with describe() blocks, beforeEach() setup, and shared helpers/traits for DRY
Forbidden assertions in tests: type-only or generic checks (e.g., toBeInstanceOf, toBeArray, not->toBeNull, expect(true)->toBeTrue) and sleep(...); prefer time mocking
Preferred assertions: assert observable behavior and interactions (e.g., domain values, validator outcomes, mock expectations)
Unit tests: mock all external dependencies, test single units in isolation, and complete in milliseconds
Integration tests: use real file operations and external processes; cover CLI commands and full workflows
Do not require PHPStan compliance in tests; avoid excessive phpdoc solely to satisfy types in tests
Files:
tests/Integration/Console/Server/ServerDeleteCommandTest.phptests/Integration/Console/Server/ServerAddCommandTest.php
🧬 Code graph analysis (2)
tests/Integration/Console/Server/ServerDeleteCommandTest.php (2)
app/DTOs/ServerDTO.php (1)
ServerDTO(7-17)app/Console/Server/ServerDeleteCommand.php (1)
execute(41-132)
tests/Integration/Console/Server/ServerAddCommandTest.php (3)
tests/TestHelpers.php (1)
mockSSHServiceWithBehavior(228-231)app/Contracts/BaseCommand.php (1)
execute(113-134)app/Console/Server/ServerAddCommand.php (1)
execute(50-202)
🔇 Additional comments (1)
tests/Integration/Console/Server/ServerDeleteCommandTest.php (1)
130-154: Good consolidation with parameterized testing.The data-driven approach effectively reduces duplication while maintaining coverage for both custom and default key path scenarios. The test structure follows best practices.
As per coding guidelines: "Prefer dataset-driven testing using ->with([...]) for multiple scenarios"
| it('displays complete server information and persists to inventory', function () { | ||
| // ARRANGE | ||
| $sshService = mockSSHServiceWithBehavior(true); | ||
| $tester = createServerAddCommandTester($sshService); | ||
|
|
||
| // ACT - Provide all required options | ||
| ob_start(); | ||
| $tester->execute([ | ||
| '--name' => 'display-test', | ||
| $exitCode = $tester->execute([ | ||
| '--name' => 'complete-test', | ||
| '--host' => 'example.com', | ||
| '--port' => '22', | ||
| '--port' => '8022', | ||
| '--username' => 'deployer', | ||
| '--private-key-path' => '~/.ssh/key', | ||
| '--skip' => true, | ||
| '--yes' => true, | ||
| ]); | ||
| ob_end_clean(); | ||
|
|
||
| // ASSERT | ||
| // ASSERT - Verify display AND persistence | ||
| $output = $tester->getDisplay(); | ||
| expect($output)->toContain('Name:') | ||
| ->and($output)->toContain('display-test') | ||
| expect($exitCode)->toBe(Command::SUCCESS) | ||
| ->and($output)->toContain('Name:') | ||
| ->and($output)->toContain('complete-test') | ||
| ->and($output)->toContain('Host:') | ||
| ->and($output)->toContain('example.com') | ||
| ->and($output)->toContain('Port:') | ||
| ->and($output)->toContain('22') | ||
| ->and($output)->toContain('8022') | ||
| ->and($output)->toContain('User:') | ||
| ->and($output)->toContain('deployer') | ||
| ->and($output)->toContain('Key:') | ||
| ->and($output)->toContain('~/.ssh/key'); | ||
| }); | ||
|
|
||
| it('shows default SSH key path when not provided', function () { | ||
| // ARRANGE | ||
| $sshService = mockSSHServiceWithBehavior(true); | ||
| $tester = createServerAddCommandTester($sshService); | ||
|
|
||
| // ACT - Provide all required options except private-key-path to test default | ||
| ob_start(); | ||
| $tester->execute([ | ||
| '--name' => 'default-key', | ||
| '--host' => '192.168.1.1', | ||
| '--port' => '22', | ||
| '--username' => 'root', | ||
| '--private-key-path' => '', | ||
| '--skip' => true, | ||
| '--yes' => true, | ||
| ]); | ||
| ob_end_clean(); | ||
|
|
||
| // ASSERT | ||
| $output = $tester->getDisplay(); | ||
| expect($output)->toContain('Key:') | ||
| ->and($output)->toContain('default') | ||
| ->and($output)->toContain('~/.ssh/id_ed25519') | ||
| ->and($output)->toContain('~/.ssh/id_rsa'); | ||
| ->and($output)->toContain('~/.ssh/key') | ||
| ->and($output)->toContain('✓') | ||
| ->and($output)->toContain('Server added successfully'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Clarify indirect persistence verification in comment.
The test verifies persistence indirectly through the success exit code and message, which is appropriate for integration tests. However, the comment at line 278 says "Verify display AND persistence" without noting this is indirect verification.
Consider updating the comment to be more explicit:
- // ASSERT - Verify display AND persistence
+ // ASSERT - Verify display and indirect persistence (via success indicators)Also, note that the test uses a custom key path. Explicit verification of default key path display (when --private-key-path is empty) appears to have been removed, though it's still implicitly tested by the "adds server with minimal options using defaults" test at lines 61-87.
Run the following verification to confirm default key path display is still tested:
🏁 Script executed:
#!/bin/bash
# Description: Verify that default SSH key path display is tested
# Search for test assertions checking default key path output
rg -nP --type=php -A5 -B5 'id_ed25519|id_rsa' tests/Integration/Console/Server/ServerAddCommandTest.phpLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any default SSH key path references in the test
rg -n "~/.ssh" -C3 tests/Integration/Console/Server/ServerAddCommandTest.phpLength of output: 867
🏁 Script executed:
#!/bin/bash
# Locate minimal-options test to verify default key-path assertion
rg -n --type=php "minimal options" -C5 tests/Integration/Console/Server/ServerAddCommandTest.phpLength of output: 479
Clarify indirect persistence verification.
- Update the assertion comment at line 278:
- // ASSERT - Verify display AND persistence
+ // ASSERT - Verify display and indirect persistence (via exit code + message)- Optional: In the “adds server with minimal options using defaults” test (lines 61–87), add an assertion for the default SSH key path (e.g.,
~/.ssh/id_rsa).
🤖 Prompt for AI Agents
In tests/Integration/Console/Server/ServerAddCommandTest.php around lines 260 to
293 (specifically update the assertion comment at line 278), clarify that the
test verifies display output and indirectly confirms persistence to inventory by
checking the success message and output strings; replace the vague comment "//
ASSERT - Verify display AND persistence" with a clearer one like "// ASSERT -
Verify display output and that command indicates server was persisted (success
message used as indirect persistence check)". Also optionally add an assertion
in the "adds server with minimal options using defaults" test (lines 61–87) to
assert the default SSH key path (e.g., expect output or stored value toContain
'~/.ssh/id_rsa') so the default key behavior is explicitly tested.
1 participant
Summary by CodeRabbit