Suggested improvement around variadic functions

[dlwyatt]

I've been looking over mockery's history behind this topic. Back in 2017, there was #123 , which introduced behavior to "unroll" variadic arguments when calls were made to the mock. You can read the PR for more details, but the gist of it was, people wanted to be able to write mock.On("MethodName", 1,2,3,4,5) instead of mock.On("MethodName", []interface{}{1,2,3,4,5}). The change in that PR did allow for that syntax to work, but the problem was that it broke the ability to pass a single value of mock.Anything to cover any number of arguments to the variadic parameter.

In #359, the --unroll-variadic=false flag was added to restore the original behavior, for people who really wanted that single mock.Anything to work, and didn't care about losing the other syntax.

What I'm proposing here may be a "best of both worlds" approach. The tests I'll list here are all working, and if anyone can think of other use cases that I haven't thrown at it yet, I'll include those as well. Essentially it inverts the approach used in #123 ; instead of "unrolling" the variadic arguments when the mocked function is called, instead "roll them up" in new wrappers around the On, AssertCalled and AssertNotCalled methods of a testify Mock. This means that you still get the nice syntax sugar of mock.On("Method", 1,2,3,4,5) without having to put things into slices yourself, and a single mock.Anything parameter will still match any number (zero or more) arguments passed to the variadic parameter. Here's the interface I worked against:

type SampleInterface interface {
	Sprintf(msg string, args ...interface{}) string
	Test(msg string) string
}

I deliberately included one non-variadic method signature in there to make sure those still work properly. My test code looks like this:

func TestVariadicWrappers(t *testing.T) {
	m := new(mocks.SampleInterface)
	m.On("Sprintf", "specific", "arguments").Return("Boo!")
	m.On("Sprintf", mock.Anything, mock.Anything).Return("Yay!")

	// deliberately passing in more than one arguments to the variadic parameter
	// this is what fails today, even though we mocked with mock.Anything
	s := m.Sprintf("I am a %s %s", "test", "two")
	assert.Equal(t, "Yay!", s)

	// Making sure things still work correctly with a non-variadic signature
	m.On("Test", "something").Return("Yay!")
	s = m.Test("something")
	assert.Equal(t, "Yay!", s)

	m.AssertCalled(t, "Test", "something")
	m.AssertCalled(t, "Sprintf", "I am a %s %s", "test", "two")

	// Also making sure calling with ZERO arguments to the variadic parameter works
	m.Sprintf("bagel")
	m.AssertCalled(t, "Sprintf", "bagel")

	m.AssertNotCalled(t, "Sprintf", "bagel", "with", "cream", "cheese")
	m.AssertNotCalled(t, "Sprintf", "donut")

	// Making sure the matchers are still working when we're not using mock.Anything
	s = m.Sprintf("specific", "arguments")
	assert.Equal(t, "Boo!", s)
}

You can see how I'm running through the paces of both variadic and non-variadic method signatures, and mocks that use mock.Anything or pass other values in. Mocks generated with today's code (whether --unroll-variadic was set to true or false) will all fail. But with this code, the tests all pass:

package mocks

