refactor: clean up tests by removing redundant cases and improving validation logic#61
refactor: clean up tests by removing redundant cases and improving validation logic#61konradmichalik merged 4 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update significantly reduces and refactors test coverage across several test suites. Many detailed and edge-case tests were removed from Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as Test Suite
participant Collector as Collector
participant Validator as Validator
participant CLI as CLI Renderer
Tester->>Collector: Test core file collection (recursive, exclusion)
Tester->>Validator: Test naming conventions (parameterized)
Tester->>CLI: Test rendering with/without issues (basic cases)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
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 (
|
… notation directly in the configurable conventions method
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/src/FileDetector/CollectorTest.php (1)
213-228: Hard-coded expectation may be brittleThe test expects exactly 4 debug log calls, which appears to correspond to the number of parser classes being checked. This hard-coded value could break if parser types are added or removed in the future.
Consider making the test more maintainable by either:
- Adding a comment explaining why exactly 4 calls are expected
- Making the expectation more flexible or based on the actual number of parser classes
$logger = $this->createMock(LoggerInterface::class); + // Expecting 4 calls: one for each parser type (Xliff, Yaml, Json, PHP) $logger->expects($this->exactly(4)) ->method('debug') ->with($this->stringContains('No files found for parser class'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/src/FileDetector/CollectorTest.php(1 hunks)tests/src/Result/ValidationResultCliRendererTest.php(5 hunks)tests/src/Result/ValidationRunTest.php(0 hunks)tests/src/Validator/KeyNamingConventionValidatorTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- tests/src/Result/ValidationRunTest.php
🔇 Additional comments (11)
tests/src/Result/ValidationResultCliRendererTest.php (5)
240-241: LGTM!The addition of the assertion for "Parsers cached: 0" appropriately tests the new statistics output.
243-257: LGTM!The test correctly verifies that statistics are not displayed in compact mode (normal verbosity).
288-308: LGTM!Good test coverage for warning handling in compact mode. The test correctly verifies that warnings result in exit code 0 and display the appropriate message.
310-331: LGTM!Good test coverage for error handling in compact mode. The test correctly verifies exit code 1 and appropriate error messaging.
353-372: LGTM!The test properly verifies warning display in verbose mode, including the [WARNING] tag.
tests/src/Validator/KeyNamingConventionValidatorTest.php (6)
707-729: LGTM!Well-structured parameterized test with clear data provider covering important edge cases for snake_case validation.
731-750: LGTM!Excellent data provider with clear, descriptive test case names and comprehensive edge case coverage.
752-771: LGTM!Well-implemented test for mixed convention detection with proper handling of optional assertions.
773-800: LGTM!Comprehensive data provider with excellent coverage of mixed convention scenarios.
802-825: LGTM!Thorough test implementation for dot notation handling across different naming conventions, with proper suggestion verification.
827-857: LGTM!Excellent data provider with comprehensive coverage of dot notation scenarios across different naming conventions.
| public function testCollectFilesRecursive(): void | ||
| { | ||
| // Create nested directory structure | ||
| mkdir($this->tempDir.'/level1', 0777, true); | ||
| mkdir($this->tempDir.'/level1/level2', 0777, true); | ||
|
|
||
| file_put_contents($this->tempDir.'/root.xlf', 'root content'); | ||
| file_put_contents($this->tempDir.'/level1/nested.xlf', 'nested content'); | ||
| file_put_contents($this->tempDir.'/level1/level2/deep.xlf', 'deep content'); | ||
|
|
||
| // Debug: verify files exist | ||
| $this->assertFileExists($this->tempDir.'/root.xlf', 'root.xlf should exist'); | ||
| $this->assertFileExists($this->tempDir.'/level1/nested.xlf', 'nested.xlf should exist'); | ||
| $this->assertFileExists($this->tempDir.'/level1/level2/deep.xlf', 'deep.xlf should exist'); | ||
|
|
||
| // Debug: test RecursiveDirectoryIterator directly | ||
| $iterator = new RecursiveIteratorIterator( | ||
| new RecursiveDirectoryIterator($this->tempDir, RecursiveDirectoryIterator::SKIP_DOTS), | ||
| RecursiveIteratorIterator::LEAVES_ONLY, | ||
| ); | ||
| $foundFiles = []; | ||
| foreach ($iterator as $file) { | ||
| $foundFiles[] = $file->getPathname(); | ||
| } | ||
| $this->assertNotEmpty($foundFiles, 'RecursiveDirectoryIterator should find files'); | ||
| $this->assertContains($this->tempDir.'/root.xlf', $foundFiles, 'Should find root.xlf'); | ||
| $this->assertContains($this->tempDir.'/level1/nested.xlf', $foundFiles, 'Should find nested.xlf'); | ||
|
|
||
| $logger = $this->createMock(LoggerInterface::class); | ||
| $detector = $this->createMock(DetectorInterface::class); | ||
| $detector->method('mapTranslationSet')->willReturn(['recursive_mapped_data']); | ||
| $detector->method('mapTranslationSet')->willReturn(['recursive_data']); | ||
|
|
||
| $collector = new Collector($logger); | ||
|
|
||
| // Test with recursive = true | ||
| $result = $collector->collectFiles([$this->tempDir], $detector, null, true); | ||
|
|
||
| $this->assertArrayHasKey(XliffParser::class, $result); | ||
| $this->assertArrayHasKey($this->tempDir, $result[XliffParser::class]); | ||
| $this->assertEquals(['recursive_mapped_data'], $result[XliffParser::class][$this->tempDir]); | ||
| } | ||
|
|
||
| public function testCollectFilesNonRecursiveOnlyFindsRootFiles(): void | ||
| { | ||
| // Create nested directory structure | ||
| mkdir($this->tempDir.'/level1', 0777, true); | ||
|
|
||
| file_put_contents($this->tempDir.'/root.xlf', 'root content'); | ||
| file_put_contents($this->tempDir.'/level1/nested.xlf', 'nested content'); | ||
|
|
||
| $logger = $this->createMock(LoggerInterface::class); | ||
| $detector = $this->createMock(DetectorInterface::class); | ||
| $detector->method('mapTranslationSet')->willReturn(['non_recursive_mapped_data']); | ||
|
|
||
| $collector = new Collector($logger); | ||
|
|
||
| // Test with recursive = false (default) | ||
| $result = $collector->collectFiles([$this->tempDir], $detector, null, false); | ||
|
|
||
| $this->assertArrayHasKey(XliffParser::class, $result); | ||
| $this->assertArrayHasKey($this->tempDir, $result[XliffParser::class]); | ||
| $this->assertEquals(['non_recursive_mapped_data'], $result[XliffParser::class][$this->tempDir]); | ||
| $this->assertEquals(['recursive_data'], $result[XliffParser::class][$this->tempDir]); | ||
| } |
There was a problem hiding this comment.
Test may not adequately verify recursive file collection behavior
The test has been simplified to only verify that the mocked data is returned, but it doesn't actually verify that files from both the root and nested directories are collected. The test creates files at different levels but doesn't assert that both files were found during recursive collection.
Consider adding assertions to verify that files from multiple directory levels are actually collected when recursive mode is enabled.
🤖 Prompt for AI Agents
In tests/src/FileDetector/CollectorTest.php around lines 152 to 167, the test
does not verify that files from both root and nested directories are collected
recursively. To fix this, add assertions that check the presence of both
'root.xlf' and 'level1/nested.xlf' files in the collected results, ensuring the
recursive collection behavior is properly tested.
Automatically generated tests are all well and good, but this has gotten a bit out of hand here.
Summary by CodeRabbit