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

Support slices in SetArg() #98

Merged
merged 3 commits into from
Aug 16, 2017
Merged

Support slices in SetArg() #98

merged 3 commits into from
Aug 16, 2017

Conversation

hatstand
Copy link
Contributor

I needed this while trying to write tests against https://godoc.org/github.com/kidoman/embd#SPIBus

SPIBus::TransferAndReceiveData([]byte) writes its output back to the input byte slice.

Would also fix #27

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@hatstand
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@balshetzer
Copy link
Collaborator

I'm not sure about this. This overloads SetArg to also fill a slice, which feels like it's not the same thing.

This need shouldn't block anyone because the easy workaround is to use the Do function to fill the slice. However, I could see the value of a function specifically for filling an input slice.

@hatstand
Copy link
Contributor Author

hatstand commented Aug 14, 2017 via email

@pasztorpisti
Copy link
Contributor

pasztorpisti commented Aug 15, 2017

I agree with @balshetzer that it's better to handle only pointer args in SetArg.

Pointer args are sometimes used as "return values" - probably by ex C programmers - despite the fact that it isn't very idiomatic in go where you can easily return multiple values.

Any other case - like modifying an array or any other input object - isn't really like simply returning a value. Modifying an incoming object is a messy business that is better done in Do. In many cases you can't even perform the full spectrum of operations on the incoming object when you want to modify it - e.g.: you can't add extra items to the slice because that might involve allocation and the caller would still have a reference only to the original underlying array.

The above problems have been discussed in #27.

In my opinion SetReturnArg or SetOutArg could be a better name for this function for the above reasons (but renaming isn't an option because of backward compatibility). We could perhaps add a godoc comment to the function better explaining its purpose.

I think it's morally the same as other uses of SetArg as it does actually change the input argument.

SetArg sets/replaces the input, what you want is modifying an input object of some kind.

Happy to implement it as a separate function though; name suggestions?

The power of gomock lies in providing only a few very solid features that cover 99% of the testing scenarios. For other edge cases it provides 1 tool - the Do method. I'd keep it this way. Narrow purpose methods (like modifying an array without being able to shrink or grow it) would make the whole implementation more messy and much less solid.

@pasztorpisti
Copy link
Contributor

pasztorpisti commented Aug 15, 2017

I agree that Do is a bit inflexible though and I was thinking about this problem a few weeks ago. You have to write the full function signature and it doesn't support return values.

One of my ideas was extending the current behaviour of Do and being able to pass a generic handler function to it that always has the same signature, something like func(args map[string]interface{}).

If Do supported this generic interface then it would be quite easy to write reusable project-local test utility functions to deal with issues, e.g.:

func NewArrayFirstByteZeroer(argName string) func(args map[string]interface{}) {
	return func(args map[string]interface{}) {
		args[argName].([]byte)[0] = 0
	}
}

... and then reusing it would be a breeze:

myMock.EXPECT().Whatever().Do(NewArrayFirstByteZeroer("data"))

Since the arg names are available only in case of source generation method (and not in case of reflection based) it might make sense to use the func(args ...interface{}) signature instead of func(args map[string]interface{}).

@hatstand
Copy link
Contributor Author

I guess the more accurate name for SetArg would have been SetArgumentPointee like gmock. gmock also has SetArrayArgument if you wanted to follow their lead.

@pasztorpisti
Copy link
Contributor

pasztorpisti commented Aug 15, 2017

You can't set an array/slice arg, you can only modify it.

I wouldn't put any new "features" into gomock other than improving Do to provide a way for people to handle edge cases. Most mocking libraries are a mess full of narrow purpose functions.

@hatstand hatstand closed this Aug 15, 2017
@balshetzer
Copy link
Collaborator

@hatstand I've thought about it and would like to accept this PR. Would you be willing to add unit tests and update the comment on SetArg to explain what it does?

@pasztorpisti I hear your points too but populating arg slices is clearly something that is done in Go and so, I think, having SetArg "do the right thing" for slices seems like it will make it more useful without much cost.

@balshetzer balshetzer reopened this Aug 15, 2017
@pasztorpisti
Copy link
Contributor

Populating arg slices usually involves complex logic (which is basically simulating the mocked logic - something for which Do was invented). Slice population isn't as simple as a Return().

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot
Copy link

CLAs look good, thanks!

@hatstand
Copy link
Contributor Author

Added documentation & unit tests.

@balshetzer balshetzer merged commit 1df903b into golang:master Aug 16, 2017
@lovung lovung mentioned this pull request Sep 17, 2021
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.

SetArg on slice silently fails
4 participants