feat: implement configuration factory and file reader for translation validator#26
feat: implement configuration factory and file reader for translation validator#26konradmichalik merged 4 commits intomainfrom
Conversation
WalkthroughThis change introduces new configuration and rendering abstractions, refactors configuration reading and validation logic into dedicated classes, and centralizes result rendering logic into an abstract base class used by CLI and JSON renderers. Comprehensive PHPUnit tests are added for the new configuration and rendering components, and minor documentation is updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConfigReader
participant ConfigFileReader
participant ConfigFactory
participant ConfigValidator
participant TranslationValidatorConfig
User->>ConfigReader: read(configPath)
ConfigReader->>ConfigFileReader: readAsConfig(configPath)
ConfigFileReader->>ConfigFactory: createFromArray(configData)
ConfigFactory->>ConfigValidator: validate(configData)
ConfigFactory->>TranslationValidatorConfig: (populate config object)
ConfigFactory-->>ConfigFileReader: TranslationValidatorConfig
ConfigFileReader-->>ConfigReader: TranslationValidatorConfig
ConfigReader-->>User: TranslationValidatorConfig
sequenceDiagram
participant Validator
participant ValidationResult
participant AbstractRenderer
participant Output
Validator->>ValidationResult: validate()
AbstractRenderer->>ValidationResult: groupIssuesByFile()
AbstractRenderer->>ValidationResult: generateMessage()
AbstractRenderer->>ValidationResult: formatStatisticsForOutput()
AbstractRenderer->>Output: render()
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
tests/src/Result/AbstractValidationResultRendererTest.php (1)
162-165: Acknowledge the unused parameter in test stub.The static analysis tool correctly identifies the unused
$validationResultparameter. Since this is an intentional test stub, consider either using the parameter or adding a suppression comment.public function render(ValidationResult $validationResult): int { + // Intentionally unused - test stub implementation + unset($validationResult); return 0; }src/Result/AbstractValidationResultRenderer.php (1)
40-83: Consider extracting validator group initialization for improved readability.The
groupIssuesByFilemethod correctly implements the grouping logic, but the nested array initialization could be extracted for better readability.Consider extracting the validator group initialization:
foreach ($distributedIssues as $filePath => $issues) { $normalizedPath = $this->normalizePath($filePath); - if (!isset($groupedByFile[$normalizedPath])) { - $groupedByFile[$normalizedPath] = []; - } - - $validatorName = $validator->getShortName(); - if (!isset($groupedByFile[$normalizedPath][$validatorName])) { - $groupedByFile[$normalizedPath][$validatorName] = [ - 'validator' => $validator, - 'type' => $validator->resultTypeOnValidationFailure()->toString(), - 'issues' => [], - ]; - } + $groupedByFile[$normalizedPath] ??= []; + $validatorName = $validator->getShortName(); + $groupedByFile[$normalizedPath][$validatorName] ??= $this->createValidatorGroup($validator); foreach ($issues as $issue) { $groupedByFile[$normalizedPath][$validatorName]['issues'][] = $issue; } }Add a new private method:
private function createValidatorGroup(ValidatorInterface $validator): array { return [ 'validator' => $validator, 'type' => $validator->resultTypeOnValidationFailure()->toString(), 'issues' => [], ]; }src/Config/ConfigValidator.php (1)
14-60: Consider refactoring repetitive validation logic to reduce code duplication.The validation logic follows a repetitive pattern that could be refactored for better maintainability. Consider extracting the validation logic into reusable methods.
Here's a suggested refactor to reduce duplication:
+ private const ARRAY_FIELDS = ['paths', 'validators', 'file-detectors', 'parsers', 'only', 'skip', 'exclude']; + private const BOOLEAN_FIELDS = ['strict', 'dry-run', 'verbose']; + public function validate(array $data): void { - if (isset($data['paths']) && !is_array($data['paths'])) { - throw new \RuntimeException("Configuration 'paths' must be an array"); - } - - if (isset($data['validators']) && !is_array($data['validators'])) { - throw new \RuntimeException("Configuration 'validators' must be an array"); - } - - if (isset($data['file-detectors']) && !is_array($data['file-detectors'])) { - throw new \RuntimeException("Configuration 'file-detectors' must be an array"); - } - - if (isset($data['parsers']) && !is_array($data['parsers'])) { - throw new \RuntimeException("Configuration 'parsers' must be an array"); - } - - if (isset($data['only']) && !is_array($data['only'])) { - throw new \RuntimeException("Configuration 'only' must be an array"); - } - - if (isset($data['skip']) && !is_array($data['skip'])) { - throw new \RuntimeException("Configuration 'skip' must be an array"); - } - - if (isset($data['exclude']) && !is_array($data['exclude'])) { - throw new \RuntimeException("Configuration 'exclude' must be an array"); - } - - if (isset($data['strict']) && !is_bool($data['strict'])) { - throw new \RuntimeException("Configuration 'strict' must be a boolean"); - } - - if (isset($data['dry-run']) && !is_bool($data['dry-run'])) { - throw new \RuntimeException("Configuration 'dry-run' must be a boolean"); - } + $this->validateArrayFields($data); + $this->validateBooleanFields($data); if (isset($data['format'])) { if (!is_string($data['format'])) { throw new \RuntimeException("Configuration 'format' must be a string"); } $this->validateFormat($data['format']); } - - if (isset($data['verbose']) && !is_bool($data['verbose'])) { - throw new \RuntimeException("Configuration 'verbose' must be a boolean"); - } } + + private function validateArrayFields(array $data): void + { + foreach (self::ARRAY_FIELDS as $field) { + if (isset($data[$field]) && !is_array($data[$field])) { + throw new \RuntimeException("Configuration '{$field}' must be an array"); + } + } + } + + private function validateBooleanFields(array $data): void + { + foreach (self::BOOLEAN_FIELDS as $field) { + if (isset($data[$field]) && !is_bool($data[$field])) { + throw new \RuntimeException("Configuration '{$field}' must be a boolean"); + } + } + }tests/src/Config/ConfigFileReaderTest.php (1)
169-177: Consider more specific YAML error testing.The YAML error test uses a generic
\Exceptionexpectation. Consider testing for more specific YAML parsing exceptions if the underlying YAML parser throws them.- $this->expectException(\Exception::class); + $this->expectException(\Symfony\Component\Yaml\Exception\ParseException::class);src/Config/ConfigFactory.php (1)
30-74: Consider using a more functional approach for property mapping.While the current implementation is clear, the repetitive pattern could be refactored for better maintainability.
+ private const PROPERTY_MAPPINGS = [ + 'paths' => 'setPaths', + 'validators' => 'setValidators', + 'file-detectors' => 'setFileDetectors', + 'parsers' => 'setParsers', + 'only' => 'setOnly', + 'skip' => 'setSkip', + 'exclude' => 'setExclude', + 'strict' => 'setStrict', + 'dry-run' => 'setDryRun', + 'format' => 'setFormat', + 'verbose' => 'setVerbose', + ]; + - if (isset($data['paths'])) { - $config->setPaths($data['paths']); - } - // ... (repeat for other properties) + foreach (self::PROPERTY_MAPPINGS as $key => $method) { + if (isset($data[$key])) { + $config->$method($data[$key]); + } + }src/Config/ConfigReader.php (2)
16-20: Consider removing the default instantiation for better testability.While convenient, instantiating
ConfigFileReaderdirectly in the constructor parameter creates a hard dependency that makes unit testing more difficult. Consider requiring the dependency to be injected.public function __construct( - private readonly ConfigFileReader $fileReader = new ConfigFileReader(), + private readonly ConfigFileReader $fileReader, ) { }This would require callers to provide the dependency but improves testability and follows dependency injection best practices.
50-70: Consider delegating composer.json reading for consistency.While other file reading operations have been delegated to
ConfigFileReader, this method still reads files directly. Consider delegating composer.json reading to maintain consistency.You could extend
ConfigFileReaderto handle composer.json parsing or create a separate reader for it. This would centralize all file I/O operations and maintain the separation of concerns established by the refactoring.src/Config/ConfigFileReader.php (2)
11-14: Consider removing the default instantiation for better testability.Similar to
ConfigReader, instantiatingConfigFactorydirectly in the constructor parameter creates a hard dependency. For consistency and better testability, consider requiring the dependency to be injected.public function __construct( - private readonly ConfigFactory $factory = new ConfigFactory(), + private readonly ConfigFactory $factory, ) { }
44-55: Consider optimizing duplicate extension parsing.The file extension is parsed twice - once here and once in
readFile(). Consider restructuring to avoid this duplication.You could refactor to parse the extension once and pass it to internal methods, or restructure the logic to avoid the duplicate parsing. For example:
public function readAsConfig(string $configPath): TranslationValidatorConfig { $extension = pathinfo($configPath, PATHINFO_EXTENSION); if ('php' === $extension) { + if (!file_exists($configPath)) { + throw new \InvalidArgumentException("Configuration file not found: {$configPath}"); + } + if (!is_readable($configPath)) { + throw new \RuntimeException("Configuration file is not readable: {$configPath}"); + } return $this->readPhpConfig($configPath); } $data = $this->readFile($configPath); return $this->factory->createFromArray($data); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
README.md(1 hunks)src/Config/ConfigFactory.php(1 hunks)src/Config/ConfigFileReader.php(1 hunks)src/Config/ConfigReader.php(1 hunks)src/Config/ConfigValidator.php(1 hunks)src/Result/AbstractValidationResultRenderer.php(1 hunks)src/Result/ValidationResultCliRenderer.php(8 hunks)src/Result/ValidationResultJsonRenderer.php(2 hunks)tests/src/Config/ConfigFactoryTest.php(1 hunks)tests/src/Config/ConfigFileReaderTest.php(1 hunks)tests/src/Config/ConfigValidatorTest.php(1 hunks)tests/src/Result/AbstractValidationResultRendererTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/src/Config/ConfigValidatorTest.php (1)
src/Config/ConfigValidator.php (2)
ConfigValidator(7-69)validate(12-60)
tests/src/Result/AbstractValidationResultRendererTest.php (3)
src/Result/AbstractValidationResultRenderer.php (6)
AbstractValidationResultRenderer(10-131)generateMessage(19-35)calculateExitCode(127-130)normalizePath(85-105)formatStatisticsForOutput(110-125)groupIssuesByFile(40-83)src/Result/ValidationResult.php (1)
ValidationResult(11-69)src/Result/ValidationStatistics.php (1)
ValidationStatistics(7-51)
🪛 PHPMD (2.15.0)
tests/src/Result/AbstractValidationResultRendererTest.php
162-162: Avoid unused parameters such as '$validationResult'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (19)
README.md (1)
71-73: Confirm cross-platform rendering of[!NOTE]callout
[!NOTE]is correctly rendered as an admonition on GitHub, but some Markdown renderers (e.g., Bitbucket, npmjs, plainmarkdownCLI tools) treat it as a literal blockquote line.
If the README is expected to be consumed outside GitHub, consider a more portable syntax (plain> **Note:**or a docs-site–specific admonition tag).src/Result/ValidationResultJsonRenderer.php (2)
12-28: Good refactoring to use parent class methods.The changes effectively delegate exit code calculation and statistics formatting to the parent class, reducing code duplication.
33-57: Excellent simplification using parent's grouping logic.The refactored
formatIssuesForJsonmethod effectively reuses the parent'sgroupIssuesByFilemethod, eliminating duplicate grouping logic while maintaining the same output structure.src/Result/AbstractValidationResultRenderer.php (2)
12-35: Well-structured message generation with comprehensive coverage.The
generateMessagemethod effectively handles all validation result scenarios using a modern match expression, with appropriate messages for each combination of result type, dry-run, and strict modes.
85-105: Robust path normalization implementation.The
normalizePathmethod properly handles both existing and non-existing paths, with appropriate normalization to create consistent relative paths.src/Result/ValidationResultCliRenderer.php (3)
18-43: Proper integration with parent class.The constructor correctly delegates to the parent class, and the
rendermethod appropriately uses the inheritedcalculateExitCodemethod.
45-143: Successful refactoring to use parent's grouping logic.Both
renderCompactOutputandrenderVerboseOutputmethods have been successfully updated to use the parent'sgroupIssuesByFilemethod, eliminating code duplication while maintaining the same output behavior.
216-241: Maintain currentgenerateMessageusageThe
generateMessage()method is explicitly defined to accept aValidationResultand is invoked in multiple places (CLI renderer, JSON renderer, and tests). Instantiating a newValidationResult([], $resultType)here:
- Aligns with the existing API signature.
- Has a negligible performance impact in a CLI context.
- Avoids a breaking change—refactoring
generateMessage()to acceptResultTypedirectly would require updates to:
- src/Result/ValidationResultJsonRenderer.php
- tests/src/Result/AbstractValidationResultRendererTest.php
- Any other callers of
generateMessage()No refactoring is needed. Leave the object creation in place.
Likely an incorrect or invalid review comment.
src/Config/ConfigValidator.php (1)
62-68: Good separation of format validation logic.The dedicated method for format validation provides good separation of concerns and makes the validation logic more maintainable.
tests/src/Config/ConfigFileReaderTest.php (1)
15-28: Good test isolation with proper setup and cleanup.The temporary directory approach ensures test isolation and proper cleanup. The unique directory naming prevents conflicts between concurrent test runs.
src/Config/ConfigFactory.php (2)
9-12: Good use of dependency injection with sensible defaults.The constructor injection pattern with a default instance provides flexibility while maintaining simplicity for common usage.
21-25: Availability check verified – no changes neededThe
isAvailable()method simply wrapsclass_exists(JsonSchema\Validator::class), matching the early-return invalidate(). Schema validation will only run when the JSON-Schema library is present, and any JSON or schema errors surface as exceptions invalidate(), which is the intended behavior.tests/src/Config/ConfigFactoryTest.php (1)
50-66: Good test design with minimal valid data.Using minimal valid data to avoid schema validation issues is a good testing practice that focuses on the specific functionality being tested.
tests/src/Config/ConfigValidatorTest.php (3)
19-37: Excellent comprehensive positive test case.The test includes all possible configuration options and verifies that valid configurations pass without exceptions. The use of
addToAssertionCount(1)is a good practice for void methods.
39-133: Thorough validation testing with proper exception assertions.Each validation rule is tested individually with specific exception messages, providing excellent coverage and debugging information when tests fail.
135-139: Good edge case coverage for empty configuration.Testing empty configuration ensures the validator doesn't fail on minimal input, which is important for optional configuration scenarios.
src/Config/ConfigReader.php (1)
24-27: Clean delegation pattern implementation.The refactoring to delegate file reading to
ConfigFileReaderis well implemented and maintains the existing interface.src/Config/ConfigFileReader.php (2)
21-39: Well-structured file reading with proper validation.Excellent implementation with proper file existence and readability checks, clear error messages, and modern PHP 8 match expression for format dispatch.
81-107: Well-implemented configuration parsers with proper error handling.Both JSON and YAML parsing methods have excellent error handling and type validation. The use of
JSON_THROW_ON_ERRORfor JSON parsing and proper type checking for both formats ensures robust configuration reading.
| final class AbstractValidationResultRendererTest extends TestCase | ||
| { | ||
| private TestableAbstractValidationResultRenderer $renderer; | ||
| private BufferedOutput $output; | ||
|
|
||
| protected function setUp(): void | ||
| { | ||
| $this->output = new BufferedOutput(); | ||
| $this->renderer = new TestableAbstractValidationResultRenderer($this->output); | ||
| } | ||
|
|
||
| public function testGenerateMessageForSuccess(): void | ||
| { | ||
| $result = new ValidationResult([], ResultType::SUCCESS); | ||
| $message = $this->renderer->testGenerateMessage($result); | ||
|
|
||
| $this->assertSame('Language validation succeeded.', $message); | ||
| } | ||
|
|
||
| public function testGenerateMessageForError(): void | ||
| { | ||
| $result = new ValidationResult([], ResultType::ERROR); | ||
| $message = $this->renderer->testGenerateMessage($result); | ||
|
|
||
| $this->assertSame('Language validation failed with errors.', $message); | ||
| } | ||
|
|
||
| public function testGenerateMessageForWarning(): void | ||
| { | ||
| $result = new ValidationResult([], ResultType::WARNING); | ||
| $message = $this->renderer->testGenerateMessage($result); | ||
|
|
||
| $this->assertSame('Language validation completed with warnings.', $message); | ||
| } | ||
|
|
||
| public function testGenerateMessageForErrorInDryRun(): void | ||
| { | ||
| $renderer = new TestableAbstractValidationResultRenderer($this->output, true); | ||
| $result = new ValidationResult([], ResultType::ERROR); | ||
| $message = $renderer->testGenerateMessage($result); | ||
|
|
||
| $this->assertSame('Language validation failed with errors in dry-run mode.', $message); | ||
| } | ||
|
|
||
| public function testGenerateMessageForWarningInDryRun(): void | ||
| { | ||
| $renderer = new TestableAbstractValidationResultRenderer($this->output, true); | ||
| $result = new ValidationResult([], ResultType::WARNING); | ||
| $message = $renderer->testGenerateMessage($result); | ||
|
|
||
| $this->assertSame('Language validation completed with warnings in dry-run mode.', $message); | ||
| } | ||
|
|
||
| public function testGenerateMessageForWarningInStrictMode(): void | ||
| { | ||
| $renderer = new TestableAbstractValidationResultRenderer($this->output, false, true); | ||
| $result = new ValidationResult([], ResultType::WARNING); | ||
| $message = $renderer->testGenerateMessage($result); | ||
|
|
||
| $this->assertSame('Language validation failed with warnings in strict mode.', $message); | ||
| } | ||
|
|
||
| public function testCalculateExitCodeForSuccess(): void | ||
| { | ||
| $result = new ValidationResult([], ResultType::SUCCESS); | ||
| $exitCode = $this->renderer->testCalculateExitCode($result); | ||
|
|
||
| $this->assertSame(0, $exitCode); | ||
| } | ||
|
|
||
| public function testCalculateExitCodeForError(): void | ||
| { | ||
| $result = new ValidationResult([], ResultType::ERROR); | ||
| $exitCode = $this->renderer->testCalculateExitCode($result); | ||
|
|
||
| $this->assertSame(1, $exitCode); | ||
| } | ||
|
|
||
| public function testCalculateExitCodeForWarning(): void | ||
| { | ||
| $result = new ValidationResult([], ResultType::WARNING); | ||
| $exitCode = $this->renderer->testCalculateExitCode($result); | ||
|
|
||
| $this->assertSame(0, $exitCode); | ||
| } | ||
|
|
||
| public function testCalculateExitCodeForWarningInStrictMode(): void | ||
| { | ||
| $renderer = new TestableAbstractValidationResultRenderer($this->output, false, true); | ||
| $result = new ValidationResult([], ResultType::WARNING); | ||
| $exitCode = $renderer->testCalculateExitCode($result); | ||
|
|
||
| $this->assertSame(1, $exitCode); | ||
| } | ||
|
|
||
| public function testCalculateExitCodeForErrorInDryRun(): void | ||
| { | ||
| $renderer = new TestableAbstractValidationResultRenderer($this->output, true); | ||
| $result = new ValidationResult([], ResultType::ERROR); | ||
| $exitCode = $renderer->testCalculateExitCode($result); | ||
|
|
||
| $this->assertSame(0, $exitCode); | ||
| } | ||
|
|
||
| public function testNormalizePathWithRealPath(): void | ||
| { | ||
| $currentDir = getcwd(); | ||
| $testFile = $currentDir.'/composer.json'; | ||
|
|
||
| if (file_exists($testFile)) { | ||
| $normalized = $this->renderer->testNormalizePath($testFile); | ||
| $this->assertSame('composer.json', $normalized); | ||
| } else { | ||
| $this->markTestSkipped('composer.json not found in current directory'); | ||
| } | ||
| } | ||
|
|
||
| public function testNormalizePathWithNonExistentPath(): void | ||
| { | ||
| $normalized = $this->renderer->testNormalizePath('./some/non/existent/path'); | ||
| $this->assertSame('some/non/existent/path', $normalized); | ||
| } | ||
|
|
||
| public function testFormatStatisticsForOutput(): void | ||
| { | ||
| $statistics = new \MoveElevator\ComposerTranslationValidator\Result\ValidationStatistics(1.234, 5, 10, 4, 3); | ||
| $result = new ValidationResult([], ResultType::SUCCESS, [], $statistics); | ||
|
|
||
| $formatted = $this->renderer->testFormatStatisticsForOutput($result); | ||
|
|
||
| $this->assertEqualsWithDelta(1.234, $formatted['execution_time'], PHP_FLOAT_EPSILON); | ||
| $this->assertSame('1.23s', $formatted['execution_time_formatted']); | ||
| $this->assertSame(5, $formatted['files_checked']); | ||
| $this->assertSame(10, $formatted['keys_checked']); | ||
| $this->assertSame(4, $formatted['validators_run']); | ||
| $this->assertSame(3, $formatted['parsers_cached']); | ||
| } | ||
|
|
||
| public function testFormatStatisticsForOutputWithoutStatistics(): void | ||
| { | ||
| $result = new ValidationResult([], ResultType::SUCCESS); | ||
| $formatted = $this->renderer->testFormatStatisticsForOutput($result); | ||
|
|
||
| $this->assertSame([], $formatted); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add test coverage for groupIssuesByFile method.
The TestableAbstractValidationResultRenderer exposes the testGroupIssuesByFile method (lines 193-196), but there are no test cases that exercise this critical functionality. This method is used by both CLI and JSON renderers to organize validation issues.
Would you like me to generate comprehensive test cases for the groupIssuesByFile method to ensure proper coverage of issue grouping logic?
🤖 Prompt for AI Agents
In tests/src/Result/AbstractValidationResultRendererTest.php around lines 13 to
158, add new test methods to cover the groupIssuesByFile functionality exposed
by TestableAbstractValidationResultRenderer's testGroupIssuesByFile method.
Create test cases that pass arrays of validation issues with different file
paths and verify that the method correctly groups issues by their file
attribute. Include edge cases such as empty input arrays and multiple issues per
file to ensure comprehensive coverage of the grouping logic.
| public function testReadFileNotReadable(): void | ||
| { | ||
| $configPath = $this->tempDir.'/config.json'; | ||
| file_put_contents($configPath, '{}'); | ||
| chmod($configPath, 0000); | ||
|
|
||
| $this->expectException(\RuntimeException::class); | ||
| $this->expectExceptionMessage('Configuration file is not readable'); | ||
|
|
||
| try { | ||
| $this->reader->readFile($configPath); | ||
| } finally { | ||
| chmod($configPath, 0644); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify file permission restoration in edge cases.
The file permission test properly restores permissions in the finally block, but consider what happens if the test fails before the permission change.
🏁 Script executed:
#!/bin/bash
# Description: Check if there are any additional file permission tests or error handling patterns in the codebase
# Test: Search for similar file permission patterns in tests
rg -A 10 -B 5 "chmod.*0000" --type php
# Test: Look for other temporary file cleanup patterns
rg -A 5 "sys_get_temp_dir" --type phpLength of output: 15168
Ensure consistent file-permission cleanup in unreadable-file tests
Several tests make files unreadable without restoring permissions or cleaning them up afterward. To prevent leftover unreadable temp files, wrap the chmod in a try/finally (or add a tearDown) just like in AbstractParserTest and ConfigFileReaderTest:
• tests/src/Parser/YamlParserTest.php
- testConstructorThrowsExceptionIfFileIsNotReadable lacks a finally to
chmod($unreadableFile, 0644)andunlink($unreadableFile).
• tests/src/Parser/XliffParserTest.php - testConstructorThrowsExceptionIfFileIsNotReadable also needs a cleanup block.
Recommended fixes:
- Add a try/finally around the exception-throwing code in both classes to reset permissions and remove the file.
- Or implement a shared tearDown() in these test classes (or a base class) that reverts perms and deletes any files created in sys_get_temp_dir().
🤖 Prompt for AI Agents
In tests/src/Parser/YamlParserTest.php and tests/src/Parser/XliffParserTest.php
around the testConstructorThrowsExceptionIfFileIsNotReadable methods, the tests
make files unreadable but do not restore file permissions or delete the files
afterward. To fix this, wrap the code that triggers the exception in a
try/finally block where the finally resets the file permissions to 0644 and
deletes the temporary file. Alternatively, implement a shared tearDown() method
in these test classes or a common base class that resets permissions and removes
any temp files created during the tests.
| public function testCreateFromArrayValidatesData(): void | ||
| { | ||
| $this->expectException(\RuntimeException::class); | ||
| $this->expectExceptionMessage('Configuration validation failed'); | ||
|
|
||
| $this->factory->createFromArray(['paths' => 'invalid']); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider testing specific validation error scenarios.
The current test only checks for a generic RuntimeException. Consider adding more specific tests for different validation failures to ensure proper error handling.
public function testCreateFromArrayValidatesData(): void
{
$this->expectException(\RuntimeException::class);
- $this->expectExceptionMessage('Configuration validation failed');
+ $this->expectExceptionMessage("Configuration 'paths' must be an array");
$this->factory->createFromArray(['paths' => 'invalid']);
}
+
+ public function testCreateFromArrayValidatesFormatValue(): void
+ {
+ $this->expectException(\RuntimeException::class);
+ $this->expectExceptionMessage("Invalid format 'invalid'. Allowed formats: cli, json, yaml, php");
+
+ $this->factory->createFromArray(['format' => 'invalid']);
+ }📝 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.
| public function testCreateFromArrayValidatesData(): void | |
| { | |
| $this->expectException(\RuntimeException::class); | |
| $this->expectExceptionMessage('Configuration validation failed'); | |
| $this->factory->createFromArray(['paths' => 'invalid']); | |
| } | |
| public function testCreateFromArrayValidatesData(): void | |
| { | |
| $this->expectException(\RuntimeException::class); | |
| $this->expectExceptionMessage("Configuration 'paths' must be an array"); | |
| $this->factory->createFromArray(['paths' => 'invalid']); | |
| } | |
| public function testCreateFromArrayValidatesFormatValue(): void | |
| { | |
| $this->expectException(\RuntimeException::class); | |
| $this->expectExceptionMessage("Invalid format 'invalid'. Allowed formats: cli, json, yaml, php"); | |
| $this->factory->createFromArray(['format' => 'invalid']); | |
| } |
🤖 Prompt for AI Agents
In tests/src/Config/ConfigFactoryTest.php around lines 85 to 91, the test only
checks for a generic RuntimeException when validation fails. Add additional test
methods that trigger specific validation errors with distinct invalid inputs,
and assert that the exceptions thrown include precise messages or exception
types for each validation failure scenario. This will ensure more granular and
accurate error handling coverage.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/src/Result/AbstractValidationResultRendererTest.php (1)
194-197: Missing test coverage for the exposed groupIssuesByFile method.The
TestableAbstractValidationResultRendererexposes thetestGroupIssuesByFilemethod, but there are no test cases that exercise this critical functionality. This method is used by both CLI and JSON renderers to organize validation issues.Would you like me to generate comprehensive test cases for the
groupIssuesByFilemethod to ensure proper coverage of issue grouping logic?
🧹 Nitpick comments (2)
tests/src/Config/ConfigFileReaderTest.php (2)
46-58: Consider improving YAML readability.The YAML content string with escaped newlines is functional but could be more readable.
Consider using a heredoc or nowdoc for better readability:
- $yamlContent = "paths:\n - translations/\nstrict: true\nformat: json"; + $yamlContent = <<<YAML +paths: + - translations/ +strict: true +format: json +YAML;
60-81: Consider extracting PHP config content to a helper method.The embedded PHP code is quite long and is duplicated in another test method. Consider extracting it to a helper method for better maintainability.
Extract the PHP content generation:
private function createPhpConfigContent(): string { return '<?php use MoveElevator\ComposerTranslationValidator\Config\TranslationValidatorConfig; $config = new TranslationValidatorConfig(); $config->setPaths([\'translations/\']) ->setStrict(true) ->setFormat(\'json\'); return $config; '; }Then use it in both test methods:
- $phpContent = '<?php -use MoveElevator\ComposerTranslationValidator\Config\TranslationValidatorConfig; - -$config = new TranslationValidatorConfig(); -$config->setPaths([\'translations/\']) - ->setStrict(true) - ->setFormat(\'json\'); - -return $config; -'; + $phpContent = $this->createPhpConfigContent();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/Config/ConfigFileReader.php(1 hunks)tests/src/Command/ValidateTranslationCommandTest.php(2 hunks)tests/src/Config/ConfigFactoryTest.php(1 hunks)tests/src/Config/ConfigFileReaderTest.php(1 hunks)tests/src/Config/ConfigReaderTest.php(4 hunks)tests/src/Config/SchemaValidatorTest.php(0 hunks)tests/src/FileDetector/CollectorTest.php(4 hunks)tests/src/FileDetector/FileDetectorRegistryTest.php(2 hunks)tests/src/Parser/AbstractParserTest.php(1 hunks)tests/src/Parser/ParserCacheTest.php(2 hunks)tests/src/Parser/ParserRegistryTest.php(2 hunks)tests/src/Parser/XliffParserTest.php(1 hunks)tests/src/Parser/YamlParserTest.php(1 hunks)tests/src/Result/AbstractValidationResultRendererTest.php(1 hunks)tests/src/Result/ValidationResultCliRendererTest.php(1 hunks)tests/src/Result/ValidationResultJsonRendererTest.php(1 hunks)tests/src/Result/ValidationRunTest.php(13 hunks)tests/src/Utility/PathUtilityTest.php(1 hunks)tests/src/Validator/AbstractValidatorTest.php(7 hunks)tests/src/Validator/SchemaValidatorTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- tests/src/Config/SchemaValidatorTest.php
✅ Files skipped from review due to trivial changes (12)
- tests/src/Validator/SchemaValidatorTest.php
- tests/src/Parser/AbstractParserTest.php
- tests/src/Parser/XliffParserTest.php
- tests/src/Parser/ParserRegistryTest.php
- tests/src/FileDetector/FileDetectorRegistryTest.php
- tests/src/Parser/YamlParserTest.php
- tests/src/Command/ValidateTranslationCommandTest.php
- tests/src/Config/ConfigReaderTest.php
- tests/src/FileDetector/CollectorTest.php
- tests/src/Parser/ParserCacheTest.php
- tests/src/Validator/AbstractValidatorTest.php
- tests/src/Utility/PathUtilityTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/src/Config/ConfigFactoryTest.php
- src/Config/ConfigFileReader.php
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/src/Result/ValidationResultJsonRendererTest.php (1)
src/Result/Issue.php (1)
Issue(7-55)
tests/src/Result/ValidationResultCliRendererTest.php (1)
src/Result/Issue.php (1)
Issue(7-55)
tests/src/Result/ValidationRunTest.php (3)
src/Result/Issue.php (1)
Issue(7-55)src/Validator/AbstractValidator.php (1)
addIssue(120-123)src/Validator/ValidatorInterface.php (1)
addIssue(41-41)
🪛 PHPMD (2.15.0)
tests/src/Result/ValidationResultJsonRendererTest.php
305-305: Avoid unused parameters such as '$issue'. (Unused Code Rules)
(UnusedFormalParameter)
tests/src/Result/ValidationResultCliRendererTest.php
660-660: Avoid unused parameters such as '$issue'. (Unused Code Rules)
(UnusedFormalParameter)
tests/src/Result/AbstractValidationResultRendererTest.php
163-163: Avoid unused parameters such as '$validationResult'. (Unused Code Rules)
(UnusedFormalParameter)
tests/src/Result/ValidationRunTest.php
323-323: Avoid unused parameters such as '$file'. (Unused Code Rules)
(UnusedFormalParameter)
348-348: Avoid unused parameters such as '$issue'. (Unused Code Rules)
(UnusedFormalParameter)
352-352: Avoid unused parameters such as '$issue'. (Unused Code Rules)
(UnusedFormalParameter)
352-352: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
367-367: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
367-367: Avoid unused parameters such as '$issues'. (Unused Code Rules)
(UnusedFormalParameter)
396-396: Avoid unused parameters such as '$file'. (Unused Code Rules)
(UnusedFormalParameter)
421-421: Avoid unused parameters such as '$issue'. (Unused Code Rules)
(UnusedFormalParameter)
425-425: Avoid unused parameters such as '$issue'. (Unused Code Rules)
(UnusedFormalParameter)
425-425: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
450-450: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
450-450: Avoid unused parameters such as '$issues'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (9)
tests/src/Result/ValidationResultJsonRendererTest.php (1)
290-431: Excellent test coverage for issue grouping functionality.The new test methods provide comprehensive coverage for the issue grouping functionality:
testGroupIssuesByFileWithMultipleValidators- Tests multiple validators reporting on the same filetestGroupIssuesByFileWithMultipleFiles- Tests single validator reporting on multiple filestestGroupIssuesByFileWithEmptyValidators- Tests edge case with no validatorstestGroupIssuesByFileWithValidatorsWithoutIssues- Tests validators without issuesThe tests properly verify the JSON output structure, ensuring issues are grouped by file path and then by validator short name. The exit code assertions also confirm correct behavior.
Note: The static analysis hint about unused parameters in callback functions (line 305) is a false positive - the parameters are required for the callback signature.
tests/src/Result/AbstractValidationResultRendererTest.php (1)
1-199: Well-structured tests for the abstract base class.The test file provides comprehensive coverage for the
AbstractValidationResultRendererfunctionality:
- Message generation for different result types and modes
- Exit code calculation logic
- Path normalization behavior
- Statistics formatting
The
TestableAbstractValidationResultRendererapproach is a good pattern for testing abstract classes by exposing protected methods.Note: The static analysis hint about unused parameter
$validationResultin the render method (line 163) is a false positive - it's a required stub implementation.tests/src/Result/ValidationResultCliRendererTest.php (1)
644-798: Comprehensive test coverage for CLI renderer grouping functionality.The new test methods provide excellent coverage for the CLI renderer's issue grouping functionality:
testGroupIssuesByFileWithMultipleValidatorsVerbose- Tests multiple validators in verbose modetestGroupIssuesByFileWithMultipleFilesVerbose- Tests single validator with multiple filestestGroupIssuesByFileWarningsNotShownInCompactMode- Tests compact mode behavior for warningstestGroupIssuesByFileWithEmptyValidators- Tests edge case with no validatorstestGroupIssuesByFileErrorsShownInCompactMode- Tests compact mode behavior for errorsThe tests properly verify both verbose and compact output modes, ensuring correct file path display and validator name grouping. The exit code assertions confirm proper behavior across different scenarios.
Note: The static analysis hint about unused parameter
$issuein the callback function (line 660) is a false positive - the parameter is required for the callback signature.tests/src/Result/ValidationRunTest.php (3)
8-11: Good improvement in code readability with imports.The addition of import statements for commonly used classes (
ParserInterface,Issue,ValidationStatistics,OutputInterface) improves code readability by allowing the use of short class names throughout the file instead of fully qualified names.Also applies to: 16-16
194-194: Proper use of imported ValidationStatistics class.The
assertInstanceOfassertions now use the importedValidationStatisticsclass instead of the fully qualified class name, making the code more readable and consistent.Also applies to: 216-216, 229-229
265-265: Mock class implementations are correct despite static analysis hints.The static analysis hints about unused parameters in the mock classes are false positives. These parameters are required by the interface contracts (
ValidatorInterface,ParserInterface) and must be present even if not used in the mock implementations.Also applies to: 323-323, 348-348, 352-352, 367-367, 396-396, 418-418, 421-421, 425-425, 450-450, 460-460, 498-498
tests/src/Config/ConfigFileReaderTest.php (3)
15-28: LGTM! Proper test setup and cleanup implemented.The setUp and tearDown methods correctly create a unique temporary directory for each test and clean up all files afterward. The use of
uniqid()ensures test isolation, and the cleanup logic properly removes files before removing the directory.
132-146: Excellent file permission handling.The try/finally block properly restores file permissions even if the test fails, preventing leftover unreadable files. This follows the best practice pattern that was missing in other similar test files.
169-177: No changes needed: Symfony YAML parser throws on malformed inputThe
readYamlConfigmethod usesSymfony\Component\Yaml\Yaml::parseFile, which will throw aParseException(a subclass of\Exception) when given the malformed content. The existing test intests/src/Config/ConfigFileReaderTest.phpcorrectly expects an exception for invalid YAML, so no further action is required.
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation