mockito / mockito-kotlin

Using Mockito with Kotlin
MIT License
3.09k stars 198 forks source link

Prevent silent mis-stubbing #479

Closed wesalvaro closed 1 year ago

wesalvaro commented 1 year ago

Consider the following test:

@Test
fun work_isTheAnswer() {
  whenever(foo.work()) doReturn 42

  assertThat(foo.work()).isEqualTo(42)
}

It fails with:

1) work_isTheAnswer
value of: work()
expected: 42
but was : 43
    at com.testing.MockTest.work_isTheAnswer

Why?

Because the foo object is not a mock but it does call a mock in its implementation. That underlying mock call also returns an Int, so Mockito happily stubs that method, instead. Yikes.

We can't solve this in when/whenever because it is not provided the Mock object, just the "result" of the (hopefully) method call to stub. In Kotlin, we can solve this in KStubbing as a requirement on its argument. This highlights a pitfall when using when/whenever that the developer must be certain they are stubbing a Mock and not a real implementation. To avoid this pitfall, we should use the doReturn|Throw family (see below) or immediately stubbing our mocks:

val foo = mock<Foo> {
  on { work() } doReturn 42
}

How have I not seen this issue before?

Probably because you never put a real implementation into when/whenever or your method was final, or the underlying method being picked up by Mockito was not returning the same value type. In these cases, Mockito will throw an error. Only in the last case, the error might be confusing:

org.mockito.exceptions.misusing.WrongTypeOfReturnValue: 
Integer cannot be returned by work()
work() should return boolean

Mockito acknowledges this pitfall in the output of the error as tip 2:

2. A spy is stubbed using when(spy.foo()).then() syntax. It is safer to stub spies - 
   - with doReturn|Throw() family of methods. More in javadocs for Mockito.spy() method.

Avoiding this pitfall

Mockito recommends against using when/whenever and instead to use the doReturn|Throw family. This changes the stubbing flow to have the when/whenever take in the mock so that it can be verified:

doReturn(42).whenever(foo).work()

Using our real implementation with this setup yields:

org.mockito.exceptions.misusing.NotAMockException: 
Argument passed to when() is not a mock!
Example of correct stubbing:
    doThrow(new RuntimeException()).when(mock).someMethod();

We can copy this check in KStubbing easily which gives developers the option to consistently construct their mocks:

foo.stub {
  on { work() } doReturn 42
}

After this PR is merged, the above will throw a similar exception:

org.mockito.exceptions.misusing.NotAMockException: Stubbing target is not a mock!
snyman commented 11 months ago

Hello! Unfortunately, this change breaks my tests. You see, we have a series of utility methods such as

inline fun <reified T> (() -> T).stubReturn(vararg returnItem: T) {
    stub { on { this@stubReturn() }.doReturnConsecutively(returnItem.toList()) }
}

fun <T> ref(ref: () -> T) = ref

Which allows us to stub method calls a little bit more concisely:

mock::someMethod.stubReturn(1, 2, 3)

ref { mock.methodWithParam(any()) }.stubReturn("one", "two", "three")

But of course that leads to KStubbing.mock being a function reference, and not the underlying mock, which triggers this check.

So we have some ways to work around this (i.e. avoid using stub in the utility methods and call Mockito.when directly), but thought we should ask: is there any chance of a change allowing for this kind of use case with KStubbing? For example maybe a "I know what I'm doing" flag that bypasses this check (with a default value that doesn't bypass)?