ArgumentCaptor struct and tests by tylersammann · Pull Request #263 · golang/mock

tylersammann · 2019-02-07T21:16:34Z

Resolves Issue: #262

added ArgumentCaptor struct that composes Matcher
added constructors for ArgumentCaptor
included unit tests for ArgumentCaptor methods

The Captor() method takes a Matcher interface to make it as flexible as possible. AnyCaptor() is just there because it seems like the anyMatcher is probably going to be used with captors the most.

- added new ArguemntCaptor interface - added argumentCaptor implementation that composes Matcher - added constructors for ArgumentCaptor - included unit tests for ArgumentCaptor methods

- added sample service and dao interface - added unit test that demonstrates `AnyCaptor` usage

Argument Captor initial release

poy · 2019-09-27T20:11:58Z

What is the advantage of this over just using .Do()? This adds quite a bit to the existing API.

poy · 2019-09-30T17:11:48Z

Also, I would like to say, I like the idea of gomock being able to generate "spies" in some way, but I want to ensure the API is solid.

tylersammann · 2019-10-01T15:45:49Z

@poy gotcha. I wrote a little bit about using .Do() here. #262 (comment)
Mostly it just requires a lot of custom code to set up anything that stores arguments for later inspection, especially if you want to inspect multiple arguments. Also, in the multiple arg senario I find that it is nice to use the named captors rather than relying on argument order or re-implementing everything in the .Do() clause for each situation.

In terms of API change size, I think I'm only adding one 37 line file to the API itself. https://github.com/golang/mock/blob/bdfd0e24d230faaafd3c9d815aab7c1266ce9685/gomock/argument_captor.go The rest of the additions are testing or sample code related. (plus one .gitignore change that we can scrap if you don't want that)

I am happy to discuss changes to the API though if you can see places to improve it! My team has been using my forked ArgumentCaptors for the last six months with a lot of success and I'd love to see them added to the main branch.

minicuts

Great feature! Can you please add some godoc?

minicuts · 2020-03-25T10:12:11Z

@codyoss the naming of ArgumentCaptor seems fine to me. Any other suggestions?

* Added GoDoc * Removed the interface for ArgumentCaptor * Renamed (and exported) the struct to ArgumentCaptor * Renamed Value and AllValues to LastValue and Values respectively * Updated unit tests

* added some small grammar changes to the docs for LastValue and Values methods

* Updated test names to reflect the changes to the method names LastValue and Values

codyoss · 2020-03-28T16:53:26Z

Thoughts on just calling it Captor vs ArgumentCaptor? The latter seems more Java like to me, but it is nice an explicit.

codyoss · 2020-03-28T17:05:19Z

