stretchr / testify

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

call.Unset() panics when call := mockObj.On("Foo", mock.AnythingOfType("foo")) #1599

Open jeandeducla opened 6 months ago

jeandeducla commented 6 months ago

Description

When you use mock.AnythingOfType("foo") in On(...) and then try to call Unset() on the resulting call, the code panics.

Step To Reproduce

Here is a simple snippet that reproduces the issue:

import (
    "testing"

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

type Foo struct {
    doer Doer
}

type Doer interface {
    DoSomething(number int) (bool, error)
}

func (f Foo) Do(number int) (bool, error) {
    return f.doer.DoSomething(number)
}

type MyMockedObject struct {
    mock.Mock
}

func (m *MyMockedObject) DoSomething(number int) (bool, error) {
    args := m.Called(number)
    return args.Bool(0), args.Error(1)
}

func TestSomething(t *testing.T) {
    testObj := new(MyMockedObject)

    foo := Foo{doer: testObj}

    call := testObj.On("DoSomething", 2).Return(true, nil)
    _, _ = foo.Do(2)
    testObj.AssertExpectations(t)
    call.Unset() // Works!

    call = testObj.On("DoSomething", mock.AnythingOfType("int")).Return(true, nil)
    _, _ = foo.Do(2)
    testObj.AssertExpectations(t)
    call.Unset() // panics...
}

Expected behavior

I expect to be able to call Unset() on a call that has been mocked with mock.AnythingOfType (or did I miss something on how to use the API? Happy to get feedback here :-))

Actual behavior

When calling Unset() on a call that has been mocked with mock.AnythingOfType, it panics.

jeandeducla commented 6 months ago

from locally debugging, I have the impression the call to Diff in Unset is not correctly comparing the arguments here; it does not cover the case of actual being a anythingOfTypeArgument. Locally I did that:

              switch expected := expected.(type) {                                                                                                 
              case anythingOfTypeArgument:
                  // type checking
~                 switch actual := actual.(type) {
~                 case anythingOfTypeArgument:     
~                     if string(actual) != string(expected) {     
~                         differences++     
+                     }     
+                 default:     
+                     if reflect.TypeOf(actual).Name() != string(expected) && reflect.TypeOf(actual).String() != string(expected) {     
+                         // not match     
+                         differences++     
+                         output = fmt.Sprintf("%s\t%d: FAIL:  type %s != type %s - %s\n", output, i, expected, reflect.TypeOf(actual).Name(), actu
+                     }     
                  }                                                                        

which does the trick.

Happy to make a PR

mavvkel commented 1 month ago

Suffering from the same bug when trying to mock argument of type *context.valueCtx. Any updates on this topic? @jeandeducla

brackendawson commented 1 month ago

Aaah Unset, how I lothe thee.

Unset is rather inappropriately using mock.Arguments.Diff to find itself in ExpectedCalls. Diff is intended to match the code under test's calls against ExpectedCalls, so it works at finding Calls in ExpectedCalls when their arguments are values, such as On("Method", 59). But it cannot be made to work for anythingOfTypeArgument without also making that into an argument that matches when calling the mock. I dislike this already but it's made worse as anythingOfTypeArgument is exported via a type alias.

This bug also occurs with the exported IsTypeArgument from mock.IsType().

This bug also occurs with argumentMatcher from mock.MatchedBy() which is impossible to compare as it contains a function. So no modification to Diff can fix this problem.

The best workaround is to never use Unset, instead just don't set it in the first place.