chore: reorganize the integration tests for DBAL\Types
#417
Conversation
martin-georgiev
changed the title
DBAL\TypesDBAL\Types
WalkthroughAdds new PHPUnit integration test infrastructure (TestCase, ScalarTypeTestCase, ArrayTypeTestCase, RangeTypeTestCase) and numerous focused tests for scalar, array, point, inet/macaddr/cidr, JSONB, and range types. Replaces and removes prior monolithic tests (DBALTypesTest, RangeOperatorsTest, RangeTestCase). Updates one utility test with attributes and method renames. Changes
Sequence Diagram(s)sequenceDiagram
participant T as ConcreteTest (e.g., InetTypeTest)
participant B as TestCase (base)
participant DB as Database
participant DT as Doctrine Type
T->>B: runTypeTest(typeName, columnType, value)
B->>DB: CREATE TABLE temp(col columnType)
B->>DB: INSERT INTO temp VALUES (:v) [bind as typeName]
B->>DB: SELECT col FROM temp
B->>DT: Type::getType(typeName)
B->>DT: convertToPHPValue(dbValue, platform)
DT-->>B: phpValue
B->>B: assertTypeValueEquals(expected, phpValue, typeName)
B->>DB: DROP TABLE temp
sequenceDiagram
participant RT as RangeTypeTestCase
participant DB as Database
participant EM as ORM/DQL Executor
RT->>DB: setUp() create containsranges + seed rows
RT->>EM: execute DQL with operator (e.g., CONTAINS)
EM-->>RT: result rows (ids)
RT->>RT: assertIds(expectedIds, result)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
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. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, 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: 7
🧹 Nitpick comments (39)
tests/Integration/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.php (5)
60-66: Extend negative coverage with non-string inputs for DB→PHP conversionPer the project’s testing approach for DB→PHP conversions (see retrieved learnings), consider adding explicit non-string inputs to assert strong type handling (e.g., TypeError or a domain exception) in addition to malformed strings.
Example new tests you can add (outside this hunk):
#[DataProvider('provideNonStringInputs')] #[Test] public function non_string_inputs_throw_exceptions(mixed $input): void { $this->expectException(\Throwable::class); // TypeError or domain exception PostgresArrayToPHPArrayTransformer::transformPostgresArrayToPHPArray($input); // @phpstan-ignore-line } /** * @return array<int, array{0: mixed}> */ public static function provideNonStringInputs(): array { return [ [null], [123], [123.45], [[1, 2, 3]], [true], [new \stdClass()], ]; }
69-79: Loosen provider docblock to allow non-string invalid inputsIf you adopt the non-string negative cases, update the provider’s PHPDoc accordingly.
- * @return array<int, array{0: array{description: string, input: string}}> + * @return array<int, array{0: array{description: string, input: mixed}}>
173-176: Use assertSame for stricter comparisonsThese are arrays of strings; assertSame is a tighter check and avoids loose type equality surprises.
- self::assertEquals( + self::assertSame( $expected, $retrieved, \sprintf('Direct retrieval failed for %s', $description) );
182-185: Use assertSame for the parsed text representation as wellKeep both code paths equally strict.
- self::assertEquals( + self::assertSame( $expected, $parsed, \sprintf('Text representation parsing failed for %s', $description) );
132-133: Re-evaluate the @phpstan-ignore-lineIf the transformer’s return type is already correctly inferred, this suppression may no longer be needed.
Can you confirm whether phpstan reports an issue here without the ignore? If not, please drop the suppression.
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TestCase.php (1)
31-73: Avoid assuming id = 1 for the first rowThe fresh table likely starts at 1, but making the SELECT resilient helps if this code evolves (multiple inserts, different sequence starts). Ordering by id and limiting is safer and keeps the test independent of sequence state.
$queryBuilder ->select($columnName) ->from(self::DATABASE_SCHEMA.'.'.$tableName) - ->where('id = 1'); + ->orderBy('id', 'DESC') + ->setMaxResults(1);Alternatively (PostgreSQL-specific), you could insert with RETURNING and fetch the value directly, but the above keeps it portable within DBAL.
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/JsonbTypeTest.php (2)
42-48: Remove unused $testName to satisfy PHPMD and simplify the APIThe dataset names already convey the case; the parameter isn’t used.
- public function can_handle_json_values(string $testName, array $json): void + public function can_handle_json_values(array $json): void { $typeName = $this->getTypeName(); $columnType = $this->getPostgresTypeName(); $this->runTypeTest($typeName, $columnType, $json); }
51-74: Align provider shape with the method signatureReturn just the JSON payload as the single argument, and update the docblock. Dataset names remain as the descriptive keys.
- /** - * @return array<string, array{string, mixed}> - */ + /** + * @return array<string, array{0: array<string, mixed>}> + */ public static function provideValidTransformations(): array { return [ - 'simple object' => ['simple object', ['foo' => 'bar', 'baz' => 123]], - 'nested structures' => ['nested structures', [ + 'simple object' => [['foo' => 'bar', 'baz' => 123]], + 'nested structures' => [[ 'user' => ['id' => 1, 'name' => 'John'], 'meta' => ['active' => true, 'roles' => ['admin', 'user']], ]], - 'mixed types' => ['mixed types', [ + 'mixed types' => [[ 'string' => 'value', 'number' => 42, 'boolean' => false, 'null' => null, 'array' => [1, 2, 3], 'object' => ['a' => 1], ]], - 'special characters' => ['special characters', [ + 'special characters' => [[ 'message' => 'Hello "World" with \'quotes\'', 'path' => '/path/with/slashes', ]], ]; }Optional follow-up: If you plan to cover non-array JSON payloads (scalars, objects), widen the method parameter to mixed and add cases accordingly.
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/PointArrayTypeTest.php (1)
74-86: Reindex arrays before element-wise compare; add param docsGuard against unexpected associative keys and make the expectations explicit to static analysis.
- /** - * Assert that two point arrays are equal with coordinate precision. - */ + /** + * Assert that two point arrays are equal with coordinate precision. + * + * @param array<int, PointValueObject> $expected + * @param array<int, PointValueObject> $actual + */ protected function assertPointArrayEquals(array $expected, array $actual, string $typeName): void { $this->assertCount(\count($expected), $actual, \sprintf('Point array count mismatch for type %s', $typeName)); - foreach ($expected as $index => $expectedPoint) { - if ($expectedPoint instanceof PointValueObject && $actual[$index] instanceof PointValueObject) { - $this->assertPointEquals($expectedPoint, $actual[$index], $typeName); - } - } + $expectedValues = \array_values($expected); + $actualValues = \array_values($actual); + + foreach ($expectedValues as $index => $expectedPoint) { + $this->assertInstanceOf(PointValueObject::class, $expectedPoint); + $this->assertInstanceOf(PointValueObject::class, $actualValues[$index]); + $this->assertPointEquals($expectedPoint, $actualValues[$index], $typeName); + } }tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/PointAssertionTrait.php (1)
37-46: Add PHPDoc to aid static analysis for isPointArrayMinor annotation to clarify intent and improve tool support.
- protected function isPointArray(array $array): bool + /** + * @param array<int, mixed> $array + */ + protected function isPointArray(array $array): bool { foreach ($array as $item) { if (!$item instanceof PointValueObject) { return false; } } return true; }tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/ScalarTypeTestCase.php (1)
9-10: Make the contract explicit with abstract accessorsDeclaring the required accessors here improves IDE/static analysis and documents expectations for subclasses. If these are already declared in the parent TestCase, ignore this.
Apply this diff:
abstract class ScalarTypeTestCase extends TestCase { + abstract protected function getTypeName(): string; + abstract protected function getPostgresTypeName(): string; + #[Test] public function can_handle_null_values(): voidtests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/CidrTypeTest.php (1)
23-46: Add negative cases for non-string PHP valuesTo cover InvalidCidrForPHPException::forInvalidType and align with the suite’s pattern, add a data provider for non-string inputs (int, float, bool, array, object).
Apply this diff:
public static function provideValidTransformations(): array { return [ 'IPv4 CIDR' => ['192.168.1.0/24'], 'IPv4 CIDR /8' => ['10.0.0.0/8'], 'IPv4 CIDR /16' => ['172.16.0.0/16'], 'IPv6 CIDR' => ['2001:db8::/32'], 'IPv6 CIDR /64' => ['2001:db8::/64'], 'IPv6 CIDR /128' => ['2001:db8::1/128'], ]; } #[Test] public function can_handle_invalid_networks(): void { $this->expectException(InvalidCidrForPHPException::class); $typeName = $this->getTypeName(); $columnType = $this->getPostgresTypeName(); $this->runTypeTest($typeName, $columnType, 'invalid-network'); } + + #[DataProvider('provideInvalidPhpValues')] + #[Test] + public function rejects_non_string_php_values(mixed $value): void + { + $this->expectException(InvalidCidrForPHPException::class); + + $typeName = $this->getTypeName(); + $columnType = $this->getPostgresTypeName(); + + $this->runTypeTest($typeName, $columnType, $value); + } + + /** + * @return array<string, array{mixed}> + */ + public static function provideInvalidPhpValues(): array + { + return [ + 'int' => [123], + 'float' => [1.23], + 'bool' => [true], + 'array' => [['192.168.1.0/24']], + 'object' => [new \stdClass()], + ]; + } }tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/IntegerArrayTypeTest.php (1)
7-9: Import exception and add invalid array item testsAdd a negative test to ensure invalid elements (out-of-range, non-integers) trigger the expected exception, mirroring patterns in Inet/Cidr array tests.
Apply this diff to add the import:
use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; +use MartinGeorgiev\Doctrine\DBAL\Types\Exceptions\InvalidIntegerArrayItemForPHPException;And append these tests and provider before the class closing brace:
public static function provideValidTransformations(): array { return [ 'simple integer array' => ['simple integer array', [1, 2, 3, 4, 5]], 'integer array with negatives' => ['integer array with negatives', [-1, 0, 1, -100, 100]], 'integer array with max values' => ['integer array with max values', [2147483647, -2147483648, 0]], ]; } + + #[DataProvider('provideInvalidArrayValues')] + #[Test] + public function rejects_invalid_array_values(string $testName, array $arrayValue): void + { + $this->expectException(InvalidIntegerArrayItemForPHPException::class); + + $typeName = $this->getTypeName(); + $columnType = $this->getPostgresTypeName(); + + $this->runTypeTest($typeName, $columnType, $arrayValue); + } + + /** + * @return array<string, array{string, array<int, mixed>}> + */ + public static function provideInvalidArrayValues(): array + { + return [ + 'out of range positive' => ['out of range positive', [2147483648]], + 'out of range negative' => ['out of range negative', [-2147483649]], + 'float element' => ['float element', [1.5]], + 'string element' => ['string element', ['123']], + 'mixed types' => ['mixed types', [1, '2']], + ]; + } }tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/BigIntArrayTypeTest.php (1)
35-39: Improve readability with numeric literal separatorsUnderscores make large literals easier to scan without changing semantics.
Apply this diff:
- 'simple bigint array' => ['simple bigint array', [9223372036854775807, 1, -9223372036854775807]], + 'simple bigint array' => ['simple bigint array', [9_223_372_036_854_775_807, 1, -9_223_372_036_854_775_807]], 'bigint array with zeros' => ['bigint array with zeros', [0, 0, 0, 1, 0]], - 'bigint array with large numbers' => ['bigint array with large numbers', [1000000000000, 2000000000000, 3000000000000]], - 'bigint array with negative numbers' => ['bigint array with negative numbers', [-1000000000000, -2000000000000, -3000000000000]], + 'bigint array with large numbers' => ['bigint array with large numbers', [1_000_000_000_000, 2_000_000_000_000, 3_000_000_000_000]], + 'bigint array with negative numbers' => ['bigint array with negative numbers', [-1_000_000_000_000, -2_000_000_000_000, -3_000_000_000_000]], 'bigint array with PHP max and min integer constants' => ['bigint array with PHP max and min integer constants', [PHP_INT_MAX, PHP_INT_MIN, 0]],tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeAssertionTrait.php (1)
23-35: Use strict assertions for strings and booleansassertSame provides stricter checks for string and boolean equality, avoiding unintended type juggling in assertions.
Apply this diff:
- $this->assertEquals( + $this->assertSame( $expected->__toString(), $actual->__toString(), 'Range string representation mismatch for type '.$typeName ); - $this->assertEquals( + $this->assertSame( $expected->isEmpty(), $actual->isEmpty(), 'Range empty state mismatch for type '.$typeName );tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/SmallIntArrayTypeTest.php (1)
29-41: Extend tests for null elements, rename “max” to “boundary”, and add out-of-range negative casePlease update tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/SmallIntArrayTypeTest.php as follows:
- Change the docblock on provideValidTransformations() to allow
nullin the value array.- Rename the “max values” case to “boundary values” for clarity.
- Add a case covering a
nullelement in a smallint array.- Introduce a new test method to assert that out-of-range values throw the correct exception (
InvalidIntegerArrayItemForPHPException).Example diff for the data provider:
@@ - /** - * @return array<string, array{string, array<int, int>}> - */ + /** + * @return array<string, array{string, array<int, int|null>}> + */ @@ - 'smallint array with max values' => ['smallint array with max values', [-32768, 0, 32767]], + 'smallint array with boundary values' => ['smallint array with boundary values', [-32768, 0, 32767]], 'smallint array with zeros' => ['smallint array with zeros', [0, 0, 0]], + 'smallint array with null elements' => ['smallint array with null elements', [1, null, 3]], 'empty smallint array' => ['empty smallint array', []],And add a new test to cover out-of-range values, for example:
public function testCannotHandleOutOfRangeValues(): void { $this->expectException(InvalidIntegerArrayItemForPHPException::class); // value 32768 is just outside the smallint range $this->getType(SmallIntArrayType::class) ->convertToPHPValue('{1,32768,3}', $this->getPlatform()); }This will ensure both valid
nullsupport and correct handling of boundary violations. [optional_refactors_recommended]tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/MacaddrArrayTypeTest.php (3)
38-45: “mixed case” scenario currently doesn’t exercise case variance.Both items are lowercase. Adjust to actually verify mixed/uppercase handling.
Apply this diff:
- 'macaddr array with mixed case' => ['macaddr array with mixed case', [ - '08:00:2b:01:02:03', - '00:0c:29:aa:bb:cc', - ]], + 'macaddr array with mixed case' => ['macaddr array with mixed case', [ + '08:00:2b:01:02:03', + '00:0C:29:AA:BB:CC', + ]],
35-47: Optionally add hyphen-separated MACs to validate normalization.Unit tests often cover alternative separators; adding a hyphen-separated case improves end-to-end coverage.
Apply this diff to extend the provider:
return [ @@ 'empty macaddr array' => ['empty macaddr array', []], + 'macaddr array with hyphen separators' => ['macaddr array with hyphen separators', [ + '08-00-2b-01-02-03', + '00-0C-29-AA-BB-CC', + ]], ];
50-59: Add a negative test for non-string items.Based on the retrieved learnings, we should cover non-string items in DB-to-PHP conversions. Ensures
forInvalidTypepaths are exercised.Apply this diff to add the test:
#[Test] public function can_handle_invalid_addresses(): void { $this->expectException(InvalidMacaddrArrayItemForPHPException::class); @@ $this->runTypeTest($typeName, $columnType, ['invalid-mac', '08:00:2b:01:02:03']); } + + #[Test] + public function can_handle_non_string_items(): void + { + $this->expectException(InvalidMacaddrArrayItemForPHPException::class); + + $typeName = $this->getTypeName(); + $columnType = $this->getPostgresTypeName(); + + $this->runTypeTest($typeName, $columnType, ['08:00:2b:01:02:03', 123]); + }tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/InetArrayTypeTest.php (2)
35-49: Add CIDR prefix cases to broaden inet[] coverage.inet supports addresses with netmasks; adding such cases improves coverage beyond pure host addresses.
Apply this diff to include a prefixes scenario:
return [ @@ 'inet array with localhost' => ['inet array with localhost', [ '127.0.0.1', '::1', ]], + 'inet array with prefixes' => ['inet array with prefixes', [ + '192.168.1.1/24', + '2001:db8::/64', + ]], 'empty inet array' => ['empty inet array', []],
52-61: Add a negative test for non-string items.This aligns with the learnings to include non-string inputs when testing DB-to-PHP conversions.
Apply this diff:
#[Test] public function can_handle_invalid_addresses(): void { $this->expectException(InvalidInetArrayItemForPHPException::class); @@ $this->runTypeTest($typeName, $columnType, ['invalid-address', '192.168.1.1']); } + + #[Test] + public function can_handle_non_string_items(): void + { + $this->expectException(InvalidInetArrayItemForPHPException::class); + + $typeName = $this->getTypeName(); + $columnType = $this->getPostgresTypeName(); + + $this->runTypeTest($typeName, $columnType, ['192.168.1.1', 42]); + }tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/PointTypeTest.php (1)
46-48: Address PHPMD “UnusedFormalParameter” by marking the param as intentionally unused.Rename the first parameter to a throwaway variable.
Apply this diff:
- public function can_handle_point_values(string $testName, PointValueObject $pointValueObject): void + public function can_handle_point_values(string $_testName, PointValueObject $pointValueObject): voidtests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RealArrayTypeTest.php (1)
45-45: Minor: the label says “large numbers” but the magnitudes are modest.Either increase magnitudes (careful with overflow) or rename for clarity.
Apply this diff:
- 'real array with large numbers' => ['real array with large numbers', [3.402823e+6, -3.402823e+6]], + 'real array with larger numbers' => ['real array with larger numbers', [3.402823e+6, -3.402823e+6]],tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/ArrayTypeTestCase.php (1)
33-39: Silence PHPMD unused parameter warning without changing behavior
$testNameexists for dataset labelling but isn’t used. Rename it to$_testNameto satisfy PHPMD (or alternatively add a suppression).- public function can_handle_array_values(string $testName, array $arrayValue): void + public function can_handle_array_values(string $_testName, array $arrayValue): voidIf you prefer keeping the original name, add:
/** @SuppressWarnings(PHPMD.UnusedFormalParameter) */above the method.
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArrayTypeTest.php (1)
34-46: Optional: Drop the empty-array dataset (already covered by the base test)
ArrayTypeTestCase::can_handle_empty_array()already verifies the empty array path. Keeping it here is harmless but duplicates coverage.- 'empty double precision array' => ['empty double precision array', []],tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/NumRangeTypeTest.php (2)
31-42: Broaden range edge-case coverage (equal bounds)Consider adding equal-bound scenarios to exercise Range::isEmpty semantics for inclusive vs exclusive bounds.
Apply this diff to extend the provider:
public static function provideValidTransformations(): array { return [ 'simple numrange' => ['simple numrange', new NumRangeValueObject(1.5, 10.7, false, false)], 'numrange with inclusive bounds' => ['numrange with inclusive bounds', new NumRangeValueObject(5.5, 15.7, true, true)], 'numrange with negative values' => ['numrange with negative values', new NumRangeValueObject(-100.5, 100.7, false, false)], 'numrange with high precision' => ['numrange with high precision', new NumRangeValueObject(1.123456789, 10.987654321, false, false)], + 'numrange equal bounds inclusive (non-empty)' => ['numrange equal bounds inclusive (non-empty)', new NumRangeValueObject(5.0, 5.0, true, true)], + 'numrange equal bounds exclusive (empty)' => ['numrange equal bounds exclusive (empty)', new NumRangeValueObject(5.0, 5.0, false, false)], ]; }
54-63: Add explicit-empty range scenarioYou currently assert an “implicit empty” (lower > upper). It’s also useful to cover the explicit empty sentinel to hit the alternate code path in Range::isEmpty.
Insert this test just before the class closing brace:
public function can_handle_empty_ranges(): void { $typeName = $this->getTypeName(); $columnType = $this->getPostgresTypeName(); // lower > upper shall result in an empty range $numericRange = new NumRangeValueObject(10.5, 5.7, false, false); $this->runTypeTest($typeName, $columnType, $numericRange); } + + #[Test] + public function can_handle_explicit_empty_ranges(): void + { + $typeName = $this->getTypeName(); + $columnType = $this->getPostgresTypeName(); + + $numericRange = NumRangeValueObject::empty(); + $this->runTypeTest($typeName, $columnType, $numericRange); + }tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/InetTypeTest.php (3)
23-31: Test method naming consistency (nitpick)Other integration tests use “can_handle_*”. Consider aligning with that convention.
36-46: Add a couple more valid inet examples to match unit coverageIPv4-mapped IPv6 and “IPv4 with leading zeros” show up in unit tests and are useful to ensure parity at the integration layer.
Apply this diff:
public static function provideValidTransformations(): array { return [ 'IPv4 address' => ['192.168.1.1'], 'IPv4 with CIDR' => ['192.168.1.0/24'], 'IPv6 address' => ['2001:db8::1'], 'IPv6 with CIDR' => ['2001:db8::/32'], 'localhost IPv4' => ['127.0.0.1'], 'localhost IPv6' => ['::1'], + 'IPv4 with leading zeros' => ['192.168.001.001'], + 'IPv4-mapped IPv6' => ['::ffff:192.168.1.1'], ]; }
48-58: Add negative test for non-string PHP valuesThis will assert InvalidInetForPHPException::forInvalidType is thrown for improper PHP input types, per exception contract.
Insert after can_handle_invalid_addresses():
public function can_handle_invalid_addresses(): void { $this->expectException(InvalidInetForPHPException::class); $typeName = $this->getTypeName(); $columnType = $this->getPostgresTypeName(); $this->runTypeTest($typeName, $columnType, 'invalid-address'); } + #[Test] + public function rejects_non_string_values(): void + { + $this->expectException(InvalidInetForPHPException::class); + + $typeName = $this->getTypeName(); + $columnType = $this->getPostgresTypeName(); + + // e.g. integer instead of string + $this->runTypeTest($typeName, $columnType, 123); + }tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TsRangeTypeTest.php (2)
31-62: Stabilize timestamps against environment timezones and add microsecond coverageSince tsrange is “timestamp without time zone,” be explicit about timezone in DateTimeImmutable to avoid environment-dependent defaults. Also add a microsecond case to verify precision.
Apply this diff:
public static function provideValidTransformations(): array { return [ - 'simple tsrange' => ['simple tsrange', new TsRangeValueObject( - new \DateTimeImmutable('2023-01-01 10:00:00'), - new \DateTimeImmutable('2023-01-01 18:00:00'), + 'simple tsrange' => ['simple tsrange', new TsRangeValueObject( + new \DateTimeImmutable('2023-01-01 10:00:00', new \DateTimeZone('UTC')), + new \DateTimeImmutable('2023-01-01 18:00:00', new \DateTimeZone('UTC')), false, false )], - 'tsrange with inclusive bounds' => ['tsrange with inclusive bounds', new TsRangeValueObject( - new \DateTimeImmutable('2023-01-01 09:00:00'), - new \DateTimeImmutable('2023-01-01 17:00:00'), + 'tsrange with inclusive bounds' => ['tsrange with inclusive bounds', new TsRangeValueObject( + new \DateTimeImmutable('2023-01-01 09:00:00', new \DateTimeZone('UTC')), + new \DateTimeImmutable('2023-01-01 17:00:00', new \DateTimeZone('UTC')), true, true )], - 'tsrange with same day' => ['tsrange with same day', new TsRangeValueObject( - new \DateTimeImmutable('2023-06-15 08:00:00'), - new \DateTimeImmutable('2023-06-15 16:00:00'), + 'tsrange with same day' => ['tsrange with same day', new TsRangeValueObject( + new \DateTimeImmutable('2023-06-15 08:00:00', new \DateTimeZone('UTC')), + new \DateTimeImmutable('2023-06-15 16:00:00', new \DateTimeZone('UTC')), false, false )], - 'tsrange with midnight' => ['tsrange with midnight', new TsRangeValueObject( - new \DateTimeImmutable('2023-01-01 00:00:00'), - new \DateTimeImmutable('2023-01-01 23:59:59'), + 'tsrange with midnight' => ['tsrange with midnight', new TsRangeValueObject( + new \DateTimeImmutable('2023-01-01 00:00:00', new \DateTimeZone('UTC')), + new \DateTimeImmutable('2023-01-01 23:59:59', new \DateTimeZone('UTC')), false, false )], + 'tsrange with microseconds' => ['tsrange with microseconds', new TsRangeValueObject( + new \DateTimeImmutable('2023-01-01 10:00:00.123456', new \DateTimeZone('UTC')), + new \DateTimeImmutable('2023-01-01 10:00:05.654321', new \DateTimeZone('UTC')), + false, + false + )], ]; }
74-89: Add explicit-empty tsrange testMirror the numeric range explicit-empty coverage to hit the explicit path.
Insert before the class closing brace:
public function can_handle_empty_ranges(): void { $typeName = $this->getTypeName(); $columnType = $this->getPostgresTypeName(); // lower > upper shall result in an empty range $tsRange = new TsRangeValueObject( new \DateTimeImmutable('2023-01-01 18:00:00'), new \DateTimeImmutable('2023-01-01 10:00:00'), false, false ); $this->runTypeTest($typeName, $columnType, $tsRange); } + + #[Test] + public function can_handle_explicit_empty_ranges(): void + { + $typeName = $this->getTypeName(); + $columnType = $this->getPostgresTypeName(); + + $tsRange = TsRangeValueObject::empty(); + $this->runTypeTest($typeName, $columnType, $tsRange); + }tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/CidrArrayTypeTest.php (1)
53-63: Add negative tests for wrong types (array items and whole value)Exercise the specific exception factories: forInvalidType (non-string item) and forInvalidArrayType (whole value not an array).
Apply this diff:
public function can_handle_invalid_networks(): void { $this->expectException(InvalidCidrArrayItemForPHPException::class); $typeName = $this->getTypeName(); $columnType = $this->getPostgresTypeName(); $this->runTypeTest($typeName, $columnType, ['invalid-network', '192.168.1.0/24']); } + + #[Test] + public function rejects_non_string_items(): void + { + $this->expectException(InvalidCidrArrayItemForPHPException::class); + + $typeName = $this->getTypeName(); + $columnType = $this->getPostgresTypeName(); + + // integer item inside the array + $this->runTypeTest($typeName, $columnType, ['192.168.1.0/24', 123]); + } + + #[Test] + public function rejects_non_array_value(): void + { + $this->expectException(InvalidCidrArrayItemForPHPException::class); + + $typeName = $this->getTypeName(); + $columnType = $this->getPostgresTypeName(); + + // value itself is not an array + /** @phpstan-ignore-next-line intentional invalid input */ + $this->runTypeTest($typeName, $columnType, '192.168.1.0/24'); + }tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int8RangeTypeTest.php (2)
38-38: Fix mismatch between test title and range inclusivity flagsTitle says “inclusive bounds” but the upper bound is exclusive (true, false). Either adjust the wording or make both bounds inclusive.
Apply this diff:
- 'int8range with inclusive bounds' => ['int8range with inclusive bounds', new Int8RangeValueObject(5, 16, true, false)], + 'int8range with inclusive bounds' => ['int8range with inclusive bounds', new Int8RangeValueObject(5, 16, true, true)],
50-52: Prefer the semantic constructor for infinite rangesUsing Int8RangeValueObject::infinite() improves readability and intent.
Apply this diff:
- $int8Range = new Int8RangeValueObject(null, null, false, false); + $int8Range = Int8RangeValueObject::infinite();tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/MacaddrTypeTest.php (2)
39-44: Correct mislabeled/misleading data provider entries
- “mixed case MAC” currently duplicates the standard lower-case sample; use an actual mixed/upper-case string.
- “MAC with single digits” label is misleading since MAC octets are always two hex digits; consider “MAC with leading zeros.”
Apply this diff:
return [ 'standard MAC address' => ['08:00:2b:01:02:03'], 'MAC with zeros' => ['00:00:00:00:00:00'], 'MAC with FF' => ['ff:ff:ff:ff:ff:ff'], - 'mixed case MAC' => ['08:00:2b:01:02:03'], - 'MAC with single digits' => ['01:02:03:04:05:06'], + 'mixed case MAC' => ['08:00:2B:01:02:03'], + 'MAC with leading zeros' => ['01:02:03:04:05:06'], ];
47-56: Add a negative test for invalid PHP typesTo cover InvalidMacaddrForPHPException::forInvalidType(), add a case with a non-string (e.g., int or array). This improves negative-path coverage.
Example addition (outside the shown range):
#[Test] public function can_handle_invalid_types(): void { $this->expectException(InvalidMacaddrForPHPException::class); $typeName = $this->getTypeName(); $columnType = $this->getPostgresTypeName(); // Non-string input should fail validation early /** @var mixed $invalid */ $invalid = 12345; $this->runTypeTest($typeName, $columnType, $invalid); }I can open a follow-up PR to add this if helpful.
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/DateRangeTypeTest.php (2)
43-48: Fix mismatch between test title and range inclusivity flagsThe title states “inclusive bounds” but upper bound is exclusive (true, false). Make both inclusive or rename.
Apply this diff:
- 'daterange with inclusive bounds' => ['daterange with inclusive bounds', new DateRangeValueObject( + 'daterange with inclusive bounds' => ['daterange with inclusive bounds', new DateRangeValueObject( new \DateTimeImmutable('2023-01-01'), new \DateTimeImmutable('2024-01-01'), - true, - false + true, + true )],
70-72: Prefer the semantic constructor for infinite rangesUsing DateRangeValueObject::infinite() makes intent explicit.
Apply this diff:
- $dateRange = new DateRangeValueObject(null, null, false, false); + $dateRange = DateRangeValueObject::infinite();
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int4RangeTypeTest.php
Show resolved
Hide resolved
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int4RangeTypeTest.php
Show resolved
Hide resolved
| 'simple int4range' => ['simple int4range', new Int4RangeValueObject(1, 10, true, false)], | ||
| 'int4range with inclusive bounds' => ['int4range with inclusive bounds', new Int4RangeValueObject(5, 15, true, false)], | ||
| 'int4range with negative values' => ['int4range with negative values', new Int4RangeValueObject(-100, 100, true, false)], | ||
| 'int4range with max values' => ['int4range with max values', new Int4RangeValueObject(-2147483648, 2147483647, true, false)], | ||
| ]; |
There was a problem hiding this comment.
Fix mislabeled dataset: “inclusive bounds” currently uses upper-exclusive
The case named “int4range with inclusive bounds” passes (true, false) which formats as [lower, upper). To reflect fully inclusive bounds [lower, upper], both flags should be true.
- 'int4range with inclusive bounds' => ['int4range with inclusive bounds', new Int4RangeValueObject(5, 15, true, false)],
+ 'int4range with inclusive bounds' => ['int4range with inclusive bounds', new Int4RangeValueObject(5, 15, true, true)],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'simple int4range' => ['simple int4range', new Int4RangeValueObject(1, 10, true, false)], | |
| 'int4range with inclusive bounds' => ['int4range with inclusive bounds', new Int4RangeValueObject(5, 15, true, false)], | |
| 'int4range with negative values' => ['int4range with negative values', new Int4RangeValueObject(-100, 100, true, false)], | |
| 'int4range with max values' => ['int4range with max values', new Int4RangeValueObject(-2147483648, 2147483647, true, false)], | |
| ]; | |
| 'simple int4range' => ['simple int4range', new Int4RangeValueObject(1, 10, true, false)], | |
| 'int4range with inclusive bounds' => ['int4range with inclusive bounds', new Int4RangeValueObject(5, 15, true, true)], | |
| 'int4range with negative values' => ['int4range with negative values', new Int4RangeValueObject(-100, 100, true, false)], | |
| 'int4range with max values' => ['int4range with max values', new Int4RangeValueObject(-2147483648, 2147483647, true, false)], |
🤖 Prompt for AI Agents
In tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int4RangeTypeTest.php
around lines 37 to 41, the dataset entry labeled "int4range with inclusive
bounds" incorrectly constructs the Int4RangeValueObject with (true, false) which
represents [lower, upper) instead of fully inclusive; change the flags to (true,
true) so the test uses [lower, upper] semantics.
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int8RangeTypeTest.php
Show resolved
Hide resolved
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RealArrayTypeTest.php
Show resolved
Hide resolved
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TstzRangeTypeTest.php
Show resolved
Hide resolved
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TstzRangeTypeTest.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/ValueObject/TstzRangeTest.php (1)
161-169: Revert timezone normalization in can_handle_different_timezones assertionThe
TstzRange::__toString()implementation usesformat('Y-m-d H:i:s.uP'), which preserves eachDateTimeInterface’s original offset rather than normalizing to UTC. The test must expect the original+02:00and-05:00offsets to align with other test cases and class semantics.• File:
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/ValueObject/TstzRangeTest.php
• Method:can_handle_different_timezones()Apply this diff:
- self::assertEquals('[2023-01-01 08:00:00.000000+00:00,2023-01-01 16:00:00.000000+00:00)', (string) $tstzRange); + self::assertEquals('[2023-01-01 10:00:00.000000+02:00,2023-01-01 18:00:00.000000-05:00)', (string) $tstzRange);
📜 Review details
Configuration used: .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 (2)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/ValueObject/TstzRangeTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#340
File: tests/MartinGeorgiev/Doctrine/DBAL/Types/InetArrayTest.php:145-145
Timestamp: 2025-04-11T11:23:44.192Z
Learning: In the PostgreSQL for Doctrine test cases, methods that test database-to-PHP conversions should use `mixed` type for parameter and include non-string test cases in their data providers, following the pattern in classes like InetTest, CidrTest, and MacaddrTest.
📚 Learning: 2025-03-11T12:32:10.726Z
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
Applied to files:
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/ValueObject/TstzRangeTest.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php
📚 Learning: 2025-04-11T11:23:44.192Z
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#340
File: tests/MartinGeorgiev/Doctrine/DBAL/Types/InetArrayTest.php:145-145
Timestamp: 2025-04-11T11:23:44.192Z
Learning: In the PostgreSQL for Doctrine test cases, methods that test database-to-PHP conversions should use `mixed` type for parameter and include non-string test cases in their data providers, following the pattern in classes like InetTest, CidrTest, and MacaddrTest.
Applied to files:
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php
📚 Learning: 2025-05-23T11:11:57.951Z
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.951Z
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.
Applied to files:
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php
📚 Learning: 2025-03-29T03:31:17.114Z
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.
Applied to files:
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php
🧬 Code Graph Analysis (1)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php (4)
src/MartinGeorgiev/Doctrine/DBAL/Types/ValueObject/Range.php (1)
Range(14-143)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TestCase.php (1)
runTypeTest(31-73)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTestCase.php (1)
RangeTestCase(9-59)tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/BaseRangeTestCase.php (1)
BaseRangeTestCase(20-178)
🪛 PHPMD (2.15.0)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php
41-41: Avoid unused parameters such as '$testName'. (Unused Code Rules)
(UnusedFormalParameter)
🪛 GitHub Actions: Integrations
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php
[error] 23-23: Range string representation mismatch for type tstzrange (dataset: 'tstzrange with different timezones'). During 'composer run-integration-tests', expected '(.000000+02:00,.000000+02:00)' but got '(.000000+00:00,.000000+00:00)'.
[error] 46-46: Range string representation mismatch for type tstzrange (dataset: 'tstzrange with different timezones'). During 'composer run-integration-tests', expected '(.000000+02:00,.000000+02:00)' but got '(.000000+00:00,.000000+00:00)'.
⏰ 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). (18)
- GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer 1.2
- GitHub Check: PHP 8.3 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.3 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + 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.2 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM latest + Doctrine Lexer 2.1
🔇 Additional comments (3)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php (3)
17-24: Type-safe Range comparison override: good use of a dedicated assertion pathOverriding the equality assertion to enforce RangeValueObject on both sides and delegating to the range-aware assertion is the right call.
26-33: Null-handling test coverage looks solidThis ensures the end-to-end mapping handles NULL correctly through the harness.
35-47: Suppress the unused$testNameparameter or remove it consistentlyPHPMD flags
$testNameas unused. Since it’s only there to make dataset keys visible in reports, you can either suppress the warning (Option A) or drop the parameter entirely (Option B). Note that Option B requires touching every subclass and its parent call.Option A — suppress the warning and signal intent:
/** * Data-driven test for Range value objects. * Subclasses should add #[DataProvider('provideValidTransformations')]. + * @SuppressWarnings(PHPMD.UnusedFormalParameter) * @param RangeValueObject<\DateTimeInterface|float|int> $rangeValueObject */ - public function can_handle_range_values(string $testName, RangeValueObject $rangeValueObject): void + public function can_handle_range_values(string $_testName, RangeValueObject $rangeValueObject): void { $typeName = $this->getTypeName(); $columnType = $this->getPostgresTypeName(); $this->runTypeTest($typeName, $columnType, $rangeValueObject); }Option B — remove the parameter (update all data providers to yield only the
RangeValueObject, and change every subclass’s method signature and parent call):
- tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TsRangeTypeTest.php
- tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TstzRangeTypeTest.php
- tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/NumRangeTypeTest.php
- tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int8RangeTypeTest.php
- tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int4RangeTypeTest.php
- tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/DateRangeTypeTest.php
Given the six subclasses and the single data‐provider in the base
RangeTypeTestCase, Option A is the least intrusive way to address the PHPMD warning.
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TstzRangeTypeTest.php (2)
12-12: Bind the generic and document intent at class levelAdd a class-level generic binding to make the template explicit for static analysis and readability.
+/** + * @extends RangeTypeTestCase<\DateTimeImmutable> + */ class TstzRangeTypeTest extends RangeTypeTestCase
24-29: Narrow the parameter type to the concrete value objectUse TstzRangeValueObject instead of the generic RangeValueObject to remove generics warnings and improve clarity. The method body stays the same.
- public function can_handle_range_values(string $testName, RangeValueObject $rangeValueObject): void + public function can_handle_range_values(string $testName, TstzRangeValueObject $rangeValueObject): void { parent::can_handle_range_values($testName, $rangeValueObject); }tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php (2)
111-124: Fix tstzrange assertion: compare instants, not offsets (pipeline failure)String-comparing tstzrange values is brittle because Postgres returns timestamptz in the session timezone. This causes false negatives like the failing “expected …+02:00 but got …+00:00”. Normalize both sides to epoch microseconds and compare inclusivity to make the test timezone-agnostic.
protected function assertRangeEquals(RangeValueObject $expected, RangeValueObject $actual, string $typeName): void { - $this->assertEquals( - $expected->__toString(), - $actual->__toString(), - 'Range string representation mismatch for type '.$typeName - ); - - $this->assertEquals( - $expected->isEmpty(), - $actual->isEmpty(), - 'Range empty state mismatch for type '.$typeName - ); + // Compare empty state first + $this->assertSame( + $expected->isEmpty(), + $actual->isEmpty(), + 'Range empty state mismatch for type '.$typeName + ); + if ($expected->isEmpty() && $actual->isEmpty()) { + return; + } + + if ($typeName === 'tstzrange') { + $e = $this->normalizeTstzRangeToEpochMicroseconds($expected); + $a = $this->normalizeTstzRangeToEpochMicroseconds($actual); + + $this->assertSame($e['lower'], $a['lower'], 'Lower bound instant differs for tstzrange'); + $this->assertSame($e['upper'], $a['upper'], 'Upper bound instant differs for tstzrange'); + $this->assertSame($e['lowerInc'], $a['lowerInc'], 'Lower inclusivity differs for tstzrange'); + $this->assertSame($e['upperInc'], $a['upperInc'], 'Upper inclusivity differs for tstzrange'); + return; + } + + // Fallback: compare exact string representation for other range types + $this->assertSame( + $expected->__toString(), + $actual->__toString(), + 'Range string representation mismatch for type '.$typeName + ); }
125-134: Add helper to normalize tstzrange for robust comparisonsCompanion to the assertion fix; parses the range string and converts bounds to epoch microseconds.
+ /** + * Normalize a tstzrange RangeValueObject to epoch microseconds and inclusivity flags. + * + * @return array{lower: ?string, upper: ?string, lowerInc: bool, upperInc: bool} + */ + private function normalizeTstzRangeToEpochMicroseconds(RangeValueObject $range): array + { + $s = (string) $range; + $lowerInc = $s[0] === '['; + $upperInc = \substr($s, -1) === ']'; + + $content = \substr($s, 1, -1); + [$lowerStr, $upperStr] = \explode(',', $content, 2); + + $toEpochMicros = static function (?string $str): ?string { + if ($str === null || $str === '') { + return null; + } + $dt = new \DateTimeImmutable($str); + return $dt->format('U.u'); + }; + + $lower = $lowerStr === '' ? null : $toEpochMicros($lowerStr); + $upper = $upperStr === '' ? null : $toEpochMicros($upperStr); + + return [ + 'lower' => $lower, + 'upper' => $upper, + 'lowerInc' => $lowerInc, + 'upperInc' => $upperInc, + ]; + }tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int4RangeTypeTest.php (3)
12-12: Bind the generic to intAdd a class-level generic binding to clarify the template parameter and satisfy static analysis.
+/** + * @extends RangeTypeTestCase<int> + */ class Int4RangeTypeTest extends RangeTypeTestCase
24-29: Narrow the parameter type to Int4RangeValueObjectImproves type-safety and removes generics warnings.
- public function can_handle_range_values(string $testName, RangeValueObject $rangeValueObject): void + public function can_handle_range_values(string $testName, Int4RangeValueObject $rangeValueObject): void { parent::can_handle_range_values($testName, $rangeValueObject); }
37-41: Fix mislabeled dataset: “inclusive bounds” currently upper-exclusiveThe entry named “int4range with inclusive bounds” uses (true, false) which is “[lower, upper)”. Make both flags true for “[lower, upper]”.
- 'int4range with inclusive bounds' => ['int4range with inclusive bounds', new Int4RangeValueObject(5, 15, true, false)], + 'int4range with inclusive bounds' => ['int4range with inclusive bounds', new Int4RangeValueObject(5, 15, true, true)],
🧹 Nitpick comments (7)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TstzRangeTypeTest.php (1)
70-71: Use the provided factory for infinite rangesPrefer the semantic factory method over manual construction.
- $tstzRange = new TstzRangeValueObject(null, null, false, false); + $tstzRange = TstzRangeValueObject::infinite();tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TsRangeTypeTest.php (3)
12-12: Bind the generic and document intent at class levelAdd a class-level generic binding for consistency with other range tests and to help static analysis.
+/** + * @extends RangeTypeTestCase<\DateTimeImmutable> + */ class TsRangeTypeTest extends RangeTypeTestCase
24-29: Narrow the parameter type to the concrete TsRange value objectUse TsRangeValueObject to make intent explicit and remove generic type noise.
- public function can_handle_range_values(string $testName, RangeValueObject $rangeValueObject): void + public function can_handle_range_values(string $testName, TsRangeValueObject $rangeValueObject): void { parent::can_handle_range_values($testName, $rangeValueObject); }
70-71: Use the factory for infinite rangesSmall readability win; aligns with other tests.
- $tsRange = new TsRangeValueObject(null, null, false, false); + $tsRange = TsRangeValueObject::infinite();tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php (2)
141-147: Silence “unused parameter” for $testNamePHPMD flags $testName as unused. Prefix with an underscore to acknowledge intent without extra noise.
- public function can_handle_range_values(string $testName, RangeValueObject $rangeValueObject): void + public function can_handle_range_values(string $_testName, RangeValueObject $rangeValueObject): void
152-158: Silence “unused parameter” for $name in operator scenariosSame PHPMD concern; rename to $_name.
- public function can_evaluate_operator_scenarios(string $name, string $dql, array $expectedIds): void + public function can_evaluate_operator_scenarios(string $_name, string $dql, array $expectedIds): voidtests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int4RangeTypeTest.php (1)
50-51: Use the factory for infinite rangesReadability nit: prefer the semantic constructor.
- $int4Range = new Int4RangeValueObject(null, null, false, false); + $int4Range = Int4RangeValueObject::infinite();
📜 Review details
Configuration used: .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 (9)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/DateRangeTypeTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int4RangeTypeTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int8RangeTypeTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/NumRangeTypeTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeOperatorsTest.php(0 hunks)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTestCase.php(0 hunks)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TsRangeTypeTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TstzRangeTypeTest.php(1 hunks)
💤 Files with no reviewable changes (2)
- tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTestCase.php
- tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeOperatorsTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/NumRangeTypeTest.php
- tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int8RangeTypeTest.php
- tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/DateRangeTypeTest.php
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#340
File: tests/MartinGeorgiev/Doctrine/DBAL/Types/InetArrayTest.php:145-145
Timestamp: 2025-04-11T11:23:44.192Z
Learning: In the PostgreSQL for Doctrine test cases, methods that test database-to-PHP conversions should use `mixed` type for parameter and include non-string test cases in their data providers, following the pattern in classes like InetTest, CidrTest, and MacaddrTest.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
📚 Learning: 2025-03-11T12:32:10.726Z
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
Applied to files:
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TstzRangeTypeTest.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TsRangeTypeTest.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int4RangeTypeTest.php
📚 Learning: 2025-04-11T11:23:44.192Z
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#340
File: tests/MartinGeorgiev/Doctrine/DBAL/Types/InetArrayTest.php:145-145
Timestamp: 2025-04-11T11:23:44.192Z
Learning: In the PostgreSQL for Doctrine test cases, methods that test database-to-PHP conversions should use `mixed` type for parameter and include non-string test cases in their data providers, following the pattern in classes like InetTest, CidrTest, and MacaddrTest.
Applied to files:
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TstzRangeTypeTest.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TsRangeTypeTest.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int4RangeTypeTest.php
📚 Learning: 2025-03-29T03:31:17.114Z
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.
Applied to files:
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TstzRangeTypeTest.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int4RangeTypeTest.php
📚 Learning: 2025-05-23T11:11:57.951Z
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.951Z
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.
Applied to files:
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TstzRangeTypeTest.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int4RangeTypeTest.php
📚 Learning: 2025-03-29T03:31:17.114Z
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.
Applied to files:
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TstzRangeTypeTest.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int4RangeTypeTest.php
🧬 Code Graph Analysis (4)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TstzRangeTypeTest.php (3)
src/MartinGeorgiev/Doctrine/DBAL/Types/ValueObject/Range.php (1)
Range(14-143)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php (4)
Test(126-133)RangeTypeTestCase(14-164)can_handle_range_values(141-147)provideOperatorScenarios(163-163)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TestCase.php (1)
runTypeTest(31-73)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php (7)
src/MartinGeorgiev/Doctrine/DBAL/Types/ValueObject/Range.php (2)
Range(14-143)isEmpty(54-70)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Contains.php (1)
Contains(18-26)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/IsContainedBy.php (1)
IsContainedBy(18-26)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Overlaps.php (1)
Overlaps(18-26)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TestCase.php (1)
runTypeTest(31-73)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTestCase.php (1)
RangeTestCase(9-59)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeOperatorsTest.php (1)
RangeOperatorsTest(11-176)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TsRangeTypeTest.php (3)
src/MartinGeorgiev/Doctrine/DBAL/Types/ValueObject/Range.php (1)
Range(14-143)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php (3)
Test(126-133)RangeTypeTestCase(14-164)provideOperatorScenarios(163-163)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TestCase.php (1)
runTypeTest(31-73)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/Int4RangeTypeTest.php (4)
src/MartinGeorgiev/Doctrine/DBAL/Types/ValueObject/Range.php (1)
Range(14-143)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php (4)
DataProvider(152-158)Test(126-133)RangeTypeTestCase(14-164)provideOperatorScenarios(163-163)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TestCase.php (1)
runTypeTest(31-73)tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/Int4RangeTest.php (1)
Int4RangeTest(13-65)
🪛 PHPMD (2.15.0)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php
141-141: Avoid unused parameters such as '$testName'. (Unused Code Rules)
(UnusedFormalParameter)
154-154: Avoid unused parameters such as '$name'. (Unused Code Rules)
(UnusedFormalParameter)
🪛 GitHub Actions: Integrations
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/RangeTypeTestCase.php
[error] 113-113: Range string representation mismatch for type tstzrange. Expected '(.000000+02:00,.000000+02:00)' but got '(.000000+00:00,.000000+00:00)'.
⏰ 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)
- GitHub Check: PHP 8.4 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM 3.0 + 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 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
|
@coderabbitai, are there any test scenarios from the old, deleted files, that got missed in the new ones and may result in test coverage regression? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 964 🏁 Script executed: Length of output: 15305 🏁 Script executed: Length of output: 13645 @martin-georgiev Based on my analysis of the test reorganization, the coverage appears to be well maintained with no significant regression. Here's my detailed assessment: ✅ Test Coverage AnalysisCurrent Types Covered (22 types):
✅ Preserved Functionality
🤔 Potential Gaps to ConsiderThe types that might have been in the old comprehensive test but aren't explicitly tested now:
However, these basic types are likely handled by Doctrine's built-in type system and may not need custom PostgreSQL-specific testing. 📊 Coverage VerdictSince Coveralls reports maintained 94.65% coverage, this strongly suggests no meaningful test coverage regression occurred. The reorganization appears to have successfully distributed the comprehensive test scenarios across focused, type-specific test files while preserving all critical functionality. The new structure actually provides better maintainability and clearer test organization without sacrificing coverage quality. 🧠 Learnings used |
Summary by CodeRabbit