Open brackendawson opened 4 months ago
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?
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:
Call
instance that is going to be deleted from the Call
instances that require itI 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?
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 Call
s 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.
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.
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.
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
Description
If you remove a call from a mock with Unset, then NotBefore still expects it.
Step To Reproduce
Expected behaviour
Test passes
Actual behaviour