feat: add filesystem service di - #22
Conversation
WalkthroughIntroduces a new FilesystemService and refactors multiple services and tests to depend on it; adds test DI guidance and new/updated test helpers and unit tests. ProcessFactory becomes readonly and several constructors now accept FilesystemService. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Env as EnvService
participant Inv as InventoryService
participant SSH as SSHService
participant Ver as VersionService
participant PF as ProcessFactory
participant FS as FilesystemService
participant SFS as SymfonyFilesystem
note over FS,SFS #D3E4CD: FilesystemService delegates to Symfony Filesystem and provides helpers
User->>Env: loadEnvFile(path)
Env->>FS: getCwd(), exists(path), readFile(path)
FS->>SFS: delegate exists/readFile
SFS-->>FS: result
FS-->>Env: file contents
Env-->>User: parsed env
User->>Inv: loadInventoryFile(path)
Inv->>FS: getCwd(), exists(path), readFile(path)
FS->>SFS: delegate
FS-->>Inv: inventory data
Inv-->>User: parsed inventory
User->>SSH: uploadFile()/executeScript()
SSH->>FS: exists/readFile/dumpFile()
FS->>SFS: delegate
FS-->>SSH: result
SSH-->>User: success/failure
User->>Ver: getVersionFromGit()
Ver->>FS: getParentDirectory(__DIR__, levels), isDirectory(projectRoot + '/.git')
alt .git present
Ver->>PF: create(...)
PF->>FS: isDirectory(cwd)
FS-->>PF: bool
PF-->>Ver: process output
Ver-->>User: git-derived version
else fallback
Ver-->>User: fallback version
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (3)**/*.php📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
Files:
tests/**/*.php📄 CodeRabbit inference engine (.cursor/rules/02-tests.mdc)
Files:
{tests/**,test/**,**/*@(Test|Spec).php}📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Files:
🧠 Learnings (1)📚 Learning: 2025-10-01T13:57:04.177ZApplied to files:
🧬 Code graph analysis (3)tests/Unit/Services/SSHServiceTest.php (2)
tests/Unit/Services/EnvServiceTest.php (1)
tests/TestHelpers.php (5)
🪛 PHPMD (2.15.0)tests/TestHelpers.php115-115: Avoid unused parameters such as '$mode'. (undefined) (UnusedFormalParameter) 🔇 Additional comments (8)
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: 2
🧹 Nitpick comments (1)
tests/Unit/Services/ProcessFactoryTest.php (1)
10-10: Mock filesystem dependencies in unit testsUsing
mockProcessFactory()currently wires the real Symfony filesystem viaFilesystemService, so these tests touch the host FS. Our test rules explicitly call for mocking external dependencies (filesystem, HTTP, processes) in unit tests; please adjust the helper or test setup to inject a mocked filesystem service instead of the concrete implementation. As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.cursor/rules/02-tests.mdc(1 hunks)app/Services/EnvService.php(4 hunks)app/Services/FilesystemService.php(1 hunks)app/Services/InventoryService.php(5 hunks)app/Services/ProcessFactory.php(2 hunks)app/Services/SSHService.php(6 hunks)app/Services/VersionService.php(3 hunks)tests/TestHelpers.php(5 hunks)tests/Unit/Services/EnvServiceTest.php(3 hunks)tests/Unit/Services/FilesystemServiceTest.php(1 hunks)tests/Unit/Services/InventoryServiceTest.php(11 hunks)tests/Unit/Services/ProcessFactoryTest.php(1 hunks)tests/Unit/Services/SSHServiceTest.php(9 hunks)tests/Unit/Services/VersionServiceTest.php(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
**/*.php: Follow PSR-12 coding standard in all PHP files
Declare strict types in all PHP files (declare(strict_types=1);)
Use PHP 8.x features where appropriate (unions, match, attributes, readonly)
Use import statements (use ...) instead of fully qualified class names in code
All methods must declare explicit return types with proper generics (e.g., Collection<int, User>)
Prefer Dependency Injection over manual class resolution/instantiation
Use Symfony component classes (e.g., Filesystem, Process) instead of native PHP functions for easier mocking
All object creation must use $container->build(ClassName::class) except for value objects, DTOs, and pure data structures
Access the container via constructor injection in production code; tests may instantiate Container directly
Services must receive dependencies via constructor injection
Use SymfonyStyle consistently for all user-facing console output
Add DocBlock comments with minimalist descriptions, parameters, and return types for classes and functions
Use comments to separate sections and explain complex logic; avoid obvious or stale comments
Use the exact section header comment format with a single newline between headers/subheaders/paragraphs
**/*.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
Files:
tests/Unit/Services/EnvServiceTest.phpapp/Services/FilesystemService.phptests/TestHelpers.phpapp/Services/VersionService.phptests/Unit/Services/ProcessFactoryTest.phptests/Unit/Services/SSHServiceTest.phptests/Unit/Services/InventoryServiceTest.phpapp/Services/ProcessFactory.phpapp/Services/EnvService.phpapp/Services/SSHService.phptests/Unit/Services/FilesystemServiceTest.phpapp/Services/InventoryService.phptests/Unit/Services/VersionServiceTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/02-tests.mdc)
tests/**/*.php: Use Pest exclusively with it() syntax for all tests.
Keep each test file under 1.8x the size of the source code it tests.
Test core business logic; do not test 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(...)->toBe(...)->and(...)).
Mock only external dependencies; do not mock internal implementation details.
Do not add performance tests unless performance is the primary concern.
Do not sacrifice readability to meet size/ratio targets.
Do not consolidate tests when they target different public methods.
Do not consolidate exception-flow tests with normal-flow tests.
Do not consolidate tests with different setup requirements.
Do not consolidate tests covering distinct business logic.
Follow the AAA pattern (Arrange, Act, Assert) in all tests; include cleanup when needed.
For exception tests, use a combined // ACT & ASSERT step when the act triggers the assertion.
Organize tests with describe() blocks, beforeEach() setup, and extract helpers/traits for DRY tests.
Do not assert only on type using toBeInstanceOf(Class::class).
Do not use generic assertions like toBeArray() that don’t validate behavior.
Avoid meaningless assertions such as not->toBeNull() without behavioral relevance.
Do not write literally meaningless assertions like expect(true)->toBeTrue().
Do not use sleep(...); use time mocking instead.
Write assertions against concrete outcomes, e.g., expect($config->getValue('host'))->toBe('example.com').
Assert validator outcomes directly, e.g., expect($this->validator->isValid($input))->toBe($expected).
Define mock expectations explicitly, e.g., $mock->shouldReceive('method')->with('param')->andReturn('result').
In unit tests, mock all external dependencies (filesystem, HTTP, processes).
Unit tests should target a single unit in isolation.
Unit tests should complete i...
Files:
tests/Unit/Services/EnvServiceTest.phptests/TestHelpers.phptests/Unit/Services/ProcessFactoryTest.phptests/Unit/Services/SSHServiceTest.phptests/Unit/Services/InventoryServiceTest.phptests/Unit/Services/FilesystemServiceTest.phptests/Unit/Services/VersionServiceTest.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/Services/EnvServiceTest.phptests/TestHelpers.phptests/Unit/Services/ProcessFactoryTest.phptests/Unit/Services/SSHServiceTest.phptests/Unit/Services/InventoryServiceTest.phptests/Unit/Services/FilesystemServiceTest.phptests/Unit/Services/VersionServiceTest.php
app/Services/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/03-commands.mdc)
Services return plain data and must not perform any console I/O
Files:
app/Services/FilesystemService.phpapp/Services/VersionService.phpapp/Services/ProcessFactory.phpapp/Services/EnvService.phpapp/Services/SSHService.phpapp/Services/InventoryService.php
🧠 Learnings (4)
📚 Learning: 2025-10-01T13:57:04.177Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-01T13:57:04.177Z
Learning: Applies to tests/**/*.php : In unit tests, mock all external dependencies (filesystem, HTTP, processes).
Applied to files:
tests/Unit/Services/EnvServiceTest.php.cursor/rules/02-tests.mdctests/TestHelpers.phptests/Unit/Services/ProcessFactoryTest.phptests/Unit/Services/SSHServiceTest.phptests/Unit/Services/VersionServiceTest.php
📚 Learning: 2025-10-01T13:57:04.177Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-01T13:57:04.177Z
Learning: Use `composer pest` to run the full test suite in parallel with coverage.
Applied to files:
.cursor/rules/02-tests.mdc
📚 Learning: 2025-10-01T13:57:04.177Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-01T13:57:04.177Z
Learning: Applies to tests/**/*.php : Use Pest exclusively with it() syntax for all tests.
Applied to files:
.cursor/rules/02-tests.mdc
📚 Learning: 2025-10-01T13:56:16.653Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-10-01T13:56:16.653Z
Learning: Applies to **/*.php : Access the container via constructor injection in production code; tests may instantiate Container directly
Applied to files:
.cursor/rules/02-tests.mdc
🧬 Code graph analysis (13)
tests/Unit/Services/EnvServiceTest.php (1)
tests/TestHelpers.php (1)
mockEnvService(137-145)
app/Services/FilesystemService.php (1)
tests/Unit/Services/FilesystemServiceTest.php (4)
__construct(17-23)exists(25-32)readFile(34-41)dumpFile(43-48)
tests/TestHelpers.php (4)
app/Services/EnvService.php (1)
EnvService(12-134)app/Services/FilesystemService.php (1)
FilesystemService(27-106)app/Services/InventoryService.php (1)
InventoryService(33-264)app/Services/ProcessFactory.php (1)
ProcessFactory(12-39)
app/Services/VersionService.php (1)
app/Services/FilesystemService.php (3)
FilesystemService(27-106)getParentDirectory(98-105)isDirectory(88-91)
tests/Unit/Services/ProcessFactoryTest.php (1)
tests/TestHelpers.php (1)
mockProcessFactory(194-198)
tests/Unit/Services/SSHServiceTest.php (2)
tests/TestHelpers.php (3)
mockFilesystemService(177-187)mockEnvService(137-145)mockFilesystem(32-130)app/Services/FilesystemService.php (1)
FilesystemService(27-106)
tests/Unit/Services/InventoryServiceTest.php (1)
tests/TestHelpers.php (1)
mockInventoryService(153-170)
app/Services/ProcessFactory.php (2)
app/Services/VersionService.php (1)
__construct(20-26)app/Services/FilesystemService.php (2)
FilesystemService(27-106)isDirectory(88-91)
app/Services/EnvService.php (2)
app/Services/FilesystemService.php (2)
FilesystemService(27-106)getCwd(75-83)tests/TestHelpers.php (2)
exists(58-89)readFile(91-110)
app/Services/SSHService.php (1)
app/Services/FilesystemService.php (1)
FilesystemService(27-106)
tests/Unit/Services/FilesystemServiceTest.php (1)
app/Services/FilesystemService.php (8)
FilesystemService(27-106)__construct(29-32)exists(41-44)readFile(51-54)dumpFile(61-64)getCwd(75-83)isDirectory(88-91)getParentDirectory(98-105)
app/Services/InventoryService.php (2)
app/Services/FilesystemService.php (2)
FilesystemService(27-106)getCwd(75-83)tests/TestHelpers.php (3)
exists(58-89)readFile(91-110)dumpFile(121-128)
tests/Unit/Services/VersionServiceTest.php (4)
tests/TestHelpers.php (1)
mockProcessFactory(194-198)app/Services/FilesystemService.php (1)
FilesystemService(27-106)app/Services/VersionService.php (1)
VersionService(18-172)app/Services/ProcessFactory.php (1)
ProcessFactory(12-39)
🪛 GitHub Actions: Rector
tests/TestHelpers.php
[error] 1-1: Rector dry-run reported changes would be applied: 'NullToStrictStringFuncCallArgRector' suggested updates to string-typed casts in several checks. Dry-run indicates 2 files would be changed by Rector. Exit code 2.
tests/Unit/Services/FilesystemServiceTest.php
[error] 1-1: Rector dry-run reported changes would be applied: 'ReadOnlyPropertyRector' suggested updates to readonly properties. Dry-run indicates 2 files would be changed by Rector. Exit code 2.
🪛 PHPMD (2.15.0)
tests/TestHelpers.php
112-112: Avoid unused parameters such as '$mode'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (29)
tests/Unit/Services/FilesystemServiceTest.php (4)
75-85: LGTM!The test correctly validates that
getCwd()returns a non-empty string. Using a realFilesysteminstance is appropriate here sincegetcwd()is a safe, side-effect-free native function.
87-101: LGTM!The dataset-driven approach validates
isDirectory()behavior across real filesystem paths (existing directory, file, nonexistent path). Using__DIR__and__FILE__is appropriate for deterministic test fixtures.
103-117: LGTM!The parameterized test validates
getParentDirectory()for 1-3 levels with expected results. Using static path fixtures is appropriate for unit testingdirname()behavior.
119-127: LGTM!Exception test correctly validates input validation (levels < 1 throws
InvalidArgumentException).tests/Unit/Services/InventoryServiceTest.php (4)
12-14: LGTM!Test setup correctly uses
mockInventoryServicehelper with empty initial data, aligning with the new FilesystemService-based testing approach.
20-43: LGTM!The set operations test correctly uses array fixtures instead of YAML strings, delegating setup to
mockInventoryService. The test validates data persistence by retrieving the stored value, which is appropriate.
49-100: LGTM!Get operations test properly uses array fixtures and validates various scenarios (deep nested, collections, non-existent paths, file not found). Dataset-driven approach is appropriate.
169-195: LGTM!Error handling tests correctly validate RuntimeException scenarios (write failures, read failures, uninitialized state) using the mock helper's throw configuration.
tests/Unit/Services/VersionServiceTest.php (4)
8-8: LGTM!Correctly includes TestHelpers to access
mockProcessFactory()helper.
13-15: LGTM!Test correctly constructs
VersionServicewith bothProcessFactory(mocked) andFilesystemService(real instance) dependencies, aligning with the new constructor signature.
45-47: LGTM!Git repository detection test correctly wires both dependencies into
VersionService.
85-87: LGTM!Non-existent package test correctly instantiates both
ProcessFactoryandFilesystemServiceas real instances for this integration-style validation.app/Services/InventoryService.php (2)
42-45: LGTM!Constructor correctly updated to depend on
FilesystemServiceinstead of Symfony'sFilesystem, following DI best practices.
104-104: LGTM!All filesystem operations correctly updated to use
$this->fs(FilesystemService) instead of direct Symfony Filesystem calls. The delegation is consistent and correct.Also applies to: 222-222, 235-235, 259-259
app/Services/EnvService.php (2)
21-25: LGTM!Constructor correctly updated to depend on
FilesystemService, maintaining the existingDotenvdependency.
79-79: LGTM!All filesystem operations correctly updated to use
$this->fs(FilesystemService). The changes are consistent and maintain existing behavior.Also applies to: 109-109, 122-122
app/Services/VersionService.php (3)
20-26: LGTM!Constructor correctly adds
FilesystemServiceas a dependency alongside existingProcessFactory, maintaining optional parameters for$packageNameand$fallbackVersion.
76-76: LGTM!Correctly uses
FilesystemService::getParentDirectory()instead of nativedirname(), improving testability.
108-108: LGTM!Correctly uses
FilesystemService::isDirectory()instead of nativeis_dir(), improving testability.app/Services/FilesystemService.php (5)
27-32: LGTM!Class correctly declared as
final readonlywith proper DI of SymfonyFilesystem. The readonly modifier ensures immutability.
41-44: LGTM!Symfony Filesystem wrapper methods (
exists,readFile,dumpFile) correctly delegate to the underlying$this->fsinstance. Simple passthrough is appropriate.Also applies to: 51-54, 61-64
75-83: LGTM!
getCwd()correctly wraps nativegetcwd()with error handling, throwingRuntimeExceptionwhen the working directory cannot be determined. This improves error clarity and testability.
88-91: LGTM!
isDirectory()correctly combines existence check withis_dir()validation. The implementation is safe and prevents false positives.
98-105: LGTM!
getParentDirectory()correctly validates input ($levels >= 1) and delegates todirname(). Input validation prevents misuse and improves error clarity.tests/Unit/Services/SSHServiceTest.php (5)
28-30: LGTM!Test correctly uses
mockFilesystemServicehelper to create a FilesystemService with configured behavior (file does not exist), then passes it toSSHServiceconstructor.
46-48: LGTM!Test correctly constructs
SSHServicewith a mockedFilesystemServicefor tilde expansion validation.
60-65: LGTM!Test correctly creates a
MockFs, wraps it inFilesystemService, and passes the service toSSHService. This pattern allows fine-grained control over filesystem state (multiple files).
78-82: LGTM!Tests correctly follow the pattern of creating
MockFs, wrapping inFilesystemService, and passing toSSHService. This approach maintains test isolation while exercising the service layer.Also applies to: 95-98, 116-119
132-135: LGTM!File validation tests correctly use
mockFilesystemServiceto create filesystem configurations (file does not exist) and validate error handling inSSHService.Also applies to: 144-147, 156-159
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/TestHelpers.php (1)
152-169: LGTM! Consider simplifying default content logic.The helper correctly composes the DI chain. The default content logic at lines 162-163 works but could be slightly clearer:
Optional: Make the default content logic more explicit:
} else { - $defaultContent = 'servers:' . PHP_EOL . ' web1:' . PHP_EOL . ' host: example.com'; - $fileContent = $data ?: ($fileExists ? $defaultContent : ''); + if ($data !== '') { + $fileContent = $data; + } elseif ($fileExists) { + $fileContent = 'servers:' . PHP_EOL . ' web1:' . PHP_EOL . ' host: example.com'; + } else { + $fileContent = ''; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.cursor/commands/create-branch-and-commits.md(1 hunks)tests/TestHelpers.php(5 hunks)tests/Unit/Services/FilesystemServiceTest.php(1 hunks)tests/Unit/TestHelpersTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .cursor/commands/create-branch-and-commits.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Unit/Services/FilesystemServiceTest.php
🧰 Additional context used
📓 Path-based instructions (3)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
**/*.php: Follow PSR-12 coding standard in all PHP files
Declare strict types in all PHP files (declare(strict_types=1);)
Use PHP 8.x features where appropriate (unions, match, attributes, readonly)
Use import statements (use ...) instead of fully qualified class names in code
All methods must declare explicit return types with proper generics (e.g., Collection<int, User>)
Prefer Dependency Injection over manual class resolution/instantiation
Use Symfony component classes (e.g., Filesystem, Process) instead of native PHP functions for easier mocking
All object creation must use $container->build(ClassName::class) except for value objects, DTOs, and pure data structures
Access the container via constructor injection in production code; tests may instantiate Container directly
Services must receive dependencies via constructor injection
Use SymfonyStyle consistently for all user-facing console output
Add DocBlock comments with minimalist descriptions, parameters, and return types for classes and functions
Use comments to separate sections and explain complex logic; avoid obvious or stale comments
Use the exact section header comment format with a single newline between headers/subheaders/paragraphs
**/*.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
Files:
tests/TestHelpers.phptests/Unit/TestHelpersTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/02-tests.mdc)
tests/**/*.php: Use Pest exclusively with it() syntax for all tests.
Keep each test file under 1.8x the size of the source code it tests.
Test core business logic; do not test 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(...)->toBe(...)->and(...)).
Mock only external dependencies; do not mock internal implementation details.
Do not add performance tests unless performance is the primary concern.
Do not sacrifice readability to meet size/ratio targets.
Do not consolidate tests when they target different public methods.
Do not consolidate exception-flow tests with normal-flow tests.
Do not consolidate tests with different setup requirements.
Do not consolidate tests covering distinct business logic.
Follow the AAA pattern (Arrange, Act, Assert) in all tests; include cleanup when needed.
For exception tests, use a combined // ACT & ASSERT step when the act triggers the assertion.
Organize tests with describe() blocks, beforeEach() setup, and extract helpers/traits for DRY tests.
Do not assert only on type using toBeInstanceOf(Class::class).
Do not use generic assertions like toBeArray() that don’t validate behavior.
Avoid meaningless assertions such as not->toBeNull() without behavioral relevance.
Do not write literally meaningless assertions like expect(true)->toBeTrue().
Do not use sleep(...); use time mocking instead.
Write assertions against concrete outcomes, e.g., expect($config->getValue('host'))->toBe('example.com').
Assert validator outcomes directly, e.g., expect($this->validator->isValid($input))->toBe($expected).
Define mock expectations explicitly, e.g., $mock->shouldReceive('method')->with('param')->andReturn('result').
In unit tests, mock all external dependencies (filesystem, HTTP, processes).
Unit tests should target a single unit in isolation.
Unit tests should complete i...
Files:
tests/TestHelpers.phptests/Unit/TestHelpersTest.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/TestHelpers.phptests/Unit/TestHelpersTest.php
🧠 Learnings (1)
📚 Learning: 2025-10-01T13:57:04.177Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-01T13:57:04.177Z
Learning: Applies to tests/**/*.php : In unit tests, mock all external dependencies (filesystem, HTTP, processes).
Applied to files:
tests/TestHelpers.phptests/Unit/TestHelpersTest.php
🧬 Code graph analysis (2)
tests/TestHelpers.php (5)
tests/Unit/Services/FilesystemServiceTest.php (1)
dumpFile(43-48)app/Services/EnvService.php (1)
EnvService(12-134)app/Services/FilesystemService.php (1)
FilesystemService(27-106)app/Services/InventoryService.php (1)
InventoryService(33-264)app/Services/ProcessFactory.php (1)
ProcessFactory(12-39)
tests/Unit/TestHelpersTest.php (1)
tests/TestHelpers.php (9)
mockFilesystem(32-129)exists(58-88)dumpFile(120-127)readFile(90-109)mkdir(111-118)mockEnvService(136-144)mockInventoryService(152-169)mockFilesystemService(176-186)setEnv(15-25)
🪛 PHPMD (2.15.0)
tests/TestHelpers.php
111-111: Avoid unused parameters such as '$mode'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (7)
tests/Unit/TestHelpersTest.php (2)
9-136: LGTM! Comprehensive coverage of mock filesystem behavior.The test suite thoroughly exercises file/directory existence checks, read/write operations, and error simulation. The regression test at lines 25-33 specifically validates the directory substring fix from the previous review.
182-208: LGTM! Proper behavioral validation.Both tests verify concrete outcomes across
$_ENV,$_SERVER, andgetenv(), following the AAA pattern correctly.tests/TestHelpers.php (5)
69-87: Correctly fixes directory substring false positive.The reordered logic now checks files first (direct match and path-ending match) before checking directories with exact path matching. This addresses the previous review concern where directory substrings caused false positives.
Based on previous review.
111-118: Unused parameter is acceptable for interface compliance.The
$modeparameter is unused because this mock doesn't simulate file permissions. However, it must be present to match the SymfonyFilesystem::mkdir()signature.The static analysis warning can be safely ignored.
136-144: LGTM! Proper DI chain for test doubles.The helper correctly composes a mock filesystem → FilesystemService → EnvService, enabling isolated unit testing of EnvService behavior.
176-186: LGTM! Clean test helper for FilesystemService.Properly wraps a mock filesystem in FilesystemService for isolated testing.
193-197: Verify intended use for integration tests.This helper uses a real
Filesysteminstance (line 195) rather than a mock. This is appropriate for integration tests whereProcessFactoryneeds to validate actual directories, but may cause issues in unit tests that expect full isolation.Confirm this helper is intended specifically for integration tests of
ProcessFactory. If unit tests need a fully mocked version, consider addingmockProcessFactoryWithMockFs():function mockProcessFactoryWithMockFs( string $cwdPath = '/tmp/test', bool $cwdExists = true ): \Bigpixelrocket\DeployerPHP\Services\ProcessFactory { $mockFs = mockFilesystem(exists: $cwdExists, initialPath: $cwdPath); $filesystemService = new \Bigpixelrocket\DeployerPHP\Services\FilesystemService($mockFs); return new \Bigpixelrocket\DeployerPHP\Services\ProcessFactory($filesystemService); }
Previously mockProcessFactory() instantiated a real Symfony Filesystem, causing unit tests to touch the host filesystem and violating the test rule to mock all external dependencies (filesystem, HTTP, processes). Changes: - Update mockProcessFactory() to accept validDirectories parameter - Create mock Filesystem with in-memory directory validation - Update ProcessFactoryTest to pass valid directories to mock - Update VersionServiceTest to pass valid directories for all test cases This ensures unit tests properly isolate filesystem operations without touching the real filesystem during test execution.
Eliminates test overlap in TestHelpersTest by consolidating 7 tests into 3 dataset-driven tests: - Consolidates 3 file existence tests into 1 with 10 scenarios - Consolidates 2 mockInventoryService tests (array/string data) - Consolidates 2 setEnv tests (set/unset operations) - Adds chained assertions throughout using ->and() - Improves test assertions from type checks to behavior validation Results: 23 lines saved (222 → 199), improved ratio from 0.996x to 0.90x All 24 tests passing with 35 assertions
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/TestHelpers.php (1)
69-88: Fix to exists() logic looks correct and resolves substring false positivesFiles checked before dirs; dirs require exact match. Good.
🧹 Nitpick comments (5)
tests/TestHelpers.php (3)
111-118: Support iterable mkdir inputs and silence unused $modeHandle arrays/iterables per Symfony signature and suppress PHPMD for $mode.
- public function mkdir(string|iterable $dirs, int $mode = 0777): void + /** + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + public function mkdir(string|iterable $dirs, int $mode = 0777): void { if ($this->throwOnMkdir) { throw new IOException('Permission denied', 0, null, (string) $dirs); } - - $this->directories[] = (string) $dirs; + if (is_iterable($dirs)) { + foreach ($dirs as $dir) { + $this->directories[] = (string) $dir; + } + return; + } + $this->directories[] = (string) $dirs; }As per coding guidelines (PHPMD hint).
142-145: Prefer imports over fully-qualified namesImport FilesystemService, ProcessFactory, and Yaml; drop leading backslashes.
- $filesystemService = new \Bigpixelrocket\DeployerPHP\Services\FilesystemService($mockFs); + $filesystemService = new FilesystemService($mockFs); return new EnvService($filesystemService, new Dotenv());- $fileContent = empty($data) ? '' : \Symfony\Component\Yaml\Yaml::dump($data, 2, 4, \Symfony\Component\Yaml\Yaml::DUMP_EMPTY_ARRAY_AS_SEQUENCE); + $fileContent = empty($data) ? '' : Yaml::dump($data, 2, 4, Yaml::DUMP_EMPTY_ARRAY_AS_SEQUENCE);- $filesystemService = new \Bigpixelrocket\DeployerPHP\Services\FilesystemService($mockFs); - return new InventoryService($filesystemService); + $filesystemService = new FilesystemService($mockFs); + return new InventoryService($filesystemService);- ): \Bigpixelrocket\DeployerPHP\Services\FilesystemService { + ): FilesystemService { - $mockFs = mockFilesystem($fileExists, $fileContent, $throwOnRead, $throwOnMkdir, $throwOnWrite, $filePath); - return new \Bigpixelrocket\DeployerPHP\Services\FilesystemService($mockFs); + $mockFs = mockFilesystem($fileExists, $fileContent, $throwOnRead, $throwOnMkdir, $throwOnWrite, $filePath); + return new FilesystemService($mockFs); }- function mockProcessFactory(array $validDirectories = []): \Bigpixelrocket\DeployerPHP\Services\ProcessFactory + function mockProcessFactory(array $validDirectories = []): ProcessFactory ... - $filesystemService = new \Bigpixelrocket\DeployerPHP\Services\FilesystemService($mockFs); - return new \Bigpixelrocket\DeployerPHP\Services\ProcessFactory($filesystemService); + $filesystemService = new FilesystemService($mockFs); + return new ProcessFactory($filesystemService);Add these imports near the top:
use Bigpixelrocket\DeployerPHP\Services\FilesystemService; use Bigpixelrocket\DeployerPHP\Services\ProcessFactory; use Symfony\Component\Yaml\Yaml;As per coding guidelines.
Also applies to: 167-169, 176-186, 195-221
17-23: Unsetting env vars: prefer putenv("KEY=") for portabilityUsing putenv("KEY") may not unset on all platforms. Consider:
- unset($_ENV[$key], $_SERVER[$key]); - putenv("{$key}"); + unset($_ENV[$key], $_SERVER[$key]); + putenv("{$key}=");Please confirm target PHP/platform behavior. Based on learnings.
tests/Unit/TestHelpersTest.php (2)
33-41: Consider adding iterable mkdir existence caseAdd a case exercising mkdir with an array/iterable to validate helper handles multiple dirs.
172-199: Unsetting env assertion may be brittle across platformsgetenv('TEST_VAR') might return '' instead of false unless putenv('KEY=') is used. Align with helper change or relax the assertion accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/TestHelpers.php(5 hunks)tests/Unit/Services/ProcessFactoryTest.php(1 hunks)tests/Unit/Services/VersionServiceTest.php(4 hunks)tests/Unit/TestHelpersTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Unit/Services/VersionServiceTest.php
- tests/Unit/Services/ProcessFactoryTest.php
🧰 Additional context used
📓 Path-based instructions (3)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
**/*.php: Follow PSR-12 coding standard in all PHP files
Declare strict types in all PHP files (declare(strict_types=1);)
Use PHP 8.x features where appropriate (unions, match, attributes, readonly)
Use import statements (use ...) instead of fully qualified class names in code
All methods must declare explicit return types with proper generics (e.g., Collection<int, User>)
Prefer Dependency Injection over manual class resolution/instantiation
Use Symfony component classes (e.g., Filesystem, Process) instead of native PHP functions for easier mocking
All object creation must use $container->build(ClassName::class) except for value objects, DTOs, and pure data structures
Access the container via constructor injection in production code; tests may instantiate Container directly
Services must receive dependencies via constructor injection
Use SymfonyStyle consistently for all user-facing console output
Add DocBlock comments with minimalist descriptions, parameters, and return types for classes and functions
Use comments to separate sections and explain complex logic; avoid obvious or stale comments
Use the exact section header comment format with a single newline between headers/subheaders/paragraphs
**/*.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
Files:
tests/TestHelpers.phptests/Unit/TestHelpersTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/02-tests.mdc)
tests/**/*.php: Use Pest exclusively with it() syntax for all tests.
Keep each test file under 1.8x the size of the source code it tests.
Test core business logic; do not test 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(...)->toBe(...)->and(...)).
Mock only external dependencies; do not mock internal implementation details.
Do not add performance tests unless performance is the primary concern.
Do not sacrifice readability to meet size/ratio targets.
Do not consolidate tests when they target different public methods.
Do not consolidate exception-flow tests with normal-flow tests.
Do not consolidate tests with different setup requirements.
Do not consolidate tests covering distinct business logic.
Follow the AAA pattern (Arrange, Act, Assert) in all tests; include cleanup when needed.
For exception tests, use a combined // ACT & ASSERT step when the act triggers the assertion.
Organize tests with describe() blocks, beforeEach() setup, and extract helpers/traits for DRY tests.
Do not assert only on type using toBeInstanceOf(Class::class).
Do not use generic assertions like toBeArray() that don’t validate behavior.
Avoid meaningless assertions such as not->toBeNull() without behavioral relevance.
Do not write literally meaningless assertions like expect(true)->toBeTrue().
Do not use sleep(...); use time mocking instead.
Write assertions against concrete outcomes, e.g., expect($config->getValue('host'))->toBe('example.com').
Assert validator outcomes directly, e.g., expect($this->validator->isValid($input))->toBe($expected).
Define mock expectations explicitly, e.g., $mock->shouldReceive('method')->with('param')->andReturn('result').
In unit tests, mock all external dependencies (filesystem, HTTP, processes).
Unit tests should target a single unit in isolation.
Unit tests should complete i...
Files:
tests/TestHelpers.phptests/Unit/TestHelpersTest.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/TestHelpers.phptests/Unit/TestHelpersTest.php
🧠 Learnings (5)
📚 Learning: 2025-10-01T13:57:04.177Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-01T13:57:04.177Z
Learning: Applies to tests/**/*.php : In unit tests, mock all external dependencies (filesystem, HTTP, processes).
Applied to files:
tests/TestHelpers.phptests/Unit/TestHelpersTest.php
📚 Learning: 2025-10-01T13:57:04.177Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-01T13:57:04.177Z
Learning: Applies to tests/**/*.php : Do not use generic assertions like toBeArray() that don’t validate behavior.
Applied to files:
tests/Unit/TestHelpersTest.php
📚 Learning: 2025-10-01T13:57:04.177Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-01T13:57:04.177Z
Learning: Applies to tests/**/*.php : Avoid meaningless assertions such as not->toBeNull() without behavioral relevance.
Applied to files:
tests/Unit/TestHelpersTest.php
📚 Learning: 2025-10-01T13:57:04.177Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-01T13:57:04.177Z
Learning: Applies to tests/**/*.php : Write assertions against concrete outcomes, e.g., expect($config->getValue('host'))->toBe('example.com').
Applied to files:
tests/Unit/TestHelpersTest.php
📚 Learning: 2025-10-01T13:57:04.177Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-01T13:57:04.177Z
Learning: Applies to tests/**/*.php : Do not assert only on type using toBeInstanceOf(Class::class).
Applied to files:
tests/Unit/TestHelpersTest.php
🧬 Code graph analysis (2)
tests/TestHelpers.php (5)
tests/Unit/Services/FilesystemServiceTest.php (3)
dumpFile(43-48)__construct(17-23)exists(25-32)app/Services/EnvService.php (2)
EnvService(12-134)__construct(21-25)app/Services/FilesystemService.php (1)
FilesystemService(27-106)app/Services/InventoryService.php (2)
InventoryService(33-264)__construct(42-45)app/Services/ProcessFactory.php (1)
ProcessFactory(12-39)
tests/Unit/TestHelpersTest.php (3)
tests/TestHelpers.php (10)
mockFilesystem(32-129)exists(58-88)exists(203-216)dumpFile(120-127)readFile(90-109)mkdir(111-118)mockEnvService(136-144)mockInventoryService(152-169)mockFilesystemService(176-186)setEnv(15-25)app/Services/EnvService.php (1)
loadEnvFile(73-90)app/Services/InventoryService.php (1)
loadInventoryFile(97-111)
🪛 PHPMD (2.15.0)
tests/TestHelpers.php
111-111: Avoid unused parameters such as '$mode'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (3)
tests/Unit/TestHelpersTest.php (3)
14-31: Existence tests are thorough and align with intended semanticsCovers direct, trailing slash, non-substring, and path-ending matches. LGTM.
126-151: Behavioral assertions for Env/Inventory services look solidTests load files and assert values rather than just types. Good.
157-169: FilesystemService helper assertions are appropriateAsserts exist/read behavior via the service wrapper. LGTM.
The anonymous class mock only overrode exists() but FilesystemService::isDirectory also relies on native is_dir(), making the mock ineffective. Replaced with real Filesystem since directory validation requires actual filesystem checks. Tests already use real directories (__DIR__), so functionality is preserved.
Extract repeated VersionService instantiation into a reusable helper function following the existing TestHelpers pattern. This reduces test setup boilerplate and removes unused imports from VersionServiceTest. - Add mockVersionService() helper with optional package name and fallback - Refactor VersionServiceTest to use the new helper (4 occurrences) - Remove unused ProcessFactory and VersionService imports - Reduce test file size by 12 lines (12.4%)
Replace fully qualified domain name (FQDN) class references with short class names and proper use statements throughout test files for improved readability and PSR-12 compliance. Changes: - Add use statements for FilesystemService, ProcessFactory, VersionService, Yaml - Replace \Bigpixelrocket\DeployerPHP\Services\* with short class names - Replace \Symfony\Component\Yaml\Yaml with imported Yaml class - Remove unnecessary blank lines for consistency All tests pass with no warnings.
1 participant
Summary by CodeRabbit
Refactor
Documentation
Tests