Update TestCoroutineContext to support structured concurrency.

[objcode]

Updates TestCoroutineContext to support structured concurrency.

Issue: #541

Changes:

• Slit up TestCoroutineContext into TestCoroutineDispatcher, TestCoroutineExceptionHandler and TestCoroutineScope
• Added DelayController interface for testing libraries to expose Dispatcher control
• Added ExceptionCaptor inerface for testing libraries to expose uncaught exceptions
• Added builder for testing coroutines runBlockingTest
• Removed old builder withTestContext

Usage (main entry points)

runBlockingTest

runBlockingTest is an immediate executor, delays will always auto-progress from either dispatch or scheduleResumeAfterDelay. Note that it provides a suspend lambda.

fun runBlockingTest(context: CoroutineContext? = null, testBody: suspend CoroutineScope.() -> Unit)

@Test
fun blockingTest() = runBlockingTest {
    delay(1_000)
    assertTrue(true)
}

TestCoroutineDispatcher / TestCoroutineScope

When used in conjunction with #810 most tests will declare a dispatcher as part of the setup method. As a convenience extension functions are provided

@Before
fun setup() {
    val dispatcher = TestCoroutineDispatcher()
    Dispatchers.setMain(dispatcher)
    scope = TestCoroutineScope(dispatcher)
}

@Test
fun oneTest() = scope.runBlockingTest {
   // for running an async test with control over execution
}

needs review: Tests that only have a dispatcher are allowed access to runBlockingTest as well without constructing a scope

val dispatcher: TestCoroutineDispatcher 

@Test
fun threeTests() = dispatcher.runBlockingTest {
   // for running a test with an immediate executor
}

runBlockingTest with time control

Tests can use runBlockingTest with explicit time control if they use the pauseDispatcher function:

@Test
fun foo() {
    runBlockingTest(dispatcher) {
          pauseDispatcher {
              // use time control in runBlockingTest
          }
    }
}

Tests can use runBlockingTest with time control if they pass the dispatcher directly:

@Test
fun foo() {
    val dispatcher = TestCoroutineDispatcher()
    dispatcher.pauseDispatcher()
    runBlockingTest(dispatcher) {
          // use time control in runBlocking
    }
}

Library friendly interfaces

To help testing libraries expose test control, the time control and exception handling are split into interfaces. This allows a testing library to expose a smaller interface to tests than TestCoroutineDispatcher as well as combine the two interfaces onto a control object. In the current impl., TestCoroutineScope delegates both interfaces.

Ideally, testing libraries would prefer to expose these smaller interfaces. For example a JUnit4 rule might expose DelayController and delegate it to an instance of TestCoroutineDispatcher.

val scope = TestCoroutineScope()
scope.runCurrent() // delegated through DelayController

(Notes) API changes on this PR

• Removal of asyncTest that would start the test with explicit time control. This API was a close duplicate of runBlockingTest, and is more naturally handled by the block version of pauseDispatcher inside a test.
• Dispatcher starts in eager mode now, but can be explicitly paused and resumed.
• Removed runBlocking support. The overlap between runBlocking and runBlockingTest is large - but there are subtle differences that make runBlockingTest better suited for testing. There is no API change for runBlockingTest for this. The main advantage runBlockingTest will eagerly execute launch bodies even if they are never joined.

[zach-klippenstein]

Why allow unhandled exceptions? Seems like this could mask actual failures or errors, especially since I think tests should automatically catch issues where the programmer used GlobalScope.launch without remembering that exceptions thrown from such coroutines will be reported as uncaught.

This could also be configurable as well, although I would expect the default to not drop exceptions.

[objcode]

Good idea - it would probably make sense to make this default to true if implemented.

In this implementation, GlobalScope.launch exceptions won't be captured - only exceptions thrown in the scope passed.

I couldn't figure out how to make this error on exceptions since the CoroutineExceptionHandler didn't get triggered in runBlocking.

@elizarov do you have any thoughts about how to accomplish either?

[zach-klippenstein]

Derp, of course GlobalScope.launch wouldn't get caught. Test scope launch with a SupervisorJob is what I meant, I think.

[zach-klippenstein]

It's not obvious to me if there's a semantic difference between the run* and advance* prefixes in these method names – is there? E.g. advanceTimeToNextDelayed seems like it could also be called runToNextDelayed.

[objcode]

Good point. I was thinking about making all the clock moving functions advance (which would rename this advanceUntilIdle to be consistent).

