Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strictness in Mockito #769

Open
mockitoguy opened this issue Nov 19, 2016 · 54 comments
Open

Strictness in Mockito #769

mockitoguy opened this issue Nov 19, 2016 · 54 comments

Comments

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Nov 19, 2016

Introduction

This issue explores the addition of "strict" behavior and APIs to Mockito. Traditionally, Mockito is a lenient mocking framework. For background and motivation, check out Szczepan's short article on LinkedIn or in-depth article on Mockito blog.

Why strictness in Mockito:

  • productivity - improved debugging and writing tests. Currently MockitoHint warns about stubbing argument mismatches. Strict stubbing would promote the warning into an exception, making the test fail fast and improve debuggability / productivity.
  • clean code - avoid unnecessary stubbings, test fails when unncessary stubs are present.

Concrete API proposals:

  • "strict stubbing" proposal: #770
  • "strict mocking" proposal: #1097
  • opt-out from strict stubbing at mock / method level: #792

Example exception message indicating subbing problem:

org.mockito.exceptions.misusing.PotentialStubbingProblem:
Strict stubbing argument mismatch. Please check:
- this invocation of 'simpleMethod' method:
    mock.simpleMethod(“Foo");
      -> at … StrictStubbingEndToEndTest.java:101
- has following stubbing(s) with different arguments:
    1. mock.simpleMethod(“foo");
      -> at … StrictStubbingEndToEndTest.java:100
Typically, stubbing argument mismatch indicates user mistake when writing tests.
Mockito fails early so that you can debug potential problem easily.

Details

Strictness in Mockito can have 2 forms:

  • Strict stubbing that requires that all declared stubs are actually used
  • Strict mocks, that require all non-void methods to be stubbed before they are called

Strictness behavior can be implemented as:

  • warning on the console
  • exception thrown

Strictness API can be implemented in:

  • JUnit runner / rule
  • TestNG runner
  • explicit method to call by the user

Future direction:

  • strict stubbing on by default, opt-out available
  • strict mocking off by default, opt-in available

Implementation as of Mockito 2.2.x:

  • JUnitRunner - fails at the end of test class when unused stubbings are present. Opt out available via JUnitRunner.Silent
  • JUnitRule - warn at the end of test method about a) unused stubs b) stubs invoked with different arguments. Opt-out available via rule().silent().

Mockito 2.x proposal:

  1. New "strict stubbing" API for JUnit rule - #770
  2. New Answer.StrictMock that fails immediately when non-void method invoked on without prior stubbing - #649
  3. new verifyStubs() method to use when rule/runner not in used

Mockito 3 proposal:

  • strict stubbing is the default
  • change behavior of verifyNoMoreInvocations()
  • runner and rule behave the same
mockitoguy added a commit that referenced this issue Nov 19, 2016
Needed for work on Mockito strictness, see #769. Decoupled JUnit API from the work Mockito does before and after every test.

No public API exposed, listeners and friends need to be refactored and documented.

Don't merge. This refactoring on its own does not add value.
@imod
Copy link

@imod imod commented Nov 21, 2016

I think from a code maintenance point of view, "behavior: warning on the console" is not really useful - in such a case a CI system would not inform anyone about unused stubs. An exception would be a lot better.

mockitoguy added a commit that referenced this issue Dec 10, 2016
Needed for work on Mockito strictness, see #769. Decoupled JUnit API from the work Mockito does before and after every test.

No public API exposed, listeners and friends need to be refactored and documented.

Don't merge. This refactoring on its own does not add value.
mockitoguy added a commit that referenced this issue Dec 11, 2016
Needed for work on Mockito strictness, see #769. Decoupled JUnit API from the work Mockito does before and after every test.

No public API exposed, listeners and friends need to be refactored and documented.

Don't merge. This refactoring on its own does not add value.
@TimvdLippe
Copy link
Contributor

@TimvdLippe TimvdLippe commented Dec 13, 2016

@szczepiq could you add list items (- [ ] in markdown) to signify what the remaining action points in this issue are? That gives a clearer overview, especially as multiple open issues are related to this single epic.

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Jan 13, 2017

@szczepiq could you add list items (- [ ] in markdown) to signify what the remaining action points in this issue are? That gives a clearer overview, especially as multiple open issues are related to this single epic.

I will do that once this epic turns into an execution plan. Currently it is intended to scope out the details and I don't know if all of them will be executed (and in what form). I annotated one item as merged by using strikethrough. I hope it is clearer now.

@micheljung
Copy link

@micheljung micheljung commented Jan 31, 2017

