Kotlin / kotlinx.coroutines

Library support for Kotlin coroutines
Apache License 2.0
12.86k stars 1.83k forks source link

New "ConfinedTestDispatcher" variation #3279

Open Mugurell opened 2 years ago

Mugurell commented 2 years ago

(Continuing from https://github.com/Kotlin/kotlinx.coroutines/issues/3246#issuecomment-1123713267)

kotlin-coroutines 1.6.0 came with a new API and multiplatform support.

Trying to decide between UnconfinedTestDispatcher and StandardTestDispatcher we saw the need for maybe a new variation that would combine the two to provide:

with the broad goal of getting serial task execution on the same thread and in that allow for testing a bit more than just one task at a time without having to sprinkle runCurrent or advanceTimeUntilIdle all throughout.

dkhalanskyjb commented 2 years ago

I don't know anything about "middleware", but from a cursory glance, it doesn't look like it utilizes the concept of a thread at all, being a thing for JS. So, are there any downsides to just using UnconfinedTestDispatcher for your use case?

Mugurell commented 2 years ago

Thank you for looking into this @dkhalanskyjb ! Continuing from the example from https://github.com/Kotlin/kotlinx.coroutines/issues/3246#issuecomment-1119345128 with one remark: everything is Kotlin used for and Android app.

Our app uses MVI stores with middlewares that may launch new coroutines that may emit new actions launching then new coroutines and so on.

In extreme cases we have to now use

store.dispatch(
    EngineAction.LoadUrlAction(
        "test-tab",
        "https://www.firefox.com"
    )
).join()

testScheduler.advanceUntilIdle()
store.waitUntilIdle() // Wait for CreateEngineSessionMiddleware to create the new session

testScheduler.advanceUntilIdle()
store.waitUntilIdle() // Wait for the LinkingMiddleware to load url
testScheduler.advanceUntilIdle()

MVI and the middleware concept can be seen for us as basically the "chain or responsability pattern" - a series of objects for handling a stream of specific actions, each action potentially being handled in a new coroutine possibily on another thread. This architecture and StandardTestDispatcher will lead to the above code snippet just to have everything running. Using UnconfinedTestDispatcher removes the need for all of those advanceUntilIdle calls which is great and the direction we went on here.

Confining everything to one thread would then come to assure us that no delay calls or heavy work being done in one coroutine (possibly running in another thread) would lead to out of sync middlewares / actions / asserts.

dkhalanskyjb commented 2 years ago

Confining everything to one thread would then come to assure us that no delay calls or heavy work being done in one coroutine (possibly running in another thread) would lead to out of sync middlewares / actions / asserts.

This is the part I don't understand: what is the thing that StandardTestDispatcher guards against that UnconfinedTestDispatcher doesn't? Could you give an example of an "out of sync" something that may happen with UnconfinedTestDispatcher? As I see it, StandardTestDispatcher and UnconfinedTestDispatcher are equivalent in regards to preventing data races: if you mock everything in your test to run with aTestDispatcher, then no races are possible, but if you don't and there are separate threads doing their thing during the tests, the races are possible with both dispatchers.

Mugurell commented 2 years ago

We were thinking about the unconfined nature of the appropriately named dispatcher.. If because of a previous coroutine a next one starts and runs on a different thread But we wanted the test to run synchronously and block on the execution of all nested coroutines would it be possible for the test to not wait for the execution of that second coroutine? If not and everything will run synchronously then we are fine with the UnconfinedTestDispatcher.

dkhalanskyjb commented 2 years ago

See the docs on Dispatchers.Unconfined: if your test does not use more than one thread on its own (by manually creating threads or using non-mocked Dispatchers.Default or Dispatchers.IO), UnconfinedTestDispatcher will not create a new one.

Mugurell commented 2 years ago

We do have some threads "manually" created. For the most common scenarios we can access it / the CoroutineScope using a ExecutorService.asCoroutineDispatcher and join that from the test but just thinking that it would be nice (not sure how it would work under the hood) to not have to do this. Also based on the example from the documentation it would be nice to have a guaranteed "top to bottom" order of execution.

dkhalanskyjb commented 2 years ago

I'm trying to find this usage that you're describing in the android-components project, but am not successful. Could you give me some pointers? I found the remark about using join unclear, so just pointing me to some tests that use this pattern would help a lot.

Regarding serialization of events, I think that, typically, UnconfinedTestDispatcher is as safe as StandardTestDispatcher, even in the presence of other threads, so if there is some test where you're uneasy about using UnconfinedTestDispatcher, could you please show it as well?

Mugurell commented 2 years ago

We have to use in many places (not all appearing in the Github query) our custom waitUntilIdle method which will join all children coroutines launched for every event in the app.

The waitUntilIdle is often used in conjuction with another joinBlocking call, seen in the search result from above.