Remove UnnecessaryStubbingException.

[lucas-pereira-ep]

Remove UnnecessaryStubbingException. It's misleading and harmful.

The documentation for this exception suggests that there are only a few rare cases when it’s legitimate to stub a method that doesn’t end up being used. That’s not true, and honestly that phrasing sounds like just an attempt at not making the previous affirmation that no such cases exist look less bad.

Even if that were the case, there are other strong reasons why throwing that exception (even if it’s optional) is a terrible idea.

I can only conceive two possible benefits of having the runner throw this exception:

• Since it’s assumed that in most cases a stubbed method should be necessarily invoked, this saves the coder the effort of writing both a “given” and a “verify” for the exactly same invocation.
• It prevents the coder that wrote the test from leaving unnecessary stubs in his test case (from the name of the exception I guess this was the intended use).

The first is a bad idea because:
1 - It makes the test harder to read, as you hide an assertion.
2 - Most of the time (not in “rare situations”) you don’t really want to verify that the method was called. Unless the method produces a relevant side-effect, verifying it is useless. If you want to ensure the value returned by that method is being used, what you need are different test cases where this stub varies while everything else remains constant. It being invoked doesn’t guarantee that it’s being properly used. Relying on this automatic “verify” feature induce coders to write sloppy tests that give an illusion of completeness.
3 - In a healthy code base hardly ever you need to both verify an invocation (it has relevant side-effects) and allow its return value to be used (stubbing). If you have to do this, that’s either a code smell, or a relevant exception you want to make as visible and explicit as possible.
4 -Many coders have the habit of organizing their test cases in “Given/When/Then” sections. With the implicit “verify” it becomes impossible.

The second is a horrid idea, because:
1 - You should never use production code to check the validity of the test, or to guide you writing (or worse, re-writing) the test.
2 - If a test case is correctly written, changing only the implementation code can’t possibly mean the test is not correct anymore, but it might make the test throw the UnnecessaryStubbingException all the same.
3 - It tends to create coupling between implementation and test, since they’re used to validate each other.

As a personal note, in years using Mockito I never saw a UnnecessaryStubbingException expose a production code bug.

UnnecessaryStubbingException in the best scenario is useless, and in the worst harmful.

[lucas-pereira-ep]

At very least, if this mechanism is kept, it should produce a message similar to a failed verify, and indicate that the assertion is implicit in the stubbing, and you should use lenient stubbing if you don't want it to behave that way.
That way at least it won't be reporting an error int the test.

[pafeu81]

Not sure if this is the right issue, but I would like to add my thoughts on the name UnnecessaryStubbingException itself.
While sometimes might suggest the solution (remove the expected call in the test) in general, especially in the context of default mode named STRICT is simply not correct and misleading.

It's even more misleading if you follow what seems to be a clear instruction how to fix it:
"Please remove unnecessary stubbings or use 'lenient' strictness."
... and how about checking the actual code of your app?

For me the word STRICT means strictly follow and verify what is expected, so if it fails then assumption should be that the tested code is wrong, and not the test itself.

I understand that the reason for this could be that in 90% it turns out to be one expectation call to much...
Still, in a non-obvious case like this it should be simply stating the fact what happened - ie. like:
"ExpectedStubNotCalledException" - the call was expected but then it was not called, but it's neither "missing" nor "unnecessary".
Only in the string message of the exception solutions may be (should even) be suggested,
but then BOTH of them ("your code missed to call the mock or your test is too strict").