Define fallback for mock methods

[ybbus]

Hi.

I would appreciate your feedback for this pull request.

It is kind of similar to: #188

For each method you can define a fallback / default behaviour by calling:

mockObject.EXPECT().FooMethod().Fallback().Returns(5)

After defining this expectation as fallback, if there is no other expectation that matches a call to this method, the above defined fallback will be executed and return 5 all the time.

This is very helpful to define a default behaviour for methods. This makes more sense than loose mocks since most of the time the default return value is not very handy (e.g. nil, "" etc..).

The following rules for Fallback() should be noted (see the :gomock/controller_test.go for examples)

• all expectations are tried to match before the fallback method is considered
• the parameters of a method expectation in a fallback are not matched, this would not make sense, so you can leave the parameters empty or use arbitrary ones
• MinTimes, MaxTimes and AnyTime have no effect on a fallback method
• calling After() on a fallback will throw an error
• using a fallback as prerequisite will throw an error
• a fallback of a method is overwritten when another fallback of the same method is defined later on
• of course still all concrete expectations must be met, otherwise Finish() will fail as usual

I tried to change as little as possible in the code for this feature to work.

[balshetzer]

I like the idea of being able to specify default behavior.

I would call it "Default" or "WillByDefault" (that's what gmock calls it in C++), or something like that rather than Fallback.

I want to think a little about the way this fits in with everything. For example, a lot of caveats have to be given, which makes the interface complicated. Some of the caveats also don't seem necessary. For example, why can't matchers be used to specify varying default behavior?

[ybbus]

I thought of "Fallback" because it tells what it is: a Fallback :)
It should be called if no expectation that was defined matches the method call.
But I am fine with something like "Default".

I think a Fallback should be something very simple that does stupidly return some value.
If you allow to use matchers you will end up with a lot of defaults / fallbacks. For me this does not sound like a "fallback" anymore. If you have the need for different defaults I think you should consider providing them as actual expectations.

I thought of fallback as something like: "I need to mock this but I do not really care what the consumer of this mock does with it. Just mock it and return some static value".

But indeed it would be maybe more clean to consider matchers.
The catch all case for a defualt then could be:
mockObject.EXPECT().FooMethod(Any()).WillByDefault().Return(5)

Looks reasonable
I will try to integrate this.
I also like the WillByDefault method name.

[ybbus]

Function is now called: WillByDefault

You can now use matchers, After(), MinTime, MaxTime. So it behaves like other expectations.
The only difference is that it has a default of min=0, max=inf and is matched only if no (non default) expectation could be matched.

Indeed seems less complex and less "special" now.

[ybbus]

@balshetzer what's your opinion?

[balshetzer]

I like it in general. I think this will solve several feature requests.

I think we went too far in the other direction. I don't think the default behavior should support min and max times or After.

I also want to make sure we understand how it fits into gomock in general, for example:
What happens when multiple defaults are created? Can default behavior be overridden or modified? Is it confusing that we use EXPECT to then specify a non-expectation? Is it confusing that WillByDefault can be called in ways that don't read well (e.g. .Return(5).WillByDefault())?

[ybbus]

I think it is okay to provide the usual methods like min, max, etc...
If you just want to use a default behavior you can omit them and will get what you expect.
But there are quite some usecases for limiting default calls I think.

Your questions:

• If two defaults exist the one that matches the call will be taken.
• If two defaults would both match a call the last one will be taken, so yes, this could be used to overwrite a previous defined default. But you should reason about your tests, since it is quite too complex if you use a lot of defaults, overwrite etc..

