Closed joshblum closed 2 years ago
Ah I caught up on the discussion in these issues a bit more and see this has already been reported and closed as 'by design'. The original issue #71 was to improve an error message, I think that breaking compatibility is a worse outcome in the end. If #601 won't be accepted as-is, perhaps we can just improve the error messaging to save other folks the github dive?
In this case I think we should improve error messaging. As detailed in #571, although this was a breaking change it fixed the semantics for the Do/DoReturn API to actually match. Another thing that could possibly done in addition I think is a new matcher like Any but works in place of variadic arguments. This way things could still be explicit and have more flexibility for those that want to opt-in to that. Thoughts?
@codyoss I'll update #601 to improve the error messaging. I don't have a strong opinion either way about adding a new matcher!
Actual behavior A clear and concise description of what the bug is.
Do
andDoReturn
can report a false positive for argument matching for variadic functions:wrong number of arguments in Do func for (...) got 1, want 3
Expected behavior A clear and concise description of what you expected to happen.
no error
To Reproduce Steps to reproduce the behavior
From https://github.com/golang/mock/pull/558 / https://github.com/golang/mock/issues/71
I think that the argument validation checks should change from
if c.methodType.NumIn() != ft.NumIn() {
toif c.methodType.NumIn() != ft.NumIn() && !ft.IsVariadic() {
Happy to supply a PR if this is the desired fix.
Additional Information
https://github.com/keybase/client/blob/master/go/kbfs/libkbfs/subscription_manager_test.go#L153-L155 has an example of this failure case.
Triage Notes for the Maintainers