Extending SetupSequence

[uecasm]

The new SetupSequence method is good, but there are a couple of things that would make it better (make test code more concise and readable).

Imagine the following sequence setup:

mock.SetupSequence(x => x.DoSomething(/* a verbose list of argument matchers here */))
    .Returns(1)
    .Returns(42)
    .Returns((a, b, c) => CalculateSomeSpecialValueFromArgs(a, b))
    .Throws(new Exception("y"))
    .Returns(72)
    .Verifiable();

This would use the following things that don't exist yet:

• Returns can accept a Func<> to call to find the return value, similar to Setup().
  • Sequenced Actions should probably add Callback(Action<args...>) instead.
• Verifiable can be specified to do something similar to Setup() -- but also implies a Times.Exactly based on the number of actions in the sequence.

So in the above case, calling mock.Verify() in the assert phase later on will report an error if the method was not called exactly 5 times (with matching arguments).

This avoids repeating the argument matching expression in a separate explicit Verify call, as well as making SetupSequence more consistent with Setup's capabilities.

(Throwing when too many calls are made doesn't work if the code that makes the call catches the exception; and still requires an explicit Verify to check it was called enough times.)

This would probably also solve #373 and #530 in a reasonably elegant fashion.

[stakx]

Hi @uecasm, thank you for taking the time to submit this proposal. Some good ideas there!

Returns can accept a Func<> to call to find the return value, similar to Setup().

This would make sense from the viewpoint of API consistency.

One question however: Given that each step in the sequence only gets executed once (unlike a regular setup's Returns callback, which might get called repeatedly), how important do you think lazy evaluation is in this case?

Verifiable can be specified to do something similar to Setup() [...].

Adding Verifiable with its usual overloads (), (string failMessage) etc. would be a good addition!

However, I disagree with this:

[...] but also implies a Times.Exactly based on the number of actions in the sequence.

This would be different from how a regular setup's Verifiable works. mock.Verify[All]() only checks whether each setup has been invoked at least once... sequenceSetup.Verifiable should play along with that rule for consistency.

PS: I'm actually divided on this. Obviously, if you're making an effort to set up 5 sequence steps, you'd expect that they are all going to be used, so your suggestion makes sense from that point of view. On the other hand:

• I wonder how consistent such a precise check would be with MockBehavior.Loose.
• Also not sure whether it would be a breaking change (wrt Mock.VerifyAll).
• If we ever add more overloads to Returns (e. g. Returns(IEnumerable<T> itemsFromSequence) or Returns(Func<T> getItem, Times count)) the suggested behavior of Verifiable might no longer be very transparent.

This would probably also solve #373 and #530 in a reasonably elegant fashion.

I think it would be less than ideal if people started using sequence setups just to circumvent the fact that Moq doesn't currently allow you to specify a Times upfront.

If specifying Times during setup is an ongoing requirement of many users, then perhaps my decision not to merge #401 was wrong and the right solution would be to merge it... though I'd prefer to first see whether the new VerifyNoOtherCalls will succeed in covering this use case, or not.

I'd be interested to hear your opinions on these points.

If the missing sequence methods are important to you, feel free to submit a PR (including tests).

[uecasm]

One question however: Given that each step in the sequence only gets executed once (unlike a regular setup's Returns callback, which might get called repeatedly), how important do you think lazy evaluation is in this case?

It's still useful because it might be getting called with different arguments each time (that still satisfy the matchers), and as shown above you might want to calculate an appropriate return from one or more of the arguments, or (eg. for actions) do some other side effects such as capturing an arg for later asserts.

For basic int arguments etc the whole idea seems kinda silly, but I'm dealing a lot with methods that pass around byte arrays or use argument-packs (pass many different internal arguments as a single temporary object that's created by the caller, so need a complex It.Is<T>(x => ...) matcher to unpack different cases).

This would be different from how a regular setup's Verifiable works. mock.Verify[All]() only checks whether each setup has been invoked at least once... sequenceSetup.Verifiable should play along with that rule for consistency.

And it would. Setup(...).Verifiable() represents a Times.AtLeastOnce cardinality, while SetupSequence(...)....Verifiable() represents a Times.Exactly(n) cardinality. It can't be an AtLeastN cardinality because there's nothing more to return after the sequence has been exhausted, by definition, and it shouldn't be an AtMostN because if you wanted that you'd just not specify Verifiable in the first place. AtLeastOnce is probably the only reasonable other contender, but if you want that then you can use Setup().Return(Func) instead.

I wonder how consistent such a precise check would be with MockBehavior.Loose.

Actually loose behaviour is exactly why I wanted this feature (well, that and not repeating the argument matchers). VerifyNoOtherCalls doesn't really work in my scenario because there are a lot of other calls going on that I don't want to explicitly setup or verify, so it's too strict.

I'm away from my dev PC at the moment so I can't test this, but how does Moq react to verifications with overlapping matchers? I'm thinking of an assertion set like this:

mock.Verify(x => x.Method("foo", 1, 34), Times.Exactly(4));
mock.Verify(x => x.Method("bar", 2, 75), Times.AtLeastOnce);
mock.Verify(x => x.Method(It.IsAny<string>(), 2, It.IsAny<int>()), Times.Never);

Where the intent of these checks is to say that at least one call with ("bar", 2, 75) is expected but there should not be any other calls with 2 as the second argument; and only 4 calls with that first set of arguments, but there are no restrictions on any calls with some other second argument. Unless it's clever about keeping track of state I'd expect that probably wouldn't work though.

I also wonder if mock.VerifyNoOtherCalls(x => x.Method) might be a useful way to express "I don't want any more calls to this method with any args I haven't already verified, but I don't care about any other methods". Though I suspect that particular syntax might be problematic if the method is overloaded, and if you have to spell out the matchers explicitly then it's probably no better than an explicit Times.Never as above.

Also not sure whether it would be a breaking change (wrt Mock.VerifyAll).

VerifyAll ignores Verifiable anyway, so it would probably just continue to do so and not do any cardinality check for sequences, I'd assume, or just an AtLeastOnce. I'm not really sure though; I've never used VerifyAll in a real test except temporarily as a debugging aid (when Strict was slightly too strict).

If we ever add more overloads to Returns (e. g. Returns(IEnumerable itemsFromSequence) or Returns(Func getItem, Times count)) the suggested behavior of Verifiable might no longer be very transparent.

In theory those should both extend the cardinality as you'd reasonably expect -- the first would add the number of items in the sequence, and the second would add the number of items in the count (though it would have to be a plain int I think; Times doesn't make much sense in this context).

Having said that they do present some challenges, such as if someone supplies an infinite sequence, or if it was extended further to Returns(Func<IEnumerable<T>>) and then the method was not called enough times to actually call that func and obtain the enumerable. Still, the former case is an obvious programmer error, and the latter case is still a failed test -- you know that the method was called an insufficient number of times, even if you don't know exactly how many times would have been sufficient.

[uecasm]

This is completely a side-note (and not really related to SetupSequence), but the other thing that specifying a cardinality up-front (and now I'm talking about ye olde Setup(...).AtMostOnce() sort of thing) is good for is giving you an exception failure right at the point of making too many calls, which makes it really easy to find the problem.

It does annoy the AAA purists a bit (although I don't really see specifying the number of times it's allowed to be called in Arrange as any worse than specifying what it should return each time), and there are some cases where it will not fail in the expected way (eg. if the caller of the over-used method catches the exception, or it happens in an awkward thread context), but those cases are rare and using a first-chance debugger avoids the whole issue anyway, so overall I think it's a nice feature to have.

Strict does the same thing, but at the cost of having to Setup every possible call, even ones you don't want to care about. (So Strict invariably leads to either brittle tests or a dependency explosion from overly-narrow interfaces.) And VerifyNoOtherCalls has the same drawback without even providing the benefit of failing quickly.

[stakx]

@uecasm: sorry for being slow in responding, I've been ill for the past 2 weeks and now on vacation for another two. Just letting you know for the moment that this hasn't been forgotten. I will reply to your comments as soon as I get a chance.

[uecasm]

Maybe a better way of explaining how I think it should work is that SetupSequence(...).Verifiable() doesn't actually define an explicit cardinality at all, but simply requires that the provided sequence be fully saturated.

So given a sequence of two items, it will signal failure if only one of those items were actually used (at time of Verify or VerifyAll) or if an attempt was made to use a third item (at time of actual call).

If sequences were in future extended to support arbitrary IEnumerables, or lambdas returning such, then it will signal failure if it has not completely exhausted all provided enumerations (at time of Verify or VerifyAll), or if a call is made when it has completely exhausted them (at time of actual call).

Conversely, using SetupSequence without Verifiable would result in the same behaviour as now: Verify does nothing, VerifyAll checks that at least one item from the sequence has been consumed (but does not require full saturation), and if the sequence is exhausted then any further calls will fail immediately.

[stakx]

@uecasm: Long answer ahead. There's a summary at the end of this post in case you want to TL;DR this. I hope I've managed to address all the points you've raised. Thank you btw. for such a detailed and well-argued proposal! :+1:

1. sequenceSetup.Verifiable()

This would be different from how a regular setup's Verifiable works. mock.Verify[All]() only checks whether each setup has been invoked at least once... sequenceSetup.Verifiable should play along with that rule for consistency.

And it would. Setup(...).Verifiable() represents a Times.AtLeastOnce cardinality, while SetupSequence(...)....Verifiable() represents a Times.Exactly(n) cardinality.

Exactly, and since Times.Exactly(n) != Times.AtLeastOnce(), you violate consistency with your proposal.

AtLeastOnce is probably the only reasonable other contender [...].

It's the only one if you want to be consistent with mock.Verify[All]'s current behavior. And consistency IMHO is a must.

So in conclusion, adding a .Verifiable() method to sequence setups makes sense, but it should have exactly the same semantics as for all other setups: marking that particular setup for inclusion in mock.Verify. It shouldn't change a setup's expected number of invocations because that's not what it does for other setup types.

2. Breaking change for mock.VerifyAll

Also not sure whether it would be a breaking change (wrt Mock.VerifyAll).

VerifyAll ignores Verifiable anyway, so it would probably just continue to do so and not do any cardinality check for sequences, I'd assume, or just an AtLeastOnce.

Your summary of how VerifyAll works is accurate, as is your assessment: There indeed wouldn't be a breaking change, since .Verifiable() currently doesn't exist for sequence setups so there is no "previous behavior" to be taken into account.

3. sequenceSetup.Returns(IEnumerable<T> sequence)

Having said that they do present some challenges, such as if someone supplies an infinite sequence, or if it was extended further to Returns(Func<IEnumerable<T>>) and then the method was not called enough times to actually call that func and obtain the enumerable. Still, the former case is an obvious programmer error, and the latter case is still a failed test -- you know that the method was called an insufficient number of times, even if you don't know exactly how many times would have been sufficient.

OK, so we're really not talking about Times.Exactly(n) anymore because as the hypothetical IEnumerable overload demonstrates, that n isn't always known.

Maybe a better way of explaining how I think it should work is that SetupSequence(...).Verifiable() doesn't actually define an explicit cardinality at all, but simply requires that the provided sequence be fully saturated.

That makes sense.

(But no matter: The fact remains that setup.Verifiable() and mock.Verify[All] already have defined meanings that, for consistency, shouldn't be different for sequence setups.)

4. sequenceSetup.Returns(Func<...> resultFunc)

One question however: Given that each step in the sequence only gets executed once (unlike a regular setup's Returns callback, which might get called repeatedly), how important do you think lazy evaluation is in this case?

It's still useful because it might be getting called with different arguments each time (that still satisfy the matchers), and as shown above you might want to calculate an appropriate return from one or more of the arguments, or (eg. for actions) do some other side effects such as capturing an arg for later asserts.

OK, agreed, this would probably be a valid addition, although I dare say it's kind of an edge case to do this kind of thing.

5. Question about how several overlapping Verify calls interact

[How] does Moq react to verifications with overlapping matchers? I'm thinking of an assertion set like this:

mock.Verify(x => x.Method("foo", 1, 34), Times.Exactly(4));
mock.Verify(x => x.Method("bar", 2, 75), Times.AtLeastOnce);
mock.Verify(x => x.Method(It.IsAny<string>(), 2, It.IsAny<int>()), Times.Never);

This wouldn't work. The 2nd verification call expects that there is a call with arguments ("bar", 2, 75).

• If indeed there was such a call (or several), then the 2nd verification call succeeds, but he next one will fail because that one call (or more) will match the It.IsAny matchers but will conflict with the Times.Never constraint.

• If there was no such call, the 2nd verification call will fail, and you'll never get to the 3rd one (which would succeed due to Times.Never).

Allow me one remark: I cannot be sure since I don't know the details of course, but I could imagine that your unit tests simply try to do too much, if you have the need to sort & pluck apart your mock's invocations like that.

6. mock.Verify(m => m.Method)

I also wonder if mock.VerifyNoOtherCalls(x => x.Method) might be a useful way to express "I don't want any more calls to this method with any args I haven't already verified, but I don't care about any other methods". Though I suspect that particular syntax might be problematic if the method is overloaded, and if you have to spell out the matchers explicitly then it's probably no better than an explicit Times.Never as above.

• You're hitting the nail on its head with the overload problem. Over at the dotnet/csharplang repo there's repeatedly been the discussion about the nameof operator, method overloads, and the refactoring issues related to it. Imagine you have an overloaded method, and you rename one of them. Should VS automatically refactor nameof usages for the original method name, or not? Because of these kinds of potential problems, I'd be prudent in considering such an addition to Moq's API. That's more of a gut feeling at this point, though; I haven't really thought through what concrete problems there might be.

• There is also the more immediate problems in providing this overload: One additional overload for VerifyNoOtherCalls isn't sufficient, you'd need at least 32 for all possible Action and Func delegate types, as well as the possibility for any other custom delegate types (e.g. for when your method has ref or out parameters). This problem is absolutely solvable, just saying that it doesn't come as cheaply as you might initially assume.

In conclusion, my personal vote would go against this particular addition. Seems like another edge case whose benefits are at this point mostly hypothetical.

7. Advantage of setting up expected invocation call upfront

This is completely a side-note (and not really related to SetupSequence), but the other thing that specifying a cardinality up-front (and now I'm talking about ye olde Setup(...).AtMostOnce() sort of thing) is good for is giving you an exception failure right at the point of making too many calls, which makes it really easy to find the problem.

This is true, and @kzu himself has previously mentioned (can't find the thread right now) that this is the thinking behind Strict mocks.

Strict does the same thing, but at the cost of having to Setup every possible call, even ones you don't want to care about. (So Strict invariably leads to either brittle tests or a dependency explosion from overly-narrow interfaces.) And VerifyNoOtherCalls has the same drawback without even providing the benefit of failing quickly.

This is correct. But it doesn't mean that Strict mocks are always a bad thing, or that VerifyNoOtherCalls always has that disadvantage. There are test cases where either of these might be the right tool.

But you're making a good argument that something is probably still missing here, and it would be solved by setting up a expected call count upfront. And now we're probably getting ahead:

• As per my above arguments, we can't just let .Verifiable() have different semantics (sets up a expected call count) for sequence setups than for other setups (doesn't set up an expected call count).

• In #401, a new overload .Verifiable(Times) was suggested. Note the additional parameter. This was what set it apart from .Verifiable() and made it clear that some additional thing happened apart from flagging the setup as verifiable.

• Something similar to #401 could be done for sequence setups along the lines of .Verifiable(SequenceSetup.MustBeFullyConsumed | SequenceSetup.MayBeOverExhausted). If we do something like this, then it'd only be fair to merge #401 as well (again, for consistency).

Summary

Let me try to summarise my position on your proposal:

1. Adding a sequenceSetup.Verifiable() method for sequence setups makes perfect sense, because it completes missing functionality. However, that method should do precisely the same that it does for other setups: marking the setup for inclusion in mock.Verify(); nothing more.

2. Adding a sequenceSetup.Returns(Func<...> resultFunc) makes sense, even though it's an edge case. Therefore it probably doesn't have high priority.

3. Adding a actionSequenceSetup.Callback(Action<...> action) makes sense, but it's probably also an edge case and therefore doesn't have high priority.

4. If we really wanted to add the capability of mock.Verify[All] checking that a sequence was precisely exhausted, let's not do this via sequenceSetup.Verifiable(), which already has other semantics. Instead, let's do it with a separate parameterized overload (along the lines of what #401 proposed).

5. If we do (4), let's also merge #401 for consistency. And conversely, if we don't merge #401, let's not do (4).

Hope this makes sense.

[stakx]

P.S.: @kzu has asked me over in the Gitter chat for Moq to freeze Moq 4's API, so he can finalize the initial release for Moq 5 without having to chase a moving target. So let's continue this discussion here—but it might eventually have to be migrated over to https://github.com/moq/moq, and the API change might have to happen in Moq 5 once it's released.

[uecasm]

I don't really see why Verifiable() on a sequence being treated as expecting exhaustion of the sequence to be an issue. If you didn't want the sequence to be exhausted, why would you specify a sequence in the first place? Why would you supply five items in the sequence if it is expected to be called only three times? If you want AtLeastOnce, use Setup(x).Returns(y).Verifiable(). If you want ExactlyOnce, use SetupSequence(x).Returns(y).Verifiable(). If you want ExactlyTwice, use SetupSequence(x).Returns(y).Returns(z).Verifiable(). Makes perfect sense to me. (Though you might need to fall back to explicit Verify(x) [or implement a variant of #401] if you want AtLeastTwice. I assume this is rarer.)

It's not really a change of behaviour since it doesn't exist yet -- it's not possible for anyone to be relying on anything it does right now.

Or perhaps again it would help to think of it not as setting up an expected call count, but instead setting up an expectation that the sequence is exhausted. SetupSequence.Verifiable defines an explicit bounded sequence that must be used and exhausted. Setup.Verifiable defines an unbounded "sequence" (just the same value endlessly repeated) that can't be exhausted but must still be used at least once. By this argument it's perhaps less different than it first appeared.

Still, if you'd rather add a different method that did the same thing as what I was proposing, that would be fine too.

Allow me one remark: I cannot be sure since I don't know the details of course, but I could imagine that your unit tests simply try to do too much, if you have the need to sort & pluck apart your mock's invocations like that.

Actually it's the reverse. As I mentioned before I'm trying to test something that implements an interface I have no control of; for whatever reason the authors of that interface chose to make heavy use of parameter packs and modes (the same method does many different things depending on what parameters are passed inside the objects actually passed as parameters). I'm trying to make the tests as focused as possible by ignoring the calls that aren't relevant to the specific test, but that requires considerable parameter filtering. (Having said that, the particular example you were responding to here was purely contrived and I thankfully don't have anything quite that weird.)

[uecasm]

Another point: There are already some fundamental differences between Setup and SetupSequence -- the former is unbounded, the latter is bounded (it will throw if the method is called too many times). The former can only have one value; the latter can have several.

So really, it's not possible for SetupSequence to act as an AtLeastOnce. The closest you could get is AtLeastOnceButAtMostTheSequenceCount. My argument is that this is more surprising and less useful than ExactlyTheSequenceCount.

Perhaps alternatively, if you do want to go the #401 route, SetupSequence(x)...Verifiable() could default to ExactlyTheSequenceCount but you could weaken this with eg. SetupSequence(x)...Verifable(Times.AtLeast(3)). This might avoid having to define a Times value (ExactlyTheSequenceCount) that wouldn't make sense in any other context, or having to manually repeat how many items were in the sequence (which would be tiresome).

[stakx]

I don't really see why Verifiable() on a sequence being treated as expecting exhaustion of the sequence to be an issue.

Like I said a few times before, because it's not consistent with how setup.Verifiable() and Mock.Verify[All]() currently works.

It's not really a change of behaviour since it doesn't exist yet[.]

That's true, but it would still be misleading API design: Verifiable() exists for other kinds of setups, and it would be illogical to have a method of the same name for sequence setups that does something different. If the method is called the same, it should also do the same thing.

Still, if you'd rather add a different method that did the same thing as what I was proposing, that would be fine too.

Glad to hear! :wink:

Setup and SetupSequence -- the former is unbounded, the latter is bounded (it will throw if the method is called too many times).

This is not correct. If you invoke a sequence setup too many times, it'll simply return a default value / do nothing.

The former can only have one value; the latter can have several.

And that is the primary difference between these two kinds of setup. This doesn't automatically give us a license to make everything else about them different, too.

So really, it's not possible for SetupSequence to act as an AtLeastOnce.

And it doesn't. It's Mock.Verify[All] that checks the "at least one call" condition, not the sequence setup.

Perhaps alternatively, if you do want to go the #401 route, SetupSequence(x)...Verifiable() could default to [...].

I was arguing that .Verifiable() (called without any arguments) already has a defined meaning, which shouldn't be changed, so any kind of default is really not an option here.

(IF we really wanted to change the semantics of that method, we'd have to find a new description of what it does that is more general than the previous, i. e. current semantics. That might be possible but IMHO there are easier solutions.)

[LazerFX]

I can provide a valid (at least, as far as my testing goes) reason for a SetupSequence.Returns(Func<T...> resultFunc) -

I'm mocking a time handling library (set up as a library so I can mock it). I want the first recall to return now. The second call should return now plus 11 minutes. The authentication system I'm using should validate the first call, but return a forbidden on the second because the time-stamp has expired.

If you've any suggestions on other ways of doing that, please speak up - but I'm not testing the time library, rather the authentication library, so the time library is getting mocked.

[stakx]

@LazerFX:

int invocationCount = 0;
mock.Setup(m => m.GetTime()).Returns(() =>
{
    switch (++invocationCount)
    {
        case 1: return DateTime.Now;
        case 2: return DateTime.Now.Add(TimeSpan.FromMinutes(11));
        ...
    }
    // alternatively, compute all values at once, put them in a collection,
    // and use `invocationCount` as an index into that to retrieve the result.
});

I believe the code would look pretty much the same if there were a sequenceSetup.Returns(Func<TResult>) overload.

It's fairly easy to do anything SetupSequence can do (and more!) by combining Callback and Returns; often, just one of these will be sufficient.

[LazerFX]

Ah, thanks @stakx - Still new to TDD and unit testing in general, so I miss some obvious things. Thanks!

[stakx]

@LazerFX - I'm a little dense today. I should've mentioned it much earlier: @snrnats just recently added sequenceSetup.Returns(Func<TResult>) to Moq, so starting with the next release of Moq (>v4.8.2) you'll be able to do this:

mock.SetupSequence(m => m.GetTime())
    .Returns(() => DateTime.Now)
    .Returns(() => DateTime.Now.Add(TimeSpan.FromMinutes(11)));
    ...

[LazerFX]

Fantastic, thanks for the update @stakx, I'll keep an eye out for the release. I've used your design for now, but that will make it more obvious the intention of the code.

[stakx]

Closing this dormant issue, but marking it as "unresolved" so it can be easily found again. Please see #642 for details.

[dapickMS]

Hey all,

another case why the ability of:

SetupSequence(x => x.DoSomething(/* a verbose list of argument */)) 
    .Returns((a, b, c) => CalculateSomeSpecialValueFromArgs(a, b))

is necessary. I have a method I want to mock which edits an enum property inside the method's passed parameter. I want to mock its behaviour to put different values at the enum property at different calls.

[dapickMS]

ping @stakx

[stakx]

Hi @dapickMS - the use case is clear... this simply hasn't been implemented, mostly (I suspect) to avoid duplicated parameter list validation logic. I'm currently in the process of refactoring setup internals to use reusable building blocks — Behaviors. Once that's done, this will be a relatively straightforward to implement without any code duplication.

[dapickMS]

Great, thank you for the update. Do you have an estimation when it will be ready?

[stakx]

Whenever someone implements it. :wink: For me personally it's not very high priority, I'd rather work on other things first.

[github-actions[bot]]

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.