Please Introduce equivalent of mockito's verifyNoMoreInteractions() and verifyZeroInteractions()

[grzesiek-galezowski]

Hi,

I have used this functionality in mockito in Java and I miss it in NSubstitute. As far as I know, NSub does not provide a way to achieve it. I especially miss verifyNoMoreInteractions()).

This may seem like getting back to the strict vs loose mock debate, but I don't feel this way. These two methods fit AAA-style testing with loose mocks very well in my mind and, at least from the usage perspective, don't demand dealing with excessive complexity.

[dtchepak]

Hi Grzegorz,

There is an undocumented ReceivedCalls() extension method that may help here.

This will let you write: Assert.That(sub.ReceivedCalls(), Is.Empty) to check for zero calls.

You can also use ReceivedCalls to check you didn't get any unexpected calls, but it is not tied in to any existing Received calls you've made so it won't work exactly the same as verifyNoMoreInteractions. It will let you do some more complex querying of exactly what calls where made to a substitute though.

Would that be sufficient for your needs?

[grzesiek-galezowski]

Hi, thanks for answering.

I know about the ReceivedCalls() and agree that it would let me implement verifyZeroInteractions() easily. The problem is with verifyNoMoreInteractions(), because while ReceivedCalls() remembers which calls were received, it does not remember which calls were matched (i.e. verified successfully). verifyNoMoreInteractions() says "verify there were no calls other than those already verified", so I would need to pass those calls again to any query I make against ReceivedCalls().

That would seem awkward, especially in the light of the fact that simplicity of use is one of the core reasons I picked NSubstitute for my team.

Please let me know your thoughts!

[dtchepak]

Yes we'd need to do a bit of work with ReceivedCalls to get the behaviour you're after.

Have you seen Received.InOrder? It uses the beginnings of a call query model, and checks the query against a SequenceInOrderAssertion. This works differently to the assertion you're after (only checks for extra calls to methods in the query while ignoring other calls, excludes properties), but if you write your own assertion code you could probably have Received.Only(...) or similar that will let you check you got the required calls, and throw if there are extra ReceivedCalls(). (If you need it, we could look at adding a NonMatchingCalls property to IQueryResults?)

[grzesiek-galezowski]

Thanks, I'll look into it and let you know what I find.

[grzesiek-galezowski]

Hi, I had a look at the Received.InOrder() and it looks like I can get what I want through the same mechanism. A query like NonMatchingCalls() would be nice, but it's not necessary - I can obtain this indirectly by accessing targets of all configured calls and aggregating ReceivedCalls() from each and then comparing with matching calls query that's already in place.

By the way, when I finish, would you be interested in a pull request to put this verification as a method inside the Received class?

[dtchepak]

Feel free to add NonMatchingCalls() to Query in a separate PR if that helps.

Thanks for offering to contribute this back to the project. Yes I'm interested in a PR for this, but I'm hesitant commit to it until I can see an example of how it is going to work because I don't want you to waste any time polishing the code for a PR only to have it knocked back.

The basic reservations I have are:

• The mockito docs are full of warnings about using this feature (over-specified tests) that we'd probably need to replicate. But if you've got a good example showing a case where this is really useful then I'll get over that pretty quickly :)
• Will the feature work obviously enough? i.e. can people guess the basic behaviour by the method name and test structure, or if necessary, by looking at the intellisense xmldoc info?
• Will property gets and sets or event subscriptions fail the assertion? And, related to the previous point, can I tell this from reading a test written with this feature?
• How well can we communicate the assertion errors?

If all that doesn't completely put you off and you're writing the code anyway, can you send through a rough version and we'll decide then whether it should go in to NSub? :)

[grzesiek-galezowski]

Sure, thanks! I'm writing the code anyway, so when I have something that works well enough, I'll let you know and prepare some examples of where I think it can be useful.

My current understanding is that for most of the usability traits, it would work similar to Received.InOrder(), but I'll wait with any bold statements until I have it finished ;-).

