Invocations.Clear() does not cause Verify to fail.

[jchesshir]

If an expected function on a Mock is called and then Invocations.Clear() is called on the Mock object, it does not erase the record of the call. Consequently, a call to a Verify function after the call to Invocations.Clear succeeds.

I would expect the code to throw an error on the call to an error.
For example:

try
{
    Mock<ITest> mockTest = new Mock<ITest>(MockBehavior.Strict);
    mockTest.Setup(t => t.CallMe());
    mockTest.Object.CallMe();
    mockTest.VerifyAll(); // Succeeds as expected.

    mockTest.Invocations.Clear();
    mockTest.VerifyAll(); // Still succeeds

    Assert.Fail("Code should not have reached this point.");
}
catch (MockException e)
{
    Console.WriteLine("Expected exception thrown.");
}
catch (AssertFailedException ex)
{
    Console.WriteLine("Code got to the last line of the try.");
}

The above code generates the output:

Code got to the last line of the try.

[stakx]

@jchesshir - I've been expecting this issue to be raised eventually, thanks for reporting it. This is one of those not-so-clear-cut cases where it is perhaps up to personal interpretation what should happen. However, there are reasons why VerifyAll works the way it does:

What Verify and VerifyAll do is to check whether all setups have been invoked at least once. (They only differ in which setups they look at, but that's irrelevant for this issue.)

You could say, Verify[All] should look at all currently recorded invocations and match them against the setups. (You appear to be expecting that this is what happens.)

On the other hand, you could also insist on the correctness of the above definition of Verify[All]: Your setup has in actual fact been matched by an earlier invocation—even though there is now no longer any record of that invocation—so it's only right that your second VerifyAll call still succeeds. (This is how Moq actually works.)

The problem with the first stance (i.e. matching the currently recorded invocations against the setups at the time of the Verify[All] call) is that it leads to problems when object state changes get involved. You'd no longer be able to do certain things. Consider this example:

public class Toy
{
    public string Color { get; set; }
}

public interface IChild
{
    void PlayWith(Toy toy);
}

var child = new Mock<IChild>();
child.Setup(c => c.PlayWith(It.Is<Toy>(t => t.Color == "Red")));
child.Setup(c => c.PlayWith(It.Is<Toy>(t => t.Color == "Green")));

var chameleon = new Toy();

chameleon.Color = "Red";
child.Object.PlayWith(chameleon);

chameleon.Color = "Green";
child.Object.PlayWith(chameleon);

child.VerifyAll();

This only works because invocations are matched against the setups at the time of the invocation. If they were matched against setups at the time of the VerifyAll call, the first setup (for the child playing with a red toy) would fail.

(The only way to have it all would require Moq to deep-clone all invocation arguments and record them along with the invocation, but there's no reliable generic way to deep-clone any object in .NET. Also, this behavior could get prohibitively expensive. So what Moq does instead is to just record arguments as they are, and use plain Equals to compare/match them... which is why state changes of objects are problematic; reference types use reference equality by default, which won't reflect state changes.)

[stakx]

[...] and then Invocations.Clear() is called on the Mock object, it does not erase the record of the call.

What didn't occur to me in all of the above is that Invocations.Clear() possibly doesn't erase traces of earlier calls thoroughly enough. Is that what you're requesting here?

It should be possible to "un-match" setups (but that'd technically be a breaking change).

[jchesshir]

Yes. That is my issue exactly. I'd like to be able to reuse the mock objects for multiple tests. As it stands I have to create and initialize a new mock for every test. That seems to make an unnecessarily resource bloated test set. While I can see how changing that behavior could be a breaking change, I've trouble understanding the use case that would benefit from the current functionality. Can you give me an example? I suppose if there is a valid case, you could just create another function or an overload to clear the invocation count in addition to/instead of what it currently does. Thank you!

…

On Thu, Jan 3, 2019, 9:55 AM stakx ***@***.*** wrote: [...] and then Invocations.Clear() is called on the Mock object, it does not erase the record of the call. What didn't occur to me in all of the above is that Invocations.Clear() simply doesn't erase traces of earlier calls thoroughly enough. It should be possible to "un-match" setups (but that'd technically be a breaking change). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#733 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AK_nRD7KZT4vlv4JS0bRE3IQYwFmihCUks5u_ifrgaJpZM4ZQZ49> .

[stakx]

I've trouble understanding the use case that would benefit from the current functionality.

I believe that the ability to clear the set of recorded invocations was introduced as a memory optimization. When you have a test that requires a very large number of invocations on your mock, these get recorded (regardless of whether your test actually requires that for later assertions or not), and that can eventually cause a OutOfMemoryException. I suspect that being able to remove invocation records was a cheap (if somewhat inelegant) way around that problem for someone in the past.

I think what you're requesting might actually improve the logical consistency of Moq's API overall, even given the breaking change. I'll give it some more thought.

[jchesshir]

That makes sense. Thank you again for considering the change.

…

On Fri, Jan 4, 2019 at 7:23 AM stakx ***@***.***> wrote: I've trouble understanding the use case that would benefit from the current functionality. I believe that the ability to clear the set of recorded invocations was introduced as a memory optimization. When you have a test that requires a very large number of invocations on your mock, these get recorded (regardless of whether your test actually requires that for later assertions or not), and that can eventually cause a OutOfMemoryException. I suspect that being able to remove invocation records was a cheap (if somewhat inelegant) way around that problem for someone in the past. I think what you're requesting might actually improve the logical consistency of Moq's API overall, even given the breaking change. I'll give it some more thought. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#733 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AK_nRJKfYZFoJX0oBRMPY7cDkxgs2vaRks5u_1W_gaJpZM4ZQZ49> .

[stakx]

Will do. One last thing I forgot to mention earlier:

I'd like to be able to reuse the mock objects for multiple tests. As it stands I have to create and initialize a new mock for every test.

I suspect that the conventional wisdom of the unit testing community would say that creating new mocks per test is actually a good thing, as it prevents state changes of one test from accidentally "bleeding over" into other tests, thus keeping them independent.

That being said, I'll still look into the request you've made as it still seems like a sensible change.

[stakx]

Smallish update, I asked kzu about this in the Moq Gitter chat to make sure we'd have good compat with Moq v5; seems we're good to go & ready to implement the change discussed above.