chore: improve test coverage and standardize style of unit tests in ORM\Query\AST\Functions namespace
#405
Conversation
martin-georgiev
changed the title
ORM\Functions namespace ORM\Query\AST\Functions namespace
WalkthroughThis change refactors and standardizes exception-related unit test methods for various Doctrine ORM query AST functions. Many test methods are renamed for clarity and descriptiveness, while some are consolidated into parameterized tests using data providers to centralize and streamline invalid argument count checks. Test logic and exception expectations remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant DataProvider
participant TestMethod
TestRunner->>DataProvider: Fetch test cases (DQL, expected message)
loop For each test case
TestRunner->>TestMethod: Run test with DQL, expected message
TestMethod->>TestMethod: Execute query, expect exception
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (12)📓 Common learnings📚 Learning: when implementing node mapping patterns in basevariadicfunction subclasses, if all parameters should...Applied to files:
📚 Learning: in sql generation code for the postgresql-for-doctrine library, it's better to fail explicitly with ...Applied to files:
📚 Learning: tests in the `tests\unit\martingeorgiev\doctrine\orm\query\ast\functions` namespace extend a custom ...Applied to files:
📚 Learning: tests in the `tests\martingeorgiev\doctrine\orm\query\ast\functions` namespace extend a custom `test...Applied to files:
📚 Learning: tests in the `tests\martingeorgiev\doctrine\orm\query\ast\functions` namespace extend a custom `test...Applied to files:
📚 Learning: in the postgresql-for-doctrine repository, postgresql range functions have distinct implementations ...Applied to files:
📚 Learning: in the postgresql for doctrine test cases, methods that test database-to-php conversions should use ...Applied to files:
📚 Learning: this project focuses on providing doctrine orm interfaces to postgresql functions. tests should vali...Applied to files:
📚 Learning: when reviewing postgresql formatting function tests, the error handling behavior should match postgr...Applied to files:
📚 Learning: this postgresql-for-doctrine project is a translation layer only, focusing on correctly converting d...Applied to files:
📚 Learning: the requiressqlcommenthint() method is intentionally kept in tests to support backwards compatibilit...Applied to files:
🧬 Code Graph Analysis (1)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtractTest.php (4)
⏰ 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). (20)
🔇 Additional comments (16)
✨ 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
Documentation and Community
|
![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 (15)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpLikeTest.php (1)
53-55: Consider collapsing both “too few / too many” checks into a single data-provider test
Many other function tests now use one parameterised method fed by a data-provider to exercise invalid-argument scenarios, reducing duplication and streamlining future changes. This file still keeps two nearly identical test bodies. Migrating to a data-provider here would align with the newer pattern and cut a few lines of boilerplate.tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTsvectorTest.php (1)
45-52: Consider upgrading this single-case method to a data-provider test for suite-wide consistencyAcross this PR most “invalid argument count” checks were consolidated into parameterized tests. Keeping a one-off method here breaks the emerging pattern and slightly increases boilerplate.
Migrating this assertion to a data provider (invalidArgumentCountProvider) would standardize structure and make adding more edge-cases trivial.tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/JsonbPathQueryFirstTest.php (1)
58-66: Minor duplication – consider a small DRY refactor
throws_exception_for_too_few_arguments()mirrors the same 3-line “arrange/act” block found in the other two negative-path tests. Extracting that snippet into a private helper (e.g.assertSqlGenerationThrows(string $dql): void) would shave a few lines and keep future tests tidy.tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpMatchTest.php (1)
54-61: Optional: collapse the two “argument-count” tests into a single data-provider testOther function test classes in this PR consolidated the “too few / too many” cases via a data provider. Doing the same here would remove duplication while keeping coverage identical.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/JsonbPathExistsTest.php (1)
68-76: Minor duplication remains – consider a data-provider refactorAll three negative-case tests differ only by input/expected message. A small data-provider would eliminate repetition and make future edge-case additions cheaper:
+#[DataProvider('invalidCalls')] +public function throws_exception_for_invalid_calls( + string $dql, + string $expectedMessage, +): void { + $this->expectException(InvalidArgumentForVariadicFunctionException::class); + $this->expectExceptionMessage($expectedMessage); + $this->buildEntityManager()->createQuery($dql)->getSQL(); +} + +public static function invalidCalls(): array +{ + $class = ContainsJsons::class; + return [ + 'too few' => [ + \sprintf('SELECT JSONB_PATH_EXISTS(e.object1) FROM %s e', $class), + 'jsonb_path_exists() requires at least 2 arguments', + ], + 'too many' => [ + \sprintf("SELECT JSONB_PATH_EXISTS(e.object1, '$.items[*].id', '{\"strict\": false}', 'true', 'extra') FROM %s e", $class), + 'jsonb_path_exists() requires between 2 and 4 arguments', + ], + ]; +}Not mandatory, but it would align this class with other newly-parameterised tests in the suite.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAddTest.php (2)
48-56: Assertion on verbose exception message is brittleThe test already verifies the concrete exception class; additionally asserting the full exception message ties the test to an exact wording that may legitimately change in the future without altering behaviour.
Unless the message itself is part of the public contract, consider droppingexpectExceptionMessage()here (and in the two following tests) to keep the test resilient to minor wording tweaks.
58-76: Combine the two argument-count tests into a single data-provider caseAcross the PR you switched many too-few / too-many arguments checks to a parameterised test + data provider. Keeping
throws_exception_for_too_few_arguments()andthrows_exception_for_too_many_arguments()as separate methods breaks that freshly introduced pattern.Suggestion: merge them into one
throws_exception_for_invalid_argument_countmethod fed by a provider like:public static function invalidArgumentCountCases(): iterable { yield 'too few' => ['SELECT DATE_ADD(e.datetimetz1) FROM %s e']; yield 'too many' => ["SELECT DATE_ADD(e.datetimetz1, '1 day', 'Europe/Sofia', 'extra') FROM %s e"]; } #[Test] #[DataProvider('invalidArgumentCountCases')] public function throws_exception_for_invalid_argument_count(string $dqlTemplate): void { $this->expectException(InvalidArgumentForVariadicFunctionException::class); $this->buildEntityManager()->createQuery(sprintf($dqlTemplate, ContainsDates::class))->getSQL(); }This keeps the suite consistent with the rest of the refactor and removes duplication.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RowToJsonTest.php (1)
56-64: Consistent naming; optional consolidation via data providerThe new
throws_exception_for_too_many_argumentsname aligns with the updated style.
If you plan additional cleanup, consider grouping “invalid argument count” cases for variadic functions into a single parameterised test (data provider) to reduce duplication across similar test classes.tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/JsonbPathQueryTest.php (1)
58-76: Consider folding the “too-few / too-many arguments” checks into a data-provider.Both tests differ only in:
- the DQL statement
- the expected exception message
A tiny data-provider would eliminate duplication and keep future maintenance (e.g. message wording tweaks) in one place.
Example sketch:
/** * @return iterable<string,array{string,string}> */ public static function invalidArgumentCountProvider(): iterable { yield 'too few' => ['SELECT JSONB_PATH_QUERY(e.object1) FROM %s e', 'requires at least 2 arguments']; yield 'too many' => ["SELECT JSONB_PATH_QUERY(e.object1, '$.items[*].id', '{\"strict\": false}', 'true', 'extra') FROM %s e", 'requires between 2 and 4 arguments']; } #[Test] #[DataProvider('invalidArgumentCountProvider')] public function throws_exception_for_invalid_argument_count(string $dqlPattern, string $message): void { $this->expectException(InvalidArgumentForVariadicFunctionException::class); $this->expectExceptionMessage($message); $dql = \sprintf($dqlPattern, ContainsJsons::class); $this->buildEntityManager()->createQuery($dql)->getSQL(); }Not mandatory, yet it keeps the test file concise and consistent with other refactors in the PR.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/JsonbPathQueryArrayTest.php (1)
48-76: Optionally consolidate the three exception checks via a data providerThe three methods differ only by:
- Expected exception message.
- DQL string.
You could eliminate duplication and tighten maintenance by parameterising them:
#[DataProvider('invalidArguments')] public function test_exceptions(string $dql, string $expectedMessage): void { $this->expectException(InvalidArgumentForVariadicFunctionException::class); $this->expectExceptionMessage($expectedMessage); $this->buildEntityManager()->createQuery($dql)->getSQL(); } public static function invalidArguments(): array { return [ 'too few' => [ \sprintf('SELECT JSONB_PATH_QUERY_ARRAY(e.object1) FROM %s e', ContainsJsons::class), 'jsonb_path_query_array() requires at least 2 arguments', ], 'too many' => [ \sprintf("SELECT JSONB_PATH_QUERY_ARRAY(e.object1, '$.items[*].id', '{\"strict\": false}', 'true', 'extra_arg') FROM %s e", ContainsJsons::class), 'jsonb_path_query_array() requires between 2 and 4 arguments', ], // keep the boolean-specific check in its own method because it throws a different exception class ]; }This mirrors the consolidation approach already introduced in other updated tests within the PR, reduces boilerplate, and keeps future changes (e.g., message wording) centralised.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/JsonbInsertTest.php (3)
38-46: PreferexpectExceptionMessageMatches()for future-proofingThe assertion locks the test to the entire exception string.
Any cosmetic tweak to the wording inInvalidBooleanException(capitalisation, punctuation, etc.) will break the test even though behaviour is unchanged.
Using a regex expectation is less brittle while still validating the essential part:- $this->expectExceptionMessage('Invalid boolean value "invalid" provided for jsonb_insert. Must be "true" or "false".'); + $this->expectExceptionMessageMatches( + '/Invalid boolean value "invalid" provided .*jsonb_insert/i' + );
48-56: Consolidate argument-count checks via a data providerBoth too few and too many argument cases exercise identical mechanics (invalid arity ➜ same exception).
Merging them into a single parameterised test eliminates duplication and keeps new edge-cases in one place:#[Test] #[TestWith(['SELECT JSONB_INSERT(e.object1)'])] #[TestWith(["SELECT JSONB_INSERT(e.object1, '{country}', '{\"iso\"}', 'true', 'extra')"])] public function throws_exception_for_invalid_argument_count(string $dql): void { $this->expectException(InvalidArgumentForVariadicFunctionException::class); $this->buildEntityManager()->createQuery( \sprintf($dql, ContainsJsons::class) )->getSQL(); }
58-66: Minor: keep DQL fixtures consistentElsewhere in the file you use uppercase
"BGR"for the ISO code; here it is lowercase"bgr".
Uniform fixtures make diffing easier when failures occur.No action required if intentional.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/JsonbSetTest.php (1)
44-72: Consider folding the three exception-path tests into a data-provider for consistencyElsewhere in this PR several function test classes (“ArrayPositionTest”, “DateSubtractTest”, “RegexpCountTest”, …) were migrated to parameterised tests that consume a data provider to cover the “too few / too many / invalid value” branches.
Replicating that pattern here would:
- Remove method-level duplication (the arrange/act logic is identical).
- Keep the naming style consistent across the
Functionstest suite.- Make it trivial to add additional invalid-input cases later.
Not blocking, but worth applying while you’re already standardising the suite.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/JsonbPathMatchTest.php (1)
59-76: DRY up the two “argument-count” tests with a data provider
Both methods differ only in the DQL string and the expected message. A single parameterised test keeps intent clear while eliminating duplication.- #[Test] - public function throws_exception_for_too_few_arguments(): void - { - $this->expectException(InvalidArgumentForVariadicFunctionException::class); - $this->expectExceptionMessage('jsonb_path_match() requires at least 2 arguments'); - - $dql = \sprintf('SELECT JSONB_PATH_MATCH(e.object1) FROM %s e', ContainsJsons::class); - $this->buildEntityManager()->createQuery($dql)->getSQL(); - } - - #[Test] - public function throws_exception_for_too_many_arguments(): void - { - $this->expectException(InvalidArgumentForVariadicFunctionException::class); - $this->expectExceptionMessage('jsonb_path_match() requires between 2 and 4 arguments'); - - $dql = \sprintf("SELECT JSONB_PATH_MATCH(e.object1, '$.items[*].id', '{\"strict\": false}', 'true', 'extra_arg') FROM %s e", ContainsJsons::class); - $this->buildEntityManager()->createQuery($dql)->getSQL(); - } + #[Test] + #[DataProvider('invalidArgumentCountProvider')] + public function throws_exception_for_invalid_argument_count(string $dql, string $expectedMessage): void + { + $this->expectException(InvalidArgumentForVariadicFunctionException::class); + $this->expectExceptionMessage($expectedMessage); + + $this->buildEntityManager()->createQuery($dql)->getSQL(); + }#[DataProvider('invalidArgumentCountProvider')] public static function invalidArgumentCountProvider(): array { return [ 'too few' => [ \sprintf('SELECT JSONB_PATH_MATCH(e.object1) FROM %s e', ContainsJsons::class), 'jsonb_path_match() requires at least 2 arguments', ], 'too many' => [ \sprintf("SELECT JSONB_PATH_MATCH(e.object1, '$.items[*].id', '{\"strict\": false}', 'true', 'extra_arg') FROM %s e", ContainsJsons::class), 'jsonb_path_match() requires between 2 and 4 arguments', ], ]; }(Suggest also adding
use PHPUnit\Framework\Attributes\DataProvider;at the top.)
Summary by CodeRabbit
Refactor
Tests