Even if you decide you don't want this code later, you've already helped me a lot!

[grzesiek-galezowski]

Hi, I have a rough version implemented at my github mini-toolkit repo:

It uses only NSubstitute's public types and methods, so you can just copy-paste the code from the linked file to any solution that has NSubstitute referenced.

I copied the unit tests from Received.InOrder() with minor modifications to two or three cases:

I left the "omit property getter" policy that was part of the Received.InOrder() as I consider it a good idea.

I have yet to prepare a good real-world example, but it would be along the lines of a case of exclusive selection, i.e. I have a code like this:

public void DoSomething(...)
{
  if(condition1) collaborator.Do1();
  else if(condition2) collaborator.Do2();
  else collaborator.Do3();
}

And my intention is really to choose the behavior based on the conditions. That doesn't happen often as I try to hide if-else and switches in factories and represent them polymorphically, but it's not always justified.

When writing tests for such logic, I usually see two choices:

1. Write a separate test for each condition and just saying that a specific call was received, e.g.

//inside a test for case when condition1 is true
mock.Received(1).Do1();

Then write separate tests for each condition not fulfilled and specifying the call is not received, e.g.:

//inside a test for case when condition1 is false
mock.DidNotReceive().Do1();

This has the drawback that it does not really represent the exclusive nature of the choice as such tests can easily pass by doing something like this:

public void DoSomething(...)
{
  if(condition1) collaborator.Do1();
  if(condition2) collaborator.Do2(); //note lack of "else"
  else collaborator.Do3();
}

2. The second choice is to use DidNotReceive() to say that a single call was received, but not the rest and represent the exclusive choice intention better, e.g.

//inside a test for case when condition1 is true
mock.Received(1).Do1();
mock.DidNotReceive().Do2();
mock.DidNotReceive().Do3();

This fixes the issue of previous approach, but in my mind makes the test more brittle than it would be if we had this functionality of verifyNoMoreInteractions(), because when I add a new condition to the if-else structure, I have to add a DidNotReceive() line to all tests that check for other conditions. In such case, I argue that having this verifyNoMoreInteractions() check would make the tests less brittle, as I would be able to write just:

//inside a test for case when condition1 is true
Received.Only(() => mock.Do1());

This is an example with a single call, but it can as well be two. A nice side-effect of using the Received.InOrder()'s syntax is that the checks can be combined. For example, we may write something like this:

Action expectedCalls = () =>
{
  mock1.Something();
  mock2.SomethingElse();
};

Received.InOrder(expectedCalls);
Received.Only(expectedCalls);

Which is even less common, but nice to have.

[dtchepak]

Thanks for this! Looks good from a quick skim - I'll take a more in-depth look as soon as I can.

I really like your point about avoiding DidNotReceive calls. I think I can come up with a good motivating example based on that.

[grzesiek-galezowski]

Thanks, the code misses some cosmetics, e.g. it throws the same exception as Received.InOrder() although maybe another exception would be more appropriate and there is some code duplication. However, that's nothing that can't be improved.

Looking forward to reading your thoughts!

[alexandrnikitin]

Let me add my 2 cents. I use both frameworks: NSubstitute and mockito. What I like in NSubstitute is that I don't need to look into documentation, I read it once and looked a couple of times after, that's it. NSubstitute API is simple and obvious. It doesn't have a lot of sugar and helpers which eventually leads to simple tests. On the other side is mockito. I find it quite verbose and I open documentation pages often enough. It has a lot of syntactic sugar. verifyNoMoreInteractions() is a good example of this. I encountered with tests using it and at first look it's easy to write verification logic using it but they aren't maintainable in the end. Based on your example, imagine you need to add one more method to the collaborator or any other dependency and this method isn't relevant to check but you have tens of tests using verifyNoMoreInteractions(). Then you need to change all verification logic. What if you need to add a few such methods? In my opinion it's better to solve such (specific) cases using "strict" mocks where you need just to change the mock instead of assertion layer. This approach is harder to implement but tests are more maintainable in the end. And NSubstitute forces you to have that "simple" way. This is just my opinion.

