New lenient() strictness setting available at mock and at stubbing level

[madsop]

Problems

Overview of strictness: #769

1. It is not possible to have any common stubbing when using Strictness.STRICT_STUBS. Strict stubs is very useful and it is most likely the future default for Mockito 3. Common stubbing typically is a part of "before" method or a test utility class that generates mocks.

public MockitoRule rule = MockitoJUnit.rule().strictness(Strictness.STRICT_STUBS);

@Before public void before() {
  //a) common stubbing, needed for most test methods
  when(mock.foo()).thenReturn("bar");  
}

2. API caveats:

@Test public void demo() {
  //b) not possible to stub the same method using 'when', with different args:
  when(mock.foo(1)).thenReturn(1);
  when(mock.foo(2)).thenReturn(2); // <- will throw PotentialStubbingProblem

  //c) code under test cannot use stubbed call with different argument (sometimes we need it)
  when(mock.foo(1)).thenReturn(1);
  mock.foo(2); // <- will throw PotentialStubbingProblem whether we need it or not
}

Suggested solution

New public API overview:

@Test public void demo() {
  //1. New method on MockSettings interface:
  Foo mock = Mockito.mock(Foo.class, withSettings().lenient());

  //2. New method on Mockito class:
  Mockito.lenient().when(mock.foo(1)).thenReturn(1);
  Mockito.lenient().doReturn(1).when(mock).foo(1);
}

Details:

• Why 2 new public methods? Sometimes common stubbing has a form of one or few stubbings in "before" method - it's best to configure leniency per stubbing. Sometimes a mock object has many common stubbings (described in detail in the ticket thread below) - it's best to configure leniency per mock.

Examples

1. Common stubbing

public MockitoRule rule = MockitoJUnit.rule().strictness(Strictness.STRICT_STUBS);
@Mock(lenient = true) Foo mock;

@Before public void before() {
  //a) common stubbing, needed for most test methods
  when(mock.foo()).thenReturn("bar");  
}

2. API caveats:

@Test public void demo() {
  //b) not possible to stub the same method using 'when', with different args:
  lenient().when(mock.foo(1)).thenReturn(1);
  when(mock.foo(2)).thenReturn(2); // <- works!

  //c) code under test cannot use stubbed call with different argument (sometimes we need it)
  lenient().when(mock.foo(1)).thenReturn(1);
  mock.foo(2); // <- works!
}

Original report

The new UnnecessaryStubbingException logic is great. However, it is sometimes useful to disable this detection for specific methods. Could it be possible to add an annotation that says "unnecessary stubs should not be detected in this method"?

This would also make it easier to migrate from Mockito 1.* to Mockito 2. This is the case in the project I am currently working at, where we have created some utility methods that creates a mock and configures it to fit most of our use cases. As we use this method at many places, where different configuration is needed, it will cause a lot of unnecessary stubbing. Thus, we would like to keep this method out of unneccessary stubbing-check, while doing this check on the rest of the code base.

[mockitoguy]

Thank you very much for this feedback!

Do you use the silent JUnit Runner currently to avoid the exception in your scenario?

Your feedback and recent code review from @bric3 made me think about enabling/disabling stubbing strictness per method or per class using annotations. For example (brainstorming):

@Mockito(strictStubs = true)
public class SomeTest {

  @Test
  @Mockito(strictStubs = false)
  public void someTestMethod() {

  }
}

[madsop]

Thanks for following up on this!

Yes, we currently use the silent runner, which works fine, but of course lacks the stubbing checking that we would like to have on the rest of the methods.

Annotations for this sounds to me like a good solution to the problem. It would be really great if this annotation could also be used on helper methods, such as in our scenario:

@Mockito(strictStubs = true)
public class SomeTest {

@Test
public void someTestMethod() {
    Code code = createCode("A");
}

@Mockito(strictStubs=false)
 private Code createCode(String codetype) {
    Code code = mock(Code.class);
    doReturn(codetype).when(code).getCodetype(); // Not always, but in most cases, invoked
 }
}

[mockitoguy]

Thanks for the suggestions! My feedback / questions:

1. Is there a reason why you use the runner and not Mockito JUnit rule?
2. The annotation API would not work on the helper methods, only on the test methods. It's because the rule only captures and provides the test method to the implementation.

