`Unset` removes all expected calls that satisfy (vs match) arguments in call being unset

[techfg]

Description

When calling Unset, any expected call that satisfies the arguments in the call being unset is removed from ExpectedCalls.

For example, in the following scenarios, both the Anything, Anything and 3,3 calls are removed:

// Scenario 1- Explicit Unset on the call
mockedService.
    On("TheExampleMethod1", Anything, Anything).Return(0).
    On("TheExampleMethod1", 2, 2).Return(0)
callToUnset := mockedService.On("TheExampleMethod1", 3, 3).Return(0)
callToUnset.Unset()

// Scenario 2 - Adding/Unsetting a call
mockedService.
    On("TheExampleMethod1", 2, 2).Return(0).
    On("TheExampleMethod1", 3, 3).Return(0).
    On("TheExampleMethod1", Anything, Anything).Return(0)
mockedService.On("TheExampleMethod1", 3, 3).Unset()

Reproduction

Go Playground is timing out on build when github.com/stretchr/testify/mock is used (likely due to dependencies downloading), so submitted #1622 with tests to reproduce.

Step To Reproduce

Run any/all of the following tests:

1. Test_Mock_UnsetOnlyUnsetsCurrentCall
2. Test_Mock_UnsetOnlyUnsetsExactArgsMatchCall
3. Test_Mock_UnsetOnlyUnsetsExactArgsMatchCallWhenPartialMatch
4. Test_Mock_UnsetOnlyUnsetsExactArgsMatchCallWhenAnythingSomeArg
5. Test_Mock_UnsetOnlyUnsetsExactArgsMatchCallWhenAnythingEveryArg

Expected behavior

Only the call(s) with the exact matched arguments should be unset/removed.

Ideally only the call that Unset is invoked against is removed but the current API is written in a manner that requires a Call to unset so unless the call is tracked (e.g., c := mockService.On(....)), On must be called first then Unset which adds the expectation only to immediately remove it along with any other "matched" expectations. #982 which added Unset actually started with Unset on Mock but then moved it to Call to support Unset on a Call but possibly it could have considered leaving Unset(methodName string, arguments ...interface{}) on Mock to remove all "matched expectations" and then Unset on Call to remove just that call expectation.

At minimum, the docs should be updated to reflect actual behavior (see first item in additional information below).

Actual behavior

All calls that satisfy the arguments are removed

Current Workarounds

1. Clear ExpectedCalls - This guidance has been given in several issues (including question / feature-suggestion: built-in way to overwrite / clear ExpectedCalls? #558 which spawned Unset being introduced) but has the downside of clearing all expected calls, not just the desired call
2. Walk Expected Calls and manually look for the match - While technically possible, there are private methods in testify that have all this logic already (a new findExactMatch is likely needed) and re-creating this is not trivial.
3. Walk Expected Calls and remove all calls for a method - This is rather straightforward to do but has two downsides: 1) It requires all expectations for that method to be setup again; 2) It's not chainable
    

Additional Information

1. Why this should be considered a bug vs expected behavior - The docs indicate "removes a mock handler from being called." (emphasis mine). However, it will remove 1 to N handlers where N is any handler that "satisfies" the arguments in the call since it removes all of them (vs. "a" handler). As mentioned above, the API requires calling On first (unless the call is tracked) which adds a new call then unsets it (and any others that satisfy) vs. just having a Unset on Mock that will remove any that match, leading to the 1 to N vs. 0 to 1 behavior. Beyond just not matching the docs, regarding why this is relevant, question / feature-suggestion: built-in way to overwrite / clear ExpectedCalls? #558 covers this well but in short, in a complicated Mock, it is common to setup default expectations and then within an individual test, change those expectations for just that test. For example, if an interface has 10 methods all of which a set of tests use but each test is testing individual portions, a NewMyServiceMock utility method (aka. builder) could establish defaults which the test than can override/reset/clear. In this case, each test gets a new Mock but the mock is created by NewMyServiceMock with defaults. Depending on what the defaults are, there is currently no way to change expectations of specific arguments (vs. any expectation that satisfies the arguments).
2. The one benefit of the current code is that it allows for "clearing all" expectations for a given method (e.g. On("TheExampleMethod1", Anything, Anything).Unset() would remove all expectations from TheExampleMethod1 that had two parameters) as there isn't an otherwise available API for this currently. This is likely an unintended benefit however, and possibly creating an UnsetAll(methodName string) would continue to support this for anyone taking advantage of it.
3. Reviewing the code, I believe this is occurring because Unset relies on logic similar to MethodCalled and therefore detects any remaining available expectation that satisfies the arguments vs. the exact arguments.
4. I'm happy to work on a PR for this however prior to doing so, want to solicit feedback on whether or not there is agreement on the issue described being a bug (vs. expected behavior) and assuming yes, it is a bug, confirming the expected behavior. My proposed solution would be as follows:
   1. Unset should do an exact match on args
   2. All exact matches would be removed from expectations
   3. if no matches found, it's a no-op - Technically, due to the way unset currently works, the call already has to exist at least once since On("TheExampleMethod1", Anything, Anything).Unset() actually adds the expectation and then removes it (along with any others that match).