Since your Javadoc asks for feedback, here's my case:

  @Test
  public void testAutoCompleteDoesntCompleteWhenTheresNoWordBeforeCaret() throws Exception {
    when(playerService.getPlayerNames()).thenReturn(FXCollections.observableSet("DummyUser", "Junit"));
    textInputControl.setText("j");
    textInputControl.positionCaret(0);

    simulate(keyEvent(KeyCode.TAB));

    assertThat(textInputControl.getText(), is("j"));
  }

It tests auto-completion of an input field.

getPlayerNames() is stubbed, but never called in working code - because if the caret is at position 0, there should be no autocompletion. If I remove the stubbing it's possible to break the autocompletion without being detected; the test would succeed not because it's properly implemented, but because there are no users being returned.

Update:
After hitting enter I realized that I can improve this using verify(playerService, never()).getPlayerNames().
I decided to not remove the comment though :)

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Feb 1, 2017

@micheljung, my recommendation for this scenario is exactly using verify + never :) I will assume that your use case is satisfied. Looks like strict stubbing nudged you to improve this test!

Thank you very much for reporting. This is exactly kind of feedback we're looking for when evaluating strictness feature.

@skestle
Copy link

@skestle skestle commented Jul 21, 2017

I would like an option to verifyNoMoreInteractions in a smart way:

  • ignoring expected/stub invocations as in STRICT_STUBS mode
  • ignoring unnecessary stubs methods as in non-strict modes.

I'm simulating an external API (Selenium) to ensure I wrap it properly.

	public void expectToFindNoElement(final By by) {
		when(this.webElement.findElement(by)).thenThrow(new NoSuchElementException("Cannot locate an element " + by));
		when(this.webElement.findElements(by)).thenReturn(Collections.emptyList());
	}

It's very easy to accidentally not throw the exception when we're expecting no element; the correct thing selenium consumers generally want to do is verify no elements.

Essentially, I can't be DRY with Strict without breaking it's strict "clean and maintainable" expectations.

BTW, I agree this is not "clean" but it is certainly very much more maintainable (and correct) than what STRICT mode allows.

@skestle
Copy link

@skestle skestle commented Jul 21, 2017

So in terms of 8.5.47's implementation, I'd like to haveUniversalTestListener.testFinished in LENIENT mode, while DefaultStubbingLookupListener.onStubbingLookup executes invocation.markVerified(), as it does in strict mode.

Probably what I want is DRY without strictness as mockito wants to define it. IMO, DefaultStubbingLookupListener and DRY verifyNoMoreInteractions() has nothing to do with strictness.
Please separate these two concepts.

@skestle
Copy link

@skestle skestle commented Jul 21, 2017

Also, having tried for half an hour trying to grok StubbingLookupListeners, CreationSettings MockitoTestListeners etc, it seems there's no way to insert invocation.markVerified() without creating my own Test Runner.

If there's some sort of workaround where I can make this auto-verification automatic, please respond. Should there be another ticket to add auto-verification of stubbed invocations (which is an awesome feature BTW) that's separate from this ticket (as it should be)?

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Jul 21, 2017

Thank you for reporting and debugging Mockito strictness! Let me give you some feedback about the use case first:

I would like an option to verifyNoMoreInteractions in a smart way:
ignoring expected/stub invocations as in STRICT_STUBS mode
ignoring unnecessary stubs methods as in non-strict modes.

It seems that you are suggesting a different Strictness level, something between "lenient" and "strict stubs":

  • lenient - allow unused stubbings, veryNoMoreInteractions catches all
  • "kinda strict stubs" - allow unused stubbings, verifyNoMoreInteractions ignores used stubbings
  • strict stubs - disallow unused stubbings, verifyNoMoreInteractions ignores used stubbings

Have you tried this:

verifyNoMoreInteractions(ignoreStubs(mock1, mock2));

Before we jump to the impl details of TestListener (which is an internal type, not intended for regular use, but absolutely open to experimentation and creative usage), let's nail down the use case first.

Thank you for reporting! This is a very interesting use case.

@karollewandowski
Copy link

@karollewandowski karollewandowski commented Aug 22, 2017

