VerifyNoOtherCalls verifies other mocks after upgrade from 4.10.1 to 4.11.0

[molszews]

having following piece of code

            repositoryMock.VerifyGet(r => r.UnitOfWork, Times.Once);
            repositoryMock.VerifyNoOtherCalls();
            unitOfWorkMock.Verify(r => r.SaveEntitiesAsync(It.IsAny<CancellationToken>()), Times.Once);
            unitOfWorkMock.VerifyNoOtherCalls();

it will break after doing Moq update with following exception:

Moq.MockException: Mock<IUnitOfWork:34>:
This mock failed verification due to the following unverified invocations:

   IUnitOfWork.SaveEntitiesAsync(CancellationToken)
    at Moq.Mock.VerifyNoOtherCalls(Mock mock, HashSet`1 verifiedMocks)
   at Moq.Mock.VerifyNoOtherCalls(Mock mock, HashSet`1 verifiedMocks)

i.e. in a line repositoryMock.VerifyNoOtherCalls(); which means it will now for some reason verifies all mocks at once. I assume it is due to the relation between these two, i.e.
repositoryMock.Setup(x => x.UnitOfWork).Returns(unitOfWorkMock.Object);

is that the intended behaviour?

[stakx]

I assume it is due to the relation between these two, i.e.
repositoryMock.Setup(x => x.UnitOfWork).Returns(unitOfWorkMock.Object);

is that the intended behaviour?

Yes.^(*) That setup establishes a kind of "ownership" relationship between repositoryMock (the owner) and unitOfWorkMock (the owned sub-mock). VerifyNoOtherCalls will automatically include such sub-mocks in its verification.

In Moq, verification via mock.Verify[All|NoOtherCalls] is generally a recursive operation; that is, all mocks reachable from the verification root mock (at least as far as Moq is able to discover such sub-mocks^(**)) will be automatically included. For VerifyNoOtherCalls, this initially wasn't the case, but has recently changed... though I'm surprised this isn't mentioned in the changelog.

I would guess that you could change your verification to either:

 repositoryMock.VerifyGet(r => r.UnitOfWork, Times.Once);
+unitOfWorkMock.Verify(r => r.SaveEntitiesAsync(It.IsAny<CancellationToken>()), Times.Once);
 repositoryMock.VerifyNoOtherCalls();
-unitOfWorkMock.Verify(r => r.SaveEntitiesAsync(It.IsAny<CancellationToken>()), Times.Once);
-unitOfWorkMock.VerifyNoOtherCalls();

or, to better express the "ownership" of repositoryMock over unitOfWorkMock, even:

 repositoryMock.VerifyGet(r => r.UnitOfWork, Times.Once);
-repositoryMock.VerifyNoOtherCalls();
-unitOfWorkMock.Verify(r => r.SaveEntitiesAsync(It.IsAny<CancellationToken>()), Times.Once);
+repositoryMock.Verify(r => r.UnitOfWork.SaveEntitiesAsync(It.IsAny<CancellationToken>()), Times.Once);
-unitOfWorkMock.VerifyNoOtherCalls();
+repositoryMock.VerifyNoOtherCalls();

---

^(*) Note however that this is more for consistency with the recursive behavior of the preexisting Verify[All] methods than for any other reason. When I implemented VerifyNoOtherCalls a year or so ago, I didn't consider this topic in great detail. It's entirely possible that recursive verification isn't always practicsl in all cases... I simply didn't know.

^(**) If you don't like the fact that Moq automatically includes the unitOfWorkMock sub-mock, you could make the following change to "hide" the relationship between the two mocks from Moq (it won't run user-provided .Returns delegates unless actually needed to generate a return value):

+repositoryMock.Setup(m => m.UnitOfWork).Returns(() => unitOfWorkMock.Object);
-repositoryMock.Setup(m => m.UnitOfWork).Returns(unitOfWorkMock.Object);

[stakx]

is that the intended behaviour?

@molszews, has your question been sufficiently answered?

[molszews]

makes perfect sense, thanks!

[penguintree]

I just ran in the same case and I feel like this change should'nt have been made.

Consider the following :

  public interface IEntity
  {
     int Id { get; }
     int X { get;  }
  }

  public interface IEntityRepository
  {
     IEntity GetEntity(int id);
  }

  public class SubjectUnderTest
  {
     private readonly IEntityRepository _repo;

     public SubjectUnderTest(IEntityRepository repo)
     {
        _repo = repo;
     }

     public int DoSomethingWith(int entityId)
     {
        var entity = _repo.GetEntity(entityId);

        return entity.X + 1;
     }
  }

And this test

  [TestMethod]
  public void SomeTest()
  {
     // Arrange
     const int entityId = 1;
     const int entityXValue = 10;

     // Setup the repository to return a given entity.
     var entity = new Mock<IEntity>();
     entity.Setup(e => e.Id).Returns(entityId);
     entity.Setup(e => e.X).Returns(entityXValue);
     var repository = new Mock<IEntityRepository>();
     repository.Setup(r => r.GetEntity(entityId)).Returns(entity.Object);

     var expectedResult = entityXValue + 1;
     
     var subject = new SubjectUnderTest(repository.Object);
     
     // Act
     int actualResult = subject.DoSomethingWith(entityId);

     // Assert
     Assert.AreEqual(expectedResult, actualResult); // Ok
     repository.Verify(r => r.GetEntity(entityId), Times.Once); // Ok.
     repository.VerifyNoOtherCalls(); // Fails because entity.X got called. Should pass because I'm verifying calls made to "repository", not "entity".
  }

I dont care about what SubjectToTest does with the entity. I care about it retriving the good entity and returning the good result. The call to entity's properties made by the method seems like implementation details to me, wich I don't have to worry about when testing.

Maybe it's usefull for other test cases. Many test strategies can be used. In this case I used mock for the IEntity simply to avoid creating a class that implements the interface as it is not needed. The workaround of using the override of Return that takes a function is quite a patch I think, it does not identify the intention.

If the behavior is still wanted, it could be nice to mark a mock as « non verifiable » when creating it, e.g. var entity = new Mock<IEntity>(verifiable = false);

[snake-scaly]

This is a very unfortunate design decision.

I only discovered this behavior today even though I used Moq for years. This alone tells you how obscure this feature is. This behavior is not documented anywhere besides this old ticket.

The main problem with this is it can make tests rot. Imagine someone wrote a test which used this feature, either because they knew about it, or most likely by accident:

// ...
aMock.Setup(x => x.Foo()).Returns(bMock.Object);
bMock.Setup(x => x.Bar()).Returns(1);
// ...
aMock.VerifyNoOtherCalls();

This implicitly verified that bMock didn't have any unexpected calls.

Now imagine I need to make a change to this test, and for some reason I need a lambda in the first setup:

aMock.Setup(x => x.Foo()).Returns(() => {
    // do stuff
    return bMock.Object;
});

The test still passes, but unbeknownst to me, I lost verification on bMock. Nothing in the test code tells me that it even was verified. It looked like extra calls on bMock weren't important.

A related issue is that the test behavior depends on an obscure implementation detail of the Moq framework, namely whether it was "smart" enough to discover the dependency. This ticket mentions that currently a workaround is to wrap the return into a lambda. But what if at some point, some maintainer decides to make it smarter and detect lambda returns? Will my tests break?

Finally, tests are a form of documentation, and documentation needs to be clear. Reliance on implicit behavior which only works sometimes diminishes documentation value.