Open vlasn opened 3 months ago
Same issue, but only after upgrading to go 1.22
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
I think there are two options here:
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.
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?
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.
Would it make sense to then instead introduce s.FloatsApproximatelyEqual
(or equivalent) or is it too niche?
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.
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 ofreflect.ValueOf(aFloat32).Convert(float64Type).Interface()
against the actualfloat64
-Convert
seems to misinterpret the underlying bits and the interface of the former operand comes out to eg.10.100000381469727
instead of10.1
Step To Reproduce
Run the following suite:
Alternatively, inject the following case into
TestObjectsAreEqualValues
:Expected behavior
I'd expect the
EqualValues(float32(10.1), float64(10.1))
assertion to pass.Actual behavior
The abovementioned assertion fails.