Set Times expectation on Setup

[skarllot]

Would be nice if I can set times expectation on Setup instead of separated Verify. e.g.:

mockInstance
    .Setup(a => a.Start(It.IsInRange(0, 1000)))
    .Count(Times.AtLeast(5))
    .Returns(Task.FromResult(true));

[tparikka]

+1

[stakx]

Being able to specify Times on a setup sounds like a handy feature. Contrary to that, the two extant setup methods .AtMostOnce() and .AtMost(n) were both deprecated and hidden from IntelliSense years ago. This old Google Code issue 216 states why:

These were hidden as we're encouraging people to manage the former occurrence constraints via explicit Verify assertions, which aligns better with AAA style testing.

I'm not against this feature, but I'm digging up this bit of history so we won't end up swinging back and forth between two opposing design decisions.

@kzu: What is your position on the ability to specify Times during setup? Do you still consider this a feature that should belong in Verify exclusively?

[IFYates]

While I completely agree with the AAA comment, the reason I submitted #401 is to cover scenarios like my CannotPostVerifyCallsByExpressionForModifiedReferenceTypeArgument test, where the post-Act Verify cannot be used to check the invocation argument values.

I did play around with having a Verify directly on IFluentInterface, which gives a better AAA approach, but it felt like a larger change and less elegant when you have many setups.

Example:

var setup1 = mock.Setup(m => m.Call1(It.Is<MyClass>(c => c.Name == "test"))).Returns("result");

Act();

setup1.Verify(Times.Exactly(2));

I realise that this is slightly an edge-case issue.
We currently solve it through argument capture and comparison, which is very inelegant.

[stakx]

@IanYates83 - that makes sense, I've come across one other issue where someone had the same problem with reference type state changing and not being verifiable after the fact. I'm generally in favour of your proposed changes. Like I already commented on your PR:

• Could you perhaps explain briefly why you chose to add a Times on the .Verifiable() method? Did you consider any alternatives (extending the IOccurrence interface; extending .Setup(); adding a new method such as .Count() as proposed by @skarllot; etc.)?

• I would prefer to leave the final decision up to @kzu: Is adding Times back to the setup (Arrange) stage OK? We appear to have one use case that the .Verify (Assert) stage can't handle easily.

[IFYates]

@stakx Responding to your comments here but will action your request on the other PR.

• When we hit the issue, it was the natural change that came from our internal discussions. I couldn't see the existing functionality (the current IOccurence isn't precise enough), so I thought I'd suggest it using the syntax we felt was naturally missing. (You mark the Setup as verifiable, so why can't we control that count assertion at the same time?)
  I also liked the fact that it was a very light change that didn't affect existing functionality.
  If the old decision is reversed, maybe extending IOccurence is the answer, or a new .ExpectedCalls(Times) method on IVerifies (Count is a bit vague, to me).

• Fine by me :)

[stakx]

@IanYates83, I've been pondering this proposed feature today, and I'm no longer sure that it's the best solution even for your edge case scenario. There are several reasons:

1. It finally dawned on me why IOccurrence doesn't have methods corresponding to e.g. Times.AtLeast or Times.Exactly: Because those can only be verified after a test's Act stage, specifying them earlier than that is potentially confusing. All existing IOccurrence members such as AtMost can always be verified, their meaning is unambiguous.

   For the same reason, I'd rather not introduce a feature for expressing e.g. a Times.AtLeast constraint. It's just not self-evident at which moment the constraint will get verified.

2. Viewed from that perspective, adding a Times parameter to Verifiable is actually reasonable and can help clarify who will check the constraint: Verify. But Verifiable(Times.Exactly(n)) reads more like "this setup is verifiable n times", than "this setup will be included in verification and it's expected to be matched 3 times".

3. The proposed feature is not necessary to capture argument values at the moment of invocation. That's exactly what Capture and CaptureMatch<T> are there for (though documentation on those might be spotty or even nonexistent). Or you can even populate a collection from a Callback.

4. You don't need the proposed feature to ensure a method won't get called with argument values other than those you've specified: you can use MockBehavior.Strict for that.

In conclusion, Moq already has all the features needed to deal with that argument-capture edge case, and introducing the proposed feature (admittedly a handy one) leads to a somewhat more ambiguous / semantically unintuitive API (IMHO).

I think to deal with the edge case, we'd do better to introduce a new overload Capture.In<T, TElement>(ICollection<TElement> collection, Func<T, TElement> selector) (or similar).

Any thoughts on this?