I like strict stubbing very much.
I've seen opinion that it allows to remove a lot of code, but I think that it rather forces to write a little more. I used to put common stubbing code to setup method even if it wasn't used by all tests. Now I have to move it to tests actually using it. I think that it's nothing bad and makes my tests clearer. When stubbing code is in test method, then there is no need to scroll source file to top to check what is stubbed and what is not. Everything is in one place and test context is explicit and close to it's usage. More code doesn't mean duplications - common stubbing can be extracted to method with descriptive name.
@szczepiq, on Twitter you mentioned that you are wondering API like when(method, optional()).then(...). I think that it's not necessary, but sometimes can be helpful.
It would allow to create optional stubbing in setup method what would break clarity provided by strict stubbing again. On the other hand, sometimes we don't care about some dependency or there are tens of tests using some stubbing and one not, so optional() would be nice to handle such cases (and it clearly says it's optional).

@adamvoss
Copy link

@adamvoss adamvoss commented Jan 30, 2018

Can this be exposed by MockSettings or otherwise be made available without requiring the Mock instance be a class member.

The following "works" but uses a whole lot more internal stuff than I like:

import org.mockito.MockSettings;
import org.mockito.Mockito;
import org.mockito.internal.creation.settings.CreationSettings;
import org.mockito.internal.listeners.StubbingLookupListener;
import org.mockito.mock.MockCreationSettings;
import org.mockito.quality.Strictness;

import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;

import static org.mockito.Mockito.withSettings;

public final class StrictMockito
{
   private StrictMockito()
   {
   }

   public static <T> T strictMock(Class<T> classToMock) {
      final MockSettings mockSettings = withSettings();
      createDefaultStubbingLookupListener(Strictness.STRICT_STUBS);
      final T mock = Mockito.mock(classToMock, mockSettings);
      final MockCreationSettings<?> mockCreationSettings = Mockito.mockingDetails(mock).getMockCreationSettings();
      ((CreationSettings<?>)mockCreationSettings).getStubbingLookupListeners().add( createDefaultStubbingLookupListener(Strictness.STRICT_STUBS));
      return mock;
   }

   private static StubbingLookupListener createDefaultStubbingLookupListener(Strictness strictness)
   {
      try
      {
         final Class<?> clazz = Class.forName("org.mockito.internal.junit.DefaultStubbingLookupListener");
         final Constructor<?> constructor = clazz.getDeclaredConstructor(Strictness.class);
         constructor.setAccessible(true);
         return (StubbingLookupListener) constructor.newInstance(strictness);
      }
      catch (ClassNotFoundException | NoSuchMethodException | IllegalAccessException | InstantiationException | InvocationTargetException e)
      {
         throw new RuntimeException(e);
      }
   }
}
@adamvoss
Copy link

@adamvoss adamvoss commented Jan 30, 2018

If possible, the failure message around passing null and collections could use improvement. As is, we can get errors like the following:

Strict stubbing argument mismatch. Please check:
 - this invocation of 'getValue' method:
    sut.getValue("", null, []);
    -> at ExampleTest.strictExample(ExampleTest.java:64)
 - has following stubbing(s) with different arguments:
    1. sut.getValue("", null, null);
      -> at ExampleTest.strictExample(ExampleTest.java:62)

It is not clear from the failure message that only the middle argument is the source of the problem. One is likely to expect the final argument is the problem because it is the entry that is different.

Code to reproduce the above:

   public interface Sut
   {
      Object getValue(String foo, Object bar, Set<String> baz);
   }

   @Mock
   Sut sut;

   @Test
   public void strictExample() {
      Mockito.when(sut.getValue(ArgumentMatchers.anyString(), ArgumentMatchers.any(Object.class),
            ArgumentMatchers.eq(Collections.emptySet()))).thenReturn(null);

      sut.getValue("", null, Collections.emptySet());
   }
@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Jan 31, 2018

Hey @adamvoss! Thanks for checking in!

Can this be exposed by MockSettings or otherwise be made available without requiring the Mock instance be a class member

Mocks don't have to be class members to leverage strictness. You can use MockitoSession object to start mocking before you create mocks. Would that solve your problem?

@adamvoss
Copy link

@adamvoss adamvoss commented Feb 16, 2018

@mockitoguy Sorry, I missed the notification on this one.

Here is what that looks like:

public final class MockTest
{
   @Test
   public void doTest() {
      final MockitoSession session = Mockito.mockitoSession().strictness(Strictness.STRICT_STUBS).startMocking();
      final IWhatever mock = Mockito.mock(IWhatever.class);
      when(mock.getResult("bar")).thenReturn("yes");
      Assert.assertNotNull(mock.getResult("foo"));
      
      // Should be in tear down (after) method
      session.finishMocking();
   }
}


interface IWhatever {
   public String getResult(String foo);
}

