Open Antonboom opened 5 months ago
Do you mean to guard against users passing an untyped nil or guard against users typing a literal nil? Because the first thing that happens in current testify is the compiler infers that the type of the literal nil
is <nil>
. At runtime we can't differentiat the source code nil
from the source code interface{}(nil)
. So there's nothing testify can do to stop users typing nil
.
If you mean to guard against users comparing untyped nil (interface{}(nil)
) with typed nil interfaces (eg: error(nil)
), then you can't because any nil interface converted to interface{}
becomes untyped nil. Even this example misbehaves:
assert.Equal(t, error(nil), fmt.GoStringer(nil)) // true !
We could "fix" these cases using type parameters, compilation would fail if you tried to compare interface{}(nil)
with error(nil)
(or any other interface type). But literal nil
would still break it, the type is inferred at compile time.
Making Equal generic may not even be a breaking change (can someone please prove me wrong on that?). But I hesitate to say this issue justifies doing that if the users can break it again by just typing nil
.
guard against users typing a literal nil
Please, look at my examples above. With typed nil everything is ok (if I am not wrong, because I confused with your snippet 🙂).
At runtime we can't differentiat
Yep, I told about untyped nil
literal.
And I fill that we can reuse some code from Nil
assertion, is not it?
With typed nil everything is ok
If I understand you correctly, that's not true. Typed nil does not work with nil interfaces, as in my example. I don't know why you omitted nilInterface
from your "The right way is to use" section?
There's definitely issues here, but I think most are limitations. Can you show me just one example of incorrect behaviour, and state what the behaviour should be?
I don't know why you omitted nilInterface
because comparison with both nils works fine for interface (look at the comment near nilInterface
asserts).
Can you show me just one example of incorrect behaviour, and state what the behaviour should be?
sure
https://go.dev/play/p/hl1CMwT2921
var nilChan chan struct{}
fmt.Println(nilChan == nil) // true, OK
fmt.Println(nilChan == (chan struct{})(nil)) // true, OK
fmt.Println(assert.Equal(t, nil, nilChan)) // false, MUST BE TRUE
fmt.Println(assert.EqualValues(t, nil, nilChan)) // false, MUST BE TRUE
fmt.Println(assert.Exactly(t, nil, nilChan)) // false (Types expected to match exactly), OK
fmt.Println(assert.NotEqual(t, nil, nilChan)) // true, MUST BE FALSE
fmt.Println(assert.NotEqualValues(t, nil, nilChan)) // true, MUST BE FALSE
fmt.Println(assert.Equal(t, (chan struct{})(nil), nilChan)) // true, OK
fmt.Println(assert.EqualValues(t, (chan struct{})(nil), nilChan)) // true, OK
fmt.Println(assert.Exactly(t, (chan struct{})(nil), nilChan)) // true, OK
fmt.Println(assert.NotEqual(t, (chan struct{})(nil), nilChan)) // false, OK
fmt.Println(assert.NotEqualValues(t, (chan struct{})(nil), nilChan)) // false, OK
fmt.Println(assert.Nil(t, nilChan)) // true, OK
fmt.Println(assert.NotNil(t, nilChan)) // false, OK
Those assertions are all behaving correctly.
For the reasons I gave above, these three calls to Equal are identical at runtime:
var nilChan chan struct{}
var nilInterface interface{}
assert.Equal(t, nil, nilChan)
assert.Equal(t, interface{}(nil), nilChan)
assert.Equal(t, nilInterface, nilChan)
When a programmer passes the literal nil
into a parameter of type interface{}
; they are asking the compiler to create a interface{}(nil)
for them, this is not considered Equal to a chan struct{}(nil)
by testify or by reflect.DeepEqual
. Testify is working correctly.
If the programmer intended to assert that it was a nil chan then they should have written one of these:
assert.Equal(t, chan struct{}(nil), nilChan)
assert.Nil(t, nilChan)
Checking that the user is using the compiler properly is the job of a linter, and testifylint does an excellent job in this case.
There is no way that testify can ever know at runtime if the user wrote a literal in the source. Testify could solve this on Equal by using type parameters to ensure both arguments are the same type at compile time, a bit like:
func TestIfy(t *testing.T) {
var nilChan chan struct{}
Equal(t, nil, nilChan) // now true because type inference decided the literal `nil` must be type `chan struct{}`
}
func Equal[T any](t assert.TestingT, expected, actual T, msgAndArgs ...interface{}) bool {
return assert.Equal(t, expected, actual, msgAndArgs...)
}
But we can't control the error message which the compiler spits out if you do it wrong:
func TestIfy(t *testing.T) {
var nilChan chan struct{}
var nilInterface interface{}
Equal(t, nilInterface, nilChan)
}
# github.com/brackendawson/kata_test [github.com/brackendawson/kata.test]
/Users/bracken/src/github.com/brackendawson/kata/kata_test.go:12:25: type chan struct{} of nilChan does not match inferred type interface{} for T
FAIL github.com/brackendawson/kata [build failed]
FAIL
Which is kind of saying "I expected an interface{}
but you gave me a chan struct{}
", because type inference picked the first argument.
EqualValues can't use type parameters to enforce the types being the same because they aren't supposed to be. According to the docs:
EqualValues asserts that two objects are equal or convertable to the same types and equal.
interface{}
is not convertible to chan struct{}
or visa versa because interfaces are not types they are type sets.
EqualValues is already behaving correctly.
Hi!
The using of
ObjectsAreEqual
-based functions with untypednil
doesn't make sense for any non-interface types, because ofexpected
andactual
are interfaces:As a consequence,
Equal
,EqualValues
andExactly
will always fail:And
NotEqual
andNotEqualValues
will always pass:The right way is to use typed
nil
:But this is verbose and we also see inconsistency of functions when working with
nilFunc
.Better to just use
Nil
/NotNil
:Full snippet: https://go.dev/play/p/ClGstUJWkYB
Example of bug/typo in testify: https://github.com/stretchr/testify/blob/db8608ed63f5bd9abdd03f669f5bb80569c114b6/mock/mock_test.go#L765
(
call.WaitFor
is(<-chan time.Time)(nil)
).Covered by testifylint#nil-compare.