Problem/Motivation

d.o builds on 8.9 and travis builds on 8.8 with php 7.1+ are failing due to changes in the assertEquals() method in phpunit 7. Price objects are no longer being rendered before comparison, so "1.00" USD isn't equal to "1.000000" USD anymore.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikelutz created an issue. See original summary.

mikelutz’s picture

We could either make a trait similar to what core does with Drupal\TestTools\PhpUnitCompatibility\PhpUnit7\TestCompatibilityTrait and intercept ->assertEquals() on the base classes and convert Price to string, or we could explicitly cast Price to string on each test. I see pros and cons to each approach, namely either a small patch which fixes all the failures at once, versus forcing the explicit declaration per assertion, which makes it much clearer in the test what is being asserted, and forces you to acknowledge it if you write further tests.

So I'll leave that decision up to you, but I'm leaning towards doing the casting in each test.

bojanz’s picture

I would introduce an assertPriceEquals() to our base classes which ensures both $expected_price and $price are instanceof Price, then does a $price->equals($expected_price) inside.

mikelutz’s picture

My stomach felt a little yucky adding something to the base module that depended on the price module, it would be fine for tests, but seeing as after typecasting in the function signature, it reduced down to one line, I decided just to see what a bulk regex S&R from
\$this->assertEquals\((new Price\([^\)]+\)), (.*)\);
to
\$this->assertTrue\($2->equals\($1\)\);

looked like. There will be a few more failures here from arrays and such I'll still have to clean up, but it's a 65k patch on it's own.

mikelutz’s picture

Status: Needs review » Postponed

Down to 4 extra failures in 8.9 over 8.8. Not too bad. I guess this is postponed on #3089178: Make the build pass against 8.8 without deprecation suppression

bojanz’s picture

Status: Postponed » Active
bojanz’s picture

Status: Active » Needs review

Wrong status.

mikelutz’s picture

Here's a completely different approach that might be better.

TIL about PHPUnit Comparators. Interdiff provided, but not useful.

bojanz’s picture

Sweet, I like how this looks!

+    $this->assertSame(-2.00, (float) $adjustments[0]->getAmount()->getNumber());
+    $this->assertSame(4.00, (float) $adjustments[1]->getAmount()->getNumber());

Could we go the other way and just quote the expected numbers? That way it's string-to-string? Since the API does specify that these are strings.

mikelutz’s picture

No because the string in the Price object is '4.000000' and in phpUnit 7 '4.00' != '4.00000'. In phpunit 6 those two strings were considered equal.

Essentially if both passed in values are strings, assertEquals now does a === comparison instead of the == it used to do to avoid things like that passing.

You could do assertEquals(4.00, $adjustments[0]->getAmount()->getNumber()); because as long as one of the values is not a string, it will drop back to the == check and perform the looser check, but my personal preference is to explicitly cast and use ===(assertSame) because I think it's a tighter numeric check.

If you do want to compare strings, you would need to compare to '4.000000' which you could do, and you would get an === comparison with both assertSame or assertEquals, and it would be an even tighter check, but to me the float cast is really checking what you are looking to check, imho.

mikelutz’s picture

bojanz’s picture

No because the string in the Price object is '4.000000' and in phpUnit 7 '4.00' != '4.00000'. In phpunit 6 those two strings were considered equal.

Essentially if both passed in values are strings, assertEquals now does a === comparison instead of the == it used to do to avoid things like that passing.

You could do assertEquals(4.00, $adjustments[0]->getAmount()->getNumber()); because as long as one of the values is not a string, it will drop back to the == check and perform the looser check, but my personal preference is to explicitly cast and use ===(assertSame) because I think it's a tighter numeric check.

I see. In that case I'd prefer the assertEquals() option. Commerce doesn't use floats in any way. I don't want random people grepping through the code to think that we use or approve of floats.

bojanz’s picture

Here's a quick reroll.

- Replaces the float casting with assertEquals() and strings.
- Adds assertions for $expect and $actual to the comparators, so that PhpStorm knows their type.
- Moves the AdjustmentComparator setup to AdjustmentTest::setUp(), where it makes more sense.

I am not super happy with the failure message in AdjustmentComparator because the adjustment label isn't actually useful. It's often something like "Promotion" or "VAT". Can we use print_r() to just output whole arrays, together with the amounts?

EDIT: Great, I broke it. Looks like I'll have to setup an environment for myself. Won't have time to touch this before Friday.

mikelutz’s picture

Yeah, you went back to comparing the numbers as strings which you can't do anymore.

$actual = '2';

$this->assertEquals('2.00', $actual); // passed in 6, fails in 7
$this->assertEquals(2.00, $actual); //passes in both
$this-assertSame(2.00, (float) $actual); //passes in both

I added one more comparator to restore the old behavior if both arguments are numeric strings.

I could have/can revert all the test changes where you swapped the order of arguments, but it's supposed to be ->assertEquals($expected, $actual) so they are now correct, so I've left them.

I also expanded the adjustment failure message. It now looks like this:

Testing Drupal\Tests\commerce_order\Kernel\AdjustmentTest
F                                                                   1 / 1 (100%)

Time: 4.11 seconds, Memory: 6.00 MB

There was 1 failure:

1) Drupal\Tests\commerce_order\Kernel\AdjustmentTest::testArithmetic
Failed asserting that Adjustment "10% off" matches expected "10% off".
--- Expected
+++ Actual
@@ @@
    'label' => '10% off',
    'amount' => 
   Drupal\commerce_price\Price::__set_state(array(
-     'number' => '6.00',
+     'number' => '5',
      'currencyCode' => 'USD',
   )),
    'percentage' => '0.1',

/Users/mike/Development/fedexdev/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit7/TestCompatibilityTrait.php:51
/Users/mike/Development/fedexdev/modules/contrib/commerce/modules/order/tests/src/Kernel/AdjustmentTest.php:206

FAILURES!
Tests: 1, Assertions: 5, Failures: 1.

mikelutz’s picture

bojanz’s picture

Assigned: mikelutz » bojanz

You're a gentleman and a scholar :)

All that is left are nitpicks now:

+ * In phpunit 6, $this->assertEquals('2.0', '2.000') would pass because
+ * numerically the two strings were equal.  This behavior was removed in
+ * phpunit 7, and the assert fails.  This comparator restores the ability to
+ * compare two strings numerically.

s/phpunit/PHPUnit
There is a double space before "This comparator".

+  public function accepts($expected, $actual) {
+    return \is_string($expected) && \is_numeric($expected) && \is_string($actual) && \is_numeric($actual);
+  }

We usually don't backslash PHP's internal functions (even though it's theoretically more correct), so I'd remove the backslashes here if possible. Same with the sprintfs in the comparators.

+    $factory = PhpUnitComparatorFactory::getInstance();
+    $factory->register(new PriceComparator());
+    $factory->register(new NumberComparator());

NumberComparator comes before PriceComparator alphabetically, so let's register it first.

Assigning to myself to do one reroll and test run pre-commit.

bojanz’s picture

  • bojanz committed b9122fc on 8.x-2.x authored by mikelutz
    Issue #3089327 by mikelutz, bojanz: Make the testsuite pass on PhpUnit 7
    
bojanz’s picture

Status: Needs review » Fixed

Huge thanks for making this happen!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.