assertSame and assertEquals incorrectly pass on some floats

[luispabon]

Q	A
PHPUnit version	7.1.5
PHP version	7.2.5
Installation Method	Composer

These two assertions pass:

        self::assertEquals(0.06999999999, 0.07);
        self::assertSame(0.06999999999, 0.07);

They shouldn't.

[keradus]

i will take assertSame for discussion docs claims it uses === for comparision: https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/Constraint/IsIdentical.php#L18

it is not true, for numbers, it has epsilon counted in: https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/Constraint/IsIdentical.php#L67

The thing is, that EPSILON can't be configured.

It was introduced in https://github.com/sebastianbergmann/phpunit/pull/273/files#diff-c228e9b2f01ec4a07f0f59042e40ded6R73 , 7 years ago...

Docs doesn't match expectations, can you clarify that @sebastianbergmann , it would help clearing the issue.

[keradus]

ping @sebastianbergmann , can you take a look at this, please ?

[sebastianbergmann]

Eventually: yes. This will take time (for thinking and discussing).

[luispabon]

Any news on this?

-- Kind regards, Luis Pabon

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity within the last 60 days. It will be closed after 7 days if no further activity occurs. Thank you for your contributions.

[luispabon]

Bump?

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity within the last 60 days. It will be closed after 7 days if no further activity occurs. Thank you for your contributions.

[luispabon]

Bumpey

[gabma]

This might look like a commun PHP issue with floats ? So not related to PHPUnit. https://www.leaseweb.com/labs/2013/06/the-php-floating-point-precision-is-wrong-by-default/

[luispabon]

Could be, but I don't think so:

~ php -a
Interactive mode enabled

php > echo phpversion();
7.3.4-1+ubuntu19.04.1+deb.sury.org+3
php > $foo = 0.06999999999;
php > $bar = 0.07;
php > var_dump($foo == $bar);
bool(false)
php > var_dump($foo === $bar);
bool(false)

~ docker-run-root phpdockerio/php72-cli php -a 
Interactive mode enabled

php > echo phpversion();
7.2.17-1+ubuntu18.04.1+deb.sury.org+3
php > $foo = 0.06999999999;
php > $bar = 0.07;
php > var_dump($foo == $bar);
bool(false)
php > var_dump($foo === $bar);
bool(false)

~ docker-run-root phpdockerio/php56-cli php -a  
Interactive mode enabled

php > echo phpversion();
5.6.40-0+deb8u2
php > $foo = 0.06999999999;
php > $bar = 0.07;
php > var_dump($foo == $bar);
bool(false)
php > var_dump($foo === $bar);
bool(false)

[keradus]

Its a concrete part of code responsible for this on phpunit level

[sebastianbergmann]

The more I think about it the more I come to the conclusion that two floating point numbers cannot (really) be compared for identity and that using assertSame() on float values does not make sense.

I do not remember why private const EPSILON = 0.0000000001 is defined and used in the IsIdentical constraint that is used by assertSame(). To be honest, until I looked the code of IsIdentical today, I was not even aware of that.

assertEquals() should not be used for floating point values either, at least not without its optional $delta parameter (in case the PHPUnit version that is used still has that). This optional parameter is deprecated in current versions and will be removed in the future. assertEqualsWithDelta() should be used in its stead.

Coming back to assertSame(), I think that it might make sense to simply disallow its use on float values. Thoughts?

[DanielRuf]

Coming back to assertSame(), I think that it might make sense to simply disallow its use on float values. Thoughts?

I think the same assertEqualsWithDelta() with a configurable $delta makes much more sense.

As described in the official PHP docs: https://www.php.net/manual/en/language.types.float.php

never trust floating number results to the last digit, and do not compare floating point numbers directly for equality

[DanielRuf]

And this might have been the initial motivation for the hardcoded epsilon value. GMP and BCMath should be used.

[keradus]

