Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TestCase::assertSame (and related exact comparisons) must compare float exactly #4966

Closed
mvorisek opened this issue May 7, 2022 · 8 comments
Labels
feature/assertion Issues related to assertions and expectations type/bug Something is broken

Comments

@mvorisek
Copy link
Contributor

mvorisek commented May 7, 2022

Q A
PHPUnit version 9.5.10
PHP version 7.4
Installation Method Composer

Summary

Currently test case like:

 $this->assertSame(8.20234376757473, 8.20234376757474);

pass even if the numbers are different. They are really stored differently, test case:

$this->assertSame(bin2hex(pack('e', 8.20234376757473)), bin2hex(pack('e', 8.20234376757474)));

fails correctly.

It seems phpunit converts float numbers using default php float to string conversion which depends on php.ini precision configuration, test case like:

ini_set('precision', '20');
$this->assertSame(8.20234376757473, 8.20234376757474);

fails as expected.

Also, the float diff output has currently limited precision.

To fix exact comparisons & diff output, I propose to convert float numbers to string using function like:

/**
 * Cast float to string with lossless precision.
 */
final public static function castFloatToString(float $value): string
{
    $precisionBackup = ini_get('precision');
    ini_set('precision', '-1');
    try {
        return (string) $value;
    } finally {
        ini_set('precision', $precisionBackup);
    }
}
@mvorisek mvorisek added the type/bug Something is broken label May 7, 2022
@mvorisek mvorisek changed the title TestCase::assertSame (and related exact comparisons) must compare scalar types binary TestCase::assertSame (and related exact comparisons) must compare float exactly May 7, 2022
@sebastianbergmann
Copy link
Owner

Feel free to send a pull request for the 8.5 branch with your proposed solution.

By the way, your example works for me with precision=14:

$ cat Test.php                             
<?php declare(strict_types=1);
use PHPUnit\Framework\TestCase;

final class Test extends TestCase
{
    public function testOne(): void
    {
        $this->assertSame(8.20234376757473, 8.20234376757474);
    }
}
$ phpunit Test.php
PHPUnit 9.5.20-35-gf53bae505 #StandWithUkraine

F                                                                   1 / 1 (100%)

Time: 00:00.037, Memory: 6.00 MB

There was 1 failure:

1) Test::testOne
Failed asserting that 8.20234376757474 is identical to 8.20234376757473.

/home/sb/Test.php:8

FAILURES!
Tests: 1, Assertions: 1, Failures: 1.
$ php -v               
PHP 8.1.5 (cli) (built: Apr 12 2022 16:19:58) (NTS gcc x86_64)
Copyright (c) The PHP Group
Zend Engine v4.1.5, Copyright (c) Zend Technologies
$ php -i | grep precision
precision => 14 => 14
serialize_precision => -1 => -1

@sebastianbergmann sebastianbergmann added the feature/assertion Issues related to assertions and expectations label May 7, 2022
@mvorisek
Copy link
Contributor Author

mvorisek commented May 7, 2022

the issue is fixed in 136fae3 since phpunit 8.5.24/9.5.14

I propose to remove https://github.com/sebastianbergmann/phpunit/blob/8.5/src/Framework/Constraint/IsIdentical.php#L63 if condition completely like:

-        if (is_float($this->value) && is_float($other) &&
-            !is_infinite($this->value) && !is_infinite($other) &&
-            !is_nan($this->value) && !is_nan($other)) {
-            $success = abs($this->value - $other) < PHP_FLOAT_EPSILON;
-        } else {
-            $success = $this->value === $other;
-        }
+        $success = $this->value === $other;

as I am not aware of any valid use case it should handle

@VincentLanglet
Copy link

HI @mvorisek, with this change, I have some float comparison which are failing.

Basically with some things like

$this->assertEquals(0.3, 0.1+0.2);

Wouldn't be useful to provide the old implementation of assertEquals to another method ?
Like assertFloatEquals ?

@mvorisek
Copy link
Contributor Author

mvorisek commented Oct 3, 2022

Hi @VincentLanglet, I am for it, but it is not that easy to do it correctly.

The old implementation was dependent on precision php.ini and when small delta was used by comparator (sebastianbergmann/comparator#102), it was absolute thus not enought for big numbers and for small numbers, it was basically compasing every number as equal - like 1e-100 and 1e-200.

assertFloatEquals should thus not accept an absolute delta, but instead a delta in sense of significant digits and defaults to 14 or 15. The only problem I see is when comparing a small number againt expected 0.0 - this concept cannot determine the expected delta and I am not sure how to address it with this relative delta concept.

Also note there is assertEqualsWithDelta assertion method.

@jenkoian
Copy link

jenkoian commented Oct 3, 2022

This issue and associated PR caused one of our tests to fail which exposed an underlying bug to us, so just wanted to say thanks for your work on reporting and fixing this 👍 🙌

@dipakmdhrm
Copy link

dipakmdhrm commented Oct 4, 2022

+1 ^

🙌 🖖

@Seb33300
Copy link

Seb33300 commented Dec 29, 2022

I was expecting that using PHP_FLOAT_EPSILON as the delta will be equivalent to the previous implementation:

$this->assertEqualsWithDelta($expected, $actual, PHP_FLOAT_EPSILON);

But my tests are still failing when they were passing before.
Am I missing something?

Edit:
Or maybe the previous implementation was equivalent to this?

$this->assertEqualsWithDelta($expected, $actual, 10 ** -ini_get('precision'));

@markkimsal
Copy link

@Seb33300
If you want to trigger the ini_get('precision') rounding, you can always cast to string

        // pass
        $this->assertEquals(
            0.0000000000000001 + 0.0000000000000002,
            0.0000000000000003,
        );

        // pass
        $this->assertEquals(
            (string) (0.1 + 0.2),
            (string) 0.3,
        );

        // fail
        $this->assertEquals(
            0.1 + 0.2,
            0.3,
        );

Any code that you have that is relying on $expected === $actual or $expected == $actual will fail because of the internals of float representation. But, if you are trying to assert that a calculation will be presented to the user or stored in the DB as a certain value, then casting to a string might be the appropriate way. (note that json serialization is different and uses ini_get('serialize_precision'))

This is the way I'm going forward with patching tests because the delta calculation 10 ** -ini_get('precision')) is a little too cryptic. The previous DoubleComparator had a default epsilon of 1.0E-10 if $delta 0 was passed in. String casting might fail where the precision is somewhere between 10 and 14 significant digits given a test that used to pass in 9.5.21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/assertion Issues related to assertions and expectations type/bug Something is broken
Projects
None yet
Development

No branches or pull requests

7 participants