I would say that is more flexible, but not exactly what I was looking for. One of the motivations being able to specify it on a per-mock setting is to be able to introduce them gradually. That way I can use strict when I want to (including existing tests) without changing any of the existing mocks.

With you wanting to make this eventually the default mode, then I can see the argument for not allowing granular configuration. (Though once on v3, people may want to gradually use some lenient mocks). That aside, it is an annoyance to need to setup and tear down a session just to create a mock, but maybe that is unavoidable.


Was there any thought to making an even stricter mode? When I was working on a reproduction to demonstrate using MockitoSession I kept failing because the mock was not as strict as I was expecting. For example, I'd want the following to fail.

public final class MockTest
{
   @Test
   public void doTest() {
      final MockitoSession session = Mockito.mockitoSession().strictness(Strictness.STRICT_STUBS).startMocking();
      final IWhatever mock = Mockito.mock(IWhatever.class);
      Assert.assertNull(mock.getResult("foo"));
   }
}

interface IWhatever {
   public String getResult(String foo);
}

and

public final class MockTest
{
   @Test
   public void doTest() {
      final MockitoSession session = Mockito.mockitoSession().strictness(Strictness.STRICT_STUBS).startMocking();
      final IWhatever mock = Mockito.mock(IWhatever.class);
      Assert.assertNull(mock.getResult());
   }
}

interface IWhatever {
   public String getResult();
}

In both cases the test will pass when I would want the strict mocks to fail because I didn't configure the methods invoked.

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Oct 24, 2018

@markusjevringgoeuro

In addition, the "Unnecessary stubbings detected" code is not great at what it does. There are many times where the stubbing is not, in fact, unnecessary, so then I have to make mockito lenient to work around it.

Thank you for feedback. We'll improve it! Can you give us an example when stubbing is necessary but Mockito flags it as "unnecessary"?

@Druckles
Copy link

@Druckles Druckles commented Oct 24, 2018

No problem. I understand some of these points are subjective, which is why I feel it shouldn't be enabled by default.

I may sometimes have intentional "unnecessary" stubs
If I have to conditionally stub it

Both of these might be seen here:

void oneThing() {
  Thing thing = createBasicMock("Alice");
  bool result = theThingToTest.theActionToTest(ImmutableList.of(thing));
  assertThat(result, is(true));
}

void severalThings() {
  List<Thing> things = ImmutableList.of(createBasicMock("Alice"), createBasicMock("Bob"),
      createBasicMock("Charlie"));
  bool result = theThingToTest.theActionToTest(things);
  assertThat(result, is(true));
}

void nowDoSomethingBad() {
  Thing badThing = createBasicMock("Alice");
  when(badThing.howAreYou).thenThrow(...);
  bool result = theThingToTest.theActionToTest(ImmutableList.of(badThing));
  assertThat(result, is(false));
}

private Thing createBasicMock(String name) {
  Thing mockedThing = Mockito.mock(Thing.class);
  when(mockedThing.getName()).thenReturn(name);
  return mockedThing;
}

There are numerous examples in this scenario where an "UnnecessaryStubbingException" could be thrown in these tests because there are numerous ways the getName() method may (or may not) be called. In severalThings, it may be that getName() is not needed for some of the objects (perhaps it is inferred). It may also be the case that getName() is only needed for logging. One may have several tests covering different exception cases in which getName() is needed so moving it out into the test methods would be unnecessary work.

Trying to work around this would require stubbing conditional (e.g. checking to see if name is null, or even overloading, both of which adds unnecessary complexity to the tests).

I've also just discovered the Issue from @EmilyBjoerk, which explains this a lot more eloquently than I have.

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Oct 25, 2018

The use case is about reusable mock fixtures. We don't recommend Mockito for this. It is better to implement reusable objects as regular classes / fluent builders in the test code. If you want to use Mockito for that, you can create lenient mocks:

mock(Foo.class, withSettings().lenient());

I've also just discovered the Issue

please link it!

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Oct 25, 2018

disagree with the "Unnecessary stubbings" presented here

@Druckles, I talk with many Mockito users. They like strictness and they are very happy with unused stubbing detection. We don't want dead test code (unused stubs) because it makes the code harder to maintain. Reduced dead code is an objective argument in favor of the feature.

I help teams write clean, testable code. We never build elaborate, reusable mock fixtures. Such fixtures are hard to maintain, understand and indicate design flaws. We write tests that have minimum amount of mock objects, stubbed/verified interactions. TDD really helps with this. If we need complex nested objects for testing, we implement fluent builders:

//instead of:
void oneThing() {
  Thing thing = createBasicMock("Alice");
   ...
}

