VerifyNoOtherCalls fails due to calls on another Mock instance

[Zaeranos]

Recently for the company I work at, we upgraded the version of Moq from 4.8.3 to 4.12.0, due to reasons of other upgrades. However by doing this a number of our unit tests started to fail all of a sudden. After some investigation we found out that the unit tests fail due to VerifiyNoOtherCalls(). And it does not fail because there were other calls made to the checked mock, but calls made to another mock entirely.

I made a simplified version of the code in our product to see what the problem seems to be. The other mock that it fails on is connected to the mock that is verified, because the mock that is verified is setup to return that specific mock in a function call.

This is the simplified code I used to test this, and as an added note, we use NUnit for the testing framework.
Subject under test

public class SubjectUnderTest
{
    private readonly IUiInputService _uiInput;
    private readonly IServiceFactory _factory;
    private readonly IStaticService _staticService;

    public SubjectUnderTest(IUiInputService uiInput, IServiceFactory factory, IStaticService staticService)
    {
        _uiInput = uiInput;
        _factory = factory;
        _staticService = staticService;
    }

    public void Execute()
    {
        var service = GetService();
        service.Start();

       _staticService.DoSomething();

       service.Stop();
    }

    private IDynamicService GetService()
    {
        switch (_uiInput.GetInput())
        {
            case UiInput.Option2:
                return _factory.GetServiceWithSet2();
            case UiInput.Option3:
                return _factory.GetServiceWithSet3();
            default:
                return _factory.GetServiceWithSet1();
        }
    }
}

public enum UiInput
{
    Option1,
    Option2,
    Option3
}

My test classes

public class SubjectUnderTestTestsFixture
{
    public SubjectUnderTestTestsFixture()
    {
        UiInputMock = new Mock<IUiInputService>();
        FactoryMock = new Mock<IServiceFactory>();
        StaticServiceMock = new Mock<IStaticService>();

        DynamicMock = new Mock<IDynamicService>();
        FactoryMock.Setup(mock => mock.GetServiceWithSet1())
            .Returns(DynamicMock.Object);
    }

    public Mock<IUiInputService> UiInputMock { get; }
    public Mock<IServiceFactory> FactoryMock { get; }
    public Mock<IStaticService> StaticServiceMock { get; }
    public Mock<IDynamicService> DynamicMock { get; }

    public SubjectUnderTest CreateSut()
    {
        return new SubjectUnderTest(UiInputMock.Object, FactoryMock.Object, StaticServiceMock.Object);
    }
}

[TestFixture]
public class SubjectUnderTestTests
{
    [Test]
    public void Execute_Should_GetService1_When_InputIsOption1()
    {
        // Arrange
        var fixture = new SubjectUnderTestTestsFixture();
        var sut = fixture.CreateSut();

        fixture.UiInputMock.Setup(mock => mock.GetInput())
            .Returns(UiInput.Option1);

        // Act
        sut.Execute();

        // Assert
        fixture.FactoryMock.Verify(mock => mock.GetServiceWithSet1(), Times.Once);
        fixture.FactoryMock.VerifyNoOtherCalls();
    }

    [Test]
    public void Execute_Should_GetService2_When_InputIsOption2()
    {
        // Arrange
        var fixture = new SubjectUnderTestTestsFixture();
        var sut = fixture.CreateSut();

        fixture.FactoryMock.Setup(mock => mock.GetServiceWithSet2())
            .Returns(fixture.DynamicMock.Object);
        fixture.UiInputMock.Setup(mock => mock.GetInput())
                .Returns(UiInput.Option2);

        // Act
        sut.Execute();

        // Assert
        fixture.FactoryMock.Verify(mock => mock.GetServiceWithSet2(), Times.Once);
        fixture.FactoryMock.VerifyNoOtherCalls();
    }

    [Test]
    public void Execute_Should_GetService3_When_InputIsOption3()
    {
        // Arrange
        var fixture = new SubjectUnderTestTestsFixture();
        var sut = fixture.CreateSut();

        fixture.FactoryMock.Setup(mock => mock.GetServiceWithSet3())
            .Returns(fixture.DynamicMock.Object);
        fixture.UiInputMock.Setup(mock => mock.GetInput())
            .Returns(UiInput.Option3);

        // Act
        sut.Execute();

        // Assert
        fixture.FactoryMock.Verify(mock => mock.GetServiceWithSet3(), Times.Once);
        fixture.FactoryMock.VerifyNoOtherCalls();
    }
}

To me this seems like a bug. When I call VerifyNoOtherCalls on a specific mock I expect it to only check that one mock for no other calls. When it should check everything I personally expect something like Mock.VerifyNoOtherCalls.

