Add suspend answer; Fix #346 #357

neworld · 2019-08-31T19:39:27Z

This code was inspired by:

I used Java code because kotlin does not allow call suspend functions or lambdas by giving own Continuation. But Kotlin does it under the hood, so Java could do it as well. For example:

fun foo(continuation: Continuation) {
  bar(continuation) //compilation error
}

suspend fun bar() {

}

nhaarman · 2019-09-08T08:20:53Z

Thanks! Looks like a useful feature 🙌

I'm not that familiar with coroutines yet, does this solution rely on any undocumented Kotlin internals? I.e. is there any chance this may break in upcoming Kotlin versions, and if so why?

neworld · 2019-09-08T13:49:58Z

Yes, it is. I am using it for some tests; otherwise, I was not able to write at all.

does this solution rely on any undocumented Kotlin internals?

I would say yes. Implementation details are not documented well.

is there any chance this may break in upcoming Kotlin versions

I believe this feature is quite safe:

The way suspend functions are working is compiler feature to add Continuation argument to all suspend functions. Kotlin devs should ensure backward compatibility. Otherwise, at some point, you could not upgrade kotlinc until all dependencies are rebuilt with a new compiler.
Retrofit is using the same approach. They don't need weird java wrapper only because their code is in java already.

I suppose there is a little chance the riskiest line could not compile with future version.

neworld · 2019-09-08T14:15:03Z

Created an issue: https://youtrack.jetbrains.com/issue/KT-33766

neworld · 2019-09-16T12:13:25Z

@nhaarman, friendly ping. Any news for this solution? At Vinted we are using this solution for multiple cases and they are working well.

neworld · 2019-09-23T15:28:30Z

Thanks to Roman Elizarov. He provided me with missing details and I was able to find a solution without Java.

Also added some tests to make sure the cases I am facing in my personal code will be covered as well.

bohsen · 2019-11-06T09:33:59Z

mockito-kotlin/src/test/kotlin/test/CoroutinesTest.kt

+        //TODO: on coroutines >1.3 it should be job.complete()
+        job.cancel()


Maybe bump version of coroutines to latest version while at it and make this change. I'm confident the tests will pass as I've done the update on another branch earlier.

@nhaarman What do you think?

Otherwise really nice job @neworld. Super simple implementation and the use case for this has been increasingly in demand since Android started supporting coroutines and since coroutines-test appeared.

Yeah I think this would be a good choice. Let's update to the latest supported syntax, which in this case is job.complete() as I understand it? (Not an expert here, so let me know if it has changed in the mean time)

mhernand40 · 2020-01-04T00:33:17Z

mockito-kotlin/src/main/kotlin/com/nhaarman/mockitokotlin2/OngoingStubbing.kt

    return thenAnswer(answer)
 }
