New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support completing the test coroutine from outside the test thread. #1206
Changes from 2 commits
baade87
d3f40a9
6d1639e
403f502
1d2a05e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ package kotlinx.coroutines.test | |
|
||
import kotlinx.coroutines.CoroutineDispatcher | ||
import kotlinx.coroutines.ExperimentalCoroutinesApi | ||
import kotlinx.coroutines.channels.ConflatedBroadcastChannel | ||
|
||
/** | ||
* Control the virtual clock time of a [CoroutineDispatcher]. | ||
|
@@ -112,9 +113,75 @@ public interface DelayController { | |
* Resumed dispatchers will automatically progress through all coroutines scheduled at the current time. To advance | ||
* time and execute coroutines scheduled in the future use, one of [advanceTimeBy], | ||
* or [advanceUntilIdle]. | ||
* | ||
* When the dispatcher is resumed, all execution be immediate in the thread that triggered it. This means | ||
* that the following code will not switch back from Dispatchers.IO after `withContext` | ||
* | ||
* ``` | ||
* runBlockingTest { | ||
* withContext(Dispatchers.IO) { doIo() } | ||
* // runBlockingTest is still on Dispatchers.IO here | ||
* } | ||
* ``` | ||
* | ||
* For tests that need accurate threading behavior, [pauseDispatcher] will ensure that the following test dispatches | ||
* on the correct thread. | ||
* | ||
* ``` | ||
* runBlockingTest { | ||
* pauseDispatcher() | ||
* withContext(Dispatchers.IO) { doIo() } | ||
* // runBlockingTest has returned to it's starting thread here | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit counter-intuitive as well, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea - this one I'm stuck on. It seems required to support the pauseDispatcher { } block in runBlockingTest but it has this side effect in the presence of multi-threading. |
||
* } | ||
* ``` | ||
*/ | ||
@ExperimentalCoroutinesApi // Since 1.2.1, tentatively till 1.3.0 | ||
public fun resumeDispatcher() | ||
|
||
/** | ||
* Represents the queue state of a DelayController. | ||
* | ||
* Tests do not normally need to use this API. It is exposed for advanced situations like integrating multiple | ||
* [TestCoroutineDispatcher] instances or creating alternatives to [runBlockingtest]. | ||
objcode marked this conversation as resolved.
Show resolved
Hide resolved
|
||
*/ | ||
public sealed class QueueState { | ||
qwwdfsad marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* A [DelayController] that is idle does not currently have any tasks to perform. | ||
* | ||
* This may happen if all coroutines in this [DelayController] have completed, or if they are all suspended | ||
* waiting on other dispatchers. | ||
*/ | ||
public object Idle: QueueState() { | ||
override fun toString() = "Idle" | ||
} | ||
|
||
/** | ||
* A [DelayController] that has a task that will execute in response to a call to [runCurrent]. | ||
* | ||
* There may also be delayed tasks scheduled, in which case [HasCurrentTask] takes priority since current tasks | ||
* will execute at an earlier virtual time. | ||
*/ | ||
public object HasCurrentTask: QueueState() { | ||
override fun toString() = "HasCurrentTask" | ||
} | ||
|
||
/** | ||
* A [DelayController] that has delayed tasks has a task scheduled for sometime in the future. | ||
* | ||
* If there are also tasks at the current time, [HasCurrentTask] will take priority. | ||
*/ | ||
public object HasDelayedTask: QueueState(){ | ||
override fun toString() = "HasDelayedTask" | ||
} | ||
} | ||
|
||
/** | ||
* A ConflatedBroadcastChannel that is up to date with the current [QueueState]. | ||
* | ||
* Tests do not normally need to use this API. It is exposed for advanced situations like integrating multiple | ||
* [TestCoroutineDispatcher] instances or creating alternatives to [runBlockingtest]. | ||
*/ | ||
public val queueState: ConflatedBroadcastChannel<QueueState> | ||
objcode marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we are here, could you please elaborate on why this behaviour is preferred over "classic" dispatching?
I am a bit concerned about how this behaviour is different from "release" builds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the main goal of this eager behavior was to make this test function testable without having to call runCurrent():
In a test that doesn't use multiple threads (TestCoroutineDispatcher is used for all coroutines) it provides eager entry into the launch body (during dispatch the launch body is immediately executed).
As this is both the normal structure for this code (at least on Android) it is nice to avoid extra calls to
runCurrent()
in this testThis thread switching behavior is an undesired side effect of that API choice.
So basically, I think there are two options here:
dispatch
to require a call torunCurrent()
in this test. This does make common test cases require an extra call, but leads to correct threading behavior if the developer hasn't injectedTestCoroutineDispatcher
throughout their code under test.WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation!
"Keep the behavior" part is, of course, more preferable as it really simplifies writing simple unit tests for end users. I've just realized that this is exactly why we thought to make
Uncofined
the default behaviour ofTestMainDispatcher
.I think it's easier to change the docs and explain that test dispatcher acts like unconfined one (and maybe implementation should use it as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a pretty good way to explain it. I'll add that to the docs in this PR since it's become all about threading.