Open dolmen opened 6 months ago
I support making these changes because:
mock
consumers are doing some complex things w/ the return types (which some may be doing), it won't affect themArgumentMatcher
type, easing cognitive load to understand codeMy 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.
@snirye Comments welcome as you are a user of mock.FunctionalOptions
.
@nbaztec Comments welcome as you submitted mock.FunctionalOptions
.
@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 You should mark your PR as draft: GitHub doc
@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")
.
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
:AnythingOfTypeArgument
should have been made privatefunc IsType(t interface{}) *IsTypeArgument
:IsTypeArgument
should have been made privatefunc MatchedBy(fn interface{}) argumentMatcher
:argumentMatcher
is private (good) but should have been fully hidden behind an interface and not be exposed in the function signaturefunc FunctionalOptions(value ...interface{}) *FunctionalOptionsArgument
:FunctionalOptionsArgument
should have been made privateWith #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:
and then to retrofit the types and functions to use that interface:
Method
Arguments.Diff
will be rewritten (simplified) by using theArgumentsMatcher
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:mock.AnythingOfTypeArgument
]()mock.IsTypeArgument
]()mock.FunctionalOptionsArgument
]()