equal-values: check EqualValues vs bigger type, and more

[ccoVeille]

Following stretchr/testify#1593 opened by Braken

Question for @Antonboom and @brackendawson:

Do you think there could have a need to detect and report something with testifylint?

I only surfaced the issue reported, I'm unsure I understood everything. But I wanted to raise the point here in case it could be a good thing to detect

[brackendawson]

The intention of the assertion is to compare the values of convertible types, it's an abomination of an assertion and an affront to the concept of "Equal". It's also rife for unintended errors with floating point precision and overflows. The best thing a linter could do is to tell you not to use it, here's a few examples:

Unnecessary use of EqualValues 1

❌

expected := int(7)
actual := GiveMeIntSeven()
assert.EqualValues(t, expected, actual)

✅ Just use Equal

expected := int(7)
actual := GiveMeIntSeven()
assert.Equal(t, expected, actual)

Unnecessary use of EqualValues 2

❌

expected := uint(7)
actual := GiveMeIntSeven()
assert.EqualValues(t, expected, actual)

✅ Change type of expectation and use Equal

expected := int(7)
actual := GiveMeIntSeven()
assert.Equal(t, expected, actual)

Avoid this false positive

⚠️ This person isn't testing for state, you can lead a horse to water but you can't make it drink.

actual1, actual2 := GiveMeTwoDifferentTypes()
assert.EqualValues(t, actual1, actual2)

[ccoVeille]

I'm fine with saying don't use it.

But what would be the legitimate usage of EqualValues ?

I assume that even if most of its usage is unnecessary (I loved the concept of being an affront to Equal 🤣), there are some use case where it would make sense, otherwise it won't have been created.

[brackendawson]

It would only be useful to assert that two values of different types are convertible and equal after conversion (which is sensitive to the direction of the conversion), and where you didn't know what either of the actual values would be when you wrote the test. So basically someone who isn't "testing for state" and instead has a "dynamic" test (shudders).

[brackendawson]

History: There was a bug where Equal had the behaviour of EqualValues, that was fixed but some people wanted their "SemiKinaEqual" assertion back: stretchr/testify#129

As I said, an affront to Equal.

[ccoVeille]

Thanks @brackendawson for giving context, I appreciate