v1.9.0 breaks float comparisons with EqualValues

[vlasn]

Description

Migrating from 1.8.4 to 1.9.0 seems to have broken some of our unit tests that relied on comparing floats of varying bit depths with EqualValues(...).

I tried fixing it myself but given I'm not very familiar with the reflect package, I didn't get much further than discovering the issue stemmed from comparing the output of reflect.ValueOf(aFloat32).Convert(float64Type).Interface() against the actual float64 - Convert seems to misinterpret the underlying bits and the interface of the former operand comes out to eg. 10.100000381469727 instead of 10.1

Step To Reproduce

Run the following suite:

package a_test

import (
	"testing"

	"github.com/stretchr/testify/suite"
)

type FailingMinimalSuite struct {
	suite.Suite
}

func (s *FailingMinimalSuite) TestFloatEqualValues() {
	s.EqualValues(float32(10.1), float64(10.1))
}

func TestRunFailingMinimalSuite(t *testing.T) {
	suite.Run(t, &FailingMinimalSuite{})
}

Alternatively, inject the following case into TestObjectsAreEqualValues:

{float32(10.1), float64(10.1), true},

Expected behavior

I'd expect the EqualValues(float32(10.1), float64(10.1)) assertion to pass.

Actual behavior

The abovementioned assertion fails.

[gordallott]

Same issue, but only after upgrading to go 1.22

[brackendawson]

This testcase also fails on v1.8.4

{float32(10.1), float64(10.1), true},

But this does not:

{float64(10.1), float32(10.1), true},

This issue happens on Go 1.17 through to 1.22.

You've discovered an old bug here, I don't think anyone would disagree that the assertion should behave the same both ways around?

The docstring says:

EqualValues asserts that two objects are equal or convertible to the same types and equal.

Both the new behaviour and the old behaviour do that, one is sensitive to the argument order, the other is not. The existing docstring leaves ambiguity as to which type the values will be converted to.

The exact floating point value is not the same, nether depth can precisely represent 10.1. To the depth which IEEE754 floats are completely accurate the value is the same and that's what fmt will show:

	thirtyTwo := float32(10.1)
	sixtyFour := float64(10.1)
	fmt.Println(strconv.FormatFloat(float64(thirtyTwo), 'f', 64, 32)) // 10.1000003814697265625000000000000000000000000000000000000000000000
	fmt.Println(strconv.FormatFloat(sixtyFour, 'f', 64, 64))          // 10.0999999999999996447286321199499070644378662109375000000000000000
	fmt.Println(thirtyTwo)                                            // 10.1
	fmt.Println(sixtyFour)                                            // 10.1

play

I think there are two options here:

1. Revert assert: Fix EqualValues to handle overflow/underflow #1531 and change the docstring to:

   EqualValues asserts that two objects are equal or convertible to the actual type and equal, even if this will cause expected to overflow or its precision to change.

2. Decide the current behaviour is correct and change the docstring to:

   EqualValues asserts that two objects are equal or convertible to the larger type and equal.

I lean towards 1.

@vispariana and @arjunmahishi as the original reporter and fixer of the overflow bug, what are your thoughts?

[arjunmahishi]

Reverting #1531 will leave us with two bugs instead of one right? (overflow and precision). And like you pointed out, the old code still fails the assertion for

{float32(10.1), float64(10.1), true}

---

For that reason, I am more inclined towards option 2.

[vlasn]

Would it make sense to then instead introduce s.FloatsApproximatelyEqual (or equivalent) or is it too niche?

[arjunmahishi]

Would it make sense to then instead introduce s.FloatsApproximatelyEqual (or equivalent) or is it too niche?

Ya. I was thinking along the same lines. The user could decide on a tolerance for the delta. But this need not be exposed as a specific functionality in this library. As a user, I would round off the values before doing the assertion instead. The would be more deterministic.

[dolmen]

@vlasn wrote:

Would it make sense to then instead introduce s.FloatsApproximatelyEqual (or equivalent) or is it too niche?

We already have:

• InDelta
• InEpsilon

[Izzette]

What about an EqualsWithFloatsAreInDelta and EqualsWithFloatsAreInEpsilon for deep-equals with floating point fields ... anyone have better naming ideas?