+}
+
+// AnyCaptor is a helper method that returns a new *ArgumentCaptor struct with the matcher set to an anyMatcher
+func AnyCaptor() *ArgumentCaptor {


Thoughts on CaptureAny()?

This would be ok with me if the call usually happened in the EXPECT line, but because captors aren't that useful unless they're declared in advance, I think it makes more sense to keep it as is and view it like a constructor.

Here's an example:

// here we don't have a way to grab the values mockDao.EXPECT().InsertIDs(gomock.CaptureAny()).Times(1) // we need to store the captor first idCaptor := gomock.AnyCaptor() mockDao.EXPECT().InsertIDs(idCaptor).Times(1) actualIDs := idCaptor.LastValue().([]int)

codyoss · 2020-03-28T17:13:29Z

@tylersammann Thanks for working on this!

tylersammann · 2020-03-30T16:29:56Z

@codyoss I like the name ArgumentCaptor because it is explicit. Captor isn't too bad though. I'm up to change it if others agree.

tylersammann · 2020-04-02T15:37:15Z

@nguyenfilip @codyoss I think I've addressed most of the PR comments and change requests. Let me know how you feel re: Value() vs. LastValue() and I'll update accordingly.

codyoss · 2020-04-08T22:19:22Z

Thanks, sorry for the delay. I will get back to review this by the end of the week.

minicuts · 2020-04-16T10:17:00Z

@tylersammann I don't have any more comments. I will let @codyoss approve because there is stil the discussion open about naming.

…or-fork

tylersammann · 2020-04-20T22:04:29Z

@nguyenfilip @codyoss this should be ready for another look. I've reverted the method names back to Value and AllValues

codyoss

Did a quick review mostly of the docs. Will come back and add some more this weekend.

codyoss · 2020-04-24T20:22:27Z

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package gomock


Would you add a couple examples of both a simple and more complex case using this as an end-user.

codyoss · 2020-06-12T13:54:13Z

@tylersammann Are you still interested in this change?

tylersammann · 2020-06-12T14:16:24Z

Yep definitely @codyoss. I can address your review this weekend

codyoss · 2020-06-12T14:19:14Z

No worries, I still owe you another through review too. I apologize for not being more responsive here. These have been unprecedented times.

tylersammann · 2020-06-16T19:54:07Z

I'll wait for you to review again before I push any changes. Thanks for looking @codyoss

tylersammann · 2020-06-26T21:40:44Z

@codyoss I came back through and addressed your first review because I had some time. Feel free to take another look!

hgl · 2021-04-16T07:06:48Z

Thanks everyone for the hard work.

Any progress on this PR?

isopropylcyanide · 2022-01-05T16:07:16Z

Thanks folks. Can we get this merged? This'd be a great feature add.

tylersammann · 2022-01-05T16:19:20Z

I'm happy to continue on this. It'll need a re-review from @codyoss and it's been a while, so a few tests and a merge conflict needs fixing.

codyoss · 2022-01-07T19:45:23Z

@tylersammann Apologies I dropped the ball on this PR. Let me take a look again here this week. One thing to consider for the future now is also generics with 1.18 coming soon. I would like to think about if generics might change surface. I will get back to you soon!

tylersammann · 2022-01-07T20:04:05Z

@codyoss thanks! I agree, generics might remove the need for type casting captured values, and that would be a big win.

codyoss · 2022-07-08T16:47:44Z

I think I am going to close this for now. I believe there might be a better surface design for this given generics now. Let's keep the chat going in the issue though.

tylersammann added 6 commits February 7, 2019 15:09

Added ArgumentCaptor

9eaaf6c

- added new ArguemntCaptor interface - added argumentCaptor implementation that composes Matcher - added constructors for ArgumentCaptor - included unit tests for ArgumentCaptor methods

add newline to argument_captor_test.go

a5e8f06

Add sample

f51a352

- added sample service and dao interface - added unit test that demonstrates `AnyCaptor` usage

fix go fmt issues

190e9a4

updated mock_dao.go with newest version of mockgen

bdfd0e2

Merge pull request #1 from tylersammann/262-argument-captor-fork

336ed0f

Argument Captor initial release

poy self-assigned this Sep 27, 2019

poy added the status: needs discussion label Sep 27, 2019

minicuts unassigned poy Jan 7, 2020

tylersammann mentioned this pull request Feb 11, 2020

Add argument captors #262

Open

codyoss requested a review from minicuts March 4, 2020 16:51

minicuts suggested changes Mar 25, 2020

View reviewed changes

Comment thread gomock/argument_captor_test.go

Comment thread gomock/argument_captor.go Outdated

Comment thread gomock/argument_captor.go Outdated

Comment thread gomock/argument_captor.go Outdated

minicuts removed the status: needs discussion label Mar 25, 2020

tylersammann added 4 commits March 27, 2020 16:45

Merge remote-tracking branch 'upstream/master'

c787188

Updates from nguyenfilip's review

7508961

* Added GoDoc * Removed the interface for ArgumentCaptor * Renamed (and exported) the struct to ArgumentCaptor * Renamed Value and AllValues to LastValue and Values respectively * Updated unit tests

Update the docs

c3ca842

* added some small grammar changes to the docs for LastValue and Values methods

Update test names

1799547

* Updated test names to reflect the changes to the method names LastValue and Values

tylersammann changed the title ~~ArgumentCaptor interface and tests~~ ArgumentCaptor struct and tests Mar 27, 2020

Updated sample to use LastValue instead of Value method

a4106f5

codyoss reviewed Mar 28, 2020

View reviewed changes

Comment thread gomock/argument_captor.go Outdated

codyoss reviewed Mar 28, 2020

View reviewed changes

Comment thread sample/captor/service_test.go Outdated

codyoss reviewed Mar 28, 2020

View reviewed changes

Comment thread gomock/argument_captor.go

use most recent version of mockgen to go generate

0984d43

minicuts previously approved these changes Apr 16, 2020

View reviewed changes

revert value methods to old names. Value and AllValues

bd7b8f4

tylersammann dismissed minicuts’s stale review via bd7b8f4 April 20, 2020 21:12

tylersammann added 4 commits April 20, 2020 16:16

rename tests to match method names

327db68

Merge remote-tracking branch 'upstream/master' into 262-argument-capt…

c5b6339

…or-fork

re-run go generate with newest mockgen

88fffe5

retrying go generate with latest mockgen

543e5c1

codyoss suggested changes Apr 24, 2020

View reviewed changes

tylersammann added 4 commits June 26, 2020 11:58

addressing PR comments from codyoss

941e979

added more examples to mockgen/internal/tests

4bc8e25

added license to model/car.go

49541e0

remove .Times(1) as it is redundant

b802eef

codyoss closed this Jul 8, 2022

kliukovkin mentioned this pull request May 6, 2026

gomock.Captor[T any] for argument inspection uber-go/mock#300

Open

Conversation

tylersammann commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poy commented Sep 27, 2019

Uh oh!

poy commented Sep 30, 2019

Uh oh!

tylersammann commented Oct 1, 2019

Uh oh!

minicuts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

minicuts commented Mar 25, 2020

Uh oh!

codyoss commented Mar 28, 2020

Uh oh!

Uh oh!

codyoss Mar 28, 2020

Choose a reason for hiding this comment

Uh oh!

tylersammann Mar 30, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codyoss commented Mar 28, 2020

Uh oh!

tylersammann commented Mar 30, 2020

Uh oh!

tylersammann commented Apr 2, 2020

Uh oh!

codyoss commented Apr 8, 2020

Uh oh!

minicuts commented Apr 16, 2020

Uh oh!

tylersammann commented Apr 20, 2020

Uh oh!

codyoss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codyoss Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

codyoss commented Jun 12, 2020

Uh oh!

tylersammann commented Jun 12, 2020

Uh oh!

codyoss commented Jun 12, 2020

Uh oh!

tylersammann commented Jun 16, 2020

Uh oh!

tylersammann commented Jun 26, 2020

Uh oh!

hgl commented Apr 16, 2021

Uh oh!

isopropylcyanide commented Jan 5, 2022

Uh oh!

tylersammann commented Jan 5, 2022

Uh oh!

codyoss commented Jan 7, 2022

Uh oh!

tylersammann commented Jan 7, 2022

Uh oh!

codyoss commented Jul 8, 2022

Uh oh!

Reviewers

Assignees

tylersammann commented Feb 7, 2019 •

edited

Loading