Fix #71 Do signature change error msg #395

cvgw · 2020-02-02T17:29:26Z

Fixes #71

Description

Update the error handling for Call.Do and Call.DoAndReturn in the case where
the argument passed does not match expectations.

panic if the argument is not a function
panic if the number of input arguments do not match those expected by Call
panic if the types of the input arguments do not match those expected
by Call

Call.DoAndReturn has additional validations on the return signature

panic if the number of return arguments do not match those expected by
Call
panic if the types of return arguments do not match those expected by
Call

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests

Reviewer Notes

The code flow looks good.
Tests added.

Release Notes

Describe any changes here so maintainer can include it in the release notes, or delete this block.

- Update validations for Do + DoAndReturn; now panics when argument passed to Do is not a function or does not match the signature of the mocked function

cvgw · 2020-02-02T21:29:10Z

Open question about this PR:
Should we verify the return args? Should we verify them for both Do and DoAndReturn?

Note: This PR verifies the return args for both Do and DoAndReturn

codyoss · 2020-02-05T20:56:55Z

gomock/call_test.go

@@ -1,6 +1,7 @@
 package gomock

 import (
+	"reflect"


Could you add the copyright to this file. Eventually we need to go and get all the files caught up.

cvgw · 2020-02-06T16:46:11Z

I did some brief testing and found that gomock currently allows arguments to Do which do not match the return signature of the mocked method. I don't think we should start validating that as it will be a break change so I'm going to revert that.

However, gomock raises an obscure error when arguments to DoAndReturn do not match the return signature of the mocked method. So I will keep the additional validation there.

gomock/internal/validate/validate.go

codyoss · 2020-02-28T15:13:50Z

gomock/call.go

+			panic(fmt.Sprintf("DoAndReturn: %s", err))
+		}
+	default:
+		panic("DoAndReturn: argument must be a function")


Since we are now explicitly panicing I think we should update the doc string.

codyoss · 2020-02-28T15:39:48Z

gomock/internal/validate/validate.go

