Export to GitHub

mockito - issue #499

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


Posted on Aug 1, 2014 by Happy Cat

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).

Comment #1

Posted on Aug 1, 2014 by Massive Monkey

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.

Comment #2

Posted on Aug 1, 2014 by Swift Hippo

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.

Comment #3

Posted on Aug 1, 2014 by Happy Cat

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.

Comment #4

Posted on Aug 1, 2014 by Happy Cat

...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.

Comment #5

Posted on Aug 1, 2014 by Happy Lion

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.

Comment #6

Posted on Aug 1, 2014 by Happy Horse

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

Comment #7

Posted on Aug 1, 2014 by Helpful Cat

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

Comment #8

Posted on Aug 1, 2014 by Happy Cat

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.

Comment #9

Posted on Aug 1, 2014 by Happy Cat

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.

Comment #10

Posted on Aug 1, 2014 by Happy Horse

Fair point

Comment #11

Posted on Aug 1, 2014 by Happy Horse

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

Comment #12

Posted on Aug 1, 2014 by Swift Hippo

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.

Comment #13

Posted on Aug 1, 2014 by Massive Monkey

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.

Comment #14

Posted on Aug 1, 2014 by Massive Monkey

Comment deleted

Comment #15

Posted on Aug 1, 2014 by Happy Cat

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.

Comment #16

Posted on Aug 1, 2014 by Swift Hippo

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.

Comment #17

Posted on Aug 2, 2014 by Happy Cat

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).

Status: New

Labels:
Type-Enhancement Priority-Low