search

devlooped / moq

The most popular and friendly mocking framework for .NET
Other
5.88k stars 800 forks source link

Fix recursive verification #1093

Open stakx opened 3 years ago

stakx commented 3 years ago

Problem statement:

Every now and then, this repository sees reports about verification unexpectedly including setups or invocations from mocks other than the one on which Verify, VerifyAll, or VerifyNoOtherCalls was called. Recently, we got these reports:

The reason for this happening is that verification is recursive, and visits all mocks safely reachable from the mock on which the verification call is made. I've come to summarise this recursion logic as "mocks own other mocks".

As the above issues (and probably others) demonstrate, this behavior isn't intuitive, and it often isn't desired. The recommended workaround to avoid it has been to replace calls of the form setup.Returns(value) with setup.Returns(() => value). Using a callback instead of a fixed value will stop verification from continuing along that axis if value is itself a mock object.

Proposal:

Perhaps the time has come to fix this problem with recursive verification. One way to do this would be to change the logic to something I'd summarize as "mocks own setups"; that is, recursive verification should focus on setups, not on mocks. It should not travel along edges from one mock to other mocks, but along edges from one mock to its setups.

(Edit: To be more specific what that means: a call to mock.Verify[All]() should verify exactly those setups that were made via a mock.Setup[Get,Set,Add,Remove,Sequence](...) call and nothing else. Further, mock.VerifyNoOtherCalls() deals with invocations, not setups, and should therefore no longer be recursive at all.)

Assumed benefits:

Here's a summary of the assumed advantages of this approach:

Some other remarks / changes:

Question to you: is it worth making this breaking change?

This would be a functional change that is pretty much guaranteed to break some existing code out there... I suspect that breakage wouldn't be too severe (it breaks 6 out of roughly 3000 test cases in Moq's own unit test suite, FWIW), but I have no real way of knowing. I think the change would be for the better, and make Moq more intuitive, but I'd love to hear @kzu's and other users' thoughts and opinions about this proposed change. How do you think (recursive) verification should work? What should get included during verification?

/cc @rleeson24, @matkoliptak, @killergege, @Zaeranos, @molszews, @penguintree (who opened / commented on the issues mentioned above)

Back this issue Back this issue

siprbaum commented 3 years ago

Trying to get a better understanding on your proposal, especially this point

  • Mocks will in some cases be associated with setups that are configured on other mocks. Say you have a setup like parentMock.Setup(p => p.Child.Property).Returns(...).Verifiable(), the setup for Property would sit on the child mock, but it would also be included when you do a parentMock.Verify().

and assuming that for your example the following interfaces are used

interface IParent {
    IChild Child { get; }
}

interface IChild {
    string Property { get; }
}

and the test then instead does the following

var parentMock = new Mock<IParent>();
var childMock = new Mock<IChild>();
childMock.Setup(c => c.Property).Returns("some value").Verifiable();
parentMock.Setup(p => p.Child).Returns(childMock.Object);

parentMock.Verify();

Would this also do the recursive verfication on childMock?

Would the answer change if this change is made on the obove test?

-parentMock.Setup(p => p.Child).Returns(childMock.Object);
+parentMock.Setup(p => p.Child).Returns(childMock.Object).Verifiable();
stakx commented 3 years ago

@siprbaum, thanks for asking!

childMock.Setup(c => c.Property).Returns("some value").Verifiable();
parentMock.Setup(p => p.Child).Returns(childMock.Object);

parentMock.Verify();

Would this also do the recursive verfication on childMock?

No, this would no longer be the case.

Would the answer change if this change is made on the obove test?


-parentMock.Setup(p => p.Child).Returns(childMock.Object);
+parentMock.Setup(p => p.Child).Returns(childMock.Object).Verifiable();

No, this would make no difference.

You'd have to do this:

-childMock.Setup(c => c.Property).Returns("some value").Verifiable();
-parentMock.Setup(p => p.Child).Returns(childMock.Object);
+parentMock.Setup(p => p.Child.Property).Returns("some value").Verifiable();

 parentMock.Verify();

The setup for Property on the child mock (i.e. Mock.Get(parentMock.Object.Child)) would then be included, but not any other setups on that same child mock unless it has also been set up via parentMock.

In other words: parentMock.Verify[All]() would verify exactly those setups that have been set up via some Setup call on parentMock.

(The choice of .Verifiable() + .Verify() vs. just .VerifyAll() is unrelated to the proposed change. Those methods would keep working as they do now.)

penguintree commented 3 years ago

Your proposal makes much sense to me. I can only speak for myself, but I don't expect much breaking changes (if any) in our code base of over 15 000 tests.

kzu commented 3 years ago

This is tricky indeed. I don't think I did a very good job of making things easily comprehensible from the beginning. Too much "magic" 😞 .

I wonder if the "ChildMocks" collection for "recursive setups" would be a useful public API to have in the vNext SDK...

With both that and public API to lookup the setups on a mock, you can verify precisely what you want, I think...

stakx commented 3 years ago

@kzu, thanks for chiming in. I would love to hear how you originally intended (recursive) verification to work, or (if that differs) how you think it should operate now.

Without having that knowledge, I would argue that ChildMocks (and the "inner mock" concept currently found in Moq 4) isn't actually needed, or, at the least, that relationships between mocks should not (cannot) be determined automatically.

kzu commented 3 years ago

TBH, I don't recall precisely what the original intended behavior. My intuition is that in general, recursive setups are "owned" by the mock you perform them on, where as "regular" setups aren't recursive. And so it seemed intuitive that for recursive setups, verifying the owning mock would also verify the child mocks. But for regular setups, that wouldn't typically be what you'd want (since that's likely why you created a separate Mock instance in the first place).

stakx commented 3 years ago

@kzu

My intuition is that in general, recursive setups are "owned" by the mock you perform them on[.]

Agreed 💯, a recursive setup like mock.Setup(m => m.Foo.Bar) should be verified together with the owning mock.

And so it seemed intuitive that for recursive setups, verifying the owning mock would also verify the child mocks.

The longer I think about it, the more I'm convinced that this should not happen. There shouldn't be any ownership or parent-child relation between mocks just because a setup on mock A happens to return (or act on) another mock B.

I'm starting to think that if you want to verify several mocks, you should explicitly use a MockRepository for that purpose. That is, retrieve all mocks that you'll want to verify together from the same repository, then do a repository.Verify[All].

This makes it your choice exactly which mocks you want to verify (as opposed to Moq having to make a guess through assumed ownership relations that may not actually reflect your intention).

kzu commented 3 years ago

Yeah, I see your point. Too many heuristics makes things harder to understand. I still haven't introduced the concept of the mock repository in v5, I was pondering whether it was necessary at all... You could trivially implement it yourself (or in a Contrib package) by just making your own factory methods that return them and keep track of them. With the rich introspection API in v5, it should be fairly trivial to implement the Verify[All] yourself, and therefore ensure it's possible to also integrate with other things like DI-based mock creation...

You can always just keep the variables around and verify yourself too, so...

stakx commented 3 years ago

Oh yes, by all means, don't add MockRepository to v5 if it wasn't planned anyway. I only mentioned it in the context of v4 because it lends itself quite well to the problem discussed.

I wanted to make sure that we are on the same page re: the semantics of recursive verification. It appears that we are, and that gives me some more confidence that "fixing" recursive verification in Moq 4 is the right thing to do, despite it being a breaking change.

snake-scaly commented 1 year ago

I wrote a comment on #858 before I discovered this ticket. I can re-post it here if it can help the discussion.

stakx commented 1 year ago

@snake-scaly, there's no need to repost, the link to your post should suffice. Thanks for taking the time to add your two cents. It's reassuring to get another data point that's in agreement with the proposed change, because while I believe it would be a good & reasonable one, it'll also break things for some users of Moq... so I am still very hesitant to make it. Enough posts like yours will hopefully tip the scales and give me the courage to go ahead.

Btw. I have had a few working drafts of an improved form of no-longer-very-recursive-at-all verification. It's been a while, but IIRC the biggest remaining uncertainty was with the intended semantics of VerifyNoOtherCalls... haven't untangled that yet quite all the way.

kzu commented 1 year ago

Ok, I refreshed my memory on this thread, and I think I agree with the proposed design of having mocks own setups and not entire mocks. I don't think the scenarios that would break would be too common.

I'd suggest testing the updated logic on the dotnet/aspnetcore repo which has a ton of tests and is a heavy Moq user and see if there's much breakage. If there isn't any (or very few), then perhaps it's fine? I there's much cry, worst case a new property/arg on verify could provide the legacy behavior?

I was never much of a fan of VerifyNoOtherCalls, TBH. If you'll end up calling that method, why not start with a strict mock + VerifyAll from the beginning... 🤷‍♀️

stakx commented 1 year ago

Thanks for the suggestion of test-driving the changed verification behaviour using the dotnet/aspnetcore project... that's a great idea. Will do!

Regarding VerifyNoOtherCalls... sorry for that. :-) IIRC the alternative approach you're suggesting above was considered in the discussion that led to this method, but found lacking in some scenarios. I cannot remember the details, but I could probably dig it up if desired. If it turns out to not be necessary at all, we could consider deprecating it... though I suspect there'd be complaints about that, too.

(Btw. by now I am fairly certain that the new behavior for VerifyNoOtherCalls should be completely non-recursive; it should only verify calls on the mock on which the method is called. This behaviour, while not the only possible one, is the easiest to explain and will therefore perhaps cause the least confusion.)

kzu commented 1 year ago

I wouldn't deprecate VerifyNoOtherCalls, no, it's already part of the shipped public API :). Perhaps for vNext 😉 .

github-actions[bot] commented 3 weeks ago

Due to lack of recent activity, this issue has been labeled as 'stale'. It will be closed if no further activity occurs within 30 more days. Any new comment will remove the label.