Mockito should provide a mechanism for classes to opt out of mocking: @Unmockable

[GoogleCodeExporter]

Mockito is easy to use, so people use it!  Unfortunately, this means that 
sometimes they ignore the helpful advice on the mockito docs, namely:

"
Remember

Do not mock types you don't own
Don't mock value objects
Don't mock everything
"

In particular, over mocking can occasionally lead to overly brittle tests that 
also increase compatibility burdens for library owners.  Also, some classes 
shouldn't be mocked because there are better test friendly ways to create 
instances.

I propose that mockito implement a mechanism whereby types can opt-out of 
mocking via a runtime annotation.  Specifically, if a class (or super class) is 
annotated with @Unmockable (name tbd) then mockito should refuse to mock it.  
Mockito could own an instance of this annotation (org.mockito.Unmockable) but 
also support understanding Unmockable annotations from other packages.  This 
will allow library owners to provide this signal to mockito without taking on a 
mockito dependency. (like how guice handles @Nullable annotations).

Original issue reported on code.google.com by LukeISan...@gmail.com on 1 Aug 2014 at 12:52

[GoogleCodeExporter]

I'm not so sure about this.  

Firstly, I fear that if we implement this, the very next feature request will 
be for a way of overriding the @Unmockable annotation.  

Secondly, the classes that really should be unmockable are things in third 
party libraries, and these shouldn't really have a dependency on a Mockito 
annotation.  It strikes me that this annotation, if it were to exist, should be 
a JDK thing, not a Mockito thing.

I would like to vote against this suggestion.

Original comment by dmwallace.nz on 1 Aug 2014 at 9:44

[GoogleCodeExporter]

I disliked the idea as soon as I heard it.

The guideline of reducing mocking is nice in theoretical terms, but is 
unreasonable as an absolute restriction, for any class.

Original comment by davidmic...@gmail.com on 1 Aug 2014 at 2:26

[GoogleCodeExporter]

Regarding the issue of this annotation living in mockito or elsewhere:
I think, if mockito were to support this system, it should respect any 
@Unmockable annotation (by matching on the simple name of the annotation 
class).  For example, guice respects any @Nullable annotation no matter what 
package it is in.  This allows libraries to use Guice and @Nullable without 
taking on a jsr305 dependency