[madsop]

Thanks for your feedback!

1. No, not really. We have not really looked at the difference nor taken a conscious choice on this.

2. Oh, I see. That means that it would be hard to use annotations to do this the way I was hoping was possible. Ideally, it would be great if we could enable unnecessary stubbing in general, but configure it so a developer could call the createCode method without having to mind the unnecessary stubbing here. Or perhaps by setting some properties on the JUnitRunner? So that createCode could be something like

   private Code createCode(String codetype) {
      jUnitRunner.DISABLE_STUBBING_CHECK();
      Code code = mock(Code.class);
      doReturn(codetype).when(code).getCodetype(); // Not always, but in most cases, invoked
      jUnitRunner.ENABLE_STUBBING_CHECK();
   }

I am not that into the Mockito codebase, but I would guess that this could require quite some rewrite?

(And it also indicates that our usage of helper methods are probably not a smart path to follow...)

[bric3]

My point on helper method or helper object for mocks is usually that the model shows mock anti pattern. Or that the granularity of the test is not narrow enough. Usually that means that a concrete object should be built instead, not a mock.

The idea may be intersting though, probably not in mockito 2. In mockito 3 (JDK8) maybe we could introduce apis like this :

mockitoRule.inConfiguration()
                     .strictness(Strictness.LENIENT)
                     .stub(() -> { 
                           doReturn(codetype).when(code).getCodetype(); // possible stub
                           // ...
                     });

The above snippet is mostly exploring idea for this kind of API as I don't have these kind of needs at this time, it may be just wrong in many aspects.

[mockitoguy]

See #840 - I created a proposal for solving this use case for JUnit rules

[GregDThomas]

As an alternative solution, could you use the MockSettings? e.g. something like ..

Code code = mock(Code.class, withSettings().noUnnecssaryStubbing());

[digulla]

I also found a use case:

I'm mocking ZK which uses a lot of map-like structures which are exposed via accessors (getAttributes(), getAttribute(String), getAttribute(String, boolean), setAttribute(), ...).

In my test setup, I create a map and I'm using thenAnswer() and doAnswer() to link the accessor methods to my test map. The code then looks like this:

    private Map<String, Object> desktopAttributes = new LinkedHashMap<>();
...
            desktopAttributes.put(ATTR_EVENT_QUEUES, desktopEventQueues);

            MockDesktop desktop = mock(MockDesktop.class);
            when(desktop.getAttributes()).thenReturn(desktopAttributes);
            when(desktop.getAttribute(ArgumentMatchers.anyString()))
            .thenAnswer(answer -> {
                String name = answer.getArgument(0);
                Object value = desktopAttributes.get(name);
                return value;
            });
            when(desktop.getAttribute(ArgumentMatchers.anyString(), Mockito.anyBoolean()))
            .thenAnswer(answer -> {
                String name = answer.getArgument(0);
                Object value = desktopAttributes.get(name);
                return value;
            });
            doAnswer(answer -> {
                String name = answer.getArgument(0);
                Object value = answer.getArgument(1);
                desktopAttributes.put(name, value);
                return null;
            }).when(desktop).setAttribute(Mockito.anyString(), Mockito.any());
            doAnswer(answer -> {
                String name = answer.getArgument(0);
                Object value = answer.getArgument(1);
                desktopAttributes.put(name, value);
                return null;
            }).when(desktop).setAttribute(Mockito.anyString(), Mockito.any(), Mockito.anyBoolean());
            when(desktop.hasAttribute(Mockito.anyString()))
            .thenAnswer(answer -> {
                String name = answer.getArgument(0);
                return desktopAttributes.containsKey(name);
            });
            when(desktop.hasAttribute(Mockito.anyString(), Mockito.anyBoolean()))
            .thenAnswer(answer -> {
                String name = answer.getArgument(0);
                return desktopAttributes.containsKey(name);
            });

I need this code block three times (desktop attribute map, session attributes, servlet context attributes). I prefer mocking over mock objects since the interfaces have hundreds of methods.

I have put this code into a shared JUnit rule. It's lazy but it's also generic: Since the rule doesn't know what parts of ZK the test will call, it can't tell which accessor methods will be needed. But I still need to know when a method is called which wasn't mocked.

