assert: NotNil accepts non-nillable types and doesn't fail

[dnephin]

testify/assert/assertions.go

Lines 390 to 410 in b89eecf

	func NotNil(t TestingT, object interface{}, msgAndArgs ...interface{}) bool {
	if h, ok := t.(tHelper); ok {
	h.Helper()
	}
	if !isNil(object) {
	return true
	}
	return Fail(t, "Expected value not to be nil.", msgAndArgs...)
	}
	// isNil checks if a specified object is nil or not, without Failing.
	func isNil(object interface{}) bool {
	if object == nil {
	return true
	}
	value := reflect.ValueOf(object)
	kind := value.Kind()
	if kind >= reflect.Chan && kind <= reflect.Slice && value.IsNil() {
	return true
	}

isNil will always return false, so NotNil will always return true for int, string, struct, etc.

I would expect it to error with "invalid type".

[ernesto-jimenez]

@dnephin that sounds good. Do you want to issue a PR?

[devdinu]

	type pred func(interface{}) bool
	var x pred
	var ptr *int
	var i interface{}
	var m map[string]string
        var ch chan string

All the above will be nil, it could be a case where the function returns a value and we generally check its not nil. assert.NotNil(t, getSomething()). So all these are valid nil case.

I wouldn't mind adding a log, but failing a test, would make the contract differ.

[dnephin]

I wouldn't mind adding a log, but failing a test, would make the contract differ.

@devdinu the contract should be different. This is a bug, not a feature request.

The problem is this:

if kind >= reflect.Chan && kind <= reflect.Slice

If kind is not in that range, isNil returns false, so NotNil assumes it's not nil. However the value can't be nil, so it should be a panic, or a failed test.

Some examples:

	require.NotNil(t, "foo")
	require.NotNil(t, "")
	require.NotNil(t, 0)
	require.NotNil(t, 1)
	require.NotNil(t, mystruct{})

None of these fail the test. This is broken. #584 looks like the correct fix.

[ernesto-jimenez]

#584 would fail the following test which currently passes.

type something struct{}

func (s something) Error() string {
        return "fail"
}

func foo() error {
        return something{}
}

func TestNotNilWithError(t *testing.T) {
        err := foo()
        NotNil(t, err)
}

If we were to do something like this, we would need something that works with interface types like error.