+
+infix fun <T> OngoingStubbing<T>.willAnswer(answer: suspend (InvocationOnMock) -> T?): OngoingStubbing<T> {


Thoughts on naming this doAnswer? The compiler will consider it as a valid overload I believe.

I believe the willAnswer variants are extensions for BDDMyOngoingStubbing. May I request that we also add suspend support for BDDMyOngoingStubbing? 🙂

The tests would look something like:

... given(fixture.suspendingWithArg(any())).willAnswer { withContext(Dispatchers.Default) { it.getArgument<Int>(0) } } ...

Thank you!

@TimvdLippe what do you think about this request? I could create willSuspendableAnswer for BDD as well.

We currently do not have any extensions for BDDMyOngoingStubbing in the existing OngoingStubbing.kt. I think we should treat that as a separate improvement, but I would happy to approve a PR on that. For now, let's keep that separate and merge this PR as-is.

mhernand40 · 2020-01-04T00:42:58Z

mockito-kotlin/src/main/kotlin/com/nhaarman/mockitokotlin2/OngoingStubbing.kt

 import org.mockito.stubbing.OngoingStubbing
-import kotlin.DeprecationLevel.ERROR
+import kotlin.coroutines.Continuation
+import kotlin.coroutines.intrinsics.startCoroutineUninterceptedOrReturn


I believe this has now been moved to an *.experimental.* package in the latest version of Kotlin.

https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.coroutines.experimental.intrinsics/start-coroutine-unintercepted-or-return.html

It looks like it was moved back again: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.coroutines.intrinsics/start-coroutine-unintercepted-or-return.html

I've tested this code on Kotlin 1.4.0 and it seems to still work.

Yes. The same code was working for all coroutine and kotlin versions. Now I am using 1.4.0 with 1.3.9 of coroutines.

You might consider replacing startCoroutineUninterceptedOrReturn with it's implementation if the definition location isn't very stable. All it does is cast to expose the continuation parameter and invoke.

(answer as ((InvocationOnMock, Continuation<T?>) -> Any)).invoke(it, continuation)

JustinBis · 2020-08-17T17:11:46Z

Ping @nhaarman, I'd love to use this in my projects as well as an official feature. Thanks @neworld for the awesome PR.

connected-rmcleod · 2020-08-26T16:22:47Z

@nhaarman Adding to the friendly pinging. Code like this is fundamentally difficult to test without a suspending answer:

class ViewModel(private val api: API) {
  var isLoading: Boolean = false

  suspend loadData(): Data {
    isLoading = true
    val data = api.fetchData() // suspending call
    isLoading = false
    return data
  }
}

If I can't stub fetchData in such a way that it doesn't immediately return, it's pretty much impossible for me to write a passing test asserting that isLoading is ever true. In fact, the only way I could figure out how to do it was by copying and pasting the code from this PR into my code (or to use something other than Mockito.)

I will add that while this PR uses low-level code, it is not marked experimental (it was briefly in an experimental package at one point but quickly moved back) and it is in the documentation: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.coroutines.intrinsics/start-coroutine-unintercepted-or-return.html

It was also the approach suggested by the team lead for Kotlin libraries in the YT issue: https://youtrack.jetbrains.com/issue/KT-33766

Would be happy to help if there's any way I can assist in getting @neworld's excellent work into a mergeable state.

mhernand40 · 2020-08-26T18:50:28Z

FWIW, in case anyone feels blocked because of this limitation, I have actually begun testing without Mockito whenever possible by leveraging interfaces and this "fake" constructor pattern I have seen being used in many well known Kotlin libraries including the Kotlinx Coroutines library. Using the fetchData() example mentioned above by @connected-rmcleod:

interface Api {
    suspend fetchData(): Data

    companion object {
        operator fun invoke(... /* dependencies */): Api {
            return ApiImpl(...)
        }
    }
}

private class ApiImpl(... /* dependencies */) {
    override suspend fetchData(): Data {
        // Implementation details here
    }
}

Or if you don't like the companion object allocation:

@Suppress("FunctionName")
fun Api(... /* dependencies */): Api {
    return ApiImpl(...)
}

interface Api {
    suspend fetchData(): Data
}

private class ApiImpl(... /* dependencies */) {
    override suspend fetchData(): Data {
        // Implementation details here
    }
}

In code, I can still "instantiate" Api using val api = Api(...) and in tests I can create an inner TestApi and back it with a CompletableDeferred like so:

    private val deferredData = CompletableDeferred<Data>()
    private val api = TestApi()

    ...

    private inner class TestApi() : Api {
        override suspend fetchData(): Data {
            return deferredData.await()
        }
    }

If I want to test the scenario where the fetch suspends indefinitely, I simply avoid calling deferredData.complete(...). If I want the fetch to return a result immediately, I simply call deferredData.complete(expectedData).

I get that this doesn't actually address the need for supporting suspend Answers in Mockito Kotlin but I figured I'd share how I have adapted my testing practices to some of the limitations. 🙂

Examples of this "fake" constructor pattern:
https://kotlin.github.io/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/-coroutine-scope.html
https://kotlin.github.io/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/-job.html
https://kotlin.github.io/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/-supervisor-job.html
https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/-list.html

Notice how these are actually funs.

neworld · 2020-08-26T19:14:40Z

Actually you do not need to wait for the release. Thanks to kotlin extensions you could just copy-paste into your project:

/**
 * This should be replaced after the release of https://github.com/nhaarman/mockito-kotlin/pull/357
 */
@Suppress("UNCHECKED_CAST")
infix fun <T> OngoingStubbing<T>.willAnswer(answer: suspend (InvocationOnMock) -> T?): OngoingStubbing<T> {
    return thenAnswer {
        //all suspend functions/lambdas has Continuation as the last argument.
        //InvocationOnMock does not see last argument
        val rawInvocation = it as InterceptedInvocation
        val continuation = rawInvocation.rawArguments.last() as Continuation<T?>

        answer.startCoroutineUninterceptedOrReturn(it, continuation)
    }
}

I am using the same snippet for a year and works perfectly.

connected-rmcleod · 2020-08-27T13:23:38Z

@mhernand40 @neworld All good advice! I'm aware that I can just roll my own test doubles or copy in the extension function from this PR. (In fact, in a lot of ways I prefer not using a mocking framework at all, but I've never found a team that agrees with me. xD) I'm just trying to emphasize that if someone is trying to write tests using mockito-kotlin and runs into this issue that it's a substantial stumbling block for them without having to resort to a different approach entirely.

