Add Times Expectation to the mock setup

[surovij45]

mock
.Setup(foo => foo.Add("my string")
.Returns(true)
.Times(Times.Once)

Add Times expectation to the mock setup, and we can verify setup using single line mock.VerifyAll() or mockRepo.VerifyAll()

[surovij45]

or

mock
.Setup(foo => foo.Add("my string"), Times.Once)
.Returns(true)

[stakx]

I'll have to look up the issue where we discussed this topic last, but IIRC we ended up with setup.Verifiable(times, [failMessage]) back then. setup.Times(times) also seems reasonable. mock.Setup(..., times) wouldn't be so great because we'd need to add a ton of method overloads to an already heavily overloaded method.

[surovij45]

Implementation is up to you

[Danielku15]

@stakx I am currently trying to migrate away from RhinoMocks and our codebase is heavily relying on the repeat functionality offered on setups. I have read through the past discussions here and here and all topics stayed unresolved due to alternatives.

I wanted to share my bits on what we would actually need and maybe there is still a chance to get some solution to our issue:

1. We have 606 setups with Repeat.Once
2. We have 30 setups with Repeat.Any + we have a thousands setups without any Repeat specified where it is expected that we allow any time of calles (exact number is hard to find out).
3. We have 26 setups with Repeat.Twice (we can ignore this, it would be fine to change on this one to some SetupSequence)
4. We have 18 setups with Repeat.Never
   5 We have 5 setups with Repeat.AsLeastOnce

We are verifying our overall Mocks with .VerifyAll(). So our actual needs are typically:

1. We want to allow a setup to only be called once. Any further attempts to use it should result in an error. Preferrably this error should already ocurr on the usage of the setup and not the verification only. It would lead to strange side effects in our tests if it continues to run without an exception raised due to unexpected logic.

2. We want to allow setups to not be relevant for the test but we might need the correct behavior of the method. This is heavily used in helper methods which setup common mocks for various tests. The setups are often not relevant for the actual logic tested and rather a pre-requisite to reach the correct place in code. It would be a nightmare to inline these helpers and do a 100% correct setup for each test which is not relevant for the business logic to be verified. To change the setups not having any repeat definition to have Any repeats, would maybe need some solution where we can define this behavior on a MockRepository/Mock level?

3. No need to care about this case. we can rework these few usages manually.

4. These are used for negative tests usually made for bugs we had. There we need to 100% guarantee that a call into a certain area must not happen. one example are some software licensing tests we we need to ensure we want to ensure that nobody tries to acquire a certain license feature when an actions are done, while they might be needed for other constellations in other tests.

5. They are partially used for mocks from helpers for multiple tests and partially for lopping logic in the code. It is important that it was called once, but it's safe to call it multiple times as part of loops or timers.

With RhinoMocks we typically got an exception within the application code as soon as some repetition was violated and we would love to get the same behavior with our future framework. This software has more than 8yrs of history already and the test suite has grown over time. Migrating our whole test suites to a separate approach is not feasable, there is a high risk we loose our coveragte by introducing bugs into the tests.

For migrational purposes it would be beneficial if such repeat configurations are possible through extension methods on the ISetup<> instances and not only as overloads. This would allow a smooth transition.

[stakx]

@Danielku15:

With RhinoMocks we typically got an exception within the application code as soon as some repetition was violated and we would love to get the same behavior with our future framework.

Moq can do some of that via the setup.AtMostOnce() and setup.AtMost(n) methods. They're marked obsolete (which was perhaps a past mistake), but they're perfectly functional and won't be going away; at least not in Moq 4.

We have 606 setups with Repeat.Once [...]
We want to allow a setup to only be called once.

mock.Setup(...).AtMostOnce();

We have 30 setups with Repeat.Any + we have a thousands setups without any Repeat specified where it is expected that we allow any time of calles (exact number is hard to find out). [...]
We want to allow setups to not be relevant for the test but we might need the correct behavior of the method.

Moq setups will not throw at invocation time if you haven't set up either a .Throws(...) or .AtMostOnce() / .AtMost(n).

However, mock.Verify() / mock.VerifyAll() expects setups to have been called at least once. You may need to exclude your Repeat.Any setups from verification altogether. This can be done by marking only the setups that are relevant for the test as .Verifiable(), then verify using mock.Verify() instead of mock.VerifyAll(). Or, verify individual calls using mock.Verify(call, ...) instead of verifying setups.

Also, #937 may or may not be relevant in your case.

We have 26 setups with Repeat.Twice. [...]
No need to care about this case. we can rework these few usages manually.

mock.Setup(...).AtMost(2);

We have 18 setups with Repeat.Never

mock.Setup(...).Throws(...);
mock.Setup(...).AtMost(0);

We have 5 setups with Repeat.AsLeastOnce.

This is the one where Moq is currently lacking something. As you've seen for the above examples, Moq can already deal with "at most N times" even before verification; but it does not offer a means to specify "at least N times" before verification.

[asmejkal]

Have you considering unmarking the AtMost methods as obsolete? They're very useful in a strict mocking style. Obsoleting them produces warnings, which is annoying when the proposed alternative (explicitly calling Verify with the same invocation signature) is not a good fit for our strict mocking style.

[Danielku15]

I am a bit reluctant to migrate our codebase to a mechanism which is marked obsolete. Removing the obsolete flagging would be step one, but it is also a strategic thing whether Moq plans to really support this functionality in future. If maybe in 1-2yrs a Moq5 comes out, it sounds like quite a risk that it still might be removed.

[stakx]

@Danielku15, it is my understanding that Moq 5's main API is really quite different from Moq 4's (it's a new major version for good reason!). It does have a special back-compat API to make the transition from 4 to 5 easier, but if you really want to update at some point, you'll likely have to adjust/rewrite some tests anyway.

That being said, I fully understand that basing code on ostensibly obsolete features doesn't feel nice. De-obsoleting AtMost[Once] is a possibility, let me get back to you all on that one. But even if they stay obsolete, they won't disappear in any future Moq 4.x version.

[Danielku15]

@stakx Of course an upgrade between major versions also imply a certain level of breaking changes. But I would say for most libraries it rarely means large conceptional changes which have implications that you have to reorganize your code in a large scale. Unfortunately this topic is one of these rare cases. Conceptionally it is a different mentality to change the repetition aspect from the arrange phase to the assert phase of a test. And this in a result leads to large scale refactorings implying quite a risk that everything is really as before.

AtMost allows defining an upper bound which will fail during the Act phase, while Verify(.., Times.*) defines also the lower bound of the expected calls. Having only Verify does not allow a fast exit path of the act phase because it cannot guaranteed anymore that the further test behavior will work as expected. This might lead to other assertions or test failures.

In our case the AtMost allows us handling this fast exit path on the act phase. I guess when time comes and we have Moq5 we can reinitiate the discussion about how to achieve the same thing in Moq5 if missing 😁

I was already able to migrate the our first product to Moq and 2686 of 2721 tests are already green. There are some small sacrifices but we can live with them until we rework these tests. Like: AtMost(2) as replacement to Repeat.Twice will not guarantee that it was really called two times, but that's what .Verify(.., Times.Exactly(2)) can be used for.

I temporarily made an own extension method and silenced the deprecation warning with a #pragma warning disable CS0618. Beside that I also needed to add some risky downcasts to IOccurence because it is not implemented on all paths of the Fluent API. ISetup<TMock> and IReturnsResult<TMock> are providing the API towards IOccurence but ISetup<TMock, TResult> does not offer it directly. It made things a bit simpler for migration as I am migrating by defining custom extension methods with the RhinoMock call signatures followed by an "Inline Method".

Just sharing some insights in the hope that future readers can learn from how I solved my issues 😉

So it would be great if we can de-obsolete AtMost[Once] for Moq4. You'd have my vote for it.

[stakx]

@asmejkal and @Danielku15, it's been some time and you have probably moved on in the meantime, but in case you're still interested: while I'm probably not going to de-deprecate setup.AtMostOnce() and setup.AtMost(n), I'm probably going to add setup.Verifiable(times) which will allow you to do the same thing (and more).