//we want:
void oneThing() {
  Thing thing = new ThingBuilder().name("Alice").build();
   ...
}

Thoughts?

@Druckles
Copy link

@Druckles Druckles commented Oct 25, 2018

please link it!

It is linked in my original post and explains the use cases much more explicitly and thoroughly. Perhaps it is worth reading through again.

It is better to implement reusable objects as regular classes / fluent builders in the test code. If you want to use Mockito for that, you can create lenient mocks

Setting a mock to lenient disables other checks which are desired. Setting them to lenient makes them in other aspects more difficult to maintain.

Implementing "reusable classes" or even builders does create unnecessary code that increases complexity and decreases maintainability.

Such fixtures are hard to maintain, understand and indicate design flaws.

I have read your blog post (and all your comments) and this is the point I am trying to disagree with. It is also an absolute: there are absolutely cases where this does not apply and "unnecessary stubbings" are very useful and increase maintain- and readability.

My final point, which is a reiteration of one of the points I listed above, is that this kind of 'strictness' has been unnecessarily mingled with another, that of unmocked stubs. We recently migrated to the Mockito-supported MockitoExtension and were faced with a tonne of broken tests. Many of them had what were deemed "Unnecessary Stubbings" but changing them would have increased the complexity. We simply had to set them to lenient, leaving them open in the future to unmocked stub problems. I believe the two things should be completely separate.

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Oct 25, 2018

Setting a mock to lenient disables other checks which are desired

What other checks? The potential stubbing problem?

I believe the two things should be completely separate.

Thank you for feedback!

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Oct 25, 2018

Implementing "reusable classes" or even builders does create unnecessary code that increases complexity and decreases maintainability.

Reusable mock fixtures is extra code, just like builders. I wouldn't call either of them "unnecessary" because we need them for testing.

We keep our recommendation (rule of a thumb) to use builders over mocking framework for creating objects for tests. It is a subjective recommendation based on our experience. Mocking framework is designed for mocking interactions, not for creating objects with many stubbings that return mocks, that return mocks, that return mocks...

Can you put together a real example (a couple of classes, push it to some repo on GitHub), where you can demonstrate that reusable mock fixtures are 1) useful 2) better option than builders.

Thank you! This is a great discussion.

@Druckles
Copy link

@Druckles Druckles commented Oct 26, 2018

What other checks? The potential stubbing problem?

Unmocked stubs, for instance. If I set my entire mock to lenient, I will miss these errors. I basically have to make my tests less robust and harder to maintain because I don't care how often getName() was called. And if I don't have it on lenient my tests are less robust and harder to maintain for the reasons outlined above.

I think this is in part due to Java's mutable nature. One assumes that method calls will have side-effects. If you're coding immutably by default (it doesn't matter how often you call a method), then "unnecessary stubbings" should not bother you. (Unless you're considering performance, but your unit tests are not for optimisations).

Reusable mock fixtures is extra code, just like builders. I wouldn't call either of them "unnecessary" because we need them for testing.

Reusable mock fixtures is less code, a single reusable method (as in the example above). Your example builder requires an entire extra class, ThingBuilder.

I didn't actually mention mocks returning mocks anywhere. Although I'd be interested how you solve this if not with a mocking framework...?

Can you put together a real example (a couple of classes, push it to some repo on GitHub), where you can demonstrate that reusable mock fixtures are 1) useful 2) better option than builders.

No, I don't think I can expand on my example or further on the description given in the linked Issue above. But this is why it's a problem; at the very best it's a subjective issue, a coding practise or, as you put it, recommendation. You are imposing this "rule of thumb" on the developers and users of Mockito.

Thank you! This is a great discussion.

Good discussion, but I feel like I'm repeating myself ;-)

@Druckles
Copy link

@Druckles Druckles commented Oct 26, 2018

Another way of looking at it: what's the point of ever calling verify if my when().then()s are doing part of the work for me?

@Druckles
Copy link

@Druckles Druckles commented Oct 26, 2018

Another couple of points on the point of what you've labelled "reusable mocks".

  • Why inject mocks if you don't expect them to be reused? I.e. why even bother with MockitoExtension. Everything it does can be accomplished within the test, i.e. without exposing the mock to reuse.
  • Why use answers if you shouldn't be reusing mocks? Just return the expected value.

The point of mocks is to abstract interactions with other, more complex objects. Reusing them reduces this complexity and thereby reduces the number of possible bugs that you might create. Add Builders or even instances of the objects and they stop being unit tests.

@rliesenfeld
Copy link