Also, as an aside I had never seen that pattern of essentially adding a default constructor to an interface before. That's pretty neat.

I came across this mostly because I was going to give a small presentation to the Android devs at my company about the differences between using Mockito + mockito-kotlin vs MockK and this stuck out as a substantial thing that MockK can do much more easily.

neworld · 2020-08-27T14:11:17Z

To be honest, recently I am trying to avoid mocking my own code at all. The topic was discussed many times and there is plenty of good arguments. The exception is for 3rd party libraries I have no control.

connected-rmcleod · 2020-08-27T14:36:39Z

@neworld Without getting too much into the weeds, I think the correct position is between those two extremes, but I agree the far more common mistake is over-mocking. Over-mocking is a cancer.

connected-rmcleod · 2021-02-03T13:52:54Z

This PR has been open for a long time. The approach also seems quite stable, and would be a pretty useful addition. Is there a substantial reason not to bring it in?

neworld · 2021-03-31T14:17:33Z

@nhaarman, friendly ping. More than a year has passed. Is any blocker for this PR? It seems quite useful for the community and the same code is working like a charm in our production environment.

TimvdLippe · 2021-03-31T14:22:54Z

The mockito-kotlin artifact recently moved to the GitHub Mockito organization and we have been busy at work with publishing to Maven Central. Now that the dust mostly has settled, we can take a look at community PRs 😄 I will put this on my review list for this week, but I will have to get accustomed to the code a bit to understand what is going on here. From a first glance this looks fine, but I just want to make sure I understand what the overall logic is.

TimvdLippe · 2021-03-31T14:24:39Z

@neworld Do you mind rebasing this PR so that I can review a clean integration? I think the only blockers are import updates.

neworld · 2021-03-31T15:06:22Z

Finished rebasing.

I just want to make sure I understand what the overall logic is

Suspend function under the hood returns Any object which could be COROUTINE_SUSPENDED in case of suspension. AFAIK all coroutines are started via startCoroutineUninterceptedOrReturn or similar primitives, which properly handles the return of the suspendable body. This block will be executed by another coroutine so it is important to return proper result as coroutines are expected:

        //all suspend functions/lambdas has Continuation as the last argument.
        //InvocationOnMock does not see last argument
        val rawInvocation = it as InterceptedInvocation
        val continuation = rawInvocation.rawArguments.last() as Continuation<T?>

        answer.startCoroutineUninterceptedOrReturn(it, continuation)

More professional explanation is here: https://youtrack.jetbrains.com/issue/KT-33766#focus=Comments-27-3707299.0-0

TimvdLippe

Sorry for the long wait. I took a look at the PR and additional context and this solution makes sense to me. There were only 2 nits that would be great to be addressed and after that we can merge this.

Thanks @neworld for driving this forward and all others who have chimed in. Once this PR lands and I got through the rest of the PRs, I will publish a new version to Maven Central.

TimvdLippe · 2021-03-31T15:14:17Z

mockito-kotlin/src/main/kotlin/org/mockito/kotlin/OngoingStubbing.kt

    return thenAnswer(answer)
 }
