mock: cleanup argument matching API about testify HOT 7 OPEN

@andrewwillette I expected #1571 to be merged before I start working on the implementation of the refactor.

@brackendawson Could you review #1571?

from testify.

I support making these changes because:

• the public function names and parameters don't change, so unless mock consumers are doing some complex things w/ the return types (which some may be doing), it won't affect them
• the return types are consolidated to single ArgumentMatcher type, easing cognitive load to understand code
• some values which need not be implementable outside the package are now private

My notes:

func AnythingOfType(t string) AnythingOfTypeArgument {

would change to

func AnythingOfType(string) ArgumentMatcher

func IsType(t interface{}) *IsTypeArgument

would change to

func IsType(interface{}) ArgumentMatcher

I won't copy/paste the changes to Anything, MatchedBy, or FunctionalOptions signatures, but they all share property of consolidating different return types to your proposed ArgumentMatcher interface. I think this is desirable, less cognitive load on programmer to understand.

I also like that the ArgumentMatcher interface has the privately defined matchesArg, making it so only the mock package can support this return type.

from testify.

@dolmen , in an effort to review this proposal more, I worked to implement the changes as specified. Link to PR towards my fork.

The item of interest is here. With matchesArg as described,

type ArgumentMatcher interface {
    matchesArg(arg interface{}) bool
}

We miss the particular reason it failed in the functionalOptionsArgument switch case matchesArg call. I looked into having matchesArg(arg interface{}) (bool, string), where the output string is also returned. The complexity that introduced didn't make it an obviously better choice.

I don't think this is reason against the proposal. I still support it. I wanted to bring up given my effort, though.

You might say that the internal Diff implementation would be somewhat (a lot?) different from current with the new ArgumentMatcher interface. In which case, I think it's valuable to draw attention to here.

from testify.

@snirye Comments welcome as you are a user of mock.FunctionalOptions.

from testify.

@nbaztec Comments welcome as you submitted mock.FunctionalOptions.

from testify.

@andrewwillette You should mark your PR as draft: GitHub doc

from testify.

I like this proposal. mock.Anything has annoyed me every time I see it for a very long time. My concern is that this does change exported functions' return types.

Could anyone have reasonably encoded the function signature as a type? In a tabled test's field for example. Because all of the functions being changed have parameters I can't contrive a sane example of this being done.

Could the returned type be encoded in someone's test? Yes:

package kata_test

import (
	"testing"

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

type myMock struct {
	mock.Mock
}

func (m *myMock) Method(arg any) {
	m.Called(arg)
}

func TestIfy(t *testing.T) {
	for name, test := range map[string]struct {
		arg          any
		expectedType mock.AnythingOfTypeArgument
	}{
		"string": {"cheese", mock.AnythingOfType("string")},
		"int":    {6, mock.AnythingOfType("int")},
		"bool":   {true, NewAnythingOfType("bool")}, // Oh no
	} {
		t.Run(name, func(t *testing.T) {
			m := &myMock{}
			m.Test(t)
			defer m.AssertExpectations(t)
			m.On("Method", test.expectedType).Return().Once()
			m.Method(test.arg)
		})
	}
}

type ArgumentMatcher interface{}

func NewAnythingOfType(t string) ArgumentMatcher {
	return mock.AnythingOfType(t)
}

Perhaps do all of the refactor, including making the existing types implement the ArgumentMatcher interface, but leave the factory functions returning the existing types in v1?

Or could the existing types be moved and their previous definitions be made an alias of mock.ArgumentMatcher? That would break mock.AnythingOfTypeArgument("string").

from testify.