For now we fixed this issue by either verifying the calls that it fails on of the other mock instance, or by verifying each individual function on the verified mock. However this invalidates my tests as soon as the code gets expanded.

[stakx]

(Duplicate of #858.)

Thanks for taking the time to report this.

The other mock that it fails on is connected to the mock that is verified, because the mock that is verified is setup to return that specific mock in a function call.

That change was made intentionally, however I think we forgot to mention it in the changelog.

To me this seems like a bug. When I call VerifyNoOtherCalls on a specific mock I expect it to only check that one mock for no other calls.

I understand, and that makes perfect sense. However, note that restoring this behavior would re-introduce an inconsistency in the verification APIs. Here's the line of thinking that led to the "bug" / behavior that you're seeing now:

• Verify and VerifyAll have always worked in a recursive fashion, say when you have a mock.Setup(m => m.SubObject.X()), then Moq created a mock sub-object and included this sub-mock in mock.Verify.

  (Due to how implicit sub-mock creation was implemented internally, this recursive verification sometimes worked and sometimes didn't. I recently tried to straighten things out as much as possible, within the constraints defined by pre-existing unit tests.)

• Why should it make a difference whether you set up such a sub-mock implicitly (as shown above) or explicitly, using mock.Setup(m => m.SubObject).Returns(new Mock<ISubObject>().Object)?

  That was an inconsistency that got fixed recently: The outer mock will include the inner mock whwn verifying, regardless* of how exactly the relationship was set up. (See final part of this post regarding the *.)

• If Verify and VerifyAll work in a recursive manner as just shown, why shouldn't the same be true for VerifyNoOtherCalls?

  That's yet another inconsistency that recently got fixed: All verification methods work in the same way with respect to recursiveness.

  VerifyNoOtherCalls should have been implemented that way right when it was first introduced, however that wasn't easily possible back then. Lots of refactoring had to be done for 4.10/4.11 to remove all these inconsistencies.

Now I'm not saying that this is necessarily a sensible state of things, however given a choice between breaking the long-time behavior of Verify[All] and the behavior of the relatively new VerifyNoOtherCalls, I obviously opted for the latter.

I'm mentioning all this in such detail in the hope to get opinions about how verification would ideally work.

For now we fixed this issue by [...]

You can also fix this by changing setup.Return(anotherMock.Object) to setup.Return(() => anotherMock.Object), which for now works because Moq will not call into user-supplied lambdas (which could have side effects) just to discover relationships between mocks.

[stakx]

P.S.: One alternate way of looking at this would be that mocks "own" setups instead of sub-mocks, and only the owned setups get included in verification. Setups might then "target" a mock that is different from the one "owning" it.

(Which leads to a whole bunch of new questions such as, "When setup S owned by mock A sets up mock B, and you inspect B's setups, will you see S? Or do you only see S when inspecting A's setups?", or, "When you verify B, will S be included? Or only when you verify A?")

This seems like a promising perspective, but I have no idea whether it would pass existing unit tests.

[Zaeranos]

Thank you for the comment, and I'm sorry for the duplication. I've tried the solution and it does work, thank you for that.

If you don't mind me making a suggestion:
Like I said to me it does not make much sense that it looks at other instances as well, when I ask it to Verify on just that instance. In my mind mocks are not connected or owned by each other, unless explicitly stated as such. To me, mocks are owned by the test. However I can understand that some may view things different than me.

So my suggestion is maybe to give VerifyNoOtherCalls an optional parameter stating the VerifyBehaviour in a similar sense as MockBehaviour. With options as 1) recursive (default), and 2) nonrecursive. In this manner the developer can make it explicit what it verifies, because "() => mock" seems not that explicit. At least if it is possible. And unintentionally it is possible to have both the earlier behavior and the intended behavior for VerifyNoOtherCalls.

[stakx]

@Zaeranos:

mocks are not connected or owned by each other, unless explicitly stated as such. To me, mocks are owned by the test.

Well said, I agree.

Thank you for the suggestion. I'll run a few tests over the weekend. Perhaps we can have our cake and eat it by fixing mock/setup ownership. An optional parameter (switch) should only be added as a last resort, I think.

[stakx]

I have a working prototype for the proposal ("mocks own setups, not other mocks").

Unfortunately, during its development, I stumbled over the following unit test (among a few others of the same kind):

https://github.com/moq/moq4/blob/a12fe5b1fc2eb7857799d5a7bb5cc9e18f61aace/tests/Moq.Tests/MockDefaultValueProviderFixture.cs#L77-L88

which has been there for at least 6 years and specifies that when it comes to verification, mocks indeed "own" other mocks.

Making the change that we've been discussing here would therefore be a breaking functional change, unfortunately.