Fixed testsuite with sebastian/comparator release #29751
Conversation
nicosomb
requested review from
FabienPapet,
matks and
atomiix
and removed request for
FabienPapet and
matks
FabienPapet
dismissed
their stale review
Ok for me as the application code is not updated.
|
@PrestaShop/prestashop-core-developers I need your opinion on this PR please. |
nicosomb
force-pushed
the
fix-testsuite
branch
from
6e49c60
to
6df72b0
Compare
| @@ -61,6 +61,6 @@ public function testGetTotalRateBug() | |||
|
|
|||
| $totalRate = $tax_calculator->getTotalRate(); | |||
|
|
|||
| $this->assertEquals(27.233, $totalRate); | |||
| $this->assertEquals(27.232999999999997, $totalRate); | |||
There was a problem hiding this comment.
I don't quite agree with this solution 🤔 Nor do I agree with the modification made in PHPUnit
PHP imprecision is at fault here, not the code we developed so the expected value is indeed 27.233
Considering the problems of float computing we should be able to define an acceptable precision in our tests to avoid using such values which make no sense Besides what guaranties that one day it will not become 27.232999999999996 😅
In that sense I think PHPUnit previous loose check method made more sense.
There was a problem hiding this comment.
Alternatively, we could use something like:
| $this->assertEquals(27.232999999999997, $totalRate); | |
| $this->assertEqualsWithDelta(27.233, $totalRate, PrestashopTestSettings::EPSILON); |
At least it's explicit. But I don't know if this method is available on older PHPUnit version that are used for versions under PHP8 It's supposed to be available in PHPUnit 8.5 at least (which of course is not compatible with PHP 7.2 and 7.3) Maybe for inferior versions we can hook into PHPUnit to add our custom implementation like a polyfill
There was a problem hiding this comment.
I don't know the specifics about this but it definitely looks like updating this like so is hiding a bug in tax calculation.
There was a problem hiding this comment.
As discussed during our meeting the idea here is to introduce a new method to PHPUnit base class that can be used in any unit tests easily, it can be:
- by adding a base class that extends the default TestCase
- by creating a trait with the added method (advantage no need to change the default class to use the method, it can be used in integration tests as well without the need to have a base class for each type of unit/integration/... tests)
We need a method assertAlmostEquals assertEqualsWithEpsilon (not sure about the naming), internally it behaves as the previous assertEquals used to behave meaning for float values it accepts an offset based on the PHP const PHP_FLOAT_EPSILON
There was a problem hiding this comment.
The goal is to keep the real expected values (which are correct the error comes from unprecise float approximation/rounding) but to use a method that explicitly allows less precision, this way we can identify which parts of our code will need to be refactored (probably using DecimalNumber)
|
I improved the |
nicosomb
force-pushed
the
fix-testsuite
branch
from
c4b32c1
to
5dd4765
Compare
|
@nicosomb Errors in Automated Tests are OK. Don't worry ;) |
|
As you said @nicosomb go merge 😄 thank you |
composer test-allI tried to change code, but after reading some PR related to sebastianbergmann/comparator#102, I updated the testsuite. Tell me if it's ok for you.
(I don't like fix tests when there is a bug... I'd prefer fix the code)