@rliesenfeld rliesenfeld commented Nov 9, 2018

My two cents: whenever you think of creating a mock/stub, consider not doing it in the first place! Use a real object, unless you have a legitimately good reason to employ mocking or stubbing. Having written thousands of JUnit/TestNG tests over many years, my experience tells me the less mocking the better.

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Nov 22, 2018

No, I don't think I can expand on my example or further on the description given in the linked Issue above. But this is why it's a problem; at the very best it's a subjective issue, a coding practise or, as you put it, recommendation. You are imposing this "rule of thumb" on the developers and users of Mockito.

@Druckles, without comprehensive code sample (linked sample is not strong enough), we won't be able to do an objective evaluation. Mockito is an opinionated framework. We are imposing our way of writing tests because we strongly believe it is good for our users. We listen to our users intently. We still make our opinionated choices. Our goal is not to make everybody happy and get consensus on all of the features. Consensus hates innovation and loves status quo.

I don't intend to close the discussion, we just need to see the code.

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Nov 22, 2018

This PR resolves many false negatives with PotentialStubbingProblem: #1539

@Druckles
Copy link

@Druckles Druckles commented Nov 25, 2018

@Druckles, without comprehensive code sample (linked sample is not strong enough), we won't be able to do an objective evaluation.

If I write a more in-depth example, you ignore the actual issue and criticize the code, claiming it's too complicated, or not following any number of "clean code" principles. Or you reuse your arguments from above and claim that a builder or an actual instance would be better. In other words, it distracts from the topic. The given example in #720 is the perfect example: it is as minimal as possible while still demonstrating the problem. I don't understand how a more "comprehensive" code sample would illustrate it any better.

Consensus hates innovation and loves status quo.

Fortune cookies hate truth and love ambiguity. This is a fallacy (possibly an appeal to fear, appeal to consequences and a strawman all in one). There's nothing wrong with innovation or the status quo in and of themselves. And agreeing with me does not "make everybody happy".

The actual issue is still that PotentialStubbingProblems and UnnecessaryStubbings have nothing to do with each other and yet are included in the same "Strictness". The former is a logical error, the second a subjective issue.

@Druckles
Copy link

@Druckles Druckles commented Nov 25, 2018

My two cents: whenever you think of creating a mock/stub, consider not doing it in the first place! Use a real object, unless you have a legitimately good reason to employ mocking or stubbing. Having written thousands of JUnit/TestNG tests over many years, my experience tells me the less mocking the better.

I'm talking about unit tests here, not integration tests. Unit tests are supposed to be self-contained and treat the code like black boxes. They require mocks. If you are not mocking the dependencies between your classes, you are not testing the piece of code. You are testing the dependencies themselves and opening your tests to unnecessary failures.

If you have been having issues with mocks, perhaps you are using them in the wrong places.

One final point: many would consider it passive-aggressive to go through a thread "voting up" the comments you agree with and "downvoting" those you don't. Not to mention childish.

@mockitoguy mockitoguy self-assigned this Nov 27, 2018
@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Nov 27, 2018

@Druckles, we got your feedback. Let's see if more users share your opinion. I like the idea of separately configurable PotentialStubbingProblems and UnnecessaryStubbings. However, I'm not sure if we can pull it off while keeping the API simple.

Thank you for the feedback!

I'm curious:

  • why do you consider unit tests "black box" tests? Aren't integration tests "black box" and unit tests "white box"?
  • why do you consider unnecessary stubs a subjective issue. Isn't dead code objectively undesired?
@Druckles
Copy link

@Druckles Druckles commented Nov 27, 2018

* why do you consider unit tests "black box" tests? Aren't integration tests "black box" and unit tests "white box"?

I guess it depends how you draw them up. I'll admit that it's not a given that they're black box and it's also somewhat down to personal preference. But the advantage of black box tests are, as mentioned in #720, they are more robust and can reduce duplication.

If you're doing test-driven design, you're treating the class under test as a black box, only worried about the input and output. If you're writing tests for an existing class, trying to cover all possible conditions, you're taking the implementation into consideration, hence more white-box.

It's also possible to write integration tests in a white-box style. The two concepts are orthogonal :-)

* why do you consider unnecessary stubs a subjective issue. Isn't dead code objectively undesired?

Yes, absolutely, in the implementation. Not only undesired, but also dangerous. Which is why a PotentialStubbingProblem is an objective issue and can be very useful. Moreover, PotentialStubbingProblems look after null pointers, which are very dangerous.

