Moq4: Verify that no unverified methods were called (alternative to Strict)

[ashmind]

My goal in few specific tests is to verify that no unverified methods were called, for example:

var mailSender = new Mock<IMailSender>();

DoSomething(mailSender.Object);

mailSender.Verify(s => s.SendSpecificThing(), Times.Once);
mailSender.VerifyNotOtherCalls();

I know I can do this with Strict, but I don't like how Strict affects the test structure and moves some expectations into the "Arrange" part of the test (Setup().Verifiable()). It seems like Verify(call) is designed to provide a nicer alternative to Strict, so it would be nice to have that as well.

Alternatively, some kind of method matcher in Verify would work as well -- e.g.
mailSender.VerifyAny(m => m.Name != nameof(SendSpecificThing), Times.Never);

[stakx]

I don't like how Strict affects the test structure and moves some expectations into the "Arrange" part of the test [...].

Can you please elaborate a little? What expectations (say: setups) do you find acceptable in the "arrange" part, and which do you want to avoid?

You also mention .Verifiable(). That has nothing to do at all with a mock's behavior. It simply flags a setup so it's included in a verification via mock.Verify() (as opposed to the non-discriminating mock.VerifyAll() which always checks all setups). Why did you mention .Verifiable() in particular?

[ashmind]

Can you please elaborate a little? What expectations (say: setups) do you find acceptable in the "arrange" part, and which do you want to avoid?

Use -- setups that arrange things -- provide meaningful behavior or state related to the test in question. E.g. "set up the repository to return a specific User object from Get(id)", or "set up file Save to throw a specific exception".

Avoid -- setups that are expectations -- do nothing, but prepare for the future Verify() or VerifyAll() and appease the strict mock.

Why did you mention .Verifiable() in particular?

Sorry for the confusion, I forgot that VerifyAll() doesn't need it. The main thing I want to avoid is an expectation-like Setup() in arrange, so the point stands even if I don't do Verifiable().

[stakx]

@ashmind - I like this idea quite a lot. I have two questions:

1. Would this feature take Moq in the right direction?

Is it wise to introduce an alternative to Strict mocks? The two distinct mock behaviors are a central feature of Moq. Is it a good idea to "weaken" this core distinction? Is it good, from a usage and maintenance point of view, to have several different ways to achieve the same thing? If not, does it mean MockBehavior.Strict ought to become [Obsolete] if this new feature makes it in? etc.

Those are questions about the direction where we want to take Moq. I personally think it would be better to have a slim, opinionated API, but perhaps in reality, Moq is more attractive if it's unopinionated and caters for different tastes, even if that makes the API surface more complex and somewhat "messier". Ideally, the decision would be made by the user community.

2. How would this feature be implemented?

Let's take a quick look at the two syntaxes that you've proposed above:

var mailSender = new Mock<IMailSender>();

DoSomething(mailSender.Object);

mailSender.Verify(s => s.SendSpecificThing(), Times.Once);
mailSender.VerifyNotOtherCalls();

For this syntax to work, verification would have to become an operation that changes state. Specifically, each Verify call would have to flag matching invocations as "verified", so that the final call to VerifyNoOtherCalls (or whatever it would be called) could check whether there are still any unflagged invocations left. This flagging behaviour would only work for one logical group of Verify* calls.

For that reason, it's not perfect from a design standpoint, but might work just fine in practice.

Implementation should be perfectly feasible, and probably even fairly easy. (If it's decided that this feature should be implemented, I'm happy to offer some guidance, if desired.)

mailSender.VerifyAny(m => m.Name != nameof(SendSpecificThing), Times.Never);

With this approach, verification could remain an operation that doesn't change state. On the other hand, I believe it's not a good fit for Moq's API because one of the explicit design goals of Moq is to have a statically, strongly-typed API. Like the .Protected() function group, resorting to nameof and a more reflection-like API goes against that goal.

I'd be interested to hear what others think of this proposed feature.

[BlythMeister]

i think this does mean a strict mock should be obsolete.
It's taking all the greatness of a strict mock (ensuring that only things you expect to be called are called) but without the fanfare of setting up an expectation that something is called followed by a verification that it was the correct number of times.

In addition to this, one thing i have never really liked with a strict mock is the way that something fails during the test execution as it's not stubbed/setup rather than as part of verification as it was called when it shouldn't have been.

I personally would prefer the "VerifyNoOtherCalls" approach as a mocked interface whereby you expect a few things to be called would make the verifyany with never a bit cumbersome.
But i can see where your coming from that this is harder to implement.

[stakx]