+}
+
+func interfaceArg(actualArg, expectedArg reflect.Type) error {
+	if !actualArg.ConvertibleTo(expectedArg) {


Honest question: Does this cover the implements?

Yes it does, if expectedArg is something like Stringer and actualArg is an instance of some type Foo which implements Stringer this line will evaluate to true.

I added another test case to make that more clear

Update the error handling for Call.Do and Call.DoAndReturn in the case where the argument passed does not match expectations. * panic if the argument is not a function * panic if the number of input arguments do not match those expected by Call * panic if the types of the input arguments do not match those expected by Call Call.DoAndReturn has additional validations on the return signature * panic if the number of return arguments do not match those expected by Call * panic if the types of return arguments do not match those expected by Call

codyoss · 2020-02-28T21:53:19Z

I was just reading over this issue. Made me question if we should be using panics at all. #158 (comment)

This PR is just making panics in this case more explicit. I wonder in the future if we should recover all of them and t.Fatal with the TestReporter we should have access to in the controller.

cvgw · 2020-02-28T23:07:58Z

I was just reading over this issue. Made me question if we should be using panics at all. #158 (comment)

This PR is just making panics in this case more explicit. I wonder in the future if we should recover all of them and t.Fatal with the TestReporter we should have access to in the controller.

I think that is totally valid but brings up back to the question of what to do for code executing outside of the main goroutine where we can't call t.Fatal. We should brainstorm about it some more.

horejsek · 2020-03-16T08:20:38Z

BTW this change was released as 1.4.2, changing only minor version. It breaks the current code. I think it should be released at least as 1.5.0.

Linus-Boehm · 2020-03-16T13:16:00Z

Can confirm that this is a breaking change. Gomock now interprets basic types wrong and complains that it wants an interface.

codyoss · 2020-03-16T14:45:24Z

Thank you for bring this up, that behavior was not intentional. Can you please provide a small example of code this breaks so we can add a better test.

horejsek · 2020-03-16T14:50:40Z

mock.EXPECT().Foo(gomock.Any()).DoAndReturn(func(...interface{}) error {return nil})

We didn't care about parameters so we left ...interface{} there which was OK and now it's panicing. The change was easy thought, just use real type instead.

codyoss · 2020-03-16T14:55:00Z

@Linus-Boehm was this a similar use case for you. I will be reverting for now and creating a new tag 1.4.3 until I have more time to evaluate how we want to move forward this this.

…413) This reverts commit f9b4ad1.

codyoss · 2020-03-16T15:08:17Z

New release with the change reverted: v1.4.3

Linus-Boehm · 2020-03-17T10:50:15Z

mock.EXPECT().Foo("some-expected-string").DoAndReturn(func(name interface{}) error {return nil})

It panics becuase "some-expected-string" is not an interface, so yeah that should be the same reason.

cvgw · 2020-03-27T17:15:00Z

mock.EXPECT().Foo("some-expected-string").DoAndReturn(func(name interface{}) error {return nil})

It panics becuase "some-expected-string" is not an interface, so yeah that should be the same reason.

@Linus-Boehm for clarity, what is the signature of method Foo on the type being mocked?

cvgw force-pushed the u/cvgw/fix-issue-71 branch 5 times, most recently from 3c6dd7e to 4fc27f1 Compare February 2, 2020 21:24

cvgw requested review from codyoss and minicuts February 2, 2020 21:28

cvgw force-pushed the u/cvgw/fix-issue-71 branch from 4fc27f1 to 7d8f57b Compare February 2, 2020 22:15

minicuts approved these changes Feb 3, 2020

View reviewed changes

codyoss reviewed Feb 5, 2020

View reviewed changes

cvgw force-pushed the u/cvgw/fix-issue-71 branch 2 times, most recently from 0501097 to 5aae07b Compare February 6, 2020 17:14

cvgw requested a review from minicuts February 6, 2020 17:17

codyoss reviewed Feb 21, 2020

View reviewed changes

gomock/internal/validate/validate.go Outdated Show resolved Hide resolved

cvgw force-pushed the u/cvgw/fix-issue-71 branch 2 times, most recently from 67f48ca to dbb6401 Compare February 28, 2020 00:34

cvgw requested a review from codyoss February 28, 2020 00:34

codyoss reviewed Feb 28, 2020

View reviewed changes

cvgw force-pushed the u/cvgw/fix-issue-71 branch from dbb6401 to e3d5d28 Compare February 28, 2020 17:05

cvgw requested a review from codyoss February 28, 2020 17:09

cvgw merged commit f9b4ad1 into master Feb 28, 2020

cvgw deleted the u/cvgw/fix-issue-71 branch February 28, 2020 23:08

codyoss added a commit that referenced this pull request Mar 16, 2020

Revert "Fix #71 Do + DoAndReturn signature change error msg (#395)" (#…

3a35fb6

…413) This reverts commit f9b4ad1.

cvgw mentioned this pull request May 8, 2020

Add tests for various Do/DoAndReturn calls #430

Merged

3 tasks

dependabot bot mentioned this pull request Mar 11, 2021

Bump github.com/golang/mock from 1.2.0 to 1.5.0 awslabs/amazon-ecs-local-container-endpoints#57

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #71 Do signature change error msg #395

Fix #71 Do signature change error msg #395

cvgw commented Feb 2, 2020 •

edited

Loading

cvgw commented Feb 2, 2020 •

edited

Loading

codyoss Feb 5, 2020

cvgw commented Feb 6, 2020

codyoss Feb 28, 2020

cvgw Feb 28, 2020

codyoss Feb 28, 2020

cvgw Feb 28, 2020

cvgw Feb 28, 2020

codyoss commented Feb 28, 2020

cvgw commented Feb 28, 2020

horejsek commented Mar 16, 2020

Linus-Boehm commented Mar 16, 2020 •

edited

Loading

codyoss commented Mar 16, 2020

horejsek commented Mar 16, 2020

codyoss commented Mar 16, 2020

codyoss commented Mar 16, 2020

Linus-Boehm commented Mar 17, 2020

cvgw commented Mar 27, 2020

Fix #71 Do signature change error msg #395

Fix #71 Do signature change error msg #395

Conversation

cvgw commented Feb 2, 2020 • edited Loading

cvgw commented Feb 2, 2020 • edited Loading

codyoss Feb 5, 2020

Choose a reason for hiding this comment

cvgw commented Feb 6, 2020

codyoss Feb 28, 2020

Choose a reason for hiding this comment

cvgw Feb 28, 2020

Choose a reason for hiding this comment

codyoss Feb 28, 2020

Choose a reason for hiding this comment

cvgw Feb 28, 2020

Choose a reason for hiding this comment

cvgw Feb 28, 2020

Choose a reason for hiding this comment

codyoss commented Feb 28, 2020

cvgw commented Feb 28, 2020

horejsek commented Mar 16, 2020

Linus-Boehm commented Mar 16, 2020 • edited Loading

codyoss commented Mar 16, 2020

horejsek commented Mar 16, 2020

codyoss commented Mar 16, 2020

codyoss commented Mar 16, 2020

Linus-Boehm commented Mar 17, 2020

cvgw commented Mar 27, 2020

cvgw commented Feb 2, 2020 •

edited

Loading

cvgw commented Feb 2, 2020 •

edited

Loading

Linus-Boehm commented Mar 16, 2020 •

edited

Loading