The proximate motivation for this request is that a project i maintain has 
recently been using @AutoValue 
(https://github.com/google/auto/tree/master/value) to author value types in our 
library.  @AutoValue has a lot of benefits, but one of the downsides is that it 
forces you to write your value types as non-final classes which opens them up 
to mocking.  Value Types should definitely never be mocked, test authors should 
just construct real instances instead.

Another issue I have had a lot of trouble with is users mocking guavas 
ListenableFuture 
(https://code.google.com/p/guava-libraries/wiki/ListenableFutureExplained) 
interface.  I have fixed dozens of tests that were mocking ListenableFuture to 
instead use the various future factory methods that guava provides already.  
These tests have always been shorter and easier to read after removing the 
mocks.

So I disagree that this would introduce an 'unreasonable' restriction.  If a 
user has a use case for mocking a class that is marked @Unmockable, then they 
should raise that with the library owners.  The library authors could then work 
with them to provide either better testing utilities or to remove the 
annotation.

Original comment by LukeISan...@gmail.com on 1 Aug 2014 at 3:08

[GoogleCodeExporter]

...to provide a little more context on the listenablefuture issue, The reason i 
was fixing these tests is because the existence of these ListenableFuture mocks 
blocked (for many many months) some basic performance enhancements to guavas 
future based utilities.  Mockito made it far too easy to make a 
ListenableFuture instance that violated the contract.

Original comment by LukeISan...@gmail.com on 1 Aug 2014 at 4:13

[GoogleCodeExporter]

I would also like to see this feature implemented.  In many cases people grab 
for Mockito because it's the tool they know and never find any of the more 
appropriate fakes because they're too busy swinging the mocking hammer at every 
nail, screw and turnip they find.  To that end, if @Unmockable had a String 
description that could be used describe why they shouldn't be mocking that type 
and what alternatives were available for testing, it would go a long way 
towards improving test quality.

Original comment by gk5...@gmail.com on 1 Aug 2014 at 4:27

[GoogleCodeExporter]

Hi,
I believe this is achievable with something like that in mockito if you provide 
your own mockmaker. The custom implementation wil check for criterias to mock 
or to not mock. If the type is mockable then it can delegate to the default 
Mockito MockMaker.

Having said that I do not lean in this approach, and I do not support an an 
implementation from within Mockito.

I will tend to close this issue as won't fix. Any one against it ?

Brice

Original comment by brice.du...@gmail.com on 1 Aug 2014 at 5:07

• Added labels: Priority-Low, Type-Enhancement
• Removed labels: Priority-Medium, Type-Defect

[GoogleCodeExporter]

Instead of trying to make the class unmockable, put in the class description 
the recommended way of testing with the class.

Original comment by Daniel.Cardenas@gmail.com on 1 Aug 2014 at 5:39

[GoogleCodeExporter]

That is an approach that I have considered, but it doesn't help in situations 
like Guava where you publish a project that is then used by third party 
developers.  There would be no reasonable mechanism to get those third_parties 
to use the MockMaker in question (the service locator pattern used is too 
inflexible to allow multiple mock makers to be registered).

And even if that could be resolved by making the mockmaker locator more 
flexible (say by allowing mock makers to be registered for particular types), 
that would definitely force  libraries like guava to take on Mockito 
dependencies which is probably not appropriate.

Original comment by LukeISan...@gmail.com on 1 Aug 2014 at 5:43

[GoogleCodeExporter]

Comments aren't an effective mechanism.  Mockito.spy has lots of comments about 
when it is or is not appropriate, no one reads it, or rather, it's not the 
users who read the docs that you need to help, it's the users who don't read 
the docs who need to be stopped.

Original comment by LukeISan...@gmail.com on 1 Aug 2014 at 5:48

[GoogleCodeExporter]

Fair point

Original comment by brice.du...@gmail.com on 1 Aug 2014 at 6:16

[GoogleCodeExporter]

Having reread the comments, I understand better the motivation and I've still 
mixed feelings about this.

Original comment by brice.du...@gmail.com on 1 Aug 2014 at 6:22

[GoogleCodeExporter]

The reality is, many developers are asked to write unit tests for existing 
code, without the ability to refactor that code.  My very large multinational 
company is one (although I don't know how much this happens within the company).

The first time one of those developers is hit by this, they're not going to ask 
for advice for convincing their management to allow them to refactor this code, 
they're going to ask how to turn this off.

It's reasonable to recommend guidelines for writing good code, but it's not 
reasonable to distrust the developer enough to try to block them from doing 
something you don't recommend.  That is not agile.

Original comment by davidmic...@gmail.com on 1 Aug 2014 at 6:44

[GoogleCodeExporter]

One of my fears is that people will overuse the @Unmockable annotation, no 
matter which library it lives in.  Then you'll get a whole lot of situations 
where developers reason like this.  "I need to mock XYZ, but it's Unmockable, 
so I can't use Mockito.  I'll use Easymock just for these tests, because it 
doesn't understand Unmockable".  Then people end up with two mocking libraries 
in their project, and everything becomes a real mess.

Original comment by dmwallace.nz on 1 Aug 2014 at 6:46

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Well currently mockito doesn't mock private or non-final classes (though 
technically, Mockito.spy can spy on private classes).  So this would really 
just be a more flexible extension of that system. 

I think we all agree that there are types users shouldn't mock (String, List, 
Set, Map...). I also understand that there is an inherent trade-off in the 
flexibility mockito provides to library authors and test authors (my proposal 
gives more flexibility to library authors).  Obviously, users can mark things 
Unmockable when they shouldn't, just like users can mock things when they 
shouldn't.  In both those cases, the real issues come up due to a lack of 
communication between library authors and test authors.  I think this proposal 
would help to improve that communication (and thus also, testing in general).

FYI: if this proposal is accepted, I will take it up with EasyMock also.  
Mockito is just more popular/better so I'm starting here.

Original comment by LukeISan...@gmail.com on 1 Aug 2014 at 6:58

[GoogleCodeExporter]

Perhaps a better and more manageable approach would be to allow tests to define 
an interceptor (even at the annotation level), which would be passed 
information about the mock request.  This would even allow you to throw an 
exception if you tried to mock List, Set, or Map, in addition to user types.

Obviously, this would require tests to use the interceptor defined by their 
organization.  That's what trust means.  It would also allow developers to 
override that interceptor for special cases.

Original comment by davidmic...@gmail.com on 1 Aug 2014 at 7:59

[GoogleCodeExporter]

davidmic...@: that option is currently available via the custom MockMaker, but 
it doesn't solve my problem of libraries being able to steer users towards more 
appropriate testing utilities.  (of course my solution won't solve the problem 
of users mocking List either,... mockito should probably just implement that 
anyway, but that is a separate bug).

Original comment by LukeISan...@gmail.com on 2 Aug 2014 at 1:11