Breaking change introduced in #558

[iamhopaul123]

Actual behavior A clear and concise description of what the bug is.

After #558, it forces a check for the number of argument for Do function. However, it doesn't work for variadic params. For example we have

UpdateAndWait(name, template string, opts ...stackset.CreateOrUpdateOption) error

and in the unit test we check

m.EXPECT().UpdateAndWait(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
	Return(nil).
	Do(func(_, template string, _, _, _, _, _ stackset.CreateOrUpdateOption) {
		configToDeploy, err := stack.AppConfigFrom(&template)
		require.NoError(t, err)
		require.ElementsMatch(t, []string{"firsttest"}, configToDeploy.Services)
		require.Empty(t, configToDeploy.Accounts, "config account list should be empty")
		require.Equal(t, 2, configToDeploy.Version)
	})

simply there's no way for us to use Do to validate one of the variadic param we pass.

Additional Information

• gomock mode (reflect or source):
• gomock version or git ref: v1.6.0
• golang version: go1.16.4 darwin/amd64

[codyoss]

Thank you for the report. Will get a hot fix out for this sometime this weekend hopefully. If this is affecting you please continue to use 1.5 for now. Sorry for the inconvenience!

[codyoss]

Just got a chance to looks at this a little more. Although this is a behavior change and validation is more strict I think this better aligns with what the original intention for Do was. The func passed into do should have the same signature as the function being mocked. In the example provided the code could be refactored to:

m.EXPECT().UpdateAndWait(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
	Return(nil).
	Do(func(_, template string, _ ...stackset.CreateOrUpdateOption) {
		configToDeploy, err := stack.AppConfigFrom(&template)
		require.NoError(t, err)
		require.ElementsMatch(t, []string{"firsttest"}, configToDeploy.Services)
		require.Empty(t, configToDeploy.Accounts, "config account list should be empty")
		require.Equal(t, 2, configToDeploy.Version)
	})

This should make the validation pass and now the signature exactly matched the method being mocked. The function supplied to Do should not match the args passed to the mock, but the original function being mocked. If I am missing something or there are other use-cases that are broken though please let me know.

[efekarakus]

Thanks for the reply @codyoss !
I think that makes sense, we can definitely refactor the tests if needed to continue receiving updates for the library. We haven't seen other scenarios that caused regressions.
However, it is a bit inconvenient. I'm not familiar enough with the reflect pkg but if there is a way to limit the number of args validation up to varargs that would be backwards-compatible I think.

[codyoss]

@efekarakus There is a way to check for variadic functions, yes. There could be special validation on if a function is variadic to do just a little to no validation but I am unclear if this is the best decision to move forward with. I think I would rather lean toward the mock code being written being more correct from the onset. This also keeps the semantics of the func passed to Do the same as a realy impl -- working with a slice of elements and not individual elements. Simplifying the example above(playground):

type Foo interface {
   Bar(string, ...int)
}

type ValidImpl struct {}
func (ValidImpl) Bar(string, ...int) {}

type InvalidImpl struct {}
func (InvalidImpl) Bar(string, int, int, int) {}

func DoSomething(Foo) {}
func DoSomethingFunc(func(string, ...int)) {}

func main() {
	DoSomething(ValidImpl{})
	DoSomethingFunc(ValidImpl{}.Bar)
	DoSomething(InvalidImpl{})
	DoSomethingFunc(InvalidImpl{}.Bar)
        // Output: cannot use InvalidImpl{} (type InvalidImpl) as type Foo in argument to DoSomething:
	// InvalidImpl does not implement Foo (wrong type for Bar method)
	//	have Bar(string, int, int, int)
	//	want Bar(string, ...int)
        // cannot use InvalidImpl{}.Bar (type func(string, int, int, int)) as type func(string, ...int) in argument to DoSomethingFunc
}

You would not expect the above code to compile, so I am am not sure we should allow a similar case with Do to pass validation. Thoughts?

[efekarakus]

Sounds good! We fixed it from our end :)

[codyoss]

Closing this issue as by design, please see my comments above.

[jeffreydwalter]

Thanks for making breaking changes in a minor version... Shame on you guys for breaking your promise.