Therefore, I need a way to tell Mockito "there might be unused stubs in the following code block but that's ok since it's shared by many tests". An annotation on the test won't work since the code is in a JUnit rule. I could pass the MockitoRule to my rule, so this approach would work:

mockitoRule.lenientStubbing(() -> { ... });

[mockitoguy]

@digulla, thank you for reporting! Some feedback:

• The code looks really hard to read (many lines of mock interactions). I suggest to rethink testing strategy, refactor the code under test so that it is easier to test, or roll out hand mocks.
• Given above, we cannot consider it as a legit use case. We want to avoid implementing features / API for code that should be cleaned up / refactored for simplicity and testability.
• Given that Tweak JUnit rule strictness at the method level #840 solves this use case ('strictness' method on the rule object), I'm closing this ticket.

Ah, it was interesting to refresh my memory about this ticket :) Hope that helps!

[digulla]

@mockitoguy Sorry, that doesn't help at all.

With ~200K on StackOverflow, that was the most simple solution I could come up with. You're welcome to show me a better solution.

The testing strategy is sound. It doesn't break easily or unexpectedly and is easy to understand for new members of the team. I could copy only the necessary lines to new tests but that would mean I would be the only one who can write new tests. Also: Violates DRY.

Refactoring is not possible. ZK is a UI framework with a huge code base and many projects use it. Asking to change the API is like asking to "fix" the Java Collections API: Understandable, maybe even reasonable but unrealistic.

Hand mocks would mean I would have to write about a lot of useless code which violates your own rule to keep tests simple.

So I'm between a rock and a hard place: You're right for open source projects which no one uses or green field commercial projects. For existing commercial projects that I can't move, it's not helpful. They have ugly and unmodifiable APIs, so I have to move the only place where I have influence: That's the tests.

So for now, I have to disable a good feature of Mockito and can no longer make sure that my tests stay clean. That really sucks.

[mockitoguy]

Thank you for describing your context! Let me think about this and I'll get back.

[digulla]

I guess my use case is "I'm writing a mocking framework for some commercial API which helps other developers to write tests." That means I'll always overmock. In my case, I have to mock several Map-like APIs which are exposed in several beans without a common interface (just like the attribute maps in J2EE ServletContext and ServletRequest and the headers in ServletResponse).

[GregDThomas]

@digulla that pretty much matches my use case too. IMHO a good solution would be to be able to specify ‘Silent’ at mocked object level rather that the test class level.

[mockitoguy]

I like the idea of adding new public API for this use case.

@digulla, with my earlier reply, I did not intend to depreciate your efforts in getting clean tests for your entire team. It’s great that you’re pushing for this!

The use case you describe could be solved more cleanly with hand stubs. Sometimes simpler code is actually more code :) What do you think about this idea:

• noop implementation of the 3rd party interface. Generated automatically with IDE, cheap to maintain regardless of amount of methods because they are all noop. Low to zero cognitive overhead for the team.
• hand stub implementation, extends the noop implementation. Tailored for what exactly the tests need.

Coming back to Mockito for this use case. I bet that our users would disable strictness per entire test, rather than build hand stubs (even if hand stubs would be a better solution). Disabling strictness per entire test nullifies strictness benefits. To provide best dev experience, Mockito should honor this use case and offer a better API.

There are 2 main alternatives:

1. strictness per mock, as suggested

mock(Foo.class, withSettings().strictness(Strictness.LENIENT));
mock(Foo.class, withSettings().lenient()); //alias

2. strictness per stubbing, for example:

lenient().when(mock.foo()).thenReturn(“boo”);
lenient().doReturn(“boo”).when(mock).foo();

Any feedback?

Thank you guys for feedback and for pushing us to reconsider. We're trying to keep Mockito API slim and avoid solving use cases better solved by refactoring / cleanup the code.

[GregDThomas]

I did wonder about something like ...

@Mock (strictness= Strictness.LENIENT) 
private Foo mockFoo;

as well, but I wonder if that syntax would over-encourage people to use the feature; IME I think the sort of test frameworks, fixtures etc. where I see this to be most useful probably use the mock(Foo.class) mechanism for instantiating mocks.

[ChristianSchwarz]

@mockitoguy

There are 2 main alternatives:

