Prevent silent mis-stubbing

[wesalvaro]

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!

[TimvdLippe]

Good catch!

[snyman]

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)?

[wesalvaro]

I'm sorry, but this has only revealed an implementation error with your utility methods...

The call to stub is correctly illegal as it's being called on something that is not itself a mock. Your code adds another example of this mis-stubbing issue. It's fine to do what you're doing, but the code you want is actually simpler (which it seems you have already discovered):

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

I guess ref is just a way of creating a lambda to fit the signature of stubReturn?
It looks like a complicated alias of whenever.
Perhaps the example you provided doesn't tell the full story,
but if I wanted to implement something like that, I would write:

fun <T> on(block: () -> T) = whenever(block())

inline fun <reified T> OngoingStubbing<T>.returnAll(vararg items: T) =
    doReturnConsecutively(items.toList())

interface MyInterface {
  fun foo(): Int
  fun bar(i: Int): Int
}
val m = mock<MyInterface>()

on(m::foo).returnAll(1,2,3)
on { m.bar(any()) }.returnAll(1, 2, 3)

But then, why even have on accept a lambda argument?
The above is equivalent to just using whenever normally:

fun <T> on(t: T) = whenever(t)

inline fun <reified T> OngoingStubbing<T>.returnAll(vararg items: T) =
    doReturnConsecutively(items.toList())

interface MyInterface {
  fun foo(): Int
  fun bar(i: Int): Int
}
val m = mock<MyInterface>()

on(m.foo()).returnAll(1,2,3)
on(m.bar(any())).returnAll(1, 2, 3)

At which point, on would be an alias of whenever. And arguably, while longer, using the methods provided by Mockito is better for consistency than rolling custom utility methods. Either way, calls to stub are only valid on mocks and this PR fixed that.

If this were my code, I would prefer the following for consistency, clarity, and safety:

m.stub {
  on { foo() } doReturnConsecutively listOf(1, 2, 3)
  on { bar(any()) } doReturnConsecutively listOf(1, 2, 3)
}