[dtchepak]

Thanks @alexandrnikitin. I agree this is an easy way to write brittle tests, but it could still be useful to restrict calls to a subset of an interface. Ideally we'd split the interface into two or more and that would automatically ensure we don't call other methods, but in cases where we're dealing with an existing codebase that isn't easily refactored maybe it is worth having this as a fallback?

As an aside, we already have Received.InOrder which is also a good way to get brittle tests and code. ;)

(@grzesiek-galezowski: i still haven't had a chance to look at this properly. I'm not blocking you or anything am I?)

[grzesiek-galezowski]

@alexandrnikitin thank you for your comments.

To comment more broadly on your example - when the situation you describe happens, the case stops being one of exclusive selection. Then it would mean that my intention about the functionality I'm testing changes and I need to state my different intention using different mechanisms. The same can happen to Received.InOrder() or argument matchers and I don't view this as a problem, in fact, given I'm using something like verifyNoMoreInteractions() to state what my intention is, not "because I can", changing the intention is a great purpose to change my tests, which document the intention. So for me this is more about stating intention than writing tests that will change for some reason. By stating intentions, I document in my tests what I consider a "legal change" and what I consider illegal. In my mind, the bad reputation of mechanisms such as verifyNoMoreInteractions() comes from abusing them to freeze the current logic for no apparent reason (much like "I have one object of this class so far, so I'll make it a singleton", despite I have yet to see a case where singleton is really the best approach, but you get the idea :-)).

To give you a more narrow comment - when my intention is about exclusive selection, then adding methods that can be called but don't have to leads me to thinking that either my tested class has too many responsibilities (and I should move these calls either outside the tested class or inside collaborator) or the collaborator has too many resposibilities, in which case I may need another collaborator implementing another interface. What I'm trying to say is that the exclusive selection in my design is strongly implied by an object's role (I think some variations or occurences of observer pattern could fall into this category), not by the fact that "it looks alright at the moment". It's very hard to discuss design choices without a concrete example, though, so I won't be surprised if what I wrote doesn't match your experiences.

All that being said, I don't mind a library to be opinionated. What we are discussing here are good practices for using libraries, but also different design approaches. E.g. In the context I work in, I use Received.InOrder() in many tests (way less than a half, but still in many) and I never had any issues maintaining them. In fact, using this assertion is the only way to test that correct thing happens, since most of the methods I write are void calls and I prefer passing context inside calls rather than returning something. This in turn makes many of my tests less brittle because when I design my interfaces well, I can take more advantage of polymorphism (e.g. it's easier to use a null object for something that has void calls only). I know this design approach does not fit everywhere, so I understand people having different views. In fact, I have some side projects which don't fit this design vision well, and then I use another one.

I'm glad we have this discussion and believe you will make the right decision. One way or another, I managed to implement this feature for my use, so if you decide not to include it, I'll just use my version. I guess my main point about this feature is - if there's a feature that does not go 100% along the opinion the library is based upon, I'd rather look whether it could still be useful and whether it obscures the cases that go 100% along this opinion than consider whether it would lead to brittle design. When my team missed some kind of feature they wanted to abuse, they just downloaded another mocking framework and used it for that purpose :-D. Having two mocking frameworks is not as bad as e.g. two test runners, so they could live with it. I am constantly amazed by how good people are at working around limitations I try to put on them through a process, a framework or a tool and do the other thing than the one I thought they would be forced to do. Thus, while I think "whether it leads to brittle tests" is still something worth considering, I wouldn't want to discuss it as the main argument.

@dtchepak no, I'm not blocked, please take your time :-)

[ant-shch]

Hello guys I have created workaround for Moq Strict Mode using NSubstitute, you can check solution in my repo https://github.com/khdevnet/nsubstitute-repository hope it will help.