golang / mock

GoMock is a mocking framework for the Go programming language.
Apache License 2.0
9.26k stars 608 forks source link

Incorrect check for function arguments for Do and DoAndReturn in 1.6 #637

Closed shenmo3 closed 2 years ago

shenmo3 commented 2 years ago

Actual behavior

In mock version 1.6, PR https://github.com/golang/mock/pull/558 added check to compare the function arguments and log if it mismatches. This is causing problems in which the variadic (...) argument in the function is counted as a fixed argument.

func test(b ...interface{}) {}

func main() {
    typ := reflect.TypeOf(test)
    fmt.Printf("NumIn: %d", typ.NumIn()) // this prints NumIn: 1
}

Due to this, if the Do function is called without any arguments, the function will fail due to arguments mismatch, even though the no argument behavior is expected.

Expected behavior

Ignore variadic argument when counting expected function arguments. Note: This issue was not seen in 1.5. Most probably PR #558 caused this.

To Reproduce Steps to reproduce the behavior

N/A

Additional Information

Triage Notes for the Maintainers

codyoss commented 2 years ago

I believe this is fixed on HEAD by #601 iirc. Would you mind installing from source to see if that fixes the issue for you. We might just need to cut a new release.

shenmo3 commented 2 years ago

Thanks for the reply. I haven't tried, but it seems like #601 will still error out on variadic functions, and log says variadic cannot be used? So basically we cannot mock variadic function? It seems like #101 has support on variadic function matches, can't we do the same thing here?

A bit more context, we are using mock gen to mock k8s client methods like create, which has two argument and a variadic argument. After we tried to upgrade to 1.6 the test is breaking on Do(func(_ context.Context, _ runtime.Object)) {...}

codyoss commented 2 years ago

If it takes a variadic func so should the method you are passing to Do, even if it is unused. There is context in the previous PR and issues that are linked. I am closing this for now unless you can reproduce on HEAD. Planning a release in the next month that will have these changes.