mock: Call.NotBefore still expects calls removed with Call.Unset

[brackendawson]

Description

If you remove a call from a mock with Unset, then NotBefore still expects it.

Step To Reproduce

package kata_test

import (
	"testing"

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

type myMock struct {
	mock.Mock
}

func (m *myMock) Do() {
	m.Called()
}

func (m *myMock) Dont() {
	m.Called()
}

func TestIfy(t *testing.T) {
	m := myMock{}
	m.Test(t)
	defer m.AssertExpectations(t)
	call1 := m.On("Dont").Return().Once()
	m.On("Do").Return().Once().NotBefore(call1)
	call1.Unset()

	m.Do()
}

Expected behaviour

Test passes

Actual behaviour

--- FAIL: TestIfy (0.00s)
    /Users/bracken/src/github.com/brackendawson/kata/mock.go:334: mock: Unexpected Method Call
        -----------------------------
        
        Do()
        		
        
        Must not be called before:
        
        Dont()
        		
    /Users/bracken/src/github.com/brackendawson/kata/panic.go:626: FAIL:	Do()
        		at: [/Users/bracken/src/github.com/brackendawson/kata/kata_test.go:26]
    /Users/bracken/src/github.com/brackendawson/kata/panic.go:626: FAIL: 0 out of 1 expectation(s) were met.
        	The code you are testing needs to make 1 more call(s).
        	at: [/usr/local/go/src/runtime/panic.go:626 /usr/local/go/src/testing/testing.go:1005 /Users/bracken/src/github.com/brackendawson/kata/kata_test.go:14 /Users/bracken/src/github.com/brackendawson/kata/kata_test.go:29]
FAIL
coverage: 0.0% of statements
FAIL	github.com/brackendawson/kata	0.537s
FAIL

[brackendawson]

This happens because in Unset, the Call removes itself from its parent Mock's ExpectedCalls. It can't do that to other Call instances which expect it because they are allowed between different Mock instances. Perhaps we could make the Call mark itself satisfied as well or instead? Or would that break other public methods that the Call has?

[st3penta]

I think the cleanest way to handle this would be to have an additional field in the Call struct similar to requires, that tracks the Calls that require the current one. It could be called requiredBy.

This new field should be checked in the Unset method, and i see two options here:

• Unset fails because the call that we want to delete is required by other calls
• Unset clears the occurrences of the Call instance that is going to be deleted from the Call instances that require it

I would prefer the first option because unsetting a call that is required elsewhere sounds like a misconfigured test setup to me.

What do you think?

[brackendawson]

I think Unset should not exist, it doesn't align with the good practice of testing for state. But it is part of our promise now.

On the approach, I think the cleanest way would be for the call being Unset to mark itself as "satisfied" somehow, but we can't becuse that is done by checking that the Parent's Mock.Calls has contains enogh calls to satisfy it. Mock.Calls is an exported field, so we can't go putting Calls that didn't happen in there.

Argument matching in mock.Mock is a bit of a mess, and it can't be cleanly fixed without unexporting most, if not all of the fields on Mock and Call. Call.Repeatability is particularly broken as Mock.On("Method").Return().Times(0) doesn't work. I think this is best fixed after a move to v2 where we can unexport the fields and replace them with useful methods.

[st3penta]

I'm still not convinced that the cleanest approach would be to mark the call as satisfied. Faking something that didn't actually happen to satisfy the Unset logic sounds more like a workaround to me, since it abuses the logic of the expected calls, and could lead to unexpected behaviours, as you also pointed out.

That's why I was proposing to add a reference to the "dependent" calls in the Call struct: so that it can be used to find and delete the expectation from those calls by removing the element from the requires slice, just as if the unset call never existed in the first place.

[brackendawson]

Okay, I think I would approve that fix in v1 if you want to make a pull request.

I'd still then like to refactor this to something simpler in v2 when we can unexport some of the internals.

[st3penta]

Yes I'd be happy to do it! Can you assign it to me?
Tomorrow i'm going offsite for 10 days though, so i'll probably be able to complete it when i come back