Dead code in tests is also undesired, particularly if it's not used anywhere in the tests. However, as already mentioned, dead code for one test may be useful for another test. And my argument is that it's a subjective issue as to whether you include some "dead code" to gain readability and simplicity in your tests.

I personally find warnings perfectly reasonable if code wasn't called at all; it can also be helpful to see what has changed in a class. But causing the test to fail and subsequently the build, is (in my opinion) dangerous as it obfuscates the meaning of failure, mixing up incorrect implementation (bad!) with dead code in the tests (not ideal).

I hope I was more articulate in this response; I feel the distinction between the two issues is important.

@imod
Copy link

@imod imod commented Nov 28, 2018

I mostly agree with @Druckles, but I definitely want our builds/ test to fail if I have stubbing issues and also if I have unnecessary stubbings. And looking at the reactions on #769 (comment) I seem not to be the only one. For me this is all about code maintenance and stubs with are not used are dead code which should be removed.
And as I commented here on #1097 (comment), I would really appreciate a global (e.g. system property) switch where I can enforce this as a rule for the whole project and don't want to repeat the same rule (enable/disable) in every single test case. This would allow me to enforce the rule while running the test on our CI system, even if the developer does not have it set lacaly.

@rliesenfeld
Copy link

@rliesenfeld rliesenfeld commented Dec 1, 2018

@Druckles You should give it a try. What's important is to have good automated tests, not whether they use mocking or not. In fact, I do use mocks/fakes in a few tests (and without any difficulties), but only when there is a good reason to (mostly to avoid issues with remote web services, and to provide the necessary testing infrastructure). Insisting on mocks when there is no need only leads to poor tests, in my experience.

Note that people like Kent Beck (JUnit, TDD), Martin Fowler, and D. H. Hansson (Rails) were never in favor of units tests with mocks. Also, those "unnecessary failures" with integration tests don't occur in practice, in my experience (Fowler wrote about this, but I can't find it now).

@jonbarksdale
Copy link

@jonbarksdale jonbarksdale commented Dec 1, 2018

As another data point, I prefer unit tests to be mostly black box, and I only verify mocked calls when there are side effects. I do it this way because I want my unit tests to be robust enough so I can refactor, not touch my unit tests, and know if I broke anything. If I have to change my unit tests every time I make a change, I increase the chances that I've introduced a bug.

I also tend to put all of my happy path stubbing into a setup method, and then overwrite any mocked calls in the actual test methods so it's immediately obvious what change is causing what behavior. Maybe I'm doing it wrong, but it's made my tests a whole lot easier to maintain, easier to read, and allows me to refactor my production code with confidence.

However, this approach doesn't work very well with the unnecessary stubbing checks, as not all of my happy path stubs will get hit when I'm testing an exception flow, for example. If it only identified stubs that weren't used by any test method, it'd be great, but I don't think that's possible with how test runners execute, or the analysis would have to be done after the entire class runs.

The only alternative I can see involves adding either a lot of very similar code blocks, or a stub setup method that's roughly as complex as the code it's testing, and I'm not a big fan of either approach. Naturally, if you have a better way for dealing with complex, dependency-filled legacy code, I'd love to hear it.

@dalewking
Copy link

@dalewking dalewking commented Dec 3, 2018

See #1527 where I laid out a more complex real world example where mocking methods that are not called is necessary. Here is an even simpler example:

The mocked interface:

public interface Rectangle
{
    int getHeight();
    int getWidth();
}

And you want to test a method that determines whether that rectangle can hold a 3x3 square. The 2 simplest implementation of that method are:

public boolean canHold3by3(Rectangle rect)
{
    return rect.getHeight() >= 3 && rect.getWidth() >= 3;
}

or reversing the checks:

public boolean canHold3by3(Rectangle rect)
{
    return rect.getWidth() >= 3 && rect.getHeight() >= 3;
}

There is no way to test this using a mock for the input AND have it pass for both versions without mocking both methods, but in either version you will have methods not called in some of the test cases.

@Evanthx
Copy link

@Evanthx Evanthx commented Jan 12, 2019

If you're looking for comments - may I suggest at least adding something to the error about how to turn strict stubbing off in the default case? I'm using MockitoExtension and only found out by reading source code and then googling based on what I read - I didn't see anything in the documentation that actually worked. It was easy once I found the right call, but hard to find that call!

I am in the camp of people that for complex test cases will set the mocks up in the setUp method. I see the reasons you use strict stubbing - but for my experience it solves a theoretical problem that I've never had, while breaking my solution for the actual problems I've had. I keep getting the feeling that it's a vote for cutting and pasting code into multiple places rather than having it in one place although I'm sure that is NOT what you intend! But setting up my mocks in one place means this exception will fire ... so I either have to cut and paste the mocking setups into multiple places, or else find out how to turn this back to lenient.