Alternatively, everything could become run, which would make the following

runUntilNextDelayed()
runUntilDelay(delayTime, unit)

I don't think runUntilDelay is a great name. I'll change this to advanceUntilIdle and leave this comment up for more input.

[zach-klippenstein]

If the prefixes don't have distinct meanings, I don't have strong opinions on the color of the bikeshed as long as it's consistent. For prior art, RxJava's TestScheduler uses advance but also only has 2 such methods.

[zach-klippenstein]

Since it's a recommended practice to have classes implement CoroutineScope to provide convenient access to coroutine builders, it would be nice if the same pattern could be used for tests. E.g.

class MyTest : TestCoroutineScope {
  override val context = TestCoroutineDispatcher() + SomeCustomElement()

  @Test fun thingsWork() = asyncTest {
    …
  }
}

The way this API is designed, it requires custom scopes to always be expressed as explicit properties. Is this required verbosity intentional?

[objcode]

I'm not a huge fan of that style in tests, since TestCoroutineDispatcher is stateful and it wouldn't take much to leak state between tests if coded like that.

However, if you preferred to code it that way, it would just require a this to ensure you're dispatching to the builder you intend, as well as ensure that dispatcher.cleanupTestCoroutines gets called @after.

@Test fun things() = this.asyncTest { }

From a real-world usage perspective, using something like a JUnit4 rule would probably be the way to go here, which would already move out of using inheritance like this.

[zach-klippenstein]

Agreed a rule is probably better. Figured this was intended to prevent that pattern, but since that's a pretty common pattern in the official docs it might be helpful to explicitly call it out in the docs (e.g. "Implementing the scope interface is not recommended, for {reasons}, instead use this pattern.").

[objcode]

Thanks for review @zach-klippenstein!

[elizarov]

I've did a first pass through the codes, docs, and example usage. All in all, I very much like this approach to testing. The issues that need to be addressed (in particular order) are:

• Since Possibility to replace default dispatchers #810 has landed and we now have a dedicated kotlinx-corutines-test module, then this API should move there. Moreover, all existing APIs in test package of kotlinx-coroutines-core module shall be deprecated with a message explaining how to migrate to the new ones in a test module.

• Implementations need to be a little big more careful with respect to thread-safety, just in case people start doing weird things in their tests (in particular, TestCoroutineExceptionHandler.exceptions shall be a thread-safe list).

• Integration with custom dispatchers needs to worked out. In particular I'm concerned about Dispatchers.IO that libraries are supposed to use to wrap around blocking IO calls and Dispatchers.Default that might be used to wrap around CPU-consuming background functions in libraries, too. These dispatchers need to be integrated with test dispatchers (we'll need to hook into their implementations) so that, at least, they report that they can report to test dispatcher that they are "busy" working on some coroutines, so that various auto-advancing features would know to wait for them. Moreover, when you do delay from inside for Default dispatcher it currently goes to internal DefaultExecutor, but should be intercepted by test dispatcher instead. You can take a look at how it is internally done in implementation of withVirtualTimeSource function.

• As a part of kotlinx-coroutines-test module we can make a global default exception hander customizable in a similar way that we make Dispatchers.Main customizable. That will allow to catch all uncaught exception that happen during the test (even if they happen inside GlobalScope.launch { ... } block).

• Last, but not the least, naming. While I like runBlockingTest name, I don't think asyncTest name clearly reflects how it is different from runBlockingTest. I would suggest to think about keeping just one top-level method runBlockingTest and make immediate/non-immediate behavior customizable in some other way. Immediate execution seems to be a good (least-surprising) default behavior to me.

[objcode]

Thanks for the review - I'm going to get it migrated to the new module then take a look at the other points once it's back up to date with master.

[objcode]

Ahoy!

