Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
brackendawson opened this issue Feb 20, 2024 · 6 comments · May be fixed by #1572
Open

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

brackendawson opened this issue Feb 20, 2024 · 6 comments · May be fixed by #1572
Assignees
@st3penta
Labels
bug pkg-mock Any issues related to Mock

Comments

@brackendawson
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link

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
Copy link
Collaborator Author

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
Copy link

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
Copy link
Collaborator Author

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
Copy link

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

@st3penta st3penta linked a pull request Mar 8, 2024 that will close this issue
Open
@dolmen dolmen changed the title mock: Call.NotBefore still expectes calls removed with Call.Unset mock: Call.NotBefore still expects calls removed with Call.Unset Mar 18, 2024
@dolmen dolmen added the pkg-mock Any issues related to Mock label Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@st3penta st3penta

Labels
bug pkg-mock Any issues related to Mock
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

mock: clear call from NotBefore expectations on Unset() st3penta/testify
3 participants
@dolmen @brackendawson @st3penta