import (
	"reflect"

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

// SampleInterface is an autogenerated mock type for the SampleInterface type
type SampleInterface struct {
	mock.Mock
}

// Sprintf provides a mock function with given fields: msg, args
func (_m *SampleInterface) Sprintf(msg string, args ...interface{}) string {
	ret := _m.Called(msg, args)

	var r0 string

	if rf, ok := ret.Get(0).(func(string, ...interface{}) string); ok {
		r0 = rf(msg, args...)
	} else {
		if rf, ok := ret.Get(0).(func(string, ...interface{}) string); ok {
			r0 = rf(msg, args...)
		} else {
			r0 = ret.Get(0).(string)
		}
	}

	return r0
}

// Test provides a mock function with given fields: msg
func (_m *SampleInterface) Test(msg string) string {
	ret := _m.Called(msg)

	var r0 string

	if rf, ok := ret.Get(0).(func(string) string); ok {
		r0 = rf(msg)
	} else {
		if rf, ok := ret.Get(0).(func(string) string); ok {
			r0 = rf(msg)
		} else {
			r0 = ret.Get(0).(string)
		}
	}

	return r0
}

func (_m *SampleInterface) On(methodName string, arguments ...interface{}) *mock.Call {
	arguments = _m.rollVariadic(methodName, arguments...)
	return _m.Mock.On(methodName, arguments...)
}

func (_m *SampleInterface) AssertCalled(t mock.TestingT, methodName string, arguments ...interface{}) bool {
	arguments = _m.rollVariadic(methodName, arguments...)
	return _m.Mock.AssertCalled(t, methodName, arguments...)
}

func (_m *SampleInterface) AssertNotCalled(t mock.TestingT, methodName string, arguments ...interface{}) bool {
	arguments = _m.rollVariadic(methodName, arguments...)
	return _m.Mock.AssertNotCalled(t, methodName, arguments...)
}

func (_m *SampleInterface) rollVariadic(methodName string, arguments ...interface{}) []interface{} {
	sig := _m.getMethodSignature(methodName)

	if !sig.IsVariadic() {
		return arguments
	}

	variadicIndex := sig.NumIn() - 1
	if len(arguments) == sig.NumIn() && arguments[variadicIndex] == mock.Anything {
		return arguments
	}

	newArgs := make([]interface{}, sig.NumIn())

	copy(newArgs, arguments[0:variadicIndex])

	if len(arguments) >= sig.NumIn() {
		newArgs[variadicIndex] = arguments[variadicIndex:]
	} else {
		newArgs[variadicIndex] = []interface{}(nil)
	}

	return newArgs
}

func (_m *SampleInterface) getMethodSignature(methodName string) reflect.Type {
	switch methodName {
	case "Sprintf":
		return reflect.TypeOf(_m.Sprintf)
	case "Test":
		return reflect.TypeOf(_m.Test)
	default:
		panic("Invalid method name")
	}
}

type mockConstructorTestingTNewSampleInterface interface {
	mock.TestingT
	Cleanup(func())
}

// NewSampleInterface creates a new instance of SampleInterface. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations.
func NewSampleInterface(t mockConstructorTestingTNewSampleInterface) *SampleInterface {
	mock := &SampleInterface{}
	mock.Mock.Test(t)

	t.Cleanup(func() { mock.AssertExpectations(t) })

	return mock
}

The significant changes from what mockery generates today are:

• The actual calls to the mocked functions behave as though --unroll-variadic were set to false; the arguments are passed straight on to mock.Called
• New wrappers are added to shadow calls to On, AssertCalled, and AssertNotCalled. These all call the underlying testify methods after first calling a new rollVariadic helper function.
• The rollVariadic function itself is static and would be the same in every generated mock file.
• A new getMethodSignature helper function is there to help rollVariadic know about the expected parameters associated with each method name. This function is dynamic, but easily generated based only on a list of method names in the mock.

If you like this approach, I'm happy to create a PR that will update the generator to actually produce this new code. The --unroll-variadic flag would be deprecated and have no further effect on the output, at that point.

[LandonTClipp]

This is a great proposal, thank you for the thought that went into this. There are a few thoughts I have on this:

1. I am trying to steer this project towards the type-safe Expecter methods. If we are to implement this fix, I would prefer to only implement it for the Expecter methods so that people are encouraged to abandon the old legacy style, which we are totally deprecating in v3.
2. If this behavior is fixed by using the Expecter methods, then I am inclined to purposefully not fix this for the legacy methods and inform people that the solution is to move to Expecters.

Could you confirm what the behavior of the Expecter methods is?

[dlwyatt]

The expecters need some work as well. The main difference is that you wouldn't need to wrap mock.On anymore since that's what the expecter already does. The wrappers around AssertCalled and AssertNotCalled are still useful, unless you have plans to make them inaccessible somehow.

I'm not really sure how you plan to force people to use them, though. All of the "legacy style" is just part of testify.Mock, not mockery itself. If you are going to completely hide the presence of testify.Mock features, you may as well not wrap that struct at all, and just make mockery a standalone package.

[dlwyatt]

Here's what the expecter calls would look like for a variadic method:

func (_e *SampleInterface_Expecter) Sprintf(msg interface{}, args ...interface{}) *SampleInterface_Sprintf_Call {
	var vararg interface{} = args
	if len(args) == 1 && args[0] == mock.Anything {
		vararg = args[0]
	}
	return &SampleInterface_Sprintf_Call{Call: _e.mock.On("Sprintf", msg, vararg)}
}

With that in place I was able to rewrite the tests so all the calls to mock.On used an expect instead:

func TestVariadicWrappersUsingExpecters(t *testing.T) {
	m := new(mocks.SampleInterface)

	m.EXPECT().Sprintf("specific", "arguments").Return("Boo!")
	m.EXPECT().Sprintf(mock.Anything, mock.Anything).Return("Yay!")

	// deliberately passing in more than one arguments to the variadic parameter
	// this is what fails today, even though we mocked with mock.Anything
	s := m.Sprintf("I am a %s %s", "test", "two")
	assert.Equal(t, "Yay!", s)

	// Making sure things still work correctly with a non-variadic signature
	m.EXPECT().Test("something").Return("Yay!")
	s = m.Test("something")
	assert.Equal(t, "Yay!", s)

	m.AssertCalled(t, "Test", "something")
	m.AssertCalled(t, "Sprintf", "I am a %s %s", "test", "two")

	// Also making sure calling with ZERO arguments to the variadic parameter works
	m.Sprintf("bagel")
	m.AssertCalled(t, "Sprintf", "bagel")

	m.AssertNotCalled(t, "Sprintf", "bagel", "with", "cream", "cheese")
	m.AssertNotCalled(t, "Sprintf", "donut")

	// Making sure the matchers are still working when we're not using mock.Anything
	s = m.Sprintf("specific", "arguments")
	assert.Equal(t, "Boo!", s)

	m.EXPECT().OneVariadic(mock.Anything).Return()

	m.OneVariadic(1, 2, 3, 4, 5)
	m.AssertCalled(t, "OneVariadic", 1, 2, 3, 4, 5)
}

I had a thought this morning on how I can break this a bit (right now the fact that my variadic parameters are all ...interface{} is hiding some potential problems), but I'll make sure it works in all cases if I'm putting together a PR.

[LandonTClipp]

I'm not really sure how you plan to force people to use them, though.

It's not that I will force people to use the expecters, but mockery in v3 will always add them and the project will officially recommend only using expecters. We'll still allow accessing the testify Mock object, but I don't want to add additional wrappers around it if there's a way we can get it to work with what already exists, if that makes sense? So I guess for your proposal, that would only axe the On wrapper, but not AssertCalled/AssertNotCalled.

[dlwyatt]

Your call. If this feature would be part of a 2.x release then the On wrapper should be part of it; you can always take it out later for v3.

[dlwyatt]

I spent some time tinkering with this tonight. It's a little bit more invasive than I had originally thought. Some of the tests currently in mockery's suite just won't work anymore, because they completely depend on the old "unroll" technique and how it passed things to the underlying testify Mock. Namely, you can't use mock.Anything or mock.AnythingOfType for elements inside of slices (including individual arguments passed to a variadic parameter). testify/mock only does its checks for mock.Anything/etc at the outermost layer; once it encounters a slice, it just passes it to reflect.DeepEqual, which will fail. Some of the old tests also tried to call mock.On using the raw slice values instead of using the variadic style; that also won't work anymore. If the function has a var arg, you must use the mock.On(1,2,3,4,5) style. (Non-issue for v3 since you'd be only interested in using the Expecters anyway.)

You can see the WIP for this at https://github.com/dlwyatt/mockery/pull/1/files if you're interested. I used the #538 branch as a starting point.

[dlwyatt]

I've submitted a PR to testify (stretchr/testify#1348) which will allow these new features to be added with fewer breaking changes to mockery behavior. We'll see if they accept it; my branch is currently pointed at my fork of testify for demonstration purposes. Assuming they merge my PR and cut a new release, then right now the only breaking change left in https://github.com/dlwyatt/mockery/pull/1/files is the ability to call the On / AssertCalled / AssertNotCalled methods and pass in the raw slice for the variadic parameter, instead of using the more natural syntax and letting mockery roll the arguments up onto a slice for you. I could add some logic to detect that and handle it, but there could be some really wonky corner cases where that winds up doing the wrong thing (where someone actually wants a slice containing another slice, something like that.) Pretty unlikely to come up in test code, but not impossible.