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

Detect and warn on zero delta/epison values in InDelta/InEpsilon #211

Open
oschwald opened this issue Jan 3, 2025 · 3 comments
Open

Detect and warn on zero delta/epison values in InDelta/InEpsilon #211

oschwald opened this issue Jan 3, 2025 · 3 comments
Labels
enhancement New feature or request

Comments

@oschwald
Copy link

oschwald commented Jan 3, 2025

At my workplace, colleagues are circumventing the float-compare rules by using a zero value for the delta or epsilon. This defeats the purpose of InDelta and InEpsilon which are meant for floating-point comparisons with a meaningful tolerance.

Example of problematic code

require.InDelta(t, expectedValue, actualValue, 0)
assert.InEpsilon(t, expectedValue, actualValue, 0)
@oschwald oschwald changed the title Detect and warn on zero delta/epison values in \InDelta/InEpsilon Detect and warn on zero delta/epison values in InDelta/InEpsilon Jan 3, 2025
@Antonboom
Copy link
Owner

Hi, @oschwald

Thank you for request.

Why this is issue? If zero tolerance is meaningless, then it should be detected by corresponding test cases (with tolerance ajusting after).

Or the issue in detecting of needs to adding such cases?

And this will be false positive warning for tests where 0 is ok 🤔

@Antonboom Antonboom added the enhancement New feature or request label Jan 5, 2025
@oschwald
Copy link
Author

oschwald commented Jan 5, 2025

The stated reason of the float-compare lints is to avoid differences due to floating-point rounding errors. Using a delta or epsilon of zero does not accomplish this as it is subject to the same issues. I would argue that it is even worse than using assert.Equal or require.Equal as it makes the test case harder to read and the output more confusing.

Consider this example:

package main

import (
	"testing"

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

func TestAssert(t *testing.T) {
	x := -1e9
	y := 1e9
	z := 1e-9

	assert.Equal(t, x+y+z, x+(y+z))

	assert.InDelta(t, x+y+z, x+(y+z), 0)
}

These both fail as floating point math is not associative. Of the three, I find the message on the assert.Equal the easiest to read:

--- FAIL: TestAssert (0.00s)
    float_test.go:14:
        	Error Trace:	float_test.go:14
        	Error:      	Not equal:
        	            	expected: 1e-09
        	            	actual  : 0
        	Test:       	TestAssert
    float_test.go:16:
        	Error Trace:	float_test.go:16
        	Error:      	Max difference between 1e-09 and 0 allowed is 0, but difference was 1e-09
        	Test:       	TestAssert
    float_test.go:18:
        	Error Trace:	float_test.go:18
        	Error:      	Relative error is too high: 0 (expected)
        	            	        < 1 (actual)
        	Test:       	TestAssert
FAIL

In most cases when doing floating point comparisons, we do not what bitwise equality but want to ensure that the two numbers are within some non-zero delta of each other. This makes the tests less brittle as they are less likely to fail on code refactoring due to meaningless changes in order of the floating point operations.

@ccoVeille
Copy link
Contributor

I agree that if we report Equal when comparing float

We should report require.InDelta(t, expectedValue, actualValue, 0) to be invalid too

Now, it's unclear if InDelta with 0 should be reported to be:

  • invalid
  • to be replaced by Equal because here it's what it does

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants