86

I have a private method in my test class that constructs a commonly used Bar object. The Bar constructor calls someMethod() method in my mocked object:

private @Mock Foo mockedObject; // My mocked object
...

private Bar getBar() {
  Bar result = new Bar(mockedObject); // this calls mockedObject.someMethod()
}

In some of my test methods I want to check someMethod was also invoked by that particular test. Something like the following:

@Test
public void someTest() {
  Bar bar = getBar();

  // do some things

  verify(mockedObject).someMethod(); // <--- will fail
}

This fails, because the mocked object had someMethod invoked twice. I don't want my test methods to care about the side effects of my getBar() method, so would it be reasonable to reset my mock object at the end of getBar()?

private Bar getBar() {
  Bar result = new Bar(mockedObject); // this calls mockedObject.someMethod()
  reset(mockedObject); // <-- is this OK?
}

I ask, because the documentation suggests resetting mock objects is generally indicative of bad tests. However, this feels OK to me.

Alternative

The alternative choice seems to be calling:

verify(mockedObject, times(2)).someMethod();

which in my opinion forces each test to know about the expectations of getBar(), for no gain.

Duncan Jones
  • 1,402

3 Answers3

76

I believe this is one of the cases where using reset() is ok. The test you are writing is testing that "some things" triggers a single call to someMethod(). Writing the verify() statement with any different number of invocations can lead to confusion.

  • atLeastOnce() allows for false positives, which is a bad thing as you want your tests to always be correct.
  • times(2) prevents the false positive, but makes it seem like you are expecting two invocations rather than saying "i know the constructor adds one". Further more, if something changes in the constructor to add an extra call, the test now has a chance for a false positive. And removing the call would cause the test to fail because the test is now wrong instead of what is being tested is wrong.

By using reset() in the helper method, you avoid both of these issues. However, you need to be careful that it will also reset any stubbing you have done, so be warned. The major reason reset() is discouraged is to prevent

bar = mock(Bar.class);
//do stuff
verify(bar).someMethod();
reset(bar);
//do other stuff
verify(bar).someMethod2();

This is not what the OP is trying to do. The OP, I'm assuming, has a test that verifies the invocation in the constructor. For this test, the reset allows for isolating this single action and it's effect. This one of the few cases with reset() can be helpful as. The other options that don't use it all have cons. The fact that the OP made this post shows that he is thinking about the situation and not just blindly utilizing the reset method.

unholysampler
  • 7,853
7

Smart Mockito users hardly use reset feature because they know it could be a sign of poor tests. Normally, you don't need to reset your mocks, just create new mocks for each test method.

Instead of reset() please consider writing simple, small and focused test methods over lengthy, over-specified tests. First potential code smell is reset() in the middle of the test method.

Extracted from the mockito docs.

My advise is that you try to avoid using reset(). In my opinion, if you call twice to someMethod, that should be tested (maybe it is a database access, or other long process that you want to take care about).

If you really don't care about that, you could use:

verify(mockedObject, atLeastOnce()).someMethod();

Note that this last could cause a false result, if you call someMethod from getBar, and not after (this is a wrong behaviour, but the test will not fail).

Martin Schröder
  • 354
greuze
  • 365
5

Absolutely not. As is so often the case, the difficulty you are having in writing a clean test is a major red flag about the design of your production code. In this case, the best solution is to refactor your code so that the constructor of Bar does not call any methods.

Constructors should construct, not execute logic. Take the return value of the method and pass it in as a constructor parameter. 

new Bar(mockedObject);

becomes:

new Bar(mockedObject.someMethod());

If this would result in duplicating this logic in many places, consider creating a factory method which can be tested independently of your Bar object:

public Bar createBar(MockedObject mockedObject) {
    Object dependency = mockedObject.someMethod();
    // ...more logic that used to be in Bar constructor
    return new Bar(dependency);
}

If this refactoring is too difficult then using reset() is a good work around. But let's be clear - it is indicating that your code is poorly designed.

tonicsoft
  • 151