+
+infix fun <T> OngoingStubbing<T>.willAnswer(answer: suspend (InvocationOnMock) -> T?): OngoingStubbing<T> {


The willX notation is already used in BDD-style mocking. To keep the naming consistent with the other methods defined, WDYT about the following: doAnswerForCoroutine?

doAnswerForCoroutine sounds weird to me, but consistency is a first-class citizen always. Just maybe we should call it doSuspendableAnswer? It will sound like on loadData do suspendable answer

doSuspendableAnswer sounds good to me.

Final touched done. I leave doSuspendableAnswer name

TimvdLippe · 2021-03-31T15:15:45Z

mockito-kotlin/src/test/kotlin/test/CoroutinesTest.kt

+        //TODO: on coroutines >1.3 it should be job.complete()
+        job.cancel()


Yeah I think this would be a good choice. Let's update to the latest supported syntax, which in this case is job.complete() as I understand it? (Not an expert here, so let me know if it has changed in the mean time)

This code was inspired by: - Java wrapper part: https://github.com/square/retrofit/blob/8c93b59dbc57841959f5237cb141ce0b3c18b778/retrofit/src/main/java/retrofit2/HttpServiceMethod.java#L168 - Kotlin wrapper part: https://github.com/square/retrofit/blob/8c93b59dbc57841959f5237cb141ce0b3c18b778/retrofit/src/main/java/retrofit2/KotlinExtensions.kt#L83-L98

TimvdLippe

Thank you so much for your continued effort and apologies that it took so long! I will make sure this change will be published to Maven Central after a successful build.

TimvdLippe · 2021-04-01T14:07:04Z

This feature is available per version 3.1.0: https://repo1.maven.org/maven2/org/mockito/kotlin/mockito-kotlin/3.1.0/

neworld mentioned this pull request Aug 31, 2019

Support suspend blocks in Answers #346

Closed

neworld force-pushed the feature/suspend_answer branch from ddfbbf6 to ecce30d Compare September 1, 2019 06:43

bohsen reviewed Nov 6, 2019

View reviewed changes

mhernand40 reviewed Jan 4, 2020

View reviewed changes

Base automatically changed from 2.x to main January 18, 2021 03:46

bohsen mentioned this pull request Mar 25, 2021

How to mock a suspend function on an existing mock object? #311

Closed

neworld force-pushed the feature/suspend_answer branch 2 times, most recently from ce055f4 to 6172d09 Compare March 31, 2021 14:51

neworld force-pushed the feature/suspend_answer branch from 6172d09 to 1d04c45 Compare March 31, 2021 15:09

TimvdLippe approved these changes Mar 31, 2021

View reviewed changes

neworld force-pushed the feature/suspend_answer branch 2 times, most recently from 3119f99 to 4a04176 Compare March 31, 2021 22:50

neworld force-pushed the feature/suspend_answer branch from 4a04176 to cd635be Compare March 31, 2021 22:52

TimvdLippe approved these changes Apr 1, 2021

View reviewed changes

TimvdLippe merged commit 3173393 into mockito:main Apr 1, 2021

neworld deleted the feature/suspend_answer branch April 1, 2021 14:52

neworld mentioned this pull request Apr 1, 2021

Add support of suspendable answers to BDD #429

Merged

		//TODO: on coroutines >1.3 it should be job.complete()
		job.cancel()

Add suspend answer; Fix #346 #357

Add suspend answer; Fix #346 #357

Uh oh!

Conversation

neworld commented Aug 31, 2019

Uh oh!

nhaarman commented Sep 8, 2019

Uh oh!

neworld commented Sep 8, 2019

Uh oh!

neworld commented Sep 8, 2019

Uh oh!

neworld commented Sep 16, 2019

Uh oh!

neworld commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bohsen Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connected-rmcleod Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JustinBis commented Aug 17, 2020

Uh oh!

connected-rmcleod commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhernand40 commented Aug 26, 2020

Uh oh!

neworld commented Aug 26, 2020

Uh oh!

connected-rmcleod commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neworld commented Aug 27, 2020

Uh oh!

connected-rmcleod commented Aug 27, 2020

Uh oh!

connected-rmcleod commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neworld commented Mar 31, 2021

Uh oh!

TimvdLippe commented Mar 31, 2021

Uh oh!

TimvdLippe commented Mar 31, 2021

Uh oh!

neworld commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimvdLippe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neworld commented Sep 23, 2019 •

edited

Loading

bohsen Nov 6, 2019 •

edited

Loading

connected-rmcleod Aug 26, 2020 •

edited

Loading

connected-rmcleod commented Aug 26, 2020 •

edited

Loading

connected-rmcleod commented Aug 27, 2020 •

edited

Loading

connected-rmcleod commented Feb 3, 2021 •

edited

Loading

neworld commented Mar 31, 2021 •

edited

Loading