chore: improve test coverage in unit tests #384
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Walkthrough
This update enhances the test coverage for array and variadic function handling in the Doctrine DBAL and ORM layers. It introduces new tests for edge cases and error handling, adjusts exception expectations to use more specific custom exceptions, and adds data providers for invalid input scenarios. It also refactors some tests to use data providers for parameterized testing and improves regex consistency in coordinate validation.
## Changes
| File(s) | Change Summary |
|---------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.php | Updated a test to expect the custom `InvalidFloatArrayItemForPHPException` instead of Doctrine's `ConversionException`. Adjusted imports accordingly. |
| tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/PointArrayTest.php | Added three new test methods and a data provider to increase coverage of `PointArray` transformation methods, focusing on handling of `null` and invalid input values, and appropriate exception throwing. |
| tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunctionTestCase.php | Added two tests for argument count validation in variadic functions, using reflection to invoke protected methods and verifying that custom exceptions are thrown for too few or too many arguments. |
| tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/BaseNetworkTypeArrayTest.php | Refactored tests to use PHPUnit data providers for parameterized testing of valid and invalid value transformations. Consolidated multiple tests into parameterized tests for converting to PHP values and for exception throwing on invalid inputs. Renamed some test methods for clarity. |
| src/MartinGeorgiev/Doctrine/DBAL/Types/ValueObject/Point.php | Extracted coordinate regex pattern into a reusable constant `COORDINATE_PATTERN` and updated `POINT_REGEX` accordingly. Updated coordinate validation to use the new pattern constant. |
| tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/PointTest.php | Added tests to verify that coordinates with exactly six decimal places are accepted and that coordinates with more than six decimal places throw an exception. |
| tests/Unit/MartinGeorgiev/Utils/PHPArrayToPostgresValueTransformerTest.php | Extended data provider with cases for arrays of nulls, booleans, and empty strings. Added a test verifying transformation of arrays containing GD image resources, skipping if GD extension is unavailable. |
| tests/Unit/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.php | Added new test cases to the data provider to cover edge cases in Postgres array string inputs, including whitespace-only input, trailing commas, and arrays with special characters or empty strings. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant TestCase
participant PointArray
participant Exception
TestCase->>PointArray: transformArrayItemForPHP(null)
PointArray-->>TestCase: return null
TestCase->>PointArray: transformArrayItemForPHP(invalid)
PointArray-->>Exception: throw InvalidPointArrayItemForPHPException
TestCase->>PointArray: transformPostgresArrayToPHPArray(invalid)
PointArray-->>TestCase: return []sequenceDiagram
participant TestCase
participant BaseVariadicFunction
participant Exception
TestCase->>BaseVariadicFunction: validateArguments(tooFewArgs)
BaseVariadicFunction-->>Exception: throw InvalidArgumentForVariadicFunctionException
TestCase->>BaseVariadicFunction: validateArguments(tooManyArgs)
BaseVariadicFunction-->>Exception: throw InvalidArgumentForVariadicFunctionException
Possibly related PRs
Poem
|
![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 (1)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunctionTestCase.php (1)
44-75: Excellent test coverage for minimum argument validation with a minor comment issue.The test correctly validates that the function throws an exception when too few arguments are provided. The anonymous class setup and reflection usage are appropriate for testing protected methods.
However, the comment on line 74 is misleading - it states "No arguments" but actually passes one argument (
$node).Update the comment to accurately reflect the test scenario:
- $reflectionMethod->invoke($function, $node); // No arguments + $reflectionMethod->invoke($function, $node); // 1 argument when minimum is 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.php(2 hunks)tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/PointArrayTest.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunctionTestCase.php(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunctionTestCase.php (3)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase, and therefore don't need an explicit import.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#383
File: tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RadiansTest.php:1-9
Timestamp: 2025-05-23T11:11:57.926Z
Learning: Tests in the `Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase directly, and therefore don't need an explicit import.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), not PHPUnit's TestCase, and therefore don't need an explicit import statement.
🧬 Code Graph Analysis (1)
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.php (1)
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatArrayItemForPHPException.php (1)
InvalidFloatArrayItemForPHPException(14-35)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: PostgreSQL 16 + PHP 8.3
- GitHub Check: PostgreSQL 17 + PHP 8.3
- GitHub Check: PostgreSQL 17 + PHP 8.2
- GitHub Check: PostgreSQL 17 + PHP 8.4
- GitHub Check: PostgreSQL 16 + PHP 8.4
- GitHub Check: PostgreSQL 17 + PHP 8.1
- GitHub Check: PostgreSQL 16 + PHP 8.2
- GitHub Check: PostgreSQL 16 + PHP 8.1
- GitHub Check: sloth
- GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM latest + Doctrine Lexer 3.0
🔇 Additional comments (7)
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.php (1)
8-8: Excellent improvement in exception specificity!Replacing the generic
ConversionExceptionwith the more specificInvalidFloatArrayItemForPHPExceptionprovides better error context and aligns with domain-driven exception handling practices. This change enhances test precision and makes error scenarios more explicit.Also applies to: 67-67
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunctionTestCase.php (2)
10-10: Good addition of necessary imports for new test functionality.The imports for
NodeandInvalidArgumentForVariadicFunctionExceptionare correctly added to support the new argument validation tests.Also applies to: 13-13
77-108: Well-implemented test for maximum argument validation.This test effectively verifies that the function throws an exception when too many arguments are provided. The anonymous class configuration and reflection approach are consistent with the previous test and appropriate for testing protected functionality.
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/PointArrayTest.php (4)
195-199: Good edge case coverage for null input handling.This test ensures that the
transformArrayItemForPHPmethod correctly handles null input, which is an important edge case for robust type transformation.
201-207: Excellent exception testing with comprehensive data provider coverage.This test effectively validates that invalid input types properly trigger the expected exception, using the data provider to ensure multiple invalid scenarios are covered.
209-217: Well-justified use of reflection for private method testing.Testing the private
transformPostgresArrayToPHPArraymethod provides valuable coverage of fallback behavior for invalid inputs. The reflection approach is appropriate here to ensure comprehensive error handling validation.
219-226: Comprehensive data provider for invalid input scenarios.The data provider covers a good range of invalid input types (integer, non-point string, malformed array) that effectively test the error handling paths in the transformation methods.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/PointTest.php (1)
134-139: Consider using a custom exception and improve test coverage.While the test logic is correct, there are a few improvements to consider:
Exception consistency: The test uses the generic
\InvalidArgumentException, but the existing codebase uses custom exceptions likeInvalidPointForPHPException. Consider using a more specific custom exception if one exists for coordinate validation.Test comprehensiveness: The test only validates the X coordinate with excessive precision. Consider testing both coordinates or using a data provider for more comprehensive coverage.
Here's an improved version using a data provider for better coverage:
+ #[DataProvider('provideCoordinatesWithExcessivePrecision')] + #[Test] + public function throws_for_coordinate_with_more_than_6_decimal_places(float $x, float $y): void + { + $this->expectException(\InvalidArgumentException::class); + new PointValueObject($x, $y); + } + + /** + * @return array<string, array{float, float}> + */ + public static function provideCoordinatesWithExcessivePrecision(): array + { + return [ + 'x coordinate with 7 decimal places' => [1.1234567, 2.0], + 'y coordinate with 7 decimal places' => [1.0, 2.1234567], + 'both coordinates with excessive precision' => [1.1234567, 2.9876543], + ]; + } - #[Test] - public function throws_for_coordinate_with_more_than_6_decimal_places(): void - { - $this->expectException(\InvalidArgumentException::class); - new PointValueObject(1.1234567, 2.0); - }tests/Unit/MartinGeorgiev/Utils/PHPArrayToPostgresValueTransformerTest.php (1)
107-118: Fix typo in test case description.The new test cases provide good coverage for homogeneous arrays. However, there's a typo in the description.
- 'with empty empty strings' => [ + 'with only empty strings' => [
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/MartinGeorgiev/Doctrine/DBAL/Types/ValueObject/Point.php(2 hunks)tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/PointTest.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunctionTestCase.php(2 hunks)tests/Unit/MartinGeorgiev/Utils/PHPArrayToPostgresValueTransformerTest.php(2 hunks)tests/Unit/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/MartinGeorgiev/Doctrine/DBAL/Types/ValueObject/Point.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunctionTestCase.php
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: sloth
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.4 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer 1.2
- GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.2 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer latest
🔇 Additional comments (3)
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/PointTest.php (1)
126-132: LGTM! Good precision validation test.This test correctly verifies that coordinates with exactly 6 decimal places are accepted and properly stored in the PointValueObject.
tests/Unit/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.php (1)
125-144:✅ Verification successful
Good edge case coverage with one questionable test case.
The new test cases enhance coverage for edge scenarios in PostgreSQL array parsing. However, the 'with trailing comma' test case needs verification.
The test case
'with trailing comma'uses input'{a,}}'which appears malformed with an extra closing brace. Please verify this is valid PostgreSQL array syntax and the expected behavior.
🏁 Script executed:
#!/bin/bash # Description: Verify PostgreSQL array syntax for trailing comma with extra brace # Expected: Confirm if '{a,}}' is valid PostgreSQL array syntax echo "Testing PostgreSQL array syntax..." echo "Input: '{a,}}'" echo "Expected output according to test: ['a']" echo "" echo "Please verify this syntax against PostgreSQL documentation or test with actual PostgreSQL instance."Length of output: 440
🏁 Script executed:
#!/bin/bash # Locate and inspect the PostgresArrayToPHPArrayTransformer implementation rg "class PostgresArrayToPHPArrayTransformer" -A 200Length of output: 41413
Approve edge case coverage
All five new tests accurately capture edge scenarios, and the transformer handles the trailing-comma case (
'{a,}}' → ['a']) by design. No further changes needed.tests/Unit/MartinGeorgiev/Utils/PHPArrayToPostgresValueTransformerTest.php (1)
253-264: Excellent resource handling test implementation.The GD resource test is well-implemented with proper conditional skipping, resource creation, transformation testing, and cleanup. This adds valuable coverage for resource type handling.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit