Solve 6.2.0 regression due to runblocking being unavailable #575
Conversation
|
The overall approach LGTM, thanks for taking extra care of this! @jselbo can you do an in-depth review once again? Thanks! |
…ine infrastructure (e.g. runBlocking) on the classpath
…ines-core library (with the runBlocking function in it) is not on the classpath.
| import org.mockito.Mockito.`when` | ||
| import org.mockito.exceptions.misusing.NotAMockException | ||
| import org.mockito.kotlin.internal.createInstance | ||
| import org.mockito.kotlin.internal.safeRunBlocking |
There was a problem hiding this comment.
I'm surprised ktfmt didn't remove the kotlinx.coroutines.runBlocking import. Let's remove it here and in OngoingStubbing.kt
There was a problem hiding this comment.
runBlocking is still used directly in 2 expicit 'blocking' stub methods (e.g. givenBlocking andwheneverBlocking) that already exist for longer time. These methods have a parameter of type suspend CoroutineScope.() -> T. The CoroutineScope receiver type is preventing the use of safeRunBlocking.
I have no clear picture whether it would be feasable to remove the receiver and simplify the parameters to suspend () -> T. I suppose that could break existing usages of these stub methods. And it should not be a problem to use runBlocking directly, because these expicit 'blocking' stub methods are typically applied in the context of projects with coroutines, so the kotlinx-coroutines-core library should be available.
There was a problem hiding this comment.
I guess we could extend the test suite you added with calls to these methods? Then we know for sure. For the one where it already took a coroutine as argument prior to your changes, it would be safe to use runBlocking. That said, for consistency and potential incorrect copy-pasting in the future, let's use safeRunBlocking in as many cases as we can.
There was a problem hiding this comment.
That's already the current state since my last commit this morning.
Only 2 spots use runBlocking directly as described above.
Talking about the new test suite, I had another thought popping up to my mind: To split the 'tests' suite in two: 1 suite for all synchronous tests in a suite without dependency to kotlinx-coroutines-core library, and 1 suite for all coroutine related tests. That would safeguard the project best for regressions I guess.
I would suggest to do that split/move in a new PR of course.
There was a problem hiding this comment.
Yes that sounds like a good idea! Ack about runBlocking. Will publish this as 6.2.1.
|
Seems reasonable, just left a few comments 👍 |
| @Deprecated("Use whenever { mock.methodCall() } instead") | ||
| fun <T> wheneverBlocking(methodCall: suspend CoroutineScope.() -> T): OngoingStubbing<T> { | ||
| return whenever(methodCall) | ||
| return runBlocking { `when`(methodCall()) } |
There was a problem hiding this comment.
Why doesn't this use safeRunBlocking?
There was a problem hiding this comment.
see my other response.
Co-authored-by: Tim van der Lippe <TimvdLippe@users.noreply.github.com>
Co-authored-by: Tim van der Lippe <TimvdLippe@users.noreply.github.com>
3 participants
Fixes issue #570 by introducing a fallback to the runBlocking function.
This PR introduces a new test module
no-coroutine-teststhat has no dependency to thekotlinx-coroutines-corelibrary.The new test class RegressionTest introduced with the new module first proofs that the regression exists.
In the last commit, with the fallback applied, the test class proofs that a (synchronous) mock can be stubbed without the need to have
kotlinx-coroutines-corelibrary on the classpath.This PR supersedes PR #571