Skip to content

Possibility to replace default dispatchers #810

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

Closed
dmytrodanylyk opened this issue Nov 8, 2018 · 18 comments
Closed

Possibility to replace default dispatchers #810

dmytrodanylyk opened this issue Nov 8, 2018 · 18 comments
Labels
Milestone

Comments

@dmytrodanylyk
Copy link
Contributor

dmytrodanylyk commented Nov 8, 2018

So Rx java has a way to inject scheduler (RxJavaPlugins), is there a plan to have a similar thing for coroutines dispatchers?

@gildor

I would add +1 for this.
Replacing dispatcher implementation is pretty common thing which of course can be solved by DI, but this adds not necessary boilerplate especially for functions and Inline classes (I have use case when would like to use Inline class to wrap blocking API with coroutines, but cannot because want to allow switch dispatcher)
This is also crucial for UI dispatcher and unit tests.
I agree that in most cases it can be handled by DI but a way to replace default dispatchers sounds reasonable for me

Christoph Sturm (from slack)

is there a blocking scope that i can use to inject into classes that i test and make them run like they were invoked with runBlocking?
I have some classes that implement interfaces that are not suspending, so they have their own coroutine scope and use scope.launch {}. but in my tests it would be nice to run them blocking, or to be able to wait on the scope

@matejdro
Copy link

I'm currently doing this by creating proxy TestableDispatchers object which mirrors normal Dispatchers object, with ability for me to override what objects it returns

/**
 * Collection of Coroutine dispatchers that can be overriden for Unit tests.
 **/
object TestableDispatchers {
    /**
     * [Dispatchers.Default] dispatcher that can be overriden by unit tests.
     */
    @JvmStatic
    val Default
        get() = dispatcherOverride {
            Dispatchers.Default
        }

    /**
     * [Dispatchers.Main] dispatcher that can be overriden by unit tests.
     */
    @JvmStatic
    val Main
        get() = dispatcherOverride {
            Dispatchers.Main
        }

    /**
     * [Dispatchers.IO] dispatcher that can be overriden by unit tests.
     */
    @JvmStatic
    val IO
        get() = dispatcherOverride {
            Dispatchers.IO
        }

    @JvmStatic
    var dispatcherOverride: (() -> CoroutineContext) -> CoroutineContext = { it() }
}

Then for tests I just use this JUnit rule to override them:

class ImmediateDispatcherRule : TestWatcher() {
    // UNCONFINED is experimental, but it is still fine to use it with tests
    @Suppress("EXPERIMENTAL_API_USAGE")
    override fun starting(description: Description?) {
        super.starting(description)

        TestableDispatchers.dispatcherOverride = { Dispatchers.Unconfined }
    }

    override fun finished(description: Description?) {
        super.finished(description)

        TestableDispatchers.dispatcherOverride = { it() }
    }
}

Of course this requires additional care when launching coroutines (you cannot just rely on DefaultDispatcher, you must explicitly set the dispatcher every time).

@elizarov
Copy link
Contributor

I am not in favor of RxJavaPlugins approach where you can just setXxx. Just like any form of global (static) mutable state this leads to lots of practical problem. However, for testing purposes this seems like a reasonable and useful request. See PR #749 that we are working on.

@matejdro
Copy link

Maybe good solution would be to include IntelliJ warning/inspection to not override dispatchers in production code?

@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Nov 19, 2018

Let me clarify: we are not going to implement it in the form of global settable dispatchers like RxJavaPlugins does. For a core library, the probability that other libraries will set these global variables is high and conflicts in such cases just cannot be reasonably resolved.
The fact that some global state is used by literally every library builder does not make it easier (=> e.g. underlying thread can be changed during coroutine execution)

If you want to have control over dispatchers, use dependency injection and inversion of control.
Scoped concurrency is here for the rescue as one doesn't have to specify dispatcher everywhere, only in scope declaration.

Maybe good solution would be to include IntelliJ warning/inspection to not override dispatchers in production code?

Inspections and warnings won't help here, because people will use it anyway ("I get used to it in RxJava, so I can ignore this warning").

@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Nov 19, 2018

However, we understand that ability to replace dispatchers is very useful during testing and we don't want our users to write additional boilerplate only for tests.

