test: improving code coverage for SatisCommandBuilder and LockProcessor #2
Conversation
WalkthroughAdds an auto-generated PHP config reference ( Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (1)
tests/Service/LockProcessorTest.php (1)
55-80: Consider testing via public interface instead of reflection.Using
ReflectionMethodto test the protectedgetRepositoriesmethod works, but it couples the test to internal implementation details. An alternative approach would be to test this filtering behavior throughprocessFile()with invalid packages in the JSON fixture.That said, if the goal is explicit coverage of the filtering logic, this approach is acceptable.
Example alternative that tests through the public interface:
public function testProcessFileFiltersInvalidPackages(): void { $json = <<<JSON { "packages": [ {"name": "vendor/valid", "source": {"url": "https://example.com/repo.git", "type": "git"}}, {"name": "vendor/invalid", "source": {"url": "", "type": ""}}, {"name": "vendor/nosource"} ] } JSON; // ... create temp file and verify only 1 repository is added }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
config/reference.php(1 hunks)src/Event/BuildEvent.php(1 hunks)src/Service/SatisManager.php(3 hunks)tests/Service/LockProcessorTest.php(1 hunks)tests/Service/SatisCommandBuilderTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Service/SatisManager.php (1)
src/Service/SatisCommandBuilder.php (1)
build(60-76)
tests/Service/LockProcessorTest.php (2)
src/Service/LockProcessor.php (2)
LockProcessor(7-77)processFile(19-23)src/Service/RepositoryManager.php (1)
RepositoryManager(14-197)
🔇 Additional comments (8)
src/Event/BuildEvent.php (1)
8-8: LGTM - Removingfinalimproves testability.Allowing
BuildEventto be extended enables easier mocking and subclassing in tests.src/Service/SatisManager.php (2)
14-14: LGTM - Removingfinalimproves testability.Allowing
SatisManagerto be extended enables easier mocking and testing.
83-85: Verify exception propagation fromgetCommandLine.Adding
JSON_THROW_ON_ERRORis good for explicit error handling, but callers ofgetCommandLine()don't catchJsonException:
run()(line 46) - JsonException would propagate, but lock is released viafinallyonBuild()(line 68) - JsonException would bypass theRuntimeExceptioncatch and propagate beforelock.release()in finallyThe
onBuild()method acquires the lock at line 72, butgetCommandLine()is called at line 68 before lock acquisition, so this is safe. However, consumers of this class should be aware thatrun()andonBuild()can now throwJsonException.Also applies to: 105-105
tests/Service/LockProcessorTest.php (1)
12-53: Good test coverage forprocessFilewith proper resource cleanup.The test properly creates a temporary file, verifies the mock expectations for both
packagesandpackages-dev, and cleans up the file handle.tests/Service/SatisCommandBuilderTest.php (3)
28-40: Good coverage of option merging and default flags.Test validates that custom options merge with defaults (
--skip-errors,--no-ansi,--verbose), ensuring the builder maintains expected behavior.
53-69: Comprehensive integration test for full command structure.This test validates the complete command array format including binary path, subcommand, config file, output directory, repository, options, and extra arguments.
10-16: No issues found—properties are intentionally public.The
configFileandoutputDirproperties are declared aspublicvia promoted constructor properties (lines 20–21 inSatisCommandBuilder.php), so the test's direct access is valid and the test will pass.config/reference.php (1)
1-3: Verify intent to commit auto-generated file.This file is marked as "auto-generated" (line 3). Typically, auto-generated files are either:
- Excluded from version control (added to
.gitignore)- Regenerated during build/deployment
Committing it may cause merge conflicts if regenerated. Confirm this is intentional for the project.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @Trusted97. * #2 (comment) The following files were modified: * `config/reference.php` * `src/Service/SatisManager.php`
chore: add docstrings to `test-improving-code-coverage`
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Service/SatisManager.php (1)
82-89: Perfect! I've found the issue. The Symfony Lock component'srelease()method is not safe to call on a lock that was never acquired. It may throw aLockReleasingException, which would mask the original exception fromacquire().Looking at the codebase patterns:
- SatisManager.run() (line 44): Correctly acquires before the try block
- PackageWebhookConsumer (line 27): Correctly acquires before the try block
- SatisManager.onBuild() (line 83): Incorrectly acquires inside the try block ❌
If
acquire(true)throws an exception in the onBuild() method, the finally block will callrelease()on a lock that was never acquired, which can result in aLockReleasingExceptionthat hides the original exception.Move the lock acquisition outside the try block for consistency and safety.
The code should be:
$this->lock->acquire(true); try { $status = $process->run(); } catch (RuntimeException $exception) { $status = 1; } finally { $this->lock->release(); }
♻️ Duplicate comments (1)
config/reference.php (1)
1274-1277:AppReference::configis undefined (previously flagged).As noted in the previous review, the call to
AppReference::config($config)at line 1276 references a class that does not exist in the codebase. This will cause a fatal error at runtime.
🧹 Nitpick comments (1)
src/Service/SatisManager.php (1)
125-128: Avoid callingbuild()twice.The
build()method is invoked twice: once for logging (line 125) and once for the return (line 127). Store the result in a variable to avoid redundant work.- $this->logger->info(\json_encode($satisCommandBuilder->build(), \JSON_THROW_ON_ERROR)); + $command = $satisCommandBuilder->build(); + $this->logger->info(\json_encode($command, \JSON_THROW_ON_ERROR)); - return $satisCommandBuilder->build(); + return $command;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/reference.php(1 hunks)src/Service/SatisManager.php(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Service/SatisManager.php (1)
src/Service/SatisCommandBuilder.php (1)
build(60-76)
🔇 Additional comments (5)
config/reference.php (2)
1-5: Verify whether this auto-generated file should be committed.This file is marked as auto-generated. Including auto-generated files in version control can lead to merge conflicts and maintenance issues. Additionally, this config reference appears unrelated to the PR's stated objective of improving test coverage for SatisCommandBuilder and LockProcessor.
Please confirm:
- Why is this auto-generated reference file included in a test coverage PR?
- Should this file be committed to version control, or should it be generated during build/deployment?
- If it must be committed, is there documentation explaining the generation process?
1345-1359: Routes class implementation looks correct.The
Routes::configmethod correctly returns the input configuration array, serving as a type-safe wrapper with comprehensive Psalm annotations for IDE support and static analysis.src/Service/SatisManager.php (3)
14-14: LGTM!Removing
finalaligns with theprotectedvisibility of class members and enables extension, which is consistent with the parallel change toBuildEvent.
65-75: LGTM!The docblock accurately describes the method's behavior, including lock lifecycle and exception handling semantics.
94-105: LGTM!The docblock correctly documents the
@throws \JsonExceptionthat can be thrown byjson_encodewithJSON_THROW_ON_ERRORon line 125.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
config/reference.php (1)
1273-1276:AppReference::config()call targets undefined class (duplicate).This issue was previously identified: the method delegates to
AppReference::config($config), butAppReferencedoes not exist in the codebase, causing a fatal error at runtime.
🧹 Nitpick comments (4)
src/Process/EnvironmentProvider.php (2)
5-10: Consider addingfinalmodifier for consistency.
ProcessFactoryis declared asfinal readonly classbutEnvironmentProvideris not. For consistency and to prevent unintended subclassing of this service class, consider making itfinalas well.-class EnvironmentProvider +final class EnvironmentProvider
12-32: Consider adding a return type annotation for better type safety.The
getEnv()method returns an associative array of strings. Adding a PHPDoc annotation would improve IDE support and static analysis.+ /** + * @return array<string, string> + */ public function getEnv(): arraysrc/Process/ProcessFactory.php (1)
17-21: Consider handling absolute paths in the command.If
command[0]is already an absolute path (starts with/), the concatenation will produce a double slash (e.g.,/root//bin/tool). While this typically works on Unix, consider normalizing or skipping the prefix for absolute paths.- $command[0] = $this->rootPath . '/' . $command[0]; + if (!str_starts_with($command[0], '/')) { + $command[0] = $this->rootPath . '/' . $command[0]; + }tests/Process/EnvironmentProviderTest.php (1)
17-26: Hardcoded path makes the test brittle.The assertion uses a hardcoded path
/app/var/composer. If the configuration changes, this test will fail unexpectedly. Consider asserting against the parameter value directly.public function testGetEnvSetsComposerDefaults(): void { $parameterBag = static::getContainer()->get(ParameterBagInterface::class); - $provider = new EnvironmentProvider($parameterBag->get('composer.home')); + $composerHome = $parameterBag->get('composer.home'); + $provider = new EnvironmentProvider($composerHome); $env = $provider->getEnv(); - $this->assertSame('/app/var/composer', $env['COMPOSER_HOME']); + $this->assertSame($composerHome, $env['COMPOSER_HOME']); $this->assertSame('1', $env['COMPOSER_NO_INTERACTION']); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
config/reference.php(1 hunks)config/services.yaml(1 hunks)src/Process/EnvironmentProvider.php(1 hunks)src/Process/ProcessFactory.php(1 hunks)src/Service/SatisManager.php(4 hunks)tests/Process/EnvironmentProviderTest.php(1 hunks)tests/Process/ProcessFactoryTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Service/SatisManager.php
🧰 Additional context used
🧬 Code graph analysis (4)
src/Process/EnvironmentProvider.php (3)
config/reference.php (1)
App(1266-1277)src/Process/ProcessFactory.php (1)
__construct(9-13)src/Service/SatisManager.php (1)
__construct(28-40)
src/Process/ProcessFactory.php (2)
src/Process/EnvironmentProvider.php (3)
__construct(7-10)EnvironmentProvider(5-33)getEnv(12-32)src/Service/SatisManager.php (1)
__construct(28-40)
tests/Process/ProcessFactoryTest.php (2)
src/Process/EnvironmentProvider.php (2)
EnvironmentProvider(5-33)getEnv(12-32)src/Service/SatisManager.php (1)
getCommandLine(110-132)
tests/Process/EnvironmentProviderTest.php (1)
src/Process/EnvironmentProvider.php (2)
EnvironmentProvider(5-33)getEnv(12-32)
🔇 Additional comments (5)
config/services.yaml (1)
54-62: LGTM!The service wiring correctly introduces the
EnvironmentProviderwith thecomposer.homeparameter and updatesProcessFactoryto depend on the provider instead of the raw path. This properly reflects the refactoring to centralize environment handling.tests/Process/ProcessFactoryTest.php (1)
10-51: Good test coverage for the core functionality.The tests effectively validate:
- Process configuration with environment injection
- Empty command validation
- Root path prefixing
Consider adding an optional test for the default timeout behavior when
nullis passed.src/Process/ProcessFactory.php (1)
7-13: Clean refactoring to use dependency injection.The
final readonlyclass declaration and constructor promotion improve immutability. InjectingEnvironmentProviderproperly separates concerns and improves testability.config/reference.php (2)
1344-1355: LGTM: Routes configurator implementation is correct.The
Routes::config()method correctly implements a pass-through pattern that provides type safety via Psalm annotations while simply returning the input configuration array. This is a valid approach for configuration validation.
1-4: This auto-generated file is correctly committed and should not be ignored.The
config/reference.phpfile is a Symfony DI container configuration reference file providing type hints for IDE/Psalm. Unlike build artifacts or cache files, this is a development tool meant to be committed for team-wide IDE autocomplete support. This follows the documented Symfony pattern for projects usingconfig/services.php. The file will regenerate automatically when Symfony bundles or dependencies change.
| protected function tearDown(): void | ||
| { | ||
| \putenv('TEST_KEY'); | ||
| unset($_SERVER['TEST_KEY'], $_ENV['TEST_ENV_KEY']); | ||
| } |
There was a problem hiding this comment.
Missing cleanup for COMPOSER_HOME in tearDown.
testComposerHomeNotOverwrittenIfProvidedInEnv sets COMPOSER_HOME via putenv() and $_SERVER (lines 57-58), but tearDown() doesn't clean these up. This could leak state to subsequent tests.
protected function tearDown(): void
{
\putenv('TEST_KEY');
- unset($_SERVER['TEST_KEY'], $_ENV['TEST_ENV_KEY']);
+ \putenv('COMPOSER_HOME');
+ unset($_SERVER['TEST_KEY'], $_SERVER['COMPOSER_HOME'], $_ENV['TEST_ENV_KEY']);
}📝 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.
| protected function tearDown(): void | |
| { | |
| \putenv('TEST_KEY'); | |
| unset($_SERVER['TEST_KEY'], $_ENV['TEST_ENV_KEY']); | |
| } | |
| protected function tearDown(): void | |
| { | |
| \putenv('TEST_KEY'); | |
| \putenv('COMPOSER_HOME'); | |
| unset($_SERVER['TEST_KEY'], $_SERVER['COMPOSER_HOME'], $_ENV['TEST_ENV_KEY']); | |
| } |
🤖 Prompt for AI Agents
In tests/Process/EnvironmentProviderTest.php around lines 11 to 15, tearDown()
currently cleans TEST_KEY and TEST_ENV_KEY but misses cleaning COMPOSER_HOME
which is set by testComposerHomeNotOverwrittenIfProvidedInEnv; update tearDown()
to also remove COMPOSER_HOME by calling putenv('COMPOSER_HOME') and unsetting
$_SERVER['COMPOSER_HOME'] and $_ENV['COMPOSER_HOME'] so the environment variable
does not leak into other tests.
| public function testComposerHomeNotOverwrittenIfProvidedInEnv(): void | ||
| { | ||
| \putenv('COMPOSER_HOME=/app/var/composer'); | ||
| $_SERVER['COMPOSER_HOME'] = '/app/var/composer'; | ||
|
|
||
| $parameterBag = static::getContainer()->get(ParameterBagInterface::class); | ||
| $provider = new EnvironmentProvider($parameterBag->get('composer.home')); | ||
|
|
||
| $env = $provider->getEnv(); | ||
|
|
||
| $this->assertSame('/app/var/composer', $env['COMPOSER_HOME']); | ||
| } |
There was a problem hiding this comment.
Test assertion doesn't prove the "not overwritten" behavior.
The test sets COMPOSER_HOME=/app/var/composer in the environment, which is the same value as the parameter default. To truly verify that the environment value is preserved and the default is not used, provide a different value in the environment than the constructor argument.
public function testComposerHomeNotOverwrittenIfProvidedInEnv(): void
{
- \putenv('COMPOSER_HOME=/app/var/composer');
- $_SERVER['COMPOSER_HOME'] = '/app/var/composer';
+ \putenv('COMPOSER_HOME=/custom/composer/path');
+ $_SERVER['COMPOSER_HOME'] = '/custom/composer/path';
$parameterBag = static::getContainer()->get(ParameterBagInterface::class);
$provider = new EnvironmentProvider($parameterBag->get('composer.home'));
$env = $provider->getEnv();
- $this->assertSame('/app/var/composer', $env['COMPOSER_HOME']);
+ $this->assertSame('/custom/composer/path', $env['COMPOSER_HOME']);
}🤖 Prompt for AI Agents
In tests/Process/EnvironmentProviderTest.php around lines 55 to 66, the test
uses the same COMPOSER_HOME value for both the environment and the constructor
parameter so it doesn't prove "not overwritten"; change the environment value to
a different path than the constructor argument (e.g. keep the constructor
argument as '/app/var/composer' but set putenv and $_SERVER['COMPOSER_HOME'] to
a different value like '/custom/composer') and assert that getEnv() returns the
environment-provided value to verify the default wasn't used.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.