PHPLIB-715: Use flexible numeric comparisons in DocumentMatchConstraint #958
Conversation
| @@ -146,13 +145,13 @@ public function errorMessageProvider() | |||
| ['foo' => ['foo' => 'bar', 'bar' => 'baz']], | |||
| ], | |||
| 'Scalar value not equal' => [ | |||
| 'Field path "foo": Failed asserting that two values are equal.', | |||
| 'Field path "foo": Failed asserting that two strings are equal.', | |||
There was a problem hiding this comment.
Error messages change due to us no longer using a direct comparison for scalar values, and instead leaving those checks to PHPUnit comparators.
| @@ -23,6 +23,14 @@ public function testMatchesDocument(): void | |||
| $this->assertResult(true, $c, ['y' => ['b' => 2, 'a' => 1], 'x' => 1], 'Root and embedded key order is not significant'); | |||
| } | |||
|
|
|||
| public function testFlexibleNumericComparison(): void | |||
There was a problem hiding this comment.
Added these for the sake of completeness (and because I wanted to make sure that this works in the unified test runner).
| } elseif ($result instanceof MapReduceResult) { | ||
| /* For mapReduce operations, we ignore the mapReduce metadata | ||
| * and only return the result iterator for evaluation. */ | ||
| $result = iterator_to_array($result->getIterator()); |
There was a problem hiding this comment.
Was this change necessary? The only other change was ASSERT_SAME_DOCUMENTS to ASSERT_DOCUMENTS_MATCH. IIRC, the result assertion logic iterates the $result variable here and metadata is never considered.
There was a problem hiding this comment.
It doesn't. We could update the logic to call iterator_to_array on any IteratorAggregate, but assertDocumentsMatch compared the MapReduceResult instance to the expected object, causing a test failure.
There was a problem hiding this comment.
ResultExpectation::assert() had entirely different handling for those cases. ASSERT_SAME_DOCUMENTS invoked TestCase::assertSameDocuments(), which unpacks the Traversable with iterator_to_array(). ASSERT_DOCUMENTS_MATCH used MongoDB\Tests\SpecTests\FunctionalTestCase::assertDocumentsMatch.
This is fine. No sense adding significant refactoring for an outdated code path.
| @@ -421,6 +401,11 @@ private function doToString() | |||
| return 'matches ' . $this->exporter()->export($this->value); | |||
| } | |||
|
|
|||
| private static function isNumeric($value): bool | |||
| { | |||
| return is_int($value) || is_float($value) || $value instanceof Int64; | |||
There was a problem hiding this comment.
prepareBSON has logic to "Convert Int64 objects to integers on 64-bit platforms for compatibility reasons." Was that sufficient to allow flexible comparisons of Int64 objects with integers and floats? Mainly asking because I don't believe we have a test for this.
Int64 is conditionally used in DocumentsMatchConstraintTest only when the platform is 32-bit.
This probably makes no difference, but the fact that you reference the class here made me ask.
There was a problem hiding this comment.
I shamelessly copied the logic from the unified spec tests where this was used for flexible numeric comparisons.
…nt (mongodb#958) * PHPLIB-715: Use flexible numeric comparisons in DocumentMatchConstraint * Fix phpcs
PHPLIB-715
This unskips a mapReduce test that fails due to strict numerical comparison. To fix this, I applied the same logic used in the unified spec tests. This comes at the cost of removing placeholder functionality in DocumentsMatchConstraint; however this wasn't used in the constraint at all. The transaction spec tests make use of the
42placeholder, but the transaction spec tests covers these in the command listener it uses to assert the commands, so it's safe to remove without breaking other tests.