We will provide a separate test module (#749) that will allow redefining Main dispatcher as a first step as it is the biggest pain for Android developers.
As the next step, I will be happy to collaborate and enchant this module in order to help developers write tests easier, but in terms of particular problems, not in terms of "replace dispatcher globally".

E.g.

I have some classes that implement interfaces that are not suspending, so they have their own coroutine scope and use scope.launch {}. but in my tests it would be nice to run them blocking, or to be able to wait on the scope

For that purpose, it is probably better to provide something like scope.awaitLaunched or any way to complete and join the scope, depending on the use case

@Test
fun myTest() = runBlocking<Unit> {
  val myService = myService.doAsyncStuff()
  myService.scope.awaitLaunched()
  // Validate 
  ...
}

@dmytrodanylyk
Copy link
Contributor Author

Sounds good. In most cases in Android result is delivered to the main thread anyway.

@matejdro
Copy link

matejdro commented Nov 28, 2018

Another use case for this is ability to replace dispatchers with TestCoroutineContext for testing code that involves timeouts and delays.

@qwwdfsad qwwdfsad added this to the Release 1.1 milestone Dec 6, 2018
@yigit
Copy link

yigit commented Dec 7, 2018

Side effects is another case that needs some help for testing.

For instance, you can write espresso tests that look like:

button.click()
assert(contentView).hasData("foo")

And if you've registered your task runners to Espresso as Idling resources, it knows how to await for them to go idle.

some code may need to do it explicitly as well:

button.click()
awaitAllExecutorsIdle()
assertSomethingDidntHappen()

It would be good to have support for these as well.

@gattacus
Copy link

It should be possible to use a custom MainCoroutineDispatcher (with whatever mechanism). There is not only Android and Swing as UI frameworks on the JVM. I develop a SWT application that uses Kotlin coroutines.

@matejdro
Copy link

matejdro commented Dec 18, 2018

I'm not sure if this commit closes this issue entirely. This issue is about replacing all the dispatchers, while commit only provides replacer for Main dispatchers.

From what I can see there is still no good way to inject TestCoroutineContext into app, except for passing around Dispatchers via DI which is very cumbersome.

@L7ColWinters
Copy link

However, we understand that ability to replace dispatchers is very useful during testing and we don't want our users to write additional boilerplate only for tests.

We will provide a separate test module (#749) that will allow redefining Main dispatcher as a first step as it is the biggest pain for Android developers.
As the next step, I will be happy to collaborate and enchant this module in order to help developers write tests easier, but in terms of particular problems, not in terms of "replace dispatcher globally".

E.g.

I have some classes that implement interfaces that are not suspending, so they have their own coroutine scope and use scope.launch {}. but in my tests it would be nice to run them blocking, or to be able to wait on the scope

For that purpose, it is probably better to provide something like scope.awaitLaunched or any way to complete and join the scope, depending on the use case

@Test
fun myTest() = runBlocking<Unit> {
  val myService = myService.doAsyncStuff()
  myService.scope.awaitLaunched()
  // Validate 
  ...
}

So we should expose the scope of our classes in order to test it? I have no other reason to expose scope and make it public.

@CharlyLafon37
Copy link

CharlyLafon37 commented Jul 19, 2019

Is there any update on this issue ? I think it would be very helpful for testing to have a way of overriding the other Dispatchers. For example, I want to test this method in my ViewModel :

fun login(login: String, password: String) = viewModelScope.launchSafe(::handleLoginExceptions) {
    // ...
    val stuff = myString.decodeBase64()
    // ....
 }
suspend fun String.decodeBase64(): String = withContext(Dispatchers.Default) {
    // decode Base64 and return it
}

login() returns Unit and I want to keep it that way. Now I can't test it because I can't find a way of making the test method waiting for what's inside launch() to finish.

@elizarov
Copy link
Contributor

@CharlyLafon37 we are looking at ways to make it working without having to replace dispatchers. A solution that you can use now to make your code testable is to inject the dispatchers into your models.

@CharlyLafon37
Copy link

@elizarov Too bad. I don't really like the workaround that you suggest as it adds dependencies to the code just for testing purposes. I hope the Kotlin team will build a real solution to this problem (which I think is critical) very soon 😄.

@JakeWharton
Copy link
Contributor

The dependencies were always there. You're just currently electing to use global static to resolve them.

@CharlyLafon37
Copy link

I meant one dependency to inject.

@matejdro
Copy link

A solution that you can use now to make your code testable is to inject the dispatchers into your models.

Problem is that this solution only works for the code I can directly edit.

If a library that I use has hardcoded withContext(Dispatchers.IO) somewhere, I'm out of luck.

@elizarov
Copy link
Contributor

We'll be working on making such libraries testable. I've created a separate issue #1365

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants