Verifiable vs VerifyNoOtherCalls

[uecasm]

Using Moq 4.8.1 and NUnit 3.9.0, with the following code:

public interface ICat
{
    string Purr(int amount);
}

[TestFixture]
public class Tests
{
    [Test]
    public void Purring1()
    {
        var cat = new Mock<ICat>();
        cat.Setup(x => x.Purr(15)).Returns("happy").Verifiable();

        var mood = cat.Object.Purr(15);

        cat.Verify();
        Assert.That(mood, Is.EqualTo("happy"));
        cat.VerifyNoOtherCalls();
    }

    [Test]
    public void Purring2()
    {
        var cat = new Mock<ICat>();
        cat.Setup(x => x.Purr(15)).Returns("happy");

        var mood = cat.Object.Purr(15);

        cat.Verify(x => x.Purr(15));
        Assert.That(mood, Is.EqualTo("happy"));
        cat.VerifyNoOtherCalls();
    }
}

Purring2 passes as expected. Purring1 unexpectedly fails with:

Moq.MockException : The following invocations were not verified:
ICat.Purr(15)
   at Moq.Mock.VerifyNoOtherCalls(Mock mock) in C:\projects\moq4\Source\Mock.cs:line 393

Why doesn't Verify() count as having verified the Verifiable() calls?

[stakx]

Yes, this can be unexpected, but this behaviour is by design.

When I got started with Moq, it took me a long time to realise that there are actually two distinct forms of Verify-ing calls that do rather different things:

1. The unparameterized mock.Verify[All]() methods simply check whether all setups have been invoked at least once. This form of verification targets the setups of a mock.

2. The parameterized mock.Verify(callExpression[, …]) method group checks whether a particular kind of call (as described by the expression) occurred. This form of verification targets the actual calls that a mock object received.

VerifyNoOtherCalls is a relatively new addition to the Moq API that belongs with the second kind of verification. For this reason, its operation isn't affected by any calls to Verify or VerifyAll.

If you want VerifyNoOtherCalls for the first kind of verification, simply use a Strict mock instead. Strict mocks don't allow any calls other than those you set up, so you'll automatically have a guarantee that there were no other calls, so there won't be any need for a VerifyNoOtherCalls.

I agree that it would perhaps seem more consistent to have VerifyNoOtherCalls also consider any previous Verify[All] calls. But this would only ever make a difference in those cases where one is mixing two different styles of call verification, which is probably not a terribly good thing to do anyway.

[uecasm]

I thought the motivation for introducing VerifyNoOtherCalls in the first place (per #527) was to make strict mocks go away? ie. it was intended as a complete replacement for them. Thus difference of behaviour between them seems like a bug.

I understand what you're saying about targeting the setups vs. targeting the actual calls, but this seems like an internal implementation detail that users of Moq shouldn't have to care about. Especially because both methods are called Verify, and the error message states that the calls were unverified. This is a meaningless distinction to a user.

[stakx]

I thought the motivation for introducing VerifyNoOtherCalls in the first place (per #527) was to make strict mocks go away?

No, that wasn't the motivation behind VerifyNoOtherCalls.

The motivation was to give those people who don't like strict mocks (i.e. those who want to adhere to strict AAA-style testing without having to mix up the Arrange and Assert phases) a tool to do (some of) what strict mocks can do.

Different people have different preferences when it comes to testing style, so VerifyNoOtherCalls isn't a replacement for strict mocks—it's an alternative.

The problem with your Purring1 test is that it mixes up strict mock-style testing (setting up expected calls in the Arrange phase, then verifying all in one go in the Assert phase) with more strict AAA-style testing (verifying all expectations only in the Assert phase). This kind of mixed up scenario isn't one we should be supporting, so basing a discussion towards changing Moq's API on it isn't a good idea IMHO.

I understand what you're saying about targeting the setups vs. targeting the actual calls, but this seems like an internal implementation detail that users of Moq shouldn't have to care about.

I agree that there shouldn't be much of a noticeable difference between Verify[All]() and other forms of Verify. Fact is, those differences do exist. Moq's API simply isn't 100 % logical and consistent everywhere. I don't see a way of explaining all forms of Verify in a simple, unified way that matches with how these methods actually work.

[uecasm]

The problem with your Purring1 test is that it mixes up strict mock-style testing (setting up expected calls in the Arrange phase, then verifying all in one go in the Assert phase) with more strict AAA-style testing (verifying all expectations only in the Assert phase).

Those are not two different things, they are the same thing.

Setup in the arrange phase is required in order to establish a return value. There's simply no way that can be avoided.

Given that you have to specify the entire method call in the Setup, it seems overly painful to require duplicating the exact same signature in the assert phase, especially when something exists (namely Verifiable()) to indicate "hey, I want to assert on this later".

So, we have two different ways that the test can be written, as shown in the original post. Both of them are pure Arrange, Act, Assert; Purring2 does all the asserts individually (repeating the signature for no benefit) while Purring1 uses a shortcut to say "assert those things I told you to remember during arrange".

If you're only verifying void methods that don't need any Setup, then sure, use the explicit Verify, no problem. But if you have to spell it out at the start anyway, why do it twice when you can just say "remember this for later"?

I can't understand how anyone could possibly argue that Purring1 is "wrong" and that Purring2 is "better".

If your argument is that Verifiable isn't an arrange thing, I disagree because it is still arranging the test (just like defining a return value); it is marking something to be asserted later, not actually doing any assertions now. The assertions actually happen in the assert phase, as expected. (And if you want strict AAA, it does this "better" than a strict mock does.)

Also, VerifyNoOtherCalls must be used in the manner shown here. Its entire reason for existence is to be called in the assert phase to cause failure at that point, instead of creating a strict mock that will fail during the act phase instead. You cannot use it at all in a non-strict test, by definition.

[stakx]

If your argument is that Verifiable isn't an arrange thing, I disagree [...]

Perhaps I'm reading you incorrectly, but I am getting the strong impression that you're not really interested in what any possible argument contrary to yours would've been anyway, so I don't see much of a point in laying out such an argument.

I can't understand how anyone could possibly argue that Purring1 is "wrong" and that Purring2 is "better".

Moq offers different things to different people. I personally lean towards the "clean AAA separation" side, where strict mocks and things like .Verifiable()—essentially a shortcut that avoids repetition of call expressions in the Arrange and Assert stages—has no place (or at least is less appealing). But that's just my preference. Use the API in a way that makes sense to you, and let others make their own choice.

As you can probably tell, I'd prefer not to have an extended debate about what is the right way of testing and what isn't.

Why doesn't Verify() count as having verified the Verifiable() calls?

I think I've answered your question above. So unless there's anything else I'd like to close this issue.

[stakx]

(Sorry, I didn't actually mean to press the close button already. 😱 Is there something else?)

[uecasm]

Perhaps I'm reading you incorrectly, but I am getting the strong impression that you're not really interested in what any possible argument contrary to yours would've been anyway, so I don't see much of a point in laying out such an argument.

It's more that I haven't seen any argument to the contrary yet, and you sound dismissive of the whole thing without really saying why. Perhaps I'm missing something in what you've said, not being as familiar with the code... but in this case I wonder if perhaps you're letting your familiarity of the code internals cloud a design decision about the front-end API. It happens to the best of us.

I understand that there are a list of setups and a list of actual calls, and that VerifyNoOtherCalls currently checks one and not the other. I don't understand why you seem to argue that this is a good reason to not fix this.

Ok, maybe the APIs were originally created for different purposes. That doesn't change that fact that the APIs exist, and it makes sense to use them in the manner I showed in Purring1, and in my opinion it does make Purring1 a better and more readable test than Purring2. Ultimately readability of tests is paramount (below correctness). AAA is itself primarily a way to make tests more readable by giving them a consistent layout. Don't Repeat Yourself is a fundamental principle, even in tests.

Added to this, the current behaviour of VerifyNoOtherCalls simply doesn't make sense. "This failed because you didn't verify the thing that you verified." Perhaps this was due to a poor choice of similar verbs for different things, but again, they exist now as is. Why not take the opportunity to make this behave more consistently?

Even if you personally prefer the Purring2 style, there are other people that prefer other styles, and this seems like a legitimate bug.

But that's just my preference. Use the API in a way that makes sense to you, and let others make their own choice.

This is what I am suggesting. I don't see anything that would break from this suggestion, only be improved. It would not affect anyone who writes their tests in a different way.

[stakx]

@uecasm, some good points there, and I see that we are already in agreement on a couple of them. Let me ponder on this for a night or so, I'll try to reply tomorrow. 😃

[Code-Grump]

To clarify:

Given a mock with one or more setups
And given an unexpected call was made to the mock
When Verify() or VerifyAll() are called
And when VerifyNoOtherCalls() is called
Then an exception should been thrown with details of the unexpected call

Is this correct?

[uecasm]

Not quite.

Calling Verify() should only apply to setups made using Verifiable(). while VerifyAll() applies to all setups, and VerifyNoOtherCalls() applies to anything that was not otherwise verified.

In the example at the top, VerifyNoOtherCalls should not have thrown an exception because a call was setup, made, and verified.

Had the call not been made, then Verify() should have thrown.
Had the Verify() not been called, then VerifyNoOtherCalls() should have thrown if the call was made anyway (unless explicitly verified with the longer form).
If the setup was not made or not Verifiable(), then Verify() does nothing, which in turn should cause VerifyNoOtherCalls() to throw if the call was made anyway (unless explicitly verified with the longer form).
Had VerifyAll been called instead of Verify, then nothing changes except that Verifiable is now optional on the setup.

Arguably, had the call been made twice, then either Verify() or VerifyNoOtherCalls() should have thrown, unless a higher cardinality was explicitly specified. (Either one is sufficiently correct and which would occur is an implementation detail.) This is more debatable but ties into what I was suggesting in a different issue that Verifiable() ought to imply an ExactlyOne cardinality rather than an AtLeastOne. (Or possibly higher cardinalities for sequence setups, but still bounded). You can ignore this as off-topic for the purposes of this issue, however.

[Code-Grump]

@uecasm You're right, the cardinality implied by Verifiable() is a separate topic; let's focus on VerifyNoOtherCalls() within the existing verifiable behaviour. If you'd like to see Verifiable() extended to allow specifying ExactlyOnce, we should talk about it in a separate issue.

As described, I don't think it's possible for VerifyNoOtherCalls() to work the way you want without some unwanted side-effects. The trouble is the difference between the Verify() and VerifyAll() models:

• In the case of using Verify() we only want things explicitly marked as Verifiable() to be considered. Internally, that means Moq maintains a list of setups it expects to verify and any invocations outside this list would cause VerifyNoOtherCalls() to throw an exception.
• In the case of using VerifyAll() we want to consider all setups, marked as verifiable or not, to be considered. Internally, Moq uses its list of setups and ignores any verifiable flags, and VerifyNoOtherCalls() is now expected to consider the complete list of setups, not just the ones marked as verifiable.

What we're asking for is the behaviour of the Moq to switch as a side-effect of using Verify() or VerifyAll(). I don't think subtle behaviour changes as a side-effect of a non-obvious call is a particularly good experience to debug, no matter how much it might make sense to read.

I can see some possible solutions by which we could make this work:

1. Create a method like VerifyNoOtherCalls() which works as the explicit partner to VerifyAll()
2. Make it explicitly illegal to mix Verify() and VerifyAll() in the API. If you called one, then the other on a mock, we'd want to throw an exception making it clear that this model of interaction isn't supported. Then we can have these methods throw an internal switch that allows VerifyNoOtherCalls() to switch between these behaviours.
3. Drastically change the API so you have to declare which model of verification you're following up-front.

Let me know if you have any thoughts to these.

[stakx]

The trouble is the difference between the Verify() and VerifyAll() models: [...]

@Tragedian, perhaps I am misunderstanding something, but I think this could be resolved rather neatly in a different fashion. Look at it this way:

1. Both Verify() and VerifyAll() select some set of setups that they verify. Let's call that set S.
2. For each setup s in S, check whether s was invoked at least once. If not, trigger a verification error. (This is how the two methods work today.)
3. If no error has been triggered, i.e. s has been successfully verified, find the set of all invocations I that correspond to s.
4. Mark each invocation i in I as successfully verified. (This is what would make VerifyNoOtherCalls() work as @uecasm expects it.)

The key point is to mark invocations as verified during the call to Verify() or VerifyAll(), so VerifyNoOtherCalls won't have to know which one (all setups vs. only verifiable setups) was used. VerifyNoOtherCalls can just continue looking at invocations only, as it does today.

One thing I am not sure about is whether verification should be an atomic operation. That is, if you do a batch verification via MockRepository.Verify[All](), and if one out of several setups trigger a verification error, should the invocations corresponding to the other successfully verified setups still be marked as verified?

The price for all this would be a much higher runtime cost for Verify() and VerifyAll(), since each call to those would now have to match a setup against all invocations. (Currently, it only compares a single number against 0.) I cannot say at this point whether this is acceptable in real-world scenarios or not.

[stakx]

@uecasm You're right, the cardinality implied by Verifiable() is a separate topic; let's focus on VerifyNoOtherCalls() within the existing verifiable behaviour. If you'd like to see Verifiable() extended to allow specifying ExactlyOnce, we should talk about it in a separate issue.

(I agree, but starting a new issue isn't necessary—this has been discussed plenty of times before. See e.g. the issues referenced in #401, which we could just reactivate. And I repeat once again that we absolutely cannot simply change the implicit default from Times.AtLeastOnce to something else. That would be a massively breaking change, it's much too late for that.)

[Code-Grump]

@stakx To be clear, I'm not questioning how this could be implemented, just whether it's a model that makes sense. I don't feel comfortable with the sequencing required here.

var mock = new Mock<IComparable>();
mock.Setup(o => o.CompareTo(It.IsAny<object>());

// Run my test.

mock.VerifyNoOtherCalls();
mock.VerifyAll();

What is the expected behaviour here?

[stakx]

@Tragedian: Nice example. I tend towards thinking that here, it's the tester's own fault for writing a test that doesn't make too much sense—it is a test that'll likely fail no matter what, due to contradictory assertions. If there was a call to CompareTo, the first assertion will fail (but the second would pass). If there has been no call, the first assertion will pass, but the second won't.

Do you think this pattern is one that people would be likely to write often?

[Code-Grump]

So what about introducing VerifyOnlySetupsCalled() or something which would function as VerifyNoOtherCalls()? I don't love adding "duplicate" functionality to an API (it does make it more confusing than it ought to be), but it would allow you to run my scenario if you replaced the call to VerifyNoOtherCalls().

[stakx]

So what about introducing VerifyOnlySetupsCalled() or something which would function as VerifyNoOtherCalls()?

I don't follow. What would that new method do? (The current VerifyNoOtherCalls has absolutely nothing to do with setups, so if that new method would be identical, it would be horribly misnamed.)

[Code-Grump]

Rather than looking at the invocations and check to see if it has been explicitly verified, it could look at the invocations and verify they all match the setups, in the same way that VerifyAll() does. The different being that where VerifyAll() tests to ensure all the setup methods have been invoked, this checks the inverse: that only the setup methods have been invoked.

It feels like the intended outcome of this work is to get to a place where you're asserting that the mock was used in exactly the specified fashion, like strict mocks model. To that end, we might want to look at a VerifyExact() or something which encompasses the behaviour of VerifyAll() and the intent of VerifyNoOtherCalls().

[uecasm]

mock.VerifyNoOtherCalls();
mock.VerifyAll();

Expected behaviour is simple:

• VerifyNoOtherCalls() will report failure if any non-verified calls are made. Since no calls have been verified yet, this means it will fail if any calls at all have been made.
• VerifyAll() will report failure if any methods that have been Setup have then never been called. Since this can only be reached if there are no calls (due to the above), it will always fail unless there are no Setups.

In short, this is silly test code, but it is silly in a blindingly obvious way. VerifyNoOtherCalls must obviously be the last verification that is done -- nothing else ever makes sense.

[stakx]

Rather than looking at the invocations and check to see if it has been explicitly verified, it could look at the invocations and verify they all match the setups, in the same way that VerifyAll() does.

No, that would defeat the purpose of why VerifyNoOtherCalls was added in the first place. Remember that there aren't always any setups present.

[...] this checks the inverse: that only the setup methods have been invoked.

This is what mocks with MockBehavior.Strict do already.

For folks who don't like those, I suppose calling both VerifyAll and VerifyNoOtherCalls (in that order) would be an acceptable compromise (assuming that the changes proposed in this issue are implemented), I see no need to add a new VerifyOnlySetupCalls method. (Like you're saying, let's try to avoid functional duplication.)

[uecasm]

Expected usage is to use a combination of Setup(...).Verifiable() with Verify() plus additional Verify(...), optionally finishing with VerifyNoOtherCalls().

If VerifyNoOtherCalls() is used, this is a strict test. If it is not used, it's a loose test.

It doesn't really make sense to use VerifyAll in the same test as VerifyNoOtherCalls. You could, and it should behave consistently with the definitions, but it's still not sensible to actually write a test that way. But VerifyAll is still useful in other tests.

This is what mocks with MockBehavior.Strict do already.

Yes, but some people didn't like using that, so VerifyNoOtherCalls was invented. Look back at the issue that introduced it, it was explicitly for writing strict tests that didn't use MockBehaviour.Strict.

[stakx]

@uecasm: I agree with all you've just said. (Regarding the last paragraph, I've edited my post above while you were replying.)

[uecasm]

It doesn't really make sense to use VerifyAll in the same test as VerifyNoOtherCalls.

Actually, I take that back. It could make sense if every method you've Setup should be called at least once; then you don't need to mark them individually as Verifiable. (I tend to put some standard stubs in the test fixture's SetUp, so there's usually something that might not be called by some test, so I don't personally use VerifyAll much.) But again yes, VerifyNoOtherCalls must be last, and this ought to be obvious to everyone from the name alone.

[Code-Grump]

Personally, I don't like an API which requires a specific call-sequence to behave reasonably. Verify and VerifyAll contain a state transition which isn't obvious to a caller. If Moq had a "fluent" verify API which better modelled this state-transition, I think I would be more comfortable. Something like:

mock
    .VerifyAll()
    .AndNoOtherCalls();

Where AndNoOtherCalls() would prevent further method-chaining, making it clear it has to be the last call.

That grievance aired, I think I understand what we want to achieve here and I can create a pull request to upgrade VerifyNoOtherCalls() to do what's asked. I don't have to like it to see how it's useful. 👍

[uecasm]

"Verify No Other Calls" explicitly states that it verifies that there are no other calls, which in turn heavily implies (due to "other") that calls which are already verified are excluded, and it will only object to calls made but not verified. (Which is indeed how it behaves with explicit verify, just not previously with no-args Verify.) This also implies that anything else you wanted to verify has to be done first, because VerifyNoOtherCalls would treat it as an "other call" and error out otherwise. I don't see how that could possibly be surprising to anyone.

[stakx]

Personally, I don't like an API which requires a specific call-sequence to behave reasonably.

@Tragedian, I never heard any complaints about the fact that you have to do Setup before calling the mock before calling Verify. There's also a specific order on setups for the various verbs like Callback, Returns etc. "Specific call sequence" already exists all over the place.

Verify and VerifyAll contain a state transition which isn't obvious to a caller.

You mean, after we make this change, or before?

I agree once more with @uecasm here... Verify and VerifyAll changing the mock's state (marking invocations as verified) will be unnoticeable unless you also use VerifyNoOtherCalls. And if you do that, its name should provide a strong enough cue about the proper call sequence.

[stakx]

@uecasm, @Tragedian - I've just pushed Moq 4.10.0 (which includes the improvement for VerifyNoOtherCalls) to NuGet. It should be available shortly.