Got it rebased to master (ended up doing a bit of manual git history since the merge conflict commit wasn't particularly useful).

Since #810 has landed and we now have a dedicated kotlinx-corutines-test module, then this API
should move there. Moreover, all existing APIs in test package of kotlinx-coroutines-core module
shall be deprecated with a message explaining how to migrate to the new ones in a test module.

Moved this over. Taking a look at the current master, subclassing EventLoop (and promoting it to a public API) seemed unnecessary. After a bit of consideration I made TestCoroutineDispatcher work with runBlockingTest, launch and async and not support runBlocking.

The runBlocking integration was always somewhat awkward, since you had to remember to pass through coroutineContext every time, which was easy to forget (enough that I had multiple test cases testing the behavior when forgotten).

Implementations need to be a little big more careful with respect to thread-safety, just in case people
start doing weird things in their tests (in particular, TestCoroutineExceptionHandler.exceptions shall be
a thread-safe list).

Done. As a result it's copy on read.

Integration with custom dispatchers needs to worked out. In particular I'm concerned about
Dispatchers.IO that libraries are supposed to use to wrap around blocking IO calls and
Dispatchers.Default that might be used to wrap around CPU-consuming background functions in
libraries, too. These dispatchers need to be integrated with test dispatchers (we'll need to hook into
their implementations) so that, at least, they report that they can report to test dispatcher that they are
"busy" working on some coroutines, so that various auto-advancing features would know to wait for
them. Moreover, when you do delay from inside for Default dispatcher it currently goes to internal
DefaultExecutor, but should be intercepted by test dispatcher instead. You can take a look at how it is
internally done in implementation of withVirtualTimeSource function.

In both of those cases, I was imagining test code doing one of two things:

1. Injecting a test dispatcher that does time control in place of Dispatchers.IO, etc. It'd be worth exploring if it's useful to swap out the global Dispatchers from test code.

In this case everything works as expected, but multi-threading is removed. From a test design standpoint, the lack of multi-threading improves test reliability so I don't think it would be advantageous to support it from the TestCoroutineDispatcher.

2. Not injecting or mocking a library fn that uses withContext, which behaves normally as a suspend function in the test dispatcher.

In this case, the a delay in a library function will lead to a test delay. However, it's not clear that time can be auto-progressed in a discover-able way in this context.

That said, some work will need to integrate coroutines with Espresso (e.g. #242). I started chatting about that but haven't figured out exactly what it will mean. The trivial implementation (let espresso know whenever the dispatcher is executing something) will capture CPU bound tasks, but won't be busy when all coroutines are suspended waiting on input. The next implementation I'm considering is "any active jobs" = busy, but that won't handle actors.

I'll give it some more thought and think about what can fit here.

As a part of kotlinx-coroutines-test module we can make a global default exception hander
customizable in a similar way that we make Dispatchers.Main customizable. That will allow to catch
all uncaught exception that happen during the test (even if they happen inside GlobalScope.launch { ... } block).

Good idea, will take a look at what that would mean. I think it fits into this PR.

Last, but not the least, naming. While I like runBlockingTest name, I don't think asyncTest name
clearly reflects how it is different from runBlockingTest. I would suggest to think about keeping just
one top-level method runBlockingTest and make immediate/non-immediate behavior customizable in
some other way. Immediate execution seems to be a good (least-surprising) default behavior to me.

Agreed. I've completely reworked the API of the dispatcher to expose auto-advance by default and allow it to be paused for advanced use cases. asyncTest is pretty easy to build on top of that if a codebase prefers that style, but runBlockingTest consistently delivers the least surprising behavior ("run this test fast, don't surprise me with event loop internals").

I've removed asyncTest from this PR.

Thanks.

I'll ping again for a review after I take a look into the remaining issues discussed above.

[elizarov]

Thanks.

I totally agree that "single-threaded tests" is the great goal to have. That is how we write most of our library tests, too. It makes functional testing & debugging of various behaviors so much easier.

Let me clarify my concerns about interaction between withContext(Dispatchers.IO) { ... } and tests. The concern is composability in highly-modular code-bases.

Suppose that I have a separate module in my project that implements some useful function. For example, it performs some long-running CPU computation in its own thread pool or using some built-in dispatcher (like Dispatchers.Default) and exposes just the suspending function to the outside world. Now, if this function is somehow used, even indirectly (somewhere deep in a call-stack), in a test, then it foils "time-auto-advance" feature of the test dispatcher.

We can require, as a coding style practice, that such dispatchers are always get injected (so they can be replaced by a test dispatcher in tests), but, IMHO, that places too much burden on all the libraries. I would rather find some global, test-only mechanism that automatically replaces all such dispatchers with a test-dispatcher (or makes test dispatcher's time-advance feature aware of alternative dispatchers), so that we don't have to manually write this dispatcher-injecting code in our libraries just for the sake of being able to use them in tests.

[elizarov]

As for integration with EspressoIdlingResource (#242) I've extracted a separate issues for replacing Dispatchers.IO and Dispatchers.Default (#982) should help with such integrations. It could also be used to replace them with test dispatcher when needed.

[objcode]

That all makes sense to me. I've updated the code based on the remaining comments.

Current API

I've added appropriate experimental and deprecation annotations throughout. One thing to get input on - there is an existing API for TestCoroutineContext that's being replaced here, does it make sense to add it on DelayController as a deprecated-on-addition API to enable ReplaceWith refactors?

The current API exposes runBlockingTest as the main builder. Advanced users can use TestCoroutineScope, TestCoroutineExceptionHandler, and TestCoroutineDispatcher to execute tests.

Espresso

I did not put espresso in this PR, but looking at @yigit's code in #242 it fits cleanly in TestCoroutineDispatcher. I'd recommend reviewing this PR with the assumption that a callback-based integration will follow shortly.

Dispatchers.IO/Default

Agreed completely, a service locator should be added for both of those otherwise it will be nearly impossible to control this for libraries you depend upon. I'm seeing the pattern of thread confinement with withContext(Dispatchers.IO) being used extensively on Android, and it's a pattern we're leaning into. There should be no dependencies from this code to the service locators.

Exception Handling outside of the testing scoep

After playing around with it I decided /not/ to include a global exception handler hook in this PR. It would add a public API for coroutines (perhaps CoroutineScope.defaultUncaughtExceptionHandler = that would be useful in contexts outside of tests.

Adding a global default exception handler is probably a good idea (especially considering the different implementations for uncaught exceptions of the bytecode and js versions). There's two reasons for an exception handler like this:

1. Metrics tracking (crash reports etc)
2. Test validation (this is less of a clear choice)

Since this isn't really a test-first API it makes sense to consider it's addition for metrics / crash reporting, especially in JS where it looks like there's no way to intercept the exceptions another way (in bytecode the thread uncaught exception handler can be used).

[objcode]

When you have a second @elizarov can you review it and I'll loop back and address any comments you have!

Thanks,
Sean

[yigit]

this behavior is a bit worrying.
Specifically, an asyc being dispatched immediately significantly alters my coroutine's execution order. Of course, there are no guaratees but consider a buggy code like this:

val atomic = AtomicBoolean(false)
async {
    if (fetchData() == true) {
        atomic.set(true)
    }
}
// do something else
if (atomic.set()) {
    // do something.
}

normally, the code above would flake since async may or may not be scheduled immediately. By altering that behavior and making it consistent; this might hide bugs in applications. If we need to pick a consistent behavior, i would prefer asyncs and launches to be deferred as much as possible (put them end of the execution queue).
That is not perfect either but i think it is better behavior since it will make it easier to catch any missing await calls for side effects.

[objcode]

This is one I've gone back and forth on - in this specific example it would be prudent for a developer to write the test like this

- test of delayed async call
runBlockingTest {
    pauseDispatcher {
         subject()
         runCurrent()
    }
}

- test of early async call
runBlockingTest {
    subject()
}

I modeled the current behavior off the Robolectric scheduler which typically works for most code that doesn't introduce either race conditions or inappropriate side-effects at launch. For code that does, pauseDispatcher provides the ability to engage in explicit ordering control.

Eager execution in tests provides a benefit over purely lazy or EventLoop style execution for this typical code:

fun subject() { // don't return job
    scope.launch { // stuff }
}

@Test
fun testSubject() = scope.runBlockingTest {
    subject()
}

In execution in the runBlocking style, launch body won't be entered until the job.join(), which isn't available. With this dispatcher, the test could pause the dispatcher then call runCurrent() after subject() which is similar to the Rx implementation.

As another detail: All of this only applies to launched coroutines when using runBlockingTest. Regular suspend functions will progress through both dispatch and delay (calling another suspend function or a delay function) in runBlockingTest which accomplishes making the test go faster than the wall clock without adding a lot of code (or introducing new call / execution semantics)

Here's a pathological test that motivates this behavior:

@Test
fun funnyCode() = scope.runBlockingTest {
    delay(5000) // what should this do?
}

Update: for clarity, when using the dispatcher without runBlockingTest current tasks will be dispatched immediately, but delays will always require explicit time advance.

Again, I have gone back and forth on this a few times and would welcome other ideas. One way to go would be to invert the behavior, and expose a way to enter auto-progressing dispatcher state (maybe resumeDispatcher could take a block?)

[ashdavies]

I really love this PR as I've found it quite difficult to consistently use TestCoroutineContext with the current hierarchical standard with Coroutines and its a recurring problem I've been having, thank you so much @objcode for proposing it! If there's anything I can do to help 🙂

[objcode]

Thanks for taking a look @ashdavies ! Definitely looking for more input about the public API so please feel free to put comments on it!

Cheers,
Sean

[yigit]

+1

[yigit]

can we make this return boolean (or int, # of executed coroutines blocks) OR provide a function to check if idle?

I'm using the old one in another test where i have multiple TestCoroutineContexts. I want to have a function that progresses all of them (and they enqueue messages into eachother) and i cannot write a custom function without reflecting into the queue.

[yigit]

to be clear, the version I need is expected to ignore delayed tasks if time is not ready.
Maybe i'm missing a function that already exists :/.
Not proud of it but here is what i'm doing there:

private fun triggerAllActions() {
        do {
            mainContext.triggerActions()
            testContext.triggerActions()
            val allIdle = listOf(mainContext, testContext).all {
                it.isIdle()
            }
        } while (!allIdle)
    }

    private fun TestCoroutineContext.isIdle() : Boolean {
        val queueField = this::class.java
            .getDeclaredField("queue")
        queueField.isAccessible = true
        val queue = queueField.get(this)
        val peekMethod = queue::class.java
            .getDeclaredMethod("peek")
        val nextTask = peekMethod.invoke(queue) ?: return true
        val timeField = nextTask::class.java.getDeclaredField("time")
        timeField.isAccessible = true
        val time = timeField.getLong(nextTask)
        return time > now()
    }

[objcode]

Write a test around MAX_INT

[streetsofboston]

I Love to follow this. I wrote the original version of TestCoroutineContext and I'm interested how it is changed to support structured concurrency. Thanks!

[elizarov]

Here's what I've done to expedite release of this PR:

1. Merged this code with the most recent changes (most notably move to the new kotlin-multiplatform plugin and the corresponding changes to the source code organization)
2. Pushed the resulting code to pr/980 branch to test on our CI infrastructure.
3. Corrected some style issues here and there, added some todos for the future. See this commit: d76456f

There is still an unresolved issue around semantics of immediate launch and doActionsUntil. See discussions between @objcode and @yigit above. I've also added some minor todos in the code for future discussion. I'd suggest that we do not wait to resolve them, but merge and release this PR asap.

All new API is marked with @ExperimentalCoroutinesApi anyway, so we can discuss all the remaining issues separately and make adjustments before this API is stabilized. Please, submit separate issues to github with specific concerns.

@objcode Please, take a look at my adjustments. We tentatively plan to merge pr/980 branch, close this PR, and release it in version 1.2.1 next week. Thanks to your effort and patience.

[elizarov]

By the way, pay attention to "expect" style sequence testing that TestBase infrastructure provides. Take a look at this test I've added in pr/980 branch: https://github.com/Kotlin/kotlinx.coroutines/blob/d76456f1b6a92868e9f2f01309167003c94c66ca/kotlinx-coroutines-test/test/TestRunBlockingOrderTest.kt
It seems quite useful for all kinds of asynchronous testing when you want to ensure that events did happen in certain order. However, I'm not sure if this style of testing makes sense for general application-writing public, beyond testing of the core libraries.

[objcode]

Thanks @elizarov taking a look at the updates and notes.

[objcode]

Took a look at all of the changes, LGTM.

This will need a readme updates for the test package to cover the new API surface. I'll draft that in an isolated commit that can cherry-picked or merged after 890.

Minor nit:

Typo at d76456f#diff-e3c98820170c8b47a0aad1428e940c2cL111

And inquiry, is the _name style the correct naming convention for all private val's in classes?

[elizarov]

And inquiry, is the _name style the correct naming convention for all private val's in classes?

No. Only for backing properties: https://kotlinlang.org/docs/reference/coding-conventions.html#property-names
We kind of use this backing-property convention to name "atomic" fields that are "backing" the real properties, sometime even though there is no real property there, simply because otherwise a use-site of count.value looks fancy ("why I would need to ask a value of count, when it is a value?").

[objcode]

Put together a README.md for this PR in a cherry-pickable commit.

objcode#1

All of the url's are pending because I wasn't sure I figured out the url format for docs.

[elizarov]

Put together a README.md for this PR in a cherry-pickable commit.
objcode#1

Great. I've cherry-picked it into pr/980

All of the url's are pending because I wasn't sure I figured out the url format for docs.

URLs links to docs are automatically fixed by running ./gradlew knit. I did it in pr/980 branch.

[elizarov]

🎉 Merged into develop branch. Closing this PR. Feel free to open separate issue for future improvements.