Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#17 | 3089327-17-phpunit7.commerce.patch | 15.44 KB | bojanz |
#15 | interdiff.3089327.14-15.txt | 1.84 KB | mikelutz |
#15 | 3089327-15.commerce.patch | 15.56 KB | mikelutz |
Comments
Comment #2
mikelutzWe 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.
Comment #3
bojanz CreditAttribution: bojanz at Centarro commentedI 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.
Comment #4
mikelutzMy 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.
Comment #5
mikelutzDown 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
Comment #6
bojanz CreditAttribution: bojanz at Centarro commented#3089178: Make the build pass against 8.8 without deprecation suppression has landed. Triggered a retest.
Comment #7
bojanz CreditAttribution: bojanz at Centarro commentedWrong status.
Comment #8
mikelutzHere's a completely different approach that might be better.
TIL about PHPUnit Comparators. Interdiff provided, but not useful.
Comment #9
bojanz CreditAttribution: bojanz at Centarro commentedSweet, I like how this looks!
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.
Comment #10
mikelutzNo 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.
Comment #11
mikelutzFixing a copypasta error...
Comment #12
bojanz CreditAttribution: bojanz at Centarro commentedI 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.
Comment #13
bojanz CreditAttribution: bojanz at Centarro commentedHere'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.
Comment #14
mikelutzYeah, you went back to comparing the numbers as strings which you can't do anymore.
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:
Comment #15
mikelutzoops again..
Comment #16
bojanz CreditAttribution: bojanz at Centarro commentedYou're a gentleman and a scholar :)
All that is left are nitpicks now:
s/phpunit/PHPUnit
There is a double space before "This comparator".
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.
NumberComparator comes before PriceComparator alphabetically, so let's register it first.
Assigning to myself to do one reroll and test run pre-commit.
Comment #17
bojanz CreditAttribution: bojanz at Centarro commentedComment #19
bojanz CreditAttribution: bojanz at Centarro commentedHuge thanks for making this happen!