I see a third one. As discussed in the JUnit 5 PR (#1221) and the mockito google group I would like to propose an annotation base approach that can be applied also at test-class and test-method level. This keeps the API consistent and slim (at least when JUnit5 is used).

@Mock
@Strictness(LENIENT)
Foo mock;

[mockitoguy]

@ChristianSchwarz, you're right! Sorry for discounting the annotation option.

[mockitoguy]

I plan to tidy up this ticket in the next few days so that it documents the use case, the implementation options and the desired implementation.

@ChristianSchwarz, strictness per method is not an ideal solution for the use case described in this ticket:

1. every time someone uses shared stub object, he needs to remember to use lenient strictness on the method level
2. method level strictness turns off useful stubbing validation for all mocks in the method, rather than only for the mocks that really need it.
3. it does not communicate the intent well. The reader of the test method does not know why lenient strictness is needed for method. This also can lead to cargo culting - developer copy paste-ing test methods along with strictness setting, without known why they need it.

I must admit that the @strictness annotation does look handsome in the test :)

What do you think about option 1 and 2? Do we want to implement both? Perhaps we start with mock level annotation for now. It solves the use case very well and is not very controversial.

[ChristianSchwarz]

@mockitoguy

@ChristianSchwarz, strictness per method is not an ideal solution for the use case described in this ticket:

Thats why I proposed to use the @Strictness annotation at field level for this use-case too.

The @Strictness annotation at field level would also fit into the annotation proposal for JUnit5. This in return would lead to a clean single API, where strictness is defined at field, method and class level the same way using @Strictness.

What do you think about option 1 and 2?

I would prefer option 1 cause:

• It feels consistent with other mock setting configurations
• It allows to implement the annotation based proposal too
• option 2 would add an other alias like API for stubbing, thats uncool

Do we want to implement both?

Option1 + @Strictness ? Yes!

Perhaps we start with mock level annotation for now. It solves the use case very well and is not very controversial.

@Mock + @Strictness(...) or @Mock(strictness=...) ? I would opt for @Strictness(...) cause it allows more uses cases.

[mockitoguy]

That's why I proposed to use the @strictness annotation at field level for this use-case too.

That's fair!

Let's discuss the 2 contention points:

1. It is awkward that some mock properties are configured as parameter of the existing @mock annotation, while some other (Strictness) is a standalone annotation.
2. We need to support Strictness at mock level via imperative API, e.g. "withSettings().lenient()" method (or something like that). This is one of our principles of API development (which we should document :), annotations are "syntax sugar" - all Mockito features are available via imperative API. Given that we implement "withSettings().lenient()", why do we need @strictness annotation at all? I'm pushing on this, because the only thing better than clean API is clean and simple API. The less methods, annotations, complexity - the better!

[digulla]

@mockitoguy Re "hand stubbed": That doesn't work since Java doesn't allow multiple inheritance. I just pasted a small part of the actual test setup. My setup class can mock several aspects of the ZK API: Session and request attributes, event queues, view models, injection. What I end up with a mix of lenient and dedicated mocks.

