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

v1.9.0 breaks float comparisons with EqualValues #1576

Open
vlasn opened this issue Mar 11, 2024 · 6 comments
Open

v1.9.0 breaks float comparisons with EqualValues #1576

vlasn opened this issue Mar 11, 2024 · 6 comments
Labels
assert.EqualValues About equality bug pkg-assert Change related to package testify/assert

Comments

@vlasn
Copy link

vlasn commented Mar 11, 2024

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.

@vlasn vlasn added the bug label Mar 11, 2024
@gordallott
Copy link

Same issue, but only after upgrading to go 1.22

@dolmen dolmen added pkg-assert Change related to package testify/assert assert.EqualValues About equality labels Mar 21, 2024
@brackendawson
Copy link
Collaborator

brackendawson commented Mar 22, 2024

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
Copy link
Collaborator

arjunmahishi commented Mar 29, 2024

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
Copy link
Author

vlasn commented Apr 4, 2024

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

@arjunmahishi
Copy link
Collaborator

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
Copy link
Collaborator

dolmen commented May 16, 2024

@vlasn wrote:

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

We already have:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert.EqualValues About equality bug pkg-assert Change related to package testify/assert
Projects
None yet
Development

No branches or pull requests

5 participants