@ashmind, @BlythMeister - I got curious, so I implemented a quick and dirty version of this feature: see https://github.com/moq/moq4/compare/develop...stakx:verify-no-other-calls.

Example:

public interface IFoo
{
    void X(int n);
    void Y();
}

[Fact]
public void Can_Verify_any_calls_then_ensure_no_other_calls_were_made_with_VerifyNoOtherCalls()
{
    var mock = new Mock<IFoo>();
    mock.Object.X(1);
    mock.Object.X(2);
    mock.Object.X(3);
    mock.Object.Y();

    mock.Verify(m => m.X(AnyEvenInt));
    mock.Verify(m => m.Y());
    mock.VerifyNoOtherCalls();
}

static int AnyEvenInt => Match.Create<int>(x => x % 2 == 0);

Unhandled Exception: Moq.MockException: Unexpected invocations:
IFoo.X(1)
IFoo.X(3)
   at Moq.Mock.VerifyNoOtherCalls(Mock mock) in ...\Mock.cs:line 421
   at Moq.Mock`1.VerifyNoOtherCalls() in ...\Mock.Generic.cs:line 522
   at ...

This would obviously need a little cleaning up and some optimizations. But is this what you had in mind?

[BlythMeister]

Yes with 1 little tweak.
If you make
Match.Create(x => x < 2);
And say Times.Exactly(2)
But that's spot on to what in would like

[stakx]

@BlythMeister - that would work just as well:

    ... // (mock instantiation and invocations as above)
    mock.Verify(m => m.X(AnyIntLessThan(3)), Times.Exactly(2));
    mock.Verify(m => m.Y());
    mock.VerifyNoOtherCalls();
}

static int AnyIntLessThan(int n) => Match.Create<int>(x => x < n);

Unhandled Exception: Moq.MockException: Unexpected invocations:
IFoo.X(3)

[BlythMeister]

I assumed it would, but wanted to be sure.

This feature is the one which will make me spend the time converting my test suite from Rhino to Moq 😁

I prepared to accept having to "Setup" and then verify times on returns, but not having to setup voids and still get the benefits of a strict mock makes me a very happy man.

[willsmith9182]

This makes me feel warm inside

[stakx]

Let's do this. A VerifyNoOtherCalls is obviously a nice to have feature. We can still decide later whether or not to make MockBehavior.Strict. For now, let's leave it as it is.

[stakx]

Probably closed this a little too early. The current implementation probably doesn't yet work correctly...

• [x] for mocks created with Mock.Of<T>
• [x] with multi-dot ("recursive") verification expressions.

Need to make sure the above are properly covered.

[stakx]

@ashmind, @BlythMeister, @willsmith9182 - I just published a pre-release version 4.8.0-rc1 on NuGet. It contains VerifyNoOtherCalls (among other things). Feel free to test away.

[BlythMeister]

@stakx i know i'm probably pushing it...but how about VerifyNoOtherCalls on a mock repository as well :)

I've been creating all my mocks off a mock repository and then calling repo.VerifyAll()
So from a usability point of view doing the VerifyNoOtherCalls on the repository feels useful.

[stakx]

@BlythMeister - Good question. I see the similarity to MockRepository.Verify[All], but does a MockRepository.VerifyNoOtherCalls() actually make sense? Calling mockRepository.VerifyNoOtherCalls() would always throw a MockException, except in the single case where none of the mocks were invoked at all.

If your repository mocks were invoked, however, you'll have to mock.Verify those invocations in order for mockRepository.VerifyNoOtherCalls() to succeed. Now that you already verify at the mock level, why not mock.VerifyNoOtherCalls() at the mock level as well?

[BlythMeister]

as an example, this is what i do

public interface IDependency
    {
        void DoStuff();
    }

    public class TestSystem
    {
        private readonly IDependency dependency;

        public TestSystem(IDependency dependency)
        {
            this.dependency = dependency;
        }

        public void Execute()
        {
            dependency.DoStuff();
        }
    }

    [TestFixture]
    public class TestSystemTests
    {
        [Test]
        public void MyTest_WithStrict()
        {
            //Arrange
            var mockRepository = new MockRepository(MockBehavior.Strict);
            var mockDependency = mockRepository.Create<IDependency>();
            var sut = new TestSystem(mockDependency.Object);

            mockDependency.Setup(x => x.DoStuff());

            //Act
            sut.Execute();

            //Assert
            mockDependency.Verify(x => x.DoStuff(), Times.Once());
            mockRepository.VerifyAll();
        }

        [Test]
        public void MyTest_WithLoose()
        {
            //Arrange
            var mockRepository = new MockRepository(MockBehavior.Loose);
            var mockDependency = mockRepository.Create<IDependency>();
            var sut = new TestSystem(mockDependency.Object);

            //Act
            sut.Execute();

            //Assert
            mockDependency.Verify(x => x.DoStuff(), Times.Once());
            mockRepository.VerifyNoOtherCalls();
        }
    }

In the loose test, i am basically saying of all the mocks created, i only want the mocks i have already verified to be called.

So, if my code called the "DoStuff" method twice, i want it to fail.
If i called any other method on IDependency i want it to fail
If i call any method on any other mock created by the mock repository, i want it to fail.

Essentially, the behaviour to mirror a strict mock factory, but without the need to "Setup" on voids.

[stakx]

So, if my code called the "DoStuff" method twice, i want it to fail.

Why not simply mockDependency.VerifyNoOtherCalls();, then?

If i call any method on any other mock created by the mock repository, i want it to fail.

There are no other mocks in your example. Why would you even instantiate mock objects that you don't need?

Unfortunately, your example doesn't really represent a realistic use case for why MockRepository.VerifyNoOtherCalls is needed. You can already do what you need without any additional effort simply by using Mock.VerifyNoOtherCalls.

[BlythMeister]

agree, this is a very simplistic example
but lets say i created 6 mocks from the mock repository.

Calling Mock.VerifyNoOtherCalls on each of these mocks would be very long winded (especially in every test.

Being able to state that i want to call it for every mock that the repository has created would be really useful in my opinion.

As an alternative, exposing a way that you can run any delegate against every mock the factory created would mean the user has full freedom.

[stakx]

Calling Mock.VerifyNoOtherCalls on each of these mocks would be very long winded (especially in every test.

Fair enough, I guess this comes down to personal preferences in how you write your tests. I was assuming that in each test, you would only instantiate the mocks that you're actually going to need—therefore there would be no superfluous VerifyNoOtherCalls calls.

I would prefer if we left MockRepository.VerifyNoOtherCalls out of version 4.8.0; first, simply because we're so close to the final release and I'd like to prevent further delays caused by additional features. Second, because I think it would be better to first see how people fare with Mock.VerifyNoOtherCalls, or if there's general demand for improvements. Feel free to open a new issue about adding MockRepository.VerifyNoOtherCalls so we can schedule it for a later milestone.

[BlythMeister]

Ok, i understand.

With regard to only creating mocks you need, it is more to satisfy the dependency being injected.
I guess i could inject a null if i don't expect it to be used, so your right with your personal preference statement.

[stakx]

With regard to only creating mocks you need, it is more to satisfy the dependency being injected.
I guess i could inject a null [...].

Your reasoning is similar to the OP's of https://github.com/moq/moq4/pull/390, who requested Mock.Of<T>(MockBehavior.Strict) for that purpose. (This call would give you a mock that throws for each and every invocation.) kzu's argument against it was that your tests—instead of relying too heavily on a particular mocking library's feature—should explicitly state their expectations in the form of assertions: If you you need to satisfy a dependency, and you don't really care what happens with it, just inject a loose mock. However, if it's important for your test's goal that this dependency doesn't actually get invoked, then you should perhaps explicitly state this expectation, e.g. via mockThatShouldNotBeInvoked.VerifyNoOtherCalls().

But in the end, that is probably also just a personal preference.

[BlythMeister]

Having just finished converting from rhino to Moq using strict factory I'm used to having a setup on void and a verify with times.

Arguably, it's verbose.
But also is verbosity in tests a good thing as it's clearer.

Doing it that way also makes you think more about the system under test and what it is doing, so creating lots of mocks but only using 1 probably means your class is too big or has too many responsibilities.

My only reasoning to add to the factory was because verify and verifyall were already there, so seemed like a logical extension on that. 🙂

[kzu]

In addition to this, one thing i have never really liked with a strict mock is the way that something fails during the test execution as it's not stubbed/setup rather than as part of verification as it was called when it shouldn't have been.

The reason why it behaves that way is that when an invocation is unexpected, you typically want to know right there and then, with a full stacktrace of the actual invocation so you can see (and fix) the test (or the code) as to why the invocation was performed. This typically requires inspecting the call stack and local variables since it's clearly an invocation you didn't expect.

I believe that no matter how nice and verbose the failed Verify makes it look, it would be very hard to diagnose why a call you didn't expect ended up happening anyway, without debugging and actually stopping at the offending call. Which is way easier to do if you just break on all exceptions and let the debugger take you there by just executing the test with a debugger attached ;)