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 type
func AnythingOfType(t string) AnythingOfTypeArgument: AnythingOfTypeArgument should have been made private
func IsType(t interface{}) *IsTypeArgument: IsTypeArgument should have been made private
func MatchedBy(fn interface{}) argumentMatcher: argumentMatcher is private (good) but should have been fully hidden behind an interface and not be exposed in the function signature
func FunctionalOptions(value ...interface{}) *FunctionalOptionsArgument: FunctionalOptionsArgument should 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 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.

@snirye

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

@nbaztec

@nbaztec Comments welcome as you submitted mock.FunctionalOptions.

@dolmen

@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

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

@andrewwillette

@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").

Navigation Menu

Repository navigation

mock: cleanup argument matching API #1578

Description

Proposed solution

Misc

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

mock: cleanup argument matching API #1578

Description

Description

Proposed solution

Misc

Activity

andrewwillette commented on Mar 20, 2024

dolmen commented on Mar 22, 2024

dolmen commented on Mar 22, 2024

andrewwillette commented on Mar 29, 2024

dolmen commented on Mar 29, 2024

dolmen commented on Apr 1, 2024

brackendawson commented on Apr 1, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions