devlooped / moq

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

If When(Func<bool> condition) is used to setup a mock, the setup verification is skipped. #959

Closed FranceyD closed 4 years ago

FranceyD commented 4 years ago

It is possible to use Verifiable() on a conditional setup (I mean a setup with When preceding the setup method), but the Verify() method doesn't actually verify it.

Example:

var mock = new Mock<ICloneable>();
mock.When(() => true)
    .Setup(o => o.Clone())
    .Verifiable();
mock.Verify(); // No exception

mock.Setup(o => o.Clone())
    .Verifiable();
mock.Verify(); // MockException, setup was not matched.

As I have seen in the source code, it looks like the method SetupCollection.ToArrayLive ignores all setups with a condition.

For me, it would be convenient that the Verify method verifies also the setup if the condition is valid during the verification.

stakx commented 4 years ago

Yes, there is indeed extra logic that filters out conditional setups during verification.

I suspect that's because the conditions may evaluate differently at verification time than at invocation time, which could easily lead to unexpected or even inconsistent verification results.

Whatever the reason, changing Moq's years old behavior would very likely break existing user code, we can't just change that anymore.

stakx commented 4 years ago

It is possible to use Verifiable() on a conditional setup

We might want to throw an InvalidOperationException when one tries to do that, however.

stakx commented 4 years ago

@FranceyD: In the spirit of better error reporting, starting in version 4.14, Moq will throw when calling .Verifiable() on conditional setups.

Also starting with version 4.14, it will be possible to verify conditional setups, however it won't (yet) be as straightforward as calling mock.Verify(). You could do this, for example:

var conditionalSetups = mock.Setups.Where(setup => setup.IsConditional && !setup.IsOverridden);
foreach (var conditionalSetup in conditionalSetups)
{
    conditionalSetup.Verify();
}

(†) I'm still considering adding a new method overload mock.Verify(bool recursive, Func<ISetup, bool> predicate) that would allow you to more easily select the setups that should be verified, e.g.:

mock.Verify(true, setup => setup.IsConditional && !setup.IsOverridden);

Not sure yet whether it's generally worth it, though.

stakx commented 3 years ago

The change that I described above turned out to cause a regression, and I'm reverting it in #1114. Conditional setups still won't be included in verification, but invocations matched by them once again will be.