mock: methods should return values from Do

[stevvooe]

Updates (*Call).call to return the values from returned from Do
functions. Makes this package useful for stubbing, in addition to
mocking.

Signed-off-by: Stephen J Day [email protected]

Closes #34.

May require more testing.

[kaedys]

Correct me if I'm wrong, but this PR seems to make it so that if Do is called period, regardless of whether Return is also called, the return values of the Do func will be returned, making this comment technically inaccurate.

This also breaks if Do has a different return signature than the parent called function. The lambda provided to Do must already have the same input argument signature as the parent function, else the reflection call panics, but the current design doesn't require that of the return value of the Do function, and since that return value isn't currently used, I'd be willing to be most Dofuncs in the wild don't return anything.

However, with this design, if the lambda provided to Do returns a different number of or types of return values, the generated code will either panic due to a slice out of bounds on the ret (if the Dofunc returns fewer return values than the parent function), or will silently fail and return a zero value, since the generated code uses a two-value type assertion, but discards the boolean. This makes the changes here problematic, because all existing usages of Do will trigger a panic if they have a Dofunc that returns nothing and a parent function that has more than zero return values.

On a behavioral side, an explicit call to Return should probably override the return-what-Do-returned behavior (if you're explicitly calling Return, it's because you want the function to return something specific), and the code in general should probably be updated to only return the Dofunc return values if they match the number and type of the parent function itself.

[kaedys]

As an example, this test case will fail due to panic when the call to Get is actually made:

func TestFunctionReturnValuePanic(t *testing.T) {
    ctlr := gomock.NewController(t)
    defer ctlr.Finish()

    mockIndex := mock_user.NewMockIndex(ctlr)

    mockIndex.EXPECT().Get("get").Do(func(interface{}) {
        return
    })

    mockIndex.Get("get")
}

[stevvooe]

@kaedys Thanks for the analysis. This PR is really an attempt to open the conversation about using this package as a tool to create stub functions.

The main problem here is that after using this mock library for simple cases, one may have to switch over to stubs if it is required to respond to input.

It is also slightly obnoxious that it is not clear that Do cannot return a value and it took time to identify this as the problem. A panic for mismatched function signature would be a gift, as it would have exposed the error in my code. Either that, or we have some compile time type-safety to give us the correct stub types.

On a behavioral side, an explicit call to Return should probably override the return-what-Do-returned behavior (if you're explicitly calling Return, it's because you want the function to return something specific), and the code in general should probably be updated to only return the Dofunc return values if they match the number and type of the parent function itself.

I considered this. Either behavior is problematic, as it may result in test failures for existing uses, making the change backwards compatible.

Perhaps, we should define a new method, Stub for this functionality.

[kaedys]

A panic for mismatched function signature would be a gift, as it would have exposed the error in my code.

This is actually noted in the current code as a standing TODO, annoyingly: https://github.com/golang/mock/blob/master/gomock/call.go#L73

Definitely something that needs to actually be implemented, though, as Do already has a standing panic case if the DoFunc doesn't have the same input argument signature as the parent function. I just didn't really want to see another panic case added to upstream, particularly when that panic case is triggered by the majority current usage of Do (ie. with functions that don't return anything).

Perhaps, we should define a new method, Stub for this functionality.

That's actually already been suggested and rejected, unfortunately: #24

Though your usage of the term seems to suggest that you had a different mechanic in mind, since that type of "Stub" wouldn't fix the issue this PR is attempting to address.

Personally, I think the mechanic you're going for, the ability to process the inputs to a call and alter the return values based on that (what I believe you're referring to when you say "stub"), is something beyond the scope of what Do() is intended for, and currently used for. As such, it might be best to make a new method entirely (say, Process()), rather than attempt to piggyback the behavior onto the existing Do() method. This would also be a good place to do the arity and type checking for both input and output arguments, which could by copied over to Do() as well. I'll see if I can't cook up some code to do that checking.

I considered this. Either behavior is problematic, as it may result in test failures for existing uses, making the change backwards compatible.

I'd actually argue that Return() overriding the return from Do() matches current behavior better. Right now, if both Do() and Return() are called, the values passed to Return() are what are actually returned, because Do() has no return behavior (it's return values aren't even stored). As such, changing it so Do() overrides Return() (as exists in this PR) will break those existing statement, and will also trigger the panic issue on those calls. However, if Return() overrides Do(), those cases don't break (and no other cases break instead, either), and are in fact protected from the return type/arity mismatch panic as well.

