testify
testify copied to clipboard
stretchr
mock: cleanup argument matching API
Description
The mock package exposes public types that should have stayed implementation details:
const Anything = "mock.Anything": should be a constant of a private typefunc AnythingOfType(t string) AnythingOfTypeArgument:AnythingOfTypeArgumentshould have been made privatefunc IsType(t interface{}) *IsTypeArgument:IsTypeArgumentshould have been made privatefunc MatchedBy(fn interface{}) argumentMatcher:argumentMatcheris private (good) but should have been fully hidden behind an interface and not be exposed in the function signaturefunc FunctionalOptions(value ...interface{}) *FunctionalOptionsArgument:FunctionalOptionsArgumentshould have been made private
With #1441 a mitigation effort has been started by deprecating AnythingOfTypeArgument (released since v1.9.0). Unfortunately that mitigation can't be applied to the other cases.
Proposed solution
I propose to define the following private (can't be implemented outside of the package) interface:
type ArgumentMatcher interface {
matchesArg(arg interface{}) bool
}
and then to retrofit the types and functions to use that interface:
const Anything = anythingArgument{}
type anythingArgument struct{}
func (anythingArgument) matchesArg(interface{}) bool {
return true
}
func AnythingOfType(string) ArgumentMatcher
type AnythingOfTypeArgument = anythingOfTypeArgument
type anythingOfTypeArgument string
func (anythingOfTypeArgument) matchesArg(interface{}) bool
func IsType(interface{}) ArgumentMatcher
type isTypeArgument struct {
T reflect.Type
}
func (*isTypeArgument) matchesArg(interface{}) bool
func MatchedBy(fn interface{}) ArgumentMatcher
type matchedByArgument struct {
F reflect.Value
}
func (*matchedByArgument) matchesArg(interface{}) bool
func FunctionalOptions(value ...interface{}) ArgumentMatcher
type functionalOptionsArgument struct {
values []interface{}
}
func (*functionalOptionsArgument) matchesArg(interface{}) bool
Method Arguments.Diff will be rewritten (simplified) by using the ArgumentsMatcher interface (instead of the type switch that directly refer to each type).
I think that this plan can be implemented without waiting for a v2 as the proposed changes would not affect correct uses of the existing API.
Misc
This ticket will also track effort to provide fixes to downstream projects which have strong references to those mock implementation details:
- fix uses of
mock.AnythingOfTypeArgument - fix uses of
mock.IsTypeArgument - fix uses of
mock.FunctionalOptionsArgument
I support making these changes because:
- the public function names and parameters don't change, so unless
mockconsumers 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
ArgumentMatchertype, 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.
@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.
@andrewwillette I expected #1571 to be merged before I start working on the implementation of the refactor.
@brackendawson Could you review #1571?
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").
