Validate there is a problem by ccoVeille · Pull Request #1 · snopan/testify

ccoVeille · 2026-03-17T23:49:31Z

This is a follow-up of

Fix: float32 and float64 comparisions with EqualValues stretchr/testify#1863

I found issue during the review of the PR

Copilot

Pull request overview

Adds targeted tests to validate edge-case behavior and invariants for ObjectsAreEqual / ObjectsAreEqualValues in the assert package, as a follow-up to prior float-comparison work.

Changes:

Extend TestObjectsAreEqual with NaN/Inf/signed-zero cases and add a symmetry assertion.
Refine TestObjectsAreEqualValues float32/float64 coverage (precision, overflow boundary, NaN/Inf, signed zero) and add a symmetry assertion.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

assert/assertions_test.go

ccoVeille · 2026-03-17T23:58:28Z

assert/assertions_test.go

-		{float32(10.123456), float64(10.12345678), false},
+
+		// cases for float32/float64 comparison, which should not be equal due to precision differences
 		{float32(1.0 / 3.0), float64(1.0 / 3.0), false},


this one won't pass now, as being

ccoVeille · 2026-03-18T00:10:41Z

Here the PR I created to showcase the issues I think that are present in the PR you opened @snopan

Please note, I might be wrong :P

snopan

Hey @ccoVeille,

Thanks for the PR and the work.

I've left some comments, I'm not sure if InEpsilon logic is the right approach because there seems to be some cases where this fails.

Also I think {float32(10.12345600), float64(10.123456789), true} should not be true, because float64 here is representing more decimal digits which is a different number to float32 value. So InDelta might also be the the wrong approach because it would allow this test case to pass?

I think rounding is correct because we only want to allow {float32(10.12345600), float64(10.123456), true}. But we do need to address the close to INF problem.

What are your thoughts?

Happy with most of the other changes not related to InEpsilon logic approach.

snopan · 2026-03-18T05:41:28Z

assert/assertions.go

-	}
-
-	return newValue == toValue.Interface()
+	if actual != actual || expected != expected {


We can probably use math.IsNaN() to be more clear?

Also this is ran in calcRelativeError(), do we need to call it again earlier?

We can probably use math.IsNaN() to be more clear?

we couldn't as IsNan expects a float64, that's why I did this

snopan · 2026-03-18T05:57:08Z

assert/assertions.go

+	// We need to convert the smaller type to the larger type and then compare.
+
+	smallestTypeValue, largestTypeValue := expectedValue, actualValue
+	if actualType.Size() < expectedType.Size() {


Should we add a comment saying
actual value is smaller than expected value, converting actual value to expected value type
I know it's kinda implicit but might be clearer to just summarize what this if clause is doing

Yes, we could. But I think my PR should be closed, and only the unit tests I'm adding should be kept

#2

snopan · 2026-03-18T06:18:00Z

assert/assertions.go

+	if smallestTypeValue.Kind() == reflect.Float32 && largestTypeValue.Kind() == reflect.Float64 {
+		a, b := expected, actual
+		if af, aIsNumber := toFloat(expected); aIsNumber && af == 0 {
+			// avoid division by zero in calcRelativeError
+			a, b = b, a
+		}
+
+		epsilon, err := calcRelativeError(a, b)
+		if err != nil {
+			return false
+		}
+
+		// The threshold of 1e-6 is somewhat arbitrary, but it is a common choice for comparing floating point numbers.
+		return epsilon <= 1e-6
+	}


I found an edge case that fails with this relative error approach

{float32(10.123457), float64(10.123456), false}

Here's a playground link to show why

Looks like the problem is when float32(10.123457 gets converted to float64, it becomes slightly less 10.123456954956055 which now is within the epsilon requirement

It seems to fail also with

{float32(10.12345), float64(10.12346), false},

I think my PR should be closed, and only the unit tests I'm adding should be kept

#2

snopan · 2026-03-18T06:25:26Z

assert/assertions_test.go

+		{float32(10.1234), float64(10.1235), false},
+
+		// so anything beyond 7 decimal digits should be ignored when comparing at float32 precision, so these should still be equal
+		{float32(10.12345600), float64(10.123456789), true},


In my opinion, this shouldn't be true because they aren't actually the same value. We should say it equates if float64 value has the same decimal digits as float32. If float64 has more decimals past what float32 can represent, they are inherently different values.

I think my PR should be closed, and only the unit tests I'm adding should be kept

#2

snopan · 2026-03-18T06:25:58Z

assert/assertions_test.go

 		{complex128(1e+10 + 1e+10i), complex64(1e+10 + 1e+10i), true},
 		{complex64(1e+10 + 1e+10i), complex128(1e+10 + 1e+10i), true},
+
+		{float32(1.0 / 3.0), float64(1.0 / 3.0), true},


Same opinion on here, the float64 actually represents more decimal digits than float32, they are inherently not the same value.

I think my PR should be closed, and only the unit tests I'm adding should be kept

#2

snopan · 2026-03-18T06:49:28Z

I have a potential solution for values approaching INF I think.

We can split the integer and decimal part of the float, then do rounding of the decimal before combining them back again

thirtyTwo := float32(10.123456)
sixtyFour := float64(10.1234560000)
convertedThirtyTwo := float64(thirtyTwo)

integerPart := math.Floor(convertedThirtyTwo)
decimalPart := convertedThirtyTwo - integerPart

scale := math.Pow(10, 6)
roundDecimal := math.Round(decimalPart*scale) / scale

playground

ccoVeille · 2026-03-18T07:05:03Z

Yes, I think you are for this what you said here

stretchr#1863 (comment)

In fact, your code seems OK based on your analysis of what is expected.

So my tests are good addition your PR.

But my fix is not good.

But what is unclear now to me is that I don't know if what you coded is what is expected.

Maybe there is something to talk about in the original issue or your PR

ccoVeille · 2026-03-18T07:28:51Z

I close my PR in favor of #2

Copilot AI review requested due to automatic review settings March 17, 2026 23:49

Copilot started reviewing on behalf of ccoVeille March 17, 2026 23:49 View session

Copilot AI reviewed Mar 17, 2026

View reviewed changes

assert/assertions_test.go Outdated Show resolved Hide resolved

assert/assertions_test.go Show resolved Hide resolved

ccoVeille force-pushed the pr-1863-snopan branch from 8fc8869 to acfb965 Compare March 17, 2026 23:58

ccoVeille commented Mar 17, 2026

View reviewed changes

Add missing unit tests to spot the issues in the PR

19cf96a

ccoVeille force-pushed the pr-1863-snopan branch from acfb965 to 215a623 Compare March 18, 2026 00:05

Fix the issue with float32 and float64 comparison

d62101f

ccoVeille force-pushed the pr-1863-snopan branch from 215a623 to d62101f Compare March 18, 2026 00:08

snopan reviewed Mar 18, 2026

View reviewed changes

ccoVeille closed this Mar 18, 2026

ccoVeille deleted the pr-1863-snopan branch March 19, 2026 06:56

Conversation

ccoVeille commented Mar 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccoVeille commented Mar 18, 2026

Uh oh!

snopan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccoVeille Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccoVeille Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccoVeille Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snopan commented Mar 18, 2026

Uh oh!

ccoVeille commented Mar 18, 2026

Uh oh!

ccoVeille commented Mar 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

snopan left a comment •

edited

Loading

ccoVeille Mar 18, 2026 •

edited

Loading

ccoVeille Mar 18, 2026 •

edited

Loading

ccoVeille Mar 18, 2026 •

edited

Loading