testifylint icon indicating copy to clipboard operation
testifylint copied to clipboard

float-compare: detect and warn on zero delta/epsilon values in `InDelta`/`InEpsilon`

Open oschwald opened this issue 1 year ago • 3 comments

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 avatar Jan 03 '25 19:01 oschwald

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 avatar Jan 05 '25 05:01 Antonboom

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.

oschwald avatar Jan 05 '25 17:01 oschwald

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

ccoVeille avatar Jan 05 '25 17:01 ccoVeille