How should I handle a UnnecessaryStubbingException that is sensitive to ordering in underlying data structures?

Question

I have a test that expects an exception to be thrown when a user is found to be suspended.

  @Test(expected = SuspendedException.class)
  public void testGetUserKeychain_WhenOneUserSuspended_ShouldThrowSuspended() throws Throwable {
    when(userKeychain.getUserStatus()).thenReturn(UserState.OK);
    when(otherUserKeychain.getUserStatus()).thenReturn(UserState.SUSPENDED);
    when(keyLookup.lookupKeychainsByUserId(any()))
        .thenReturn(CompletableFuture.completedFuture(ImmutableMap.copyOf(multiUserKeychains)));
    try {
      padlockUtil.getKeychains(
          Sets.newSet("userid", "otheruserid")).toCompletableFuture().get();
    } catch (ExecutionException e) {
      throw e.getCause();
    }
  }

But the exception I get instead is:

org.mockito.exceptions.misusing.UnnecessaryStubbingException: 
Unnecessary stubbings detected in test class: PadlockUtilTest
Clean & maintainable test code requires zero unnecessary code.
Following stubbings are unnecessary (click to navigate to relevant line of code):
  1. -> at com.xyz.server.padlock.PadlockUtilTest.testGetUserKeychain_WhenOneUserSuspended_ShouldThrowSuspended(PadlockUtilTest.java:119)
Please remove unnecessary stubbings or use 'lenient' strictness. More info: javadoc for UnnecessaryStubbingException class.

I believe this is because in PadlockUtil::getKeychains, the suspended user is encountered before the OK user, so Mockito is complaining that the OK user didn't need to be stubbed.

Because if I swap who's suspended and not...

when(userKeychain.getUserStatus()).thenReturn(UserState.SUSPENDED);
when(otherUserKeychain.getUserStatus()).thenReturn(UserState.OK);

...then Mockito is happy. As opposed to just switching "userid" and "otheruserid"; there Mockito is still unhappy, presumably because that's not where the order is determined later.

It may be true that in this specific example I've set up, it's not necessary to stub the first user. But that could be misleading in the future; I want the stub to exist, and it's not because of "lenience," IMO. I could also suspend the first user instead of the second, but it doesn't explicitly address this subtlety, and it could come up again later, confusing developers.

What is the proper way to do this so the underlying order of operations (we're dealing with Sets and Maps here, nothing else) is not a factor in the test?

Answer 1

Lenient mocks are what you want, if you can't just use a real UserKeychain.

Mockito.lenient().when(userKeychain.getUserStatus()).thenReturn(UserState.OK);
Mockito.lenient().when(otherUserKeychain.getUserStatus()).thenReturn(UserState.SUSPENDED);

Mockito is designed to replace systems where you can't use the real system in your test, particularly in systems that predictably invoke services instead of getting properties from data objects (or other idempotent actions). Because your system doesn't call those methods in a deterministic order, and because the calls aren't expensive and don't have side effects, I would recommend just going with the "lenient" option.

---

Imagine this case instead, where you are testing deleting user 1001:

when(userRpc.deleteUser(1001)).thenReturn(RPC_SUCCESS);
when(userRpc.deleteUser(1002)).thenReturn(RPC_SUCCESS);  // unnecessary

The test might pass if you ever delete the wrong user: Over-stubbing has masked a problem. Compare with this:

when(userRpc.fetchExpensiveUserDetails(1001)).thenReturn(user1001);
when(userRpc.fetchExpensiveUserDetails(1002)).thenReturn(user1002);  // unnecessary

Depending on what you're testing, this might be dangerous, but might not be so bad. Simulating a slow mobile network or with expensive data, maybe it is entirely against spec for you to fetch too much. However, in other cases, it may be acceptable. Finally, compare this case:

when(calculationResult.getRealComponent()).thenReturn(-1d);
when(calculationResult.getComplexComponent()).thenReturn(5);
when(calculationResult.getShortString()).thenReturn("-1 + 5i");

calculationResult looks an awful lot like a data object, and it is probably not a critical part of your test which of your methods to call or whether you're calling all of them. This is a case where Mockito's strict stubbing hinders you rather than helping you, and might be a case where you want to make some of those stubbings lenient. You might also choose to make the entire mock lenient, which particularly makes sense if you were to create a test helper method like stubCalculationResult(-1, 5) that prepared an entire object for you like that.

The only option better than that is to use a real object. In my example, if CalculationResult is an existing well-defined object, it may be lower risk overall to use a real one than to mock the behavior you believe at test-writing time to be correct. Similarly for your case, if you have access to a UserKeychain constructor that populates UserStatus etc, then it may be safer to use that in a test.

Though this might appear at first glance to be a slippery slope into turning a unit test into an integration test, I'd like to clarify that I'm recommending this only for data objects, which have no dependencies, and which ideally are immutable objects that have no methods with side-effects. If you use dependency injection, these are the type of single-implementation data holders that you would call new on rather than getting from your graph. This is also a good reason to separate your data objects so they are immutable and easy to construct, and to shift your services to consume those objects rather than giving methods to the data objects (favoring loginService.login(user) rather than user.login(loginService)).

Answer 2

I have used @MockitoSettings(strictness = Strictness.LENIENT) annotation in class that has thrown Unnecessary Stubbing Exception. It has resolved the "Please remove unnecessary stubbing or use 'lenient' strictness" error.