On the other hand, I love seeing the passion for this subject in this thread. Agree or disagree, I LOVE talking to people that can also get heavily involved in a topic about whether mocks should default to strict or lenient!

@toyg
Copy link

@toyg toyg commented Jan 28, 2019

Another use-case for reusable stubbing: I must mock HTTP calls made by a 3rd-party library. I don't want to setup a custom webserver and then specify an address somewhere in tests and maintain the whole machinery, I just mock the http calls by the lib. That means dozens of LOCs for every call, which I abstract away in static methods for the most common scenarios (e.g. mockHttpCall(someUrl, responseCode, responseHeaders) ). However, some of the mocked interfaces will not be called all the time - client code will care only about response code in one place, headers in another, body in another, etc etc . Making tests explode in such situations is really uncout.

@cosenmarco
Copy link

@cosenmarco cosenmarco commented Mar 12, 2019

One case where this is clearly controversial is in the case where we extend some base class which is setting up some default basic stubbing for some (but not all) of the child classes. I know using inheritance in tests is an anti-pattern but the team has historically doing this and a refactoring is out of scope. Is there a way to address this without having to use org.mockito.runners.MockitoJUnitRunner.Silent? I'd like still to keep the benefits of strictness but just be lenient when some stubs are used by some of the child classes

@Druckles
Copy link

@Druckles Druckles commented Jul 25, 2019

While writing an integration test I came across a specific example where this Strictness may actually be useful. In this example, I want to explicitly mock a call to a service and ensure it is definitely called. I have, for example, the following method:

public void unzipRemotely(String location, String filename) {
    File original = service.download(location, filename);
    ZipFile zip = new ZipFile(original);
    File tempDirectory = createTempDirectory();
    zip.unzip(tempDirectory);
    service.uploadDirectory(location, removeExtension(filename), tempDirectory, true);
}

The two things I need to test here are:

  • Were the files unzipped?
  • Was uploadDirectory called?

I don't extrinsically care whether download was called; it's implied by the other two facts.

To test this, I need something like the following:

@Test
@DisplayName("")
void success() {
    when(service.download("somewhere", "file.zip")).thenReturn(zippedFile);
    when(service.uploadDirectory(eq("somewhere"), eq("file"), any(File.class), eq(true)))
        .thenAnswer(invocationOnMock -> {
            File sourceDirectory = invocationOnMock.getArgument(2);
            assertThatAllFilesAreThere(sourceDirectory);
        });

    unzipper.unzipRemotely("somewhere", "file.zip");
}

The problem here is that there is no guarantee that uploadDirectory was ever called. There are three existing solutions to this:

  • Assert that some flag is set
  • Rely on Strict mode
  • verify

Assert some flag is set

AtomicBoolean wasCalled = new AtomicBoolean(false);
when(service. ...)
    .thenAnswer(invocationOnMock -> {
        // ...
        wasCalled.set(true);
    });
assertThat(wasCalled, is(true));

Obvious problem with this: it requires more code. It's also not as succinct for describing what is happening.

Rely on Strict mode

This is the current solution. You called when. You should expect it to be called.

Possible issues:

  • Anyone can come along and add LENIENT over the tests to expand them. It's no longer verified.
  • It's not explicit from the name. "When" is pretty close to "if". In fact the Germans don't even distinguish between the two. In English, "when" means both, "when this thing definitely occurs in the future" and also more generally "whenever this happens". If you say, "when trees fall they make a noise", it doesn't mean a tree in that forest over there will fall.

Verify

The most natural of the options, I want to explicitly verify that something happened. But for that, it need the following line:

verify(service).uploadDirectory(eq("somewhere"), eq("file"), any(File.class), eq(true));

Problem: duplicated code. Pretty obvious that that's not ideal.

An alternative solution

What about instead of changing the API (which is what Strict-mode does; it means you have to change all your tests in some way), the API is expanded with an extra command:

expect(service.uploadDirectory(eq("somewhere"), eq("file"), any(File.class), eq(true)))
    .andAnswer(...);
  • Works just like when but must be called at least once
  • Like verify it accepts a VerificationMode like times
  • Explicitly describes what it does
  • when no longer verifies that the stubbed method was called

This would solve all the above issues. Non-mocked method calls could still throw an exception for missing stubbings (which does indicate a problem).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.