With a hand stubbed approach, I would have to copy a lot of code from test to test. My goal is to have lenient mocks for unimportant parts like session attributes (code under test will complain when they are missing) but strict checking for things like events published (I really don't want to miss those).

The test can then say "hey, wire up event publishing" if they expect events. Without the setup, any code using events will just crash with NPE. With the setup, Mockito should complain when no events related code was executed to keep the tests clean (to avoid devs doing copy&paste of code they don't understand).

Re annotations: I don't think that the annotations are a good solution here. They are nice for code which needs to be migrated from old Mockito but for new code, they are too coarse for all my use cases.

On the other hand, I don't like the idea of having to add withSettings().lenient() in many places. It would be great if I could execute a bunch of stub calls in a single lenient code block. If that was possible, I'd opt for an annotation on the setup method which calls the when()/doReturn() but I don't think there is an easy way to implement this when the setup method is not inside the test class but in a JUnit @Rule implementation.

[mockitoguy]

Thank you for reply! I'm still interested in your use case hence my further questions.

With a hand stubbed approach, I would have to copy a lot of code from test to test.

Why? Hand stub implementation should have all the common reusable code. In the test, you can just call "mock.setupEventPublishing()" or something like that.

On the other hand, I don't like the idea of having to add withSettings().lenient() in many places. It would be great if I could execute a bunch of stub calls in a single lenient code block.

Provided that we add both: stubbing + mock level strictness setting, then you have 2 options:

1. Only add "withSettings().lenient()" at mock creation time, only for mocks that act as "reusable business stubs" (wasn't sure how to call them). When you write stubbings, you don't specify "lenient()".
2. For other cases, where you have a default stubbing reused in most of the test methods, you declare the default stubbing using "lenient()" method at the stubbing level.

Thoughts?

[digulla]

@mockitoguy Thanks. I feel that I don't have a completely clear picture, yet.

Hand stub

So a base class with a lot of empty methods and then using spy() to overwrite them? That might work but I don't like spy() (had too many problems with that approach in the past).

It still feels like more effort than simply wire some map methods to API methods using Answer.

2 options

Option #1 is too coarse for me. With option #2, I would have to add 7 lines of code to the block in #792 (comment) and you were complaining that the code was already too complicated :-)

That's why I suggested to have an "enable some options for a code" block approach. In my case, I could then wrap the call to the setupDesktopAttributes() method with that.

[mockitoguy]

So a base class with a lot of empty methods and then using spy() to overwrite them?

Hand stub/mock is a class that is completely tailored for the business of your tests. Sometimes referred as "test fixture code", "test utils", "test support classes". Hand mocks don't use Mockito, not even spy(). Hand stub needs to be coded and maintained but it provides superior readability and usefulness because it is tailored to your domain.

Option #1 is too coarse for me. With option #2, I would have to add 7 lines of code to the block

We could potentially provide a method like:

Mockito.lenient(() -> { ... });

Not sure about this, though. It's a very different API than what we have so far. Per-mock + per-stubbing leniency feels like a happy medium: if repeated "lenient()" is a problem, you can configure it per the entire mock. Per-mock is coarse grained but on the other hand it is conventional and simple. Big "lenient" block might lead to weird test code, developers putting more code inside the block, the intent of the code getting obscured.

[mockitoguy]

Updated the description with the problem statement and proposed API changes. Feedback before we start coding?

[mockitoguy]

Update: work in progress in PR #1272.

Friendly ping for review of the design outlined in this ticket description!

[digulla]

I'm pondering the two API designs: mock(..., withSettings().lenient()) and lenient().when(...)
I feel it would be better if the two were more similar.

Why not when(..., withSettings().lenient())? That way, the API would be more symmetric. When adding more settings, they would naturally flow into the existing code. But it would be hard to add options only for mock() (= problems can only be discovered at runtime).

What about mock(...).lenient()? Or lenient().mock(...)? That would be the opposite approach to make the two APIs similar. Here, the chaining pattern can make sure that you can't use options that are illegal.

Also, this is shorter. I feel withSettings() adds a lot of visual bloat.

[mockitoguy]

I'm pondering the two API designs: mock(..., withSettings().lenient()) and lenient().when(...)
I feel it would be better if the two were more similar.

Yeah, possibly. We also want to have those new API methods consistent / similar with the existing API:

• currently the way to configure mocks is: withSettings() or annotation
• stubbing is inconsistent (when vs. doReturn, when vs. given) due to the API evolution and Java limitations. There is no concept of configuring a "stubbing" in Mockito API currently (assuming that declaring what happens when stubbed method is called is part of the stubbing, and not an operation of "configuring stubbing").

Why not when(..., withSettings().lenient())?

How would this play with doReturn syntax?

What about mock(...).lenient()? Or lenient().mock(...)?

The former is not possible because mock() returns the type we're mocking. The latter is interesting, problem is that it is inconsistent with how currently mocks are configured (withSettings()).

Thank you for thoughtful feedback!

[digulla]

How about a fluent API to mock several methods of a mock at once? That way, I could specify the lenient once at the top?

[mockitoguy]

How about a fluent API to mock several methods of a mock at once? That way, I could specify the lenient once at the top?

Can you write a comment with an example how it would look? This would help us make a decision. Sorry for late answer. Xmas break :) I made progress on the ticket, though!

[mockitoguy]

I'm finalizing the implementation in #1272, it will be released within days.