Repository: golang/mock
Pull request #98 (2017-08-14 14:39:53)

Support slices in SetArg()
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 https://github.com/golang/mock/issues/27

googlebot: 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).

:memo: **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](https://cla.developers.google.com/clas) and verify that your [email is set on your git commits](https://help.github.com/articles/setting-your-email-in-git/).
- 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.

<!-- need_sender_cla -->
hatstand: I signed it!
googlebot: CLAs look good, thanks!

<!-- ok -->
balshetzer: 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: I think it's morally the same as other uses of SetArg as it does actually
change the input argument.

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

On Mon, 14 Aug 2017, 23:20 Hesky Fisher, <notifications@github.com> wrote:

> 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.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/golang/mock/pull/98#issuecomment-322325804>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AATMe764MsJfjLHJZ_U6rLtsD-SdIbppks5sYMgcgaJpZM4O2dRo>
> .
>

pasztorpisti: 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 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.
pasztorpisti: 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.:

```go
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:

```go
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: 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: You can't set an array 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.
balshetzer: @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.
pasztorpisti: 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: So there's good news and bad news.

:thumbsup: 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.

:confused: 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.*

<!-- need_author_consent -->

Patch:
diff --git a/gomock/call.go b/gomock/call.go
index 9f3ae9c..dc2a479 100644
--- a/gomock/call.go
+++ b/gomock/call.go
@@ -118,7 +118,8 @@ func (c *Call) Times(n int) *Call {
 }
 
 // SetArg declares an action that will set the nth argument's value,
-// indirected through a pointer.
+// indirected through a pointer. Or, in the case of a slice, SetArg
+// will copy value's elements into the nth argument.
 func (c *Call) SetArg(n int, value interface{}) *Call {
 	if c.setArgs == nil {
 		c.setArgs = make(map[int]reflect.Value)
@@ -142,8 +143,10 @@ func (c *Call) SetArg(n int, value interface{}) *Call {
 		}
 	case reflect.Interface:
 		// nothing to do
+	case reflect.Slice:
+		// nothing to do
 	default:
-		c.t.Fatalf("SetArg(%d, ...) referring to argument of non-pointer non-interface type %v [%s]",
+		c.t.Fatalf("SetArg(%d, ...) referring to argument of non-pointer non-interface non-slice type %v",
 			n, at, c.origin)
 	}
 	c.setArgs[n] = reflect.ValueOf(value)
@@ -243,7 +246,12 @@ func (c *Call) call(args []interface{}) (rets []interface{}, action func()) {
 		action = func() { c.doFunc.Call(doArgs) }
 	}
 	for n, v := range c.setArgs {
-		reflect.ValueOf(args[n]).Elem().Set(v)
+		switch reflect.TypeOf(args[n]).Kind() {
+		case reflect.Slice:
+			setSlice(args[n], v)
+		default:
+			reflect.ValueOf(args[n]).Elem().Set(v)
+		}
 	}
 
 	rets = c.rets
@@ -265,3 +273,10 @@ func InOrder(calls ...*Call) {
 		calls[i].After(calls[i-1])
 	}
 }
+
+func setSlice(arg interface{}, v reflect.Value) {
+	va := reflect.ValueOf(arg)
+	for i := 0; i < v.Len(); i++ {
+		va.Index(i).Set(v.Index(i))
+	}
+}


Test patch:
diff --git a/gomock/call_test.go b/gomock/call_test.go
index 3ae7263..71a2078 100644
--- a/gomock/call_test.go
+++ b/gomock/call_test.go
@@ -1,6 +1,9 @@
 package gomock
 
-import "testing"
+import (
+	"reflect"
+	"testing"
+)
 
 type mockTestReporter struct {
 	errorCalls int
@@ -45,3 +48,32 @@ func TestCall_After(t *testing.T) {
 		}
 	})
 }
+
+func TestCall_SetArg(t *testing.T) {
+	t.Run("SetArgSlice", func(t *testing.T) {
+		c := &Call{
+			methodType: reflect.TypeOf(func([]byte) {}),
+		}
+		c.SetArg(0, []byte{1, 2, 3})
+
+		in := []byte{4, 5, 6}
+		c.call([]interface{}{in})
+
+		if in[0] != 1 || in[1] != 2 || in[2] != 3 {
+			t.Error("Expected SetArg() to modify input slice argument")
+		}
+	})
+
+	t.Run("SetArgPointer", func(t *testing.T) {
+		c := &Call{
+			methodType: reflect.TypeOf(func(*int) {}),
+		}
+		c.SetArg(0, 42)
+
+		in := 43
+		c.call([]interface{}{&in})
+		if in != 42 {
+			t.Error("Expected SetArg() to modify value pointed to by argument")
+		}
+	})
+}