Also worth noting that the current functionality of "on call, do this thing, and also return these other things" is impossible with the current implementation in this PR unless your "do this thing" function returns those "other things", making test cases more complicated than they need to be. Making Return() override Do(), however, removes no existing functionality.

Here are the existing possible combinations for the parent function Foo(a A) B:

• EXPECT().Foo(a).Returns(b) - no change from this PR either way
• EXPECT().Foo(a).Do(func(a A) B { return b }) - changed so the B returned by the lambda passed to Do() is actually processed and then returned by the Foo() call itself. This occurs regardless of the Do()/Return() resolution.
• EXPECT().Foo(a).Returns(b).Do(func(a A) B { return b2 }) - this is the problem case. Currently, this returns b. If we use this PR's current setup, the call to Return() is completely ignored, and b2 is returned, despite the programmer clearly intending b to be returned. If we make Return() override Do(), the existing semantic of b being returned persists, with the new behavior of returning the DoFunc return value only occurring if one does not explicitly indicate the desired return value.

Though, again, I think this would be a better implementation if it weren't piggybacked on top of Do(), and instead was a separate (and new) method call like Process().

[stevvooe]

@kaedys I agree with your analysis.

At this point, I am not sure if using this mock library warrants the extra complexity. Failures are cryptic and hard to resolve. I'm not sure if adding a new method that is subtly different from the others is a good solution either, as that will just result in similar frustration from others trying to add or modify unit tests.

This PR can either be closed or someone else may carry it.

The alternative that I'm using is described below.

---

At this point, I've just start writing mocks like this:

type InterfaceUnderTest {
  MethodOne(...) (...)
  MethodTwo(...) (...)
}

type mockInterface struct {
  InterfaceUnderTest
}

Because the interface is embedded, the type implements the interface with no extra code. With a method-less definitions, it will panic over every method call, reported from the call site in the code under test. Tracking down the error during development is straightforward. When we want to add an implementation, we simply override the target method:

func (m *mockInterface) MethodOne(...) (...) { return ... }

Now, when (*mockInterface).MethodOne is called, we get our implementation. When (*mockInterface).MethodTwo is called, we get a panic, which may be intended as part of the test, ensuring failure. If we need something more sophisticated, we can start adding additions to support what is needed for a particular set of tests.

Let's say we need to replaced the implementation of (*mockInterface).MethodOne:

type mockInterface struct {
  InterfaceUnderTest

  MethodOneFn func(m *mockInterface, ...) (...) // signature must match, add receiver
}

func (m *mockInterface) MethodOne(m *mockInterface, ...) (...) { 
  return m.MethodOneFn(m, ...)
}

This let's us replace the implementation dynamically. This covers most of the cases I've needed but what about call ordering?

We can simply track the method call order and test against it later:

type call struct {
  method interface{} // bound method instance
  args []interface{}    // arguments, as an interface slice
}

type mockInterface struct {
  InterfaceUnderTest
  MethodOneFn func(m *mockInterface, ...) (...) // signature must match, add receiver

  calls []call
}

func (m *mockInterface) MethodOne(m *mockInterface, ...) (...) { 
  m.calls = append(calls, call{m.MethodOne, []interface{...}})
  return m.MethodOneFn(m, ...)
}

func TestMyTest(t *testing.T) {
  // .. setup mocks
  if !reflect.DeepEqual(mock.calls, []call{
    {method: mock.MethodOne, args: []interface{"arg1", "arg2")}  
  }) {
    t.Fatalf("unexpected call chain")
  }
}

More sophisticated checking could be pulled up into a package. This would include helper functions to keep track of calls and match different styles of call chains, as well as an embeddable type for tracking and matching calls, similar to testify/mock.Mock. A lot of this can be codegen'd, as well but it is probably better to make the tests clear and explicit.

[kaedys]

That's actually exactly the style I was considering for my own mock system, with each function in the interface being mocked having a method on the mock struct, which calls a function field, and that function field can be replaced as desired by either named functions or anonymous enclosures.

[balshetzer]

This has been fixed in a different way by #126