refactor: tests container based mocking - #32
Conversation
Add bind() method to Container class to allow registering concrete instances for testing. This enables dependency injection mocking while maintaining the existing auto-wiring behavior for production code. - Add bindings array to store registered instances - Add bind() method with fluent interface - Modify build() to check bindings before auto-wiring - Maintain backward compatibility with existing usage
…iner Replace the redundant mockTestConsoleCommand() function with a more flexible mockCommandContainer() that creates a Container with all BaseCommand dependencies pre-bound for testing. - Remove mockTestConsoleCommand() function (35 lines eliminated) - Add mockCommandContainer() with configurable service overrides - Remove unused TestConsoleCommand import - Add PrompterService import for proper type hints This centralizes command testing setup and reduces maintenance burden when adding new services to BaseCommand.
Replace manual dependency injection with container-based approach using mockCommandContainer() for all server command tests. - ServerAddCommandTest: 16 lines → 3 lines (81% reduction) - ServerListCommandTest: 21 lines → 8 lines (62% reduction) - ServerDeleteCommandTest: 21 lines → 8 lines (62% reduction) - Remove unused Container and ServerRepository imports This eliminates the need to manually pass all BaseCommand dependencies, making tests more maintainable and consistent.
Update ConsoleInputTrait, ConsoleOutputTrait, and ServerHelpersTrait tests to use the new mockCommandContainer() approach instead of the deprecated mockTestConsoleCommand() function. - Replace mockTestConsoleCommand() calls with container->build() - Maintain same test behavior with cleaner setup - Consistent with other command tests
Add tests for the new Container::bind() method to ensure proper functionality and integration with existing auto-wiring system. - Test basic binding and retrieval of instances - Test bound instances override auto-wiring - Test bound instances propagate through dependency chains - Test fluent interface return value Also update TestHelpersTest to replace mockTestConsoleCommand tests with mockCommandContainer tests, including verification of service override capabilities and inventory data handling.
WalkthroughAdds a binding API to the DI Container (Container::bind) that lets tests register concrete instances; container build short-circuits to return bound instances. Test helpers and many tests are refactored to return/use a container (mockCommandContainer) and to build commands via the container with bound service overrides. Docs updated to show binding usage. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant C as Container
participant B as Bindings Map
participant R as Resolver
participant Cmd as Command
Tester->>C: bind(ServiceClass, mockedInstance)
Note right of C: store bound instance
Tester->>C: build(CommandClass)
C->>B: has(CommandClass)?
alt Bound
B-->>C: return bound instance
C-->>Tester: Command instance (from bindings)
else Not bound
C->>R: resolve(CommandClass)
R->>C: construct with deps (using bindings if present)
C-->>Tester: Command instance
end
sequenceDiagram
autonumber
actor Tester
participant C as Container
participant B as Bindings Map
participant R as Resolver
participant Repo as ServerRepository
participant Cmd as ServerDeleteCommand
Tester->>C: bind(ServerRepository, TestRepo)
Tester->>C: build(ServerDeleteCommand)
C->>B: has(ServerDeleteCommand)?
alt Not bound
C->>R: resolve(ServerDeleteCommand)
R->>C: inject(ServerRepository = TestRepo)
C-->>Tester: ServerDeleteCommand (with TestRepo)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used📓 Path-based instructions (3)**/*.php📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Files:
{tests/**,test/**,**/*@(Test|Spec).php}📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Files:
tests/**/*.php📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
Files:
🧠 Learnings (7)📚 Learning: 2025-10-04T14:36:00.065ZApplied to files:
📚 Learning: 2025-10-04T14:36:28.928ZApplied to files:
📚 Learning: 2025-10-04T14:36:28.928ZApplied to files:
📚 Learning: 2025-10-04T14:36:28.928ZApplied to files:
📚 Learning: 2025-10-04T14:36:28.928ZApplied to files:
📚 Learning: 2025-10-04T14:36:28.928ZApplied to files:
📚 Learning: 2025-10-04T14:36:28.928ZApplied to files:
🧬 Code graph analysis (3)tests/Unit/ContainerTest.php (2)
tests/TestHelpers.php (5)
tests/Integration/Console/Server/ServerAddCommandTest.php (2)
🔇 Additional comments (7)
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: 3
🧹 Nitpick comments (1)
app/Container.php (1)
46-49: Validate bound instance types.Please guard
bind()with aninstanceof $classNamecheck (throwing immediately) so accidental misbindings fail fast instead of surfacing later during constructor injection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/Container.php(2 hunks)tests/Integration/Console/Server/ServerAddCommandTest.php(3 hunks)tests/Integration/Console/Server/ServerDeleteCommandTest.php(1 hunks)tests/Integration/Console/Server/ServerListCommandTest.php(1 hunks)tests/TestHelpers.php(2 hunks)tests/Unit/ContainerTest.php(1 hunks)tests/Unit/TestHelpersTest.php(1 hunks)tests/Unit/Traits/ConsoleInputTraitTest.php(1 hunks)tests/Unit/Traits/ConsoleOutputTraitTest.php(1 hunks)tests/Unit/Traits/ServerHelpersTraitTest.php(1 hunks)
🧰 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 for all PHP code
Declare strict_types and write code assuming strict typing
Prefer PHP 8.x language features where appropriate (union types, match, attributes, readonly)
Use import statements (use ...) instead of fully qualified class names in code
All methods must have explicit return types, and use proper generics in types (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), except value objects, DTOs, and pure data structures
In production code, access the container via constructor injection (no service locator or global access)
Add DocBlock comments with minimalist descriptions, parameters, and return types for classes and functions
Use comments as visual separators with section headers/subheaders and avoid obvious or stale comments
Files:
tests/Integration/Console/Server/ServerListCommandTest.phptests/Unit/Traits/ConsoleOutputTraitTest.phptests/Unit/TestHelpersTest.phptests/Unit/Traits/ServerHelpersTraitTest.phptests/Integration/Console/Server/ServerAddCommandTest.phptests/Unit/ContainerTest.phptests/TestHelpers.phptests/Integration/Console/Server/ServerDeleteCommandTest.phptests/Unit/Traits/ConsoleInputTraitTest.phpapp/Container.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/ServerListCommandTest.phptests/Unit/Traits/ConsoleOutputTraitTest.phptests/Unit/TestHelpersTest.phptests/Unit/Traits/ServerHelpersTraitTest.phptests/Integration/Console/Server/ServerAddCommandTest.phptests/Unit/ContainerTest.phptests/TestHelpers.phptests/Integration/Console/Server/ServerDeleteCommandTest.phptests/Unit/Traits/ConsoleInputTraitTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
tests/**/*.php: In tests, it's allowed to instantiate the Container directly for isolation
Exclude tests from PHPStan static analysis
tests/**/*.php: Write Pest tests usingit()syntax
In unit tests, prefer manual instantiation with explicit mocks (e.g.,new ClassName(...)) over using the DI container
Use the DI container only in integration tests when verifying multiple services together or DI configuration
DI container conventions for production do not apply to tests
Apply the AAA pattern (Arrange, Act, Assert) in every test
For exception tests, combine phases with a// ACT & ASSERTcomment when the act triggers the assertion
Organize tests withdescribe()blocks and shared setup viabeforeEach(); extract helpers/traits for DRY tests
Keep test files under 1.8x the size of the source they test, without sacrificing readability
Focus tests on core business logic; avoid testing the framework itself
Use dataset-driven testing with->with([])for multiple scenarios
Eliminate overlap: avoid multiple tests covering the same functionality
Consolidate assertions using chained expectations (e.g.,expect($x)->toBe(1)->and($y)->toBe(2))
Mock only external dependencies; do not perform real I/O in unit tests
Avoid performance tests unless performance is the primary concern
Forbidden in tests:toBeInstanceOf(...),toBeArray(), genericnot->toBeNull(),expect(true)->toBeTrue(), andsleep(...)(use time mocking)
Prefer meaningful assertions on behavior and values (e.g., checking specific config values, validator outcomes, and configured mock interactions)
Unit tests: mock all external dependencies, test single units in isolation, complete in milliseconds
Integration tests: allow real file operations and external processes; cover CLI commands and full workflows
Do not run or gate tests on PHPStan compliance; ignore PHPStan issues in tests and avoid excessive phpdoc just for types
Files:
tests/Integration/Console/Server/ServerListCommandTest.phptests/Unit/Traits/ConsoleOutputTraitTest.phptests/Unit/TestHelpersTest.phptests/Unit/Traits/ServerHelpersTraitTest.phptests/Integration/Console/Server/ServerAddCommandTest.phptests/Unit/ContainerTest.phptests/TestHelpers.phptests/Integration/Console/Server/ServerDeleteCommandTest.phptests/Unit/Traits/ConsoleInputTraitTest.php
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-04T14:36:28.928Z
Learning: Applies to tests/**/*.php : Use the DI container only in integration tests when verifying multiple services together or DI configuration
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-04T14:36:28.928Z
Learning: Applies to tests/**/*.php : In unit tests, prefer manual instantiation with explicit mocks (e.g., `new ClassName(...)`) over using the DI container
📚 Learning: 2025-10-04T14:36:28.928Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-04T14:36:28.928Z
Learning: Applies to tests/**/*.php : In unit tests, prefer manual instantiation with explicit mocks (e.g., `new ClassName(...)`) over using the DI container
Applied to files:
tests/Unit/Traits/ConsoleOutputTraitTest.php
📚 Learning: 2025-10-04T14:36:00.065Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-10-04T14:36:00.065Z
Learning: Applies to tests/**/*.php : In tests, it's allowed to instantiate the Container directly for isolation
Applied to files:
tests/Unit/ContainerTest.php
🧬 Code graph analysis (9)
tests/Integration/Console/Server/ServerListCommandTest.php (2)
tests/TestHelpers.php (1)
mockCommandContainer(373-401)app/Container.php (1)
build(59-85)
tests/Unit/Traits/ConsoleOutputTraitTest.php (3)
tests/TestHelpers.php (1)
mockCommandContainer(373-401)app/Container.php (1)
build(59-85)tests/Fixtures/TestConsoleCommand.php (1)
TestConsoleCommand(25-236)
tests/Unit/TestHelpersTest.php (3)
tests/TestHelpers.php (2)
mockCommandContainer(373-401)mockSSHServiceWithBehavior(228-231)app/Container.php (1)
build(59-85)app/Repositories/ServerRepository.php (2)
ServerRepository(15-168)all(83-93)
tests/Unit/Traits/ServerHelpersTraitTest.php (3)
tests/TestHelpers.php (1)
mockCommandContainer(373-401)app/Container.php (1)
build(59-85)tests/Fixtures/TestConsoleCommand.php (1)
TestConsoleCommand(25-236)
tests/Integration/Console/Server/ServerAddCommandTest.php (3)
tests/TestHelpers.php (1)
mockCommandContainer(373-401)app/Container.php (1)
build(59-85)app/Repositories/ServerRepository.php (1)
ServerRepository(15-168)
tests/Unit/ContainerTest.php (2)
tests/Fixtures/ContainerFixtures.php (6)
SimpleService(11-17)ServiceWithMultipleDeps(43-58)getSimple(49-52)getComplex(54-57)getDependency(37-40)getDependency(165-168)app/Container.php (2)
bind(46-50)build(59-85)
tests/TestHelpers.php (6)
app/Services/PrompterService.php (1)
PrompterService(24-246)app/Services/EnvService.php (1)
EnvService(12-134)app/Services/InventoryService.php (1)
InventoryService(33-264)app/Repositories/ServerRepository.php (1)
ServerRepository(15-168)app/Services/SSHService.php (1)
SSHService(40-327)app/Container.php (2)
Container(23-250)bind(46-50)
tests/Integration/Console/Server/ServerDeleteCommandTest.php (2)
tests/TestHelpers.php (1)
mockCommandContainer(373-401)app/Container.php (1)
build(59-85)
tests/Unit/Traits/ConsoleInputTraitTest.php (3)
tests/TestHelpers.php (1)
mockCommandContainer(373-401)app/Container.php (1)
build(59-85)tests/Fixtures/TestConsoleCommand.php (1)
TestConsoleCommand(25-236)
🔇 Additional comments (7)
tests/Integration/Console/Server/ServerListCommandTest.php (1)
30-31: LGTM! Container-based assembly aligns with integration test guidelines.The refactor correctly uses
mockCommandContainerto construct the command via the container, simplifying dependency management in integration tests.Based on learnings: Integration tests are the appropriate place for container usage when verifying multiple services together.
tests/Integration/Console/Server/ServerDeleteCommandTest.php (1)
30-31: LGTM! Consistent container-based pattern.The refactor matches the approach in other integration tests, maintaining consistency across the test suite.
tests/Integration/Console/Server/ServerAddCommandTest.php (2)
19-20: LGTM! Simplified test setup.Container-based construction eliminates manual dependency wiring, making the test helper cleaner and more maintainable.
289-290: LGTM! Proper integration test pattern for persistence verification.Building the command and then resolving the repository from the same container correctly verifies that the command's operations persisted data through the shared container dependencies.
Also applies to: 307-307
tests/Unit/TestHelpersTest.php (1)
172-210: LGTM! Unit tests appropriately validate container behavior.These tests verify that
mockCommandContainercorrectly binds services and supports overrides. Testing the container's binding mechanism is an appropriate use of the container in unit tests, as it validates test infrastructure rather than business logic.tests/TestHelpers.php (2)
11-11: LGTM! Import added for PrompterService.The new import supports the expanded parameter list in
mockCommandContainer.
348-401: LGTM! Well-designed container-based test helper.The refactor from
mockTestConsoleCommandtomockCommandContainerprovides:
- Flexible service override capability via optional parameters
- Clean binding of mock services to the container
- Support for inventory data injection
- Clear documentation with practical examples
This enables integration tests to use container-based assembly while maintaining testability through service overrides.
| $container = mockCommandContainer(); | ||
| $this->command = $container->build(\Bigpixelrocket\DeployerPHP\Tests\Fixtures\TestConsoleCommand::class); | ||
| $this->tester = new CommandTester($this->command); | ||
| }); |
There was a problem hiding this comment.
Container-based setup violates unit-test rule.
Please stop using mockCommandContainer()->build(...) in this unit spec; construct the command explicitly so the test remains a true unit test per our standards. Based on learnings
🤖 Prompt for AI Agents
In tests/Unit/Traits/ConsoleInputTraitTest.php around lines 13 to 16, the test
currently uses mockCommandContainer()->build(...) which relies on the container
and violates the unit-test rule; replace the container-based construction by
instantiating the TestConsoleCommand directly and supplying any dependencies as
explicit mocks or stubs. Remove the mockCommandContainer() call, create
necessary mock dependencies (or use real lightweight/fake implementations) and
pass them into new
\Bigpixelrocket\DeployerPHP\Tests\Fixtures\TestConsoleCommand(...), then create
the CommandTester with that instance so the test remains a true unit test.
| $container = mockCommandContainer(); | ||
| $this->command = $container->build(\Bigpixelrocket\DeployerPHP\Tests\Fixtures\TestConsoleCommand::class); | ||
| $this->tester = new CommandTester($this->command); | ||
| }); |
There was a problem hiding this comment.
Unit test should avoid the container.
These trait tests now resolve the command through the DI container, which conflicts with our unit-testing rule to instantiate subjects manually with explicit mocks. Please revert to direct construction (e.g., new TestConsoleCommand(...)) and keep container usage for integration coverage. Based on learnings
🤖 Prompt for AI Agents
In tests/Unit/Traits/ConsoleOutputTraitTest.php around lines 13 to 16, the test
currently resolves the command via the DI container
(mockCommandContainer()->build(...)); change this to construct the command
directly (new TestConsoleCommand(...)) and pass explicit mock dependencies
required by the command (create mocks/stubs for each constructor parameter) so
the test remains a pure unit test; then instantiate CommandTester with the newly
constructed command. Leave any container-based resolution to integration tests
only.
| $container = mockCommandContainer(); | ||
| $this->command = $container->build(\Bigpixelrocket\DeployerPHP\Tests\Fixtures\TestConsoleCommand::class); | ||
| $this->tester = new CommandTester($this->command); |
There was a problem hiding this comment.
Inline subject creation for unit scope.
Same concern here: building the command via the container breaks the “manual wiring in unit tests” guideline. Instantiate TestConsoleCommand directly with the mocked services instead. Based on learnings
🤖 Prompt for AI Agents
In tests/Unit/Traits/ServerHelpersTraitTest.php around lines 14 to 16, the test
builds the command via the DI container which violates the unit test guideline
to manually wire dependencies; replace the container build with a direct
instantiation of \Bigpixelrocket\DeployerPHP\Tests\Fixtures\TestConsoleCommand
by creating or reusing the mocked services used in this test and passing them
into the command's constructor, then create the CommandTester with that instance
(remove the container usage and ensure all required dependencies are explicitly
mocked and injected).
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
app/Container.php (1)
38-50: Consider broader use case in docblock.The docblock states "Bind a concrete instance to a class name for testing," but this binding mechanism could be useful beyond testing (e.g., for runtime configuration or plugin systems). Consider updating the docblock to reflect a broader scope or clarifying that while designed primarily for testing, it's a general-purpose feature.
That said, the implementation is clean:
- Fluent interface for chaining
- Proper type safety with generics
- Simple and focused
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/Container.php(2 hunks)tests/Integration/Console/Server/ServerAddCommandTest.php(3 hunks)tests/Integration/Console/Server/ServerDeleteCommandTest.php(1 hunks)tests/Integration/Console/Server/ServerListCommandTest.php(1 hunks)tests/TestHelpers.php(2 hunks)tests/Unit/ContainerTest.php(1 hunks)tests/Unit/TestHelpersTest.php(1 hunks)tests/Unit/Traits/ConsoleInputTraitTest.php(1 hunks)tests/Unit/Traits/ConsoleOutputTraitTest.php(1 hunks)tests/Unit/Traits/ServerHelpersTraitTest.php(1 hunks)
🧰 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 for all PHP code
Declare strict_types and write code assuming strict typing
Prefer PHP 8.x language features where appropriate (union types, match, attributes, readonly)
Use import statements (use ...) instead of fully qualified class names in code
All methods must have explicit return types, and use proper generics in types (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), except value objects, DTOs, and pure data structures
In production code, access the container via constructor injection (no service locator or global access)
Add DocBlock comments with minimalist descriptions, parameters, and return types for classes and functions
Use comments as visual separators with section headers/subheaders and avoid obvious or stale comments
Files:
tests/Unit/ContainerTest.phptests/Unit/Traits/ServerHelpersTraitTest.phptests/Unit/Traits/ConsoleInputTraitTest.phptests/Unit/TestHelpersTest.phpapp/Container.phptests/Integration/Console/Server/ServerDeleteCommandTest.phptests/Unit/Traits/ConsoleOutputTraitTest.phptests/Integration/Console/Server/ServerAddCommandTest.phptests/Integration/Console/Server/ServerListCommandTest.phptests/TestHelpers.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/Unit/ContainerTest.phptests/Unit/Traits/ServerHelpersTraitTest.phptests/Unit/Traits/ConsoleInputTraitTest.phptests/Unit/TestHelpersTest.phptests/Integration/Console/Server/ServerDeleteCommandTest.phptests/Unit/Traits/ConsoleOutputTraitTest.phptests/Integration/Console/Server/ServerAddCommandTest.phptests/Integration/Console/Server/ServerListCommandTest.phptests/TestHelpers.php
tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
tests/**/*.php: In tests, it's allowed to instantiate the Container directly for isolation
Exclude tests from PHPStan static analysis
tests/**/*.php: Write Pest tests usingit()syntax
In unit tests, prefer manual instantiation with explicit mocks (e.g.,new ClassName(...)) over using the DI container
Use the DI container only in integration tests when verifying multiple services together or DI configuration
DI container conventions for production do not apply to tests
Apply the AAA pattern (Arrange, Act, Assert) in every test
For exception tests, combine phases with a// ACT & ASSERTcomment when the act triggers the assertion
Organize tests withdescribe()blocks and shared setup viabeforeEach(); extract helpers/traits for DRY tests
Keep test files under 1.8x the size of the source they test, without sacrificing readability
Focus tests on core business logic; avoid testing the framework itself
Use dataset-driven testing with->with([])for multiple scenarios
Eliminate overlap: avoid multiple tests covering the same functionality
Consolidate assertions using chained expectations (e.g.,expect($x)->toBe(1)->and($y)->toBe(2))
Mock only external dependencies; do not perform real I/O in unit tests
Avoid performance tests unless performance is the primary concern
Forbidden in tests:toBeInstanceOf(...),toBeArray(), genericnot->toBeNull(),expect(true)->toBeTrue(), andsleep(...)(use time mocking)
Prefer meaningful assertions on behavior and values (e.g., checking specific config values, validator outcomes, and configured mock interactions)
Unit tests: mock all external dependencies, test single units in isolation, complete in milliseconds
Integration tests: allow real file operations and external processes; cover CLI commands and full workflows
Do not run or gate tests on PHPStan compliance; ignore PHPStan issues in tests and avoid excessive phpdoc just for types
Files:
tests/Unit/ContainerTest.phptests/Unit/Traits/ServerHelpersTraitTest.phptests/Unit/Traits/ConsoleInputTraitTest.phptests/Unit/TestHelpersTest.phptests/Integration/Console/Server/ServerDeleteCommandTest.phptests/Unit/Traits/ConsoleOutputTraitTest.phptests/Integration/Console/Server/ServerAddCommandTest.phptests/Integration/Console/Server/ServerListCommandTest.phptests/TestHelpers.php
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-04T14:36:28.928Z
Learning: Applies to tests/**/*.php : Use the DI container only in integration tests when verifying multiple services together or DI configuration
📚 Learning: 2025-10-04T14:36:00.065Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-10-04T14:36:00.065Z
Learning: Applies to tests/**/*.php : In tests, it's allowed to instantiate the Container directly for isolation
Applied to files:
tests/Unit/ContainerTest.php
📚 Learning: 2025-10-04T14:36:28.928Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-04T14:36:28.928Z
Learning: Applies to tests/**/*.php : In unit tests, prefer manual instantiation with explicit mocks (e.g., `new ClassName(...)`) over using the DI container
Applied to files:
tests/Unit/ContainerTest.php
📚 Learning: 2025-10-04T14:36:28.928Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-04T14:36:28.928Z
Learning: Applies to tests/**/*.php : DI container conventions for production do not apply to tests
Applied to files:
tests/Unit/ContainerTest.php
📚 Learning: 2025-10-04T14:36:28.928Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-04T14:36:28.928Z
Learning: Applies to tests/**/*.php : Use the DI container only in integration tests when verifying multiple services together or DI configuration
Applied to files:
tests/Unit/ContainerTest.php
🧬 Code graph analysis (9)
tests/Unit/ContainerTest.php (2)
tests/Fixtures/ContainerFixtures.php (6)
SimpleService(11-17)ServiceWithMultipleDeps(43-58)getSimple(49-52)getComplex(54-57)getDependency(37-40)getDependency(165-168)app/Container.php (2)
bind(46-50)build(59-85)
tests/Unit/Traits/ServerHelpersTraitTest.php (3)
tests/TestHelpers.php (1)
mockCommandContainer(373-401)app/Container.php (1)
build(59-85)tests/Fixtures/TestConsoleCommand.php (1)
TestConsoleCommand(25-236)
tests/Unit/Traits/ConsoleInputTraitTest.php (3)
tests/TestHelpers.php (1)
mockCommandContainer(373-401)app/Container.php (1)
build(59-85)tests/Fixtures/TestConsoleCommand.php (1)
TestConsoleCommand(25-236)
tests/Unit/TestHelpersTest.php (4)
tests/TestHelpers.php (2)
mockCommandContainer(373-401)mockSSHServiceWithBehavior(228-231)app/Container.php (1)
build(59-85)tests/Fixtures/TestConsoleCommand.php (1)
TestConsoleCommand(25-236)app/Repositories/ServerRepository.php (2)
ServerRepository(15-168)all(83-93)
tests/Integration/Console/Server/ServerDeleteCommandTest.php (2)
tests/TestHelpers.php (1)
mockCommandContainer(373-401)app/Container.php (1)
build(59-85)
tests/Unit/Traits/ConsoleOutputTraitTest.php (3)
tests/TestHelpers.php (1)
mockCommandContainer(373-401)app/Container.php (1)
build(59-85)tests/Fixtures/TestConsoleCommand.php (1)
TestConsoleCommand(25-236)
tests/Integration/Console/Server/ServerAddCommandTest.php (3)
tests/TestHelpers.php (1)
mockCommandContainer(373-401)app/Container.php (1)
build(59-85)app/Repositories/ServerRepository.php (1)
ServerRepository(15-168)
tests/Integration/Console/Server/ServerListCommandTest.php (2)
tests/TestHelpers.php (1)
mockCommandContainer(373-401)app/Container.php (1)
build(59-85)
tests/TestHelpers.php (3)
app/Services/PrompterService.php (1)
PrompterService(24-246)app/Repositories/ServerRepository.php (1)
ServerRepository(15-168)app/Container.php (2)
Container(23-250)bind(46-50)
🪛 GitHub Actions: Rector
tests/TestHelpers.php
[error] 384-384: Rector dry-run indicated 1 file would be changed. Applied rules: NullCoalescingOperatorRector. Exit code 2 from 'vendor/bin/rector --dry-run'.
🔇 Additional comments (9)
tests/Integration/Console/Server/ServerListCommandTest.php (1)
30-31: LGTM!Container-based command construction in integration tests aligns with the coding guidelines, which state that the DI container should be used "when verifying multiple services together or DI configuration."
Based on learnings.
tests/Unit/ContainerTest.php (1)
135-173: LGTM!The new tests thoroughly verify the bind() functionality:
- Fluent interface returns the container
- Bound instances are returned on subsequent builds
- Bound instances propagate through dependency resolution
- Bindings correctly override auto-wiring
The tests follow the AAA pattern and provide good coverage of the new feature.
tests/Unit/TestHelpersTest.php (1)
172-209: LGTM!The new tests thoroughly verify mockCommandContainer functionality:
- Container correctly binds all BaseCommand dependencies
- Service overrides work as expected
- Inventory data overrides propagate through the repository
The tests follow the AAA pattern and provide good coverage for the new helper.
app/Container.php (1)
61-65: LGTM!The binding check in build() is correctly placed at the beginning to short-circuit auto-wiring when a binding exists. The type annotation ensures type safety is preserved.
tests/Integration/Console/Server/ServerDeleteCommandTest.php (1)
30-31: LGTM!Container-based command construction in integration tests aligns with the coding guidelines, which state that the DI container should be used "when verifying multiple services together or DI configuration."
Based on learnings.
tests/Integration/Console/Server/ServerAddCommandTest.php (2)
19-21: Good switch to container-built command in integration testsBuilding the command via the container is appropriate here and aligns with the new bind/build mechanism.
289-291: Persistence check correctly reuses the bound repositoryFetching ServerRepository from the same container ensures you validate real persistence on the exact instance the command used. Solid integration assertion.
Also applies to: 307-307
tests/TestHelpers.php (2)
11-11: Import looks correctPrompterService import is needed for the new binding.
348-401: Fix CI: install dev dependencies & apply Rector fixes. Ensurevendor/bin/rectoris available by runningcomposer install --dev, then runvendor/bin/rector --dry-runandvendor/bin/rector; commit the resulting diff to unblock CI.
| if (!function_exists('mockCommandContainer')) { | ||
| /** | ||
| * Create a TestConsoleCommand for testing with mocked dependencies. | ||
| * Create a Container with mocked dependencies for command testing. | ||
| * | ||
| * Returns a fully configured command with all dependencies injected. | ||
| * Useful for testing BaseCommand, console traits, and server helpers. | ||
| * Returns a Container with sensible mock defaults for all BaseCommand dependencies. | ||
| * Override specific services by passing them as arguments. | ||
| * | ||
| * @example | ||
| * // Default configuration | ||
| * $command = mockTestConsoleCommand(); | ||
| * // Build command with default mocks | ||
| * $container = mockCommandContainer(); | ||
| * $command = $container->build(ServerListCommand::class); | ||
| * | ||
| * @example | ||
| * // Override SSH service for connection testing | ||
| * $ssh = mockSSHServiceWithBehavior(canConnect: false); | ||
| * $container = mockCommandContainer(ssh: $ssh); | ||
| * $command = $container->build(ServerAddCommand::class); | ||
| * | ||
| * @example | ||
| * // Custom environment and inventory | ||
| * $command = mockTestConsoleCommand( | ||
| * envFileExists: true, | ||
| * envContent: 'API_KEY=secret', | ||
| * inventoryFileExists: true, | ||
| * inventoryData: ['servers' => []] | ||
| * // Override inventory data for pre-populated servers | ||
| * $container = mockCommandContainer( | ||
| * inventoryData: ['servers' => ['web1' => ['host' => '192.168.1.1']]] | ||
| * ); | ||
| * $command = $container->build(ServerListCommand::class); | ||
| */ | ||
| function mockTestConsoleCommand( | ||
| function mockCommandContainer( | ||
| ?EnvService $env = null, | ||
| ?InventoryService $inventory = null, | ||
| ?ServerRepository $servers = null, | ||
| ?SSHService $ssh = null, | ||
| ?PrompterService $prompter = null, | ||
| bool $envFileExists = true, | ||
| string $envContent = 'API_KEY=test_value', | ||
| bool $inventoryFileExists = true, | ||
| array|string $inventoryData = [] | ||
| ): TestConsoleCommand { | ||
| ): Container { | ||
| $container = new Container(); | ||
| $env = mockEnvService($envFileExists, $envContent); | ||
| $inventory = mockInventoryService($inventoryFileExists, $inventoryData); | ||
| $servers = mockServerRepository($inventoryFileExists, $inventoryData); | ||
| $ssh = mockSSHService(); | ||
| $prompter = mockPrompter(); | ||
|
|
||
| return new TestConsoleCommand($container, $env, $inventory, $servers, $ssh, $prompter); | ||
| // Build or use provided services | ||
| $env = $env ?? mockEnvService($envFileExists, $envContent); | ||
| $inventory = $inventory ?? mockInventoryService($inventoryFileExists, $inventoryData); | ||
| $servers = $servers ?? mockServerRepository($inventoryFileExists, $inventoryData); | ||
| $ssh = $ssh ?? mockSSHService(); | ||
| $prompter = $prompter ?? mockPrompter(); | ||
|
|
||
| // Bind services to container | ||
| $container->bind(EnvService::class, $env); | ||
| $container->bind(InventoryService::class, $inventory); | ||
| $container->bind(ServerRepository::class, $servers); | ||
| $container->bind(SSHService::class, $ssh); | ||
| $container->bind(PrompterService::class, $prompter); | ||
|
|
||
| return $container; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Limit container helper usage to integration tests
Avoid using mockCommandContainer in unit tests. Unit tests should manually instantiate the SUT with explicit mocks; reserve the container for integration tests that verify DI wiring. Based on learnings
Run to spot accidental usage in unit tests:
🏁 Script executed:
#!/bin/bash
# Find any unit tests using the container-based helper
rg -n -C2 -g 'tests/Unit/**' -P '\bmockCommandContainer\s*\('Length of output: 2788
Avoid mockCommandContainer in unit tests
Unit tests under tests/Unit/** (e.g., tests/Unit/TestHelpersTest.php and tests/Unit/Traits/*.php) call mockCommandContainer(). Replace these with direct instantiation of the SUT and explicit mocks; reserve mockCommandContainer for integration tests that verify DI wiring.
🧰 Tools
🪛 GitHub Actions: Rector
[error] 384-384: Rector dry-run indicated 1 file would be changed. Applied rules: NullCoalescingOperatorRector. Exit code 2 from 'vendor/bin/rector --dry-run'.
| // Build or use provided services | ||
| $env = $env ?? mockEnvService($envFileExists, $envContent); | ||
| $inventory = $inventory ?? mockInventoryService($inventoryFileExists, $inventoryData); | ||
| $servers = $servers ?? mockServerRepository($inventoryFileExists, $inventoryData); | ||
| $ssh = $ssh ?? mockSSHService(); | ||
| $prompter = $prompter ?? mockPrompter(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Bind ServerRepository to the same InventoryService instance
Currently, InventoryService is created twice: once for the container binding and once inside mockServerRepository(). This can desynchronize repo state from any code resolving InventoryService from the container. Initialize the repository with the already-created $inventory to keep state consistent.
Apply this diff:
- $env = $env ?? mockEnvService($envFileExists, $envContent);
- $inventory = $inventory ?? mockInventoryService($inventoryFileExists, $inventoryData);
- $servers = $servers ?? mockServerRepository($inventoryFileExists, $inventoryData);
- $ssh = $ssh ?? mockSSHService();
- $prompter = $prompter ?? mockPrompter();
+ $env = $env ?? mockEnvService($envFileExists, $envContent);
+ $inventory = $inventory ?? mockInventoryService($inventoryFileExists, $inventoryData);
+ if ($servers === null) {
+ $servers = new ServerRepository();
+ $servers->loadInventory($inventory);
+ }
+ $ssh = $ssh ?? mockSSHService();
+ $prompter = $prompter ?? mockPrompter();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Build or use provided services | |
| $env = $env ?? mockEnvService($envFileExists, $envContent); | |
| $inventory = $inventory ?? mockInventoryService($inventoryFileExists, $inventoryData); | |
| $servers = $servers ?? mockServerRepository($inventoryFileExists, $inventoryData); | |
| $ssh = $ssh ?? mockSSHService(); | |
| $prompter = $prompter ?? mockPrompter(); | |
| // Build or use provided services | |
| $env = $env ?? mockEnvService($envFileExists, $envContent); | |
| $inventory = $inventory ?? mockInventoryService($inventoryFileExists, $inventoryData); | |
| if ($servers === null) { | |
| $servers = new ServerRepository(); | |
| $servers->loadInventory($inventory); | |
| } | |
| $ssh = $ssh ?? mockSSHService(); | |
| $prompter = $prompter ?? mockPrompter(); |
🤖 Prompt for AI Agents
In tests/TestHelpers.php around lines 386 to 392, the InventoryService is being
created twice which can desynchronize state: replace the call that creates a new
InventoryService inside mockServerRepository with the already-initialized
$inventory instance so the repository shares the same InventoryService; update
the $servers assignment to use $inventory (e.g. $servers = $servers ??
mockServerRepository($inventory)) and ensure mockServerRepository accepts an
InventoryService parameter and the container binding uses that same $inventory.
| $container = mockCommandContainer(); | ||
| $this->command = $container->build(\Bigpixelrocket\DeployerPHP\Tests\Fixtures\TestConsoleCommand::class); |
There was a problem hiding this comment.
Reconsider container usage in unit tests.
This unit test now uses the DI container to construct the test command, which conflicts with the coding guidelines. According to the guidelines:
"In unit tests, prefer manual instantiation with explicit mocks (e.g.,
new ClassName(...)) over using the DI container""Use the DI container only in integration tests when verifying multiple services together or DI configuration"
For unit tests that focus on a single trait's behavior, consider reverting to direct instantiation with explicit mocks. The container approach is more appropriate for integration tests.
Based on learnings.
🤖 Prompt for AI Agents
In tests/Unit/Traits/ConsoleInputTraitTest.php around lines 13-14, the test is
constructing the command via the DI container which violates unit-test guidance;
replace the container usage with direct instantiation of TestConsoleCommand and
pass explicit PHPUnit mocks for any constructor dependencies (use
$this->createMock(...) for each dependency the command requires), remove
mockCommandContainer() from this unit test, and adjust setUp/teardown so the
command and its mocked dependencies are created and injected directly in the
test file.
| $container = mockCommandContainer(); | ||
| $this->command = $container->build(\Bigpixelrocket\DeployerPHP\Tests\Fixtures\TestConsoleCommand::class); |
There was a problem hiding this comment.
Reconsider container usage in unit tests.
This unit test now uses the DI container to construct the test command, which conflicts with the coding guidelines. According to the guidelines:
"In unit tests, prefer manual instantiation with explicit mocks (e.g.,
new ClassName(...)) over using the DI container""Use the DI container only in integration tests when verifying multiple services together or DI configuration"
For unit tests that focus on a single trait's behavior, consider reverting to direct instantiation with explicit mocks as was done previously. The container approach is more appropriate for integration tests.
Based on learnings.
🤖 Prompt for AI Agents
In tests/Unit/Traits/ConsoleOutputTraitTest.php around lines 13-14, the test is
constructing the command via the DI container which violates unit test
guidelines; replace container construction with direct instantiation and
explicit mocks: create any required dependency doubles (mocks/stubs) for the
command's constructor arguments and instantiate the TestConsoleCommand with new
using those mocks so the test isolates the trait behavior without relying on the
container.
| $container = mockCommandContainer(); | ||
| $this->command = $container->build(\Bigpixelrocket\DeployerPHP\Tests\Fixtures\TestConsoleCommand::class); |
There was a problem hiding this comment.
Reconsider container usage in unit tests.
This unit test now uses the DI container to construct the test command, which conflicts with the coding guidelines. According to the guidelines:
"In unit tests, prefer manual instantiation with explicit mocks (e.g.,
new ClassName(...)) over using the DI container""Use the DI container only in integration tests when verifying multiple services together or DI configuration"
For unit tests that focus on a single trait's behavior, consider reverting to direct instantiation with explicit mocks. The container approach is more appropriate for integration tests.
Based on learnings.
🤖 Prompt for AI Agents
In tests/Unit/Traits/ServerHelpersTraitTest.php around lines 14-15, the test
uses the DI container to build the TestConsoleCommand which violates unit-test
guidelines; replace the container usage with direct instantiation (new
TestConsoleCommand(...)) and supply explicit mocks/stubs for any constructor
dependencies the command needs (use PHPUnit mocks for services, input/output, or
other collaborators), update any setup to inject those mocks, and remove the
container helper so the test focuses on the trait in isolation.
Update architecture and testing rules to document the new mockCommandContainer() pattern and Container::bind() method. Testing Rules: - Add mockCommandContainer() examples for command tests - Document service override and inventory data patterns - Add maintenance note for updating mockCommandContainer() - Clarify when to use manual instantiation vs container Architecture Rules: - Document Container::bind() for test mocking - Show how bound instances override auto-wiring - Maintain clear separation between production and test usage This ensures the new pattern is properly documented and becomes the standard approach for command testing.
…tency - Fix architecture violation in ServerAddCommandTest persistence test - Use consistent helper pattern across all server command tests - Reorder mockCommandContainer parameters (most common first: ssh, prompter) - Add singleton behavior test for Container bind() method - Simplify persistence test assertions (verify via output) All tests pass (267 tests, 583 assertions)
1 participant
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation