stretchr / testify

A toolkit with common assertions and mocks that plays nicely with the standard library
MIT License
22.92k stars 1.58k forks source link

Mock panics when argument of nil type checked against AnythingOfType #1209

Open brackendawson opened 2 years ago

brackendawson commented 2 years ago

Reported by @cfiderer: https://github.com/stretchr/testify/issues/1155#issuecomment-1126159634

Raising a new issue because I think the fix to 1155 will not fix this.

This deadlocks in testify v1.7.4:

package kata

import (
    "testing"

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

type mockThing struct {
    mock.Mock
}

func (m *mockThing) Do(arg interface{}) string {
    return m.Called(arg).String(0)
}

func TestTestify(t *testing.T) {
    m := &mockThing{}
    defer m.AssertExpectations(t)
    m.On("Do", mock.AnythingOfType("string")).Return("Nae")
    m.Do(nil)
}
brackendawson commented 2 years ago

The problem isn't that passing actual of type nil into AnythingOfType matcher causes deferred AssertExpectations to deadlock per se, but simply that passing actual of type nil into an AnythingOfType matcher just causes a panic, and any panic while diffing argument will deadlock this testcase. We should fix the panic.

package kata

import (
    "testing"

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

type mockThing struct {
    mock.Mock
}

func (m *mockThing) Do(arg interface{}) string {
    return m.Called(arg).String(0)
}

func TestTestify(t *testing.T) {
    m := &mockThing{}
    // defer m.AssertExpectations(t)
    m.On("Do", mock.AnythingOfType("string")).Return("Nae")
    m.Do(nil)
}

I'm tempted to modify AnythingOfType to be a high order function that returns an argumentMatcher rather than being this alias for string, but this would be a breaking change. If anyone has a tabled test with a struct field of type mock.AnythingOfType, doing do would break them.

Instead I just need to make the code that matches AnythingOfType not panic when it is given a nil type.

cfiderer commented 2 years ago

Thanks for taking a closer look!

brackendawson commented 1 year ago

mock.IsType is affected by this too.