One idea could be (didn't look into the code yet to check how complex the change would be):
You do not only have EXPECT() but also DEFAULT() or something like that.
So you don't have EXPECT() in front of WillByDefault and you also make sure, that WillByDefault or DEFAULT() is the first method in a method chain.

We could also call the method ByDefault() then both would look ok:

• Return(5).ByDefault()
• ByDefault().Return(5)

[ybbus]

Hi.

I renamed WillByDefault to ByDefault, so it reads well with Returns() before and after ByDefault.

All XTimes() methods are not allowed anymore. After() is also not allowed. Using default call as prerequisite is also not allowed.

I think this is the best I can provide without changing too much code.
Introducing an extra method besides EXPECT() would be a quite complex change.

I think this is a quick win for providing defaults.

@balshetzer

[balshetzer]

Thanks. I'll review as soon as I can.

[ybbus]

@balshetzer something new on this? I need this feature very often, could you just give me a hint when you will have time for a review? So I can decide if I use a fork in my projects.
Thanks, Alex.

[ybbus]

Hi.

Has this been reviewed, yet?

[ybbus]

@balshetzer
Hey.
Is there a chance for this to be merged?
Otherwise it can also be closed, since it has been more than a year now...

[poy]

@ybbus Sorry for the delay.

As you mentioned earlier, this is an attempt to add the ability for looser mocks. I'm hoping to avoid adding too many more methods as this adds more complexity.

Would your use case be solved by #246's solution? This better matches gmock and wouldn't require more methods. WDYT?

[codyoss]

@ybbus Thoughts ^

[ybbus]

@poy
I am not really sure about #246 .
It seems like some hack to me, like doing reverse order to solve the problem of having defaults.

I think this is quite error prone if you define more than one expectation in your tests and forget to reorder them.

Also for the reader of a test it may look quite strange if he/she does not knwo about / overlooks the reverseorder flag.

I am not a fan of this approach, but still better than no defaults at all.

In my opinion an explicit "ByDefault" methods is more clear.

[waynr]

As you mentioned earlier, this is an attempt to add the ability for looser mocks.

@poy @ybbus To me this PR is about ergonomics, not about going out of our way to violate anyone's preconception about what a mock library should or shouldn't do.

To illustrate what I mean, let me explain my situation:

• i have several dozen internal gRPC services in my organization that my service could interact with
• the behavior I'm testing isn't decomposable into simple unit tests
  • or rather it is and we do write unit tests at the decomposed level, but the really meaningful boundary conditions i want to test are at the level of complex interactions between those decomposable units
• i have a set of "smoke/e2e" tests that do integrate with staging deployments of the aforementioned gRPC services and do test some of the most fundamental interactions between different units of my service
  • however, interacting with live services is unwieldy and even at an acceptable level of parallelization (which is rather small given that we share these staging services with other teams and shouldn't expect to be able to monopolize the infrastructure resources) they take orders of magnitude longer to run than using fakes or mocks
• for many of the boundary conditions i am trying to validate in my test cases it's often only a select few gRPC methods (almost always specifically methods that mutate external state) for one or two of the previously-mentioned gRPC services that i really want to assert on being called with a given series of arguments (often in a specific order)
• for the large majority of the gRPC methods that might or might not be called (usually non-mutating methods) by my service what i really want is to be able to parameterize some kind of "happy path" behavior if they happen to be called.

That last bullet point reaches the crux of why I consider this PR an issue of ergonomics: having to write these "happy path" behaviors as mock expectations over and over again (for the sake of using this library to assert on calls that we really do care about in the boundary condition testing) in ways that tend to mimic the business logic of the code being tested leads to a massive amount of tedious-to-write and difficult-to-read boilerplate.

To work around this limitation in gomock while at the same time benefiting from being able to assert on key state-mutating calls while avoiding lengthy and repetitive boilerplate that mimics application business logic for non-essential or non-mutating calls, I have experimented with writing some difficult-to-understand test fixtures that combine the benefits of fake and mock implementations of the same external services that hide the complexity of populating mock method DoAndReturn(...).AnyTimes() with the corresponding fake method in the absence of more specific mock Calls. All of this jankiness could be avoided if gomock were to support a fallback feature like what's demonstrated in this PR.

Would your use case be solved by #246's solution? This better matches gmock and wouldn't require more methods. WDYT?

See the description of my use case above; I suppose reversing the order of call evaluation could do the trick in the sense that it would enable AnyTimes() to stand in as a hack to work around the lack of a proper fallback as @ybbus mentioned, but it's not clear to me what exactly is being reversed and if I would subsequently have to reverse the order of all my mock expectations.

Something else that could help me write the kind of fixture I described would be to be able to detect what call expectations have already been set on a given mock so that I could add the AnyTimes() "fallback" at the last minute after other call expectations have had a chance to be set.

[waynr]

but it's not clear to me what exactly is being reversed and if I would subsequently have to reverse the order of all my mock expectations.

To see why I am skeptical of #246, consider:

• it would lead to a violation of readers' intuition about the ordering at the point where call expectations are set
• it would introduced mixed reverse/forward evaluation in existing code bases where there is already a large corpus of mock calls defined in forward order

[ybbus]

For me in unit tests the most important behavior of mocks is (as already stated) is the happy path and some standard stupid behavior /return value.

That's why I am still using a forked version of gomock, where this default mechanism is available, since this is my most used feature for me.
I think most of the times when unit testing you should not even care about how often the mocked dependencies are called and with what arguments, since this already does imply a lot of the internal implementation of your unit (and how it uses the dependencies). But this is just my opinion.

If interfaces are quite easy I even just have some standard mock implementation that looks something like this (and is also all about default behavior):

type SomeInterface interface {
	SomeMethod(int) (int, error)
}

Mock:

type SomeInterfaceMock struct {
	SomeMethodFunc func(int) (int, error)
}

func (s *SomeInterfaceMock) SomeMethod(i int) (int, error) {
	if s.SomeMethodFunc != nil {
		return s.SomeMethodFunc(i) // if I need something special
	}

	return 0, nil // Default
}

This works in 90% of all test cases. And if I need something special, I just replace the method for the specific case (and reset it with a defer).

I think a Default() is essential and should be important enough to get its own Method, not by doing some hacky ordering stuff, that a ready hardly understands.

[waynr]

@ybbus while I think you and I come down on the same side of "let's enable defaults for mock methods" I think it's useful if we all follow the same definition of key terms like "mock". If we take the googlemock definition as our starting point, then the example you gave is a kind of fake implementation rather than a mock (i guess some pedants might call it a stub implementation).

But again I essentially agree that as a matter of ergonomics (and ignoring any dogmatic definitions of what a "mock" is or should be) we should be able to provide fake-like behaviors via default method implementations.

[ybbus]

@waynr You are totally right here regarding the definition. I just wanted to emphasize that this Default feature would really add a lot of value to the library and is an essential tool in many cases.

[cvgw]

I'd like to see the requirements and user story clarified here; I think that will help us clearly evaluate a solution.

As others have pointed out this comes down to the discussion of mocks vs fakes as conceptualized by Google.
gomock was created as a mock library, not a library for fakes. There has been lots of discussion of this and resistance to adding support for fakes.

I have definitely experienced the UX pain that is being described here; especially with complex interfaces like RPC clients!

I am not opposed to adding support for fakes, but I am opposed to expanding the API surface of gomock without a strong value proposition.

Breaking changes are basically out of the question (not suggesting what is being proposed would result in a breaking change).

As a starting point of discussion, why is creating a "default" expectation by making it the last declared expectation not sufficient?
EG

func TestFoo(t *testing.T) {
  // set up controller and create mock
  mockFoo.EXPECT().Bar(1, 2)

  // bunch of other expectations

  // add defaults
  // If this is the last expectations it will be a default
  mockFoo.EXPECT().Bar(gomock.Any(), gomock.Any()).Returns(true).AnyTimes()

  // do testing
}

Because gomock evaluates expectations in the order they were declared, this allows the default to be triggered when no other calls match or all potentially matching calls have been exhausted.

If the concern is boilerplate, that seems easily solved with some project specific code

func setDefaults(mockFoo *MockFoo) {
  // add defaults
  mockFoo.EXPECT().Bar(gomock.Any(), gomock.Any()).Returns(true).AnyTimes()
}

[codyoss]

@ybbus Would you mind making an issue for this for further discussion?

[ybbus]

@cvgw
This may work, but I think my idea came from this kind of code (e.g. using go convey):

Convey("Test some functionality", t, func() {

  // this will be called before every inner test
   mock := NewMock(controller)
   mock.EXPECT().MyMethod(gomock.Any()).ByDefault().Return(123, nil)

  Convey("Test call fails", func() {
    // here we may easily "override" the default
   // --> this would not work with your example, since the default would catch everything
     mock.EXPECT().MyMethod(gomock.Any()).Return(0, errors.New("some error")
  })

  Convey("Test standard behavior", func() {
    // here we don't need specific behavior and fallback to the default
  })

  // ... many other tests with or without custom behavior

 // called after every inner test
  Reset(func() {
    controller.Finish()
  })
})

[oriser]

@codyoss / @poy Can we merge this? This is a pretty small change that will add a lot of value. I was just about to open an issue + PR about the exact same thing and I saw several issues opened regarding this (EX: #238, #246, #137).
Currently there is no easy workaround to that issue using the library and it makes it less useful in some cases or makes the code less clean.

[ybbus]

Hey, 5 years later... :)

@codyoss I still see big value here, since I use it in a fork of this project all the time.

But if you don't, we may close the PR.