Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

feat validate Do & DoReturn args #558

Merged
merged 1 commit into from
May 14, 2021
Merged

feat validate Do & DoReturn args #558

merged 1 commit into from
May 14, 2021

Conversation

codyoss
Copy link
Member

@codyoss codyoss commented May 14, 2021

Previous behavior was a panic would happen if the number of args
did not match. Now the test should fail more gracefully and allow
for better debugging.

Fixes: #71

Previous behavior was a panic would happen if the number of args
did not match. Now the test should fail more gracefully and allow
for better debugging.

Fixes: #71
@codyoss codyoss merged commit 82ce4a7 into golang:master May 14, 2021
@codyoss codyoss deleted the gm71 branch May 14, 2021 19:07
@bconway
Copy link
Contributor

bconway commented Jun 11, 2021

Apologies, my reflection-fu is a little weak. Does this correctly handle variadic functions? I have a bunch of DoAndReturn with throw-away input in the form of .DoAndReturn(func(_ ...interface{}) error that worked without issue in 1.5.0 and now throw:

wrong number of arguments in DoAndReturn func for *notifier.Mockeventer.Create: got 1, want 2

in 1.6.0.

Or perhaps this is a Hyrum’s Law thing?

@codyoss
Copy link
Member Author

codyoss commented Jun 11, 2021

@bconway Would you please open an issue with a simple example for discussion. This may be too strict in the case of varargs... need to take a closer look.

@efekarakus
Copy link

Hi! We also encountered the same behavior as @bconway on 1.6.0: #571 which broke our tests

codyoss pushed a commit that referenced this pull request Jun 12, 2021
… and into function commentary (#572)

Regardless of the result of #558, this is not a new requirement (I believe), and the location of it in the example code was easy to miss.

The `Do` version excludes a reference to the output argument count, as the returns are ignored.
timebertt added a commit to timebertt/gardener that referenced this pull request Jun 14, 2021
There were a lot of DoAndReturn usages in our tests that didn't correctly use the
client func signatures. With golang/mock#558 this is now validated and tests
fail, if the signature of the provided func literal is not the same as the one of
the real func. This commit fixes our wrong usages to comply with the newest
gomock behavior.
rfranzke pushed a commit to gardener/gardener that referenced this pull request Jun 14, 2021
* Vendor golang/mock@v1.6.0

* Fix wrong DoAndReturn usages

There were a lot of DoAndReturn usages in our tests that didn't correctly use the
client func signatures. With golang/mock#558 this is now validated and tests
fail, if the signature of the provided func literal is not the same as the one of
the real func. This commit fixes our wrong usages to comply with the newest
gomock behavior.
@schmurfy
Copy link

schmurfy commented Jul 8, 2021

I have the same issue, I reverted back to 1.5.0, variadic arguments are useful when you don't care about the argument themselves but want to do something when called.

@codyoss
Copy link
Member Author

codyoss commented Jul 8, 2021

@schmurfy Please see my comments on #571

@schmurfy
Copy link

@codyoss
I really not sure I agree with this being by design but I will survive, using Do(...interface{}) is a good way to clearly state in your test:
I don't give a sh*t about the arguments and they are irrelevant in this context

When you see it that way having variadic arguments does have a meaning, it is not just there because the author was lazy.

In one of my tests using it calling a method will trigger two parallel call to an external interface, I have a waitgroup and use Do to watch the external calls simply calling wg.Done() in them.

omertuc added a commit to omertuc/assisted-installer that referenced this pull request Feb 9, 2022
golang/mock@82ce4a7
(golang/mock#558)
introduced a breaking change to how go-mock works, this broke the dry-mode mock.
This commit fixes it

Also replaces ginkgo with a simple logger for more clear errors when
mocks go wrong
openshift-merge-robot pushed a commit to openshift/assisted-installer that referenced this pull request Feb 10, 2022
* NO-ISSUE: Swarm fix - controller looks at wrong env var to cluster hosts config

* NO-ISSUE: Fix go-mock 1.5.0->1.6.0 backwards compat issue

golang/mock@82ce4a7
(golang/mock#558)
introduced a breaking change to how go-mock works, this broke the dry-mode mock.
This commit fixes it

Also replaces ginkgo with a simple logger for more clear errors when
mocks go wrong
krgostev pushed a commit to krgostev/gardener that referenced this pull request Apr 21, 2022
* Vendor golang/mock@v1.6.0

* Fix wrong DoAndReturn usages

There were a lot of DoAndReturn usages in our tests that didn't correctly use the
client func signatures. With golang/mock#558 this is now validated and tests
fail, if the signature of the provided func literal is not the same as the one of
the real func. This commit fixes our wrong usages to comply with the newest
gomock behavior.
krgostev pushed a commit to krgostev/gardener that referenced this pull request Jul 5, 2022
* Vendor golang/mock@v1.6.0

* Fix wrong DoAndReturn usages

There were a lot of DoAndReturn usages in our tests that didn't correctly use the
client func signatures. With golang/mock#558 this is now validated and tests
fail, if the signature of the provided func literal is not the same as the one of
the real func. This commit fixes our wrong usages to comply with the newest
gomock behavior.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hard to debug when a Do(fn) function no longer matches its mock signature
4 participants