Coming back to assertSame(), I think that it might make sense to simply disallow its use on float values. Thoughts?

This can be a huge BC breaker on assertSame contract It also creates complexity what if only expected parameter is float, but other is not. Or if actual is float, but other is not. Easy to decide if both are, but what if not?

[sebastianbergmann]

The current behavior of assertSame() for floating point values is broken. Whichever way this is fixed will break backward compatibility, I guess.

[CJDennis]

I've just run up against the same issue. It makes sense to be a little fuzzy at the end of floating point numbers. However, the following test is massively broken:

  public function testShouldDifferentiateBetweenVastlyDifferentFloatingPointNumbers() {
    $this->assertSame(1.23e-12, 9.87e-123);
  }

Ideally, you'd want to ignore the difference from the last few bits in the IEEE 754 representation compared with the maximum of the absolute values, or in other words, the magnitude of the highest exponent should determine the epsilon to use.

[CJDennis]

This looks like the first change to introduce EPSILON: https://github.com/sebastianbergmann/phpunit/commit/0a952dbc9199e2fac2f1a5bed0b3e06c0858ce2b#diff-25c92442dce593a06a598ee89a3ac318

[CJDennis]

Here is my workaround to compare floating point numbers exactly while still allowing an epsilon/a delta:

FloatComparator.php

<?php
use SebastianBergmann\Comparator\DoubleComparator;
use SebastianBergmann\Comparator\NumericComparator;

class FloatComparator extends DoubleComparator {
  public function assertEquals($expected, $actual, $delta = null, $canonicalize = false, $ignoreCase = false) {
    if ($delta === null) {
      $delta = static::EPSILON;
    }

    NumericComparator::assertEquals($expected, $actual, $delta, $canonicalize, $ignoreCase);
  }
}

MyTest.php

<?php
use PHPUnit\Framework\TestCase;
use SebastianBergmann\Comparator\Factory as ComparatorFactory;

class MyTest extends TestCase {
  protected function setUp() {
    ComparatorFactory::getInstance()->reset();
    ComparatorFactory::getInstance()->register(new FloatComparator);
  }

  protected function tearDown() {
  }

  public function testZeroEqualsZero() {
    $this->assertEqualsWithDelta(0.0, 0.0, 0.0);
  }

  public function testSmallestValueAboveZeroDoesNotEqualZero() {
    $this->assertNotEqualsWithDelta(4.94e-324, 0.0, 0.0);
  }

  public function testSmallestValueAboveZeroEqualsZeroWithDefaultEpsilon() {
    $this->assertEqualsWithDelta(4.94e-324, 0.0, DoubleComparator::EPSILON);
  }

  public function testEpsilonEqualsZeroWithDefaultEpsilon() {
    $this->assertEqualsWithDelta(DoubleComparator::EPSILON, 0.0, DoubleComparator::EPSILON);
  }

  public function testAboveEpsilonDoesNotEqualZeroWithDefaultEpsilon() {
    $this->assertNotEqualsWithDelta(DoubleComparator::EPSILON * 1.0000000001, 0.0, DoubleComparator::EPSILON);
  }
}

Now instead of 0.0 triggering using DoubleComparator::EPSILON, null triggers using DoubleComparator::EPSILON. You can't actually pass null into assertEqualsWithDelta() or assertNotEqualsWithDelta(), but other than that it works as expected, and you can always explicitly pass in DoubleComparator::EPSILON which makes the test's behaviour clearer.

[mvorisek]

I place my vote here, the epsilon must be relative, 0.0000000001 absolute value is very dangerous as it allows to pass tests even if small values differs in orders of magnitude.

Float has precision of 14+ decimal places. Phpunit should check for at least 12 decimal places equality.

[marijnvanwezel]

This issue can probably be closed because of #4874. This fix also does not break any backwards-compatibility promises.

[mvorisek]

yes - https://3v4l.org/570q1 - and 👍 for the PR!

[sebastianbergmann]

Thanks!