test: add unit tests for Separate enum and DocBlockHeader generator #2
Conversation
WalkthroughThree new PHPUnit test classes were introduced, each targeting a different component: the Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as Test Suite
participant Enum as Separate Enum
participant Generator as DocBlockHeader
participant Fixer as DocBlockHeaderFixer
Tester->>Enum: Test enum cases and methods
Tester->>Generator: Test creation, validation, and array conversion
Tester->>Fixer: Test DocBlock building, insertion, merging, and config behavior
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/src/Enum/SeparateTest.php (3)
40-41: Consider removing the PHPStan suppressionThe
@phpstan-ignore-next-linecomment on line 40 suppresses a legitimate static analysis warning. Since PHPStan can infer the count from the enum definition, this test might be redundant or could be restructured to avoid the suppression.Consider either removing this assertion (as PHPStan already validates it at static analysis time) or documenting why runtime validation is necessary despite static guarantees.
48-58: Excessive PHPStan suppressions indicate potential over-testingEvery assertion in this test requires a PHPStan suppression because the static analyzer already knows these values from the enum definition. Consider whether these runtime assertions provide value beyond what static analysis already guarantees.
If runtime validation is necessary (e.g., for regression testing), consider consolidating the suppressions:
public function testEnumValues(): void { - /* @phpstan-ignore-next-line staticMethod.alreadyNarrowedType */ - self::assertSame('top', Separate::Top->value); - /* @phpstan-ignore-next-line staticMethod.alreadyNarrowedType */ - self::assertSame('bottom', Separate::Bottom->value); - /* @phpstan-ignore-next-line staticMethod.alreadyNarrowedType */ - self::assertSame('both', Separate::Both->value); - /* @phpstan-ignore-next-line staticMethod.alreadyNarrowedType */ - self::assertSame('none', Separate::None->value); + // PHPStan knows these values, but we test them at runtime for regression protection + /** @phpstan-ignore-next-line */ + $values = [ + Separate::Top->value => 'top', + Separate::Bottom->value => 'bottom', + Separate::Both->value => 'both', + Separate::None->value => 'none', + ]; + + foreach ($values as $actual => $expected) { + self::assertSame($expected, $actual); + } }
67-82: Redundant type assertions with PHPStan suppressionsThe return type of
getList()is already declared asnon-empty-list<string>which guarantees:
- The array is not empty
- Keys are integers (by definition of list)
- Values are strings
These PHPStan suppressions indicate redundant runtime checks.
Consider focusing on behavior rather than type checking:
public function testGetListReturnsNonEmptyList(): void { $list = Separate::getList(); - /* @phpstan-ignore-next-line staticMethod.alreadyNarrowedType */ - self::assertNotEmpty($list); - /* @phpstan-ignore-next-line staticMethod.alreadyNarrowedType */ - self::assertIsArray($list); - - foreach ($list as $key => $value) { - /* @phpstan-ignore-next-line staticMethod.alreadyNarrowedType */ - self::assertIsInt($key); - /* @phpstan-ignore-next-line staticMethod.alreadyNarrowedType */ - self::assertIsString($value); - } + // Verify it returns a proper list (sequential integer keys starting from 0) + self::assertSame(array_values($list), $list); + self::assertCount(4, $list); }tests/src/Rules/DocBlockHeaderFixerTest.php (1)
424-425: Imprecise assertions for separation testingUsing
assertGreaterThanOrEqualwith newline counts is imprecise for testing separation behavior. Consider checking for specific whitespace patterns or exact formatting.Instead of counting newlines, consider checking for specific patterns:
- self::assertGreaterThanOrEqual(3, substr_count($result, "\n")); + // For 'top' separation, expect a newline before the DocBlock + self::assertMatchesRegularExpression('/\n\s*\/\*\*/', $result);Or verify the exact structure:
- self::assertGreaterThanOrEqual(3, substr_count($result, "\n")); + $expected = "<?php \n/**\n * @author John Doe\n */\nclass Foo {}"; + self::assertSame($expected, $result);Also applies to: 441-442, 458-459
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/src/Enum/SeparateTest.php(1 hunks)tests/src/Generators/DocBlockHeaderTest.php(1 hunks)tests/src/Rules/DocBlockHeaderFixerTest.php(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/src/Enum/SeparateTest.php (1)
src/Enum/Separate.php (2)
getList(36-39)Separate(26-40)
tests/src/Generators/DocBlockHeaderTest.php (2)
src/Generators/DocBlockHeader.php (2)
DocBlockHeader(31-100)create(43-51)tests/src/Enum/SeparateTest.php (1)
PHPUnit(33-119)
🔇 Additional comments (13)
tests/src/Rules/DocBlockHeaderFixerTest.php (1)
88-121: Good fix for DocBlock formattingRemoving the trailing newlines from the expected DocBlock strings is correct. DocBlocks should end with
*/without additional newlines.tests/src/Generators/DocBlockHeaderTest.php (12)
1-38: LGTM! Well-structured test class with proper header and imports.The file header, namespace, imports, and class declaration are all correctly implemented. The
@internalannotation and#[CoversClass]attribute are appropriate for a test class.
39-45: LGTM! Proper interface implementation test.The test correctly verifies that
DocBlockHeaderimplements theGeneratorinterface using a valid instance creation.
47-55: LGTM! Comprehensive test for basic creation scenario.The test properly validates object creation with valid annotations and verifies all default parameter values are correctly set.
57-69: LGTM! Good test coverage for custom parameters.The test properly validates that custom parameter values (non-defaults) are correctly stored in the created instance.
71-80: LGTM! Important test for array-valued annotations.The test correctly validates that
DocBlockHeadersupports both string and array values for annotations, which is a key feature of the class.
82-98: LGTM! Thorough test for configuration serialization.The test properly validates the
__toArray()method output structure, including the correct configuration key format and enum value serialization.
100-116: LGTM! Good complementary test for default parameter serialization.The test ensures that
__toArray()correctly handles default parameter values, providing comprehensive coverage alongside the custom parameters test.
118-158: LGTM! Comprehensive validation test for all allowed annotations.The test thoroughly validates that all allowed annotation keys from the source code are properly accepted. The annotation list matches the implementation exactly.
160-207: LGTM! Excellent validation error handling tests.The tests comprehensively cover all validation failure scenarios with proper exception expectations and error messages. The PHPStan ignore comment on line 165 is appropriate for intentionally testing invalid input types.
209-233: LGTM! Well-designed edge case tests for annotation key validation.The tests properly distinguish between syntactic validation (regex pattern) and semantic validation (allowed annotations list), covering important edge cases with hyphens, underscores, and numbers in keys.
235-254: LGTM! Good edge case coverage and honest assessment of readonly testing limitations.The empty annotations test provides valuable edge case coverage. The readonly properties test, while limited by language-level enforcement, still provides useful validation of property values.
256-270: LGTM! Important architectural validation using reflection.The reflection-based tests properly validate critical design constraints: private constructor (enforcing factory pattern) and final class (preventing inheritance).
Summary by CodeRabbit