Alternative Solution

If its determined that this is not a bug or even if it is a bug but its determined that changing behavior would not be desirable, alternate approaches could be:

Option 1

1. Add UnsetOnly (or similar) to Call to remove only the call its invoked against
2. Add UnsetAll() to Mock to remove all expectations
3. Add UnsetMethod(methodName string) to Mock to remove all expectations for a method
4. Add UnsetCall(string methodName, methodName string, arguments ...interface{}) to Mock which would behave identical to what Call.Unset() does currently but avoids having to call On only to then call Unset
5. Add UnsetExactCall(string methodName, methodName string, arguments ...interface{}) to Mock to remove all calls that have exact matched arguments
   ** Could combine 3 & 4 to UnsetCall(exact bool, string methodName, methodName string, arguments ...interface{})

Option 2

1. Expose, either directly or indirectly, findClosestCall, findExpectedCall and findExactCalls (which would be new and return 0 to N calls) to provide package consumers control over manually manipulating Mock.ExpectedCalls. This is likely less than ideal but at least would give consumers of package an option to accomplish the desired behavior

Related Issues

#1511 proposes a Replace API which is often why Unset is needed. If #1511 were merged currently, it would replace all "matched" expectations which wouldn't necessarily be only one expectation

[brackendawson]

As I mentioned in #1707:

We should stop re-using Diff to find the Call being unset in the Mock's ExpectedCalls. It's possible this could be fixed by giving each Call a globally unique ID. An atomic.UInt64 in global scope in the mock package could serve as the source for this.

[brackendawson]

Actually, we can't do that. This test:

testify/mock/mock_test.go

Line 642 in 7c367bb

mock1.On("TheExampleMethod", 1, 2, 3).

And Unset's docs:

Unset removes a mock handler from being called.
test.On("func", mock.Anything).Unset()

Show that this is the intended behaviour. It's supposed to be able to unset mocks without any reference to the original mock like this:

package kata_test

import (
	"testing"

	"github.com/stretchr/testify/mock"
)

type My struct {
	mock.Mock
}

func (m *My) Do(i int) {
	m.Called(i)
}

func TestAnything(t *testing.T) {
	m := &My{}
	m.Test(t)
	defer m.AssertExpectations(t)

	m.On("Do", 1).Return().Once()
	m.On("Do", 1).Return().Once().Unset()

}

And because this causes another mock to be registered it has to remove all matching mocks.

Unset should never have been designed this way, in fact it should never have been designed. But it is this way and I don't think there is anything we can do about it.

[techfg]

@brackendawson -

Thanks for looking in to this!

Unset should never have been designed this way, in fact it should never have been designed. But it is this way and I don't think there is anything we can do about it.

Agreed that there would have been better ways to design that now present some limitations/issues with backwards compat. As mentioned in my OP, the design originally started on Mock and then moved to Call and leaving on Mock or another way to achieve would have been better.

In short, the main issue currently is that there is no way to unset exact match arguments and more importantly, the documentation indicates "removes a mock handler from being called" (emphasis on the "a" is mine) which infers a single handler, not all handlers.

Any thoughts on one of the Alternate Solutions I mentioned and/or at least updating the docs to be clear about behavior?

[brackendawson]

We should definitely change the documentation to match the implementation. I think in this case the unit tests are more authoritative than the doc string. I'll accept a PR to the effect of correcting "a" to "all". We should probably also document that it's only supported for Call instances with static input arguments.

[techfg]

We should probably also document that it's only supported for Call instances with static input arguments.

@brackendawson - If #1425 lands, is this no longer true and non-static input would be expected to work? Possibly I'm misunderstanding what your saying here and/or what #1425 addresses.

[brackendawson]

I don't think the approach in #1425 can be made to work the same way as the original Unset design because function equality tests aren'y possible. We probably have to live with them not working.