kotlinx.coroutines icon indicating copy to clipboard operation
kotlinx.coroutines copied to clipboard

Integrate test dispatchers with libraries using Dispatchers.Xxx

Open elizarov opened this issue 4 years ago • 12 comments

Make sure that libraries can use hard-coded withContext(Dispatchers.IO) { ... } and still be testable with test dispatchers. See also discussion in #810 and #1202.

elizarov avatar Jul 23 '19 09:07 elizarov

The plan is to make TestDefaultDispatcher and TestIoDispatcher similar to TestMainDispatcher and integrate in into kotlinx-coroutines-test as soon as #1206 is here

qwwdfsad avatar Jul 23 '19 16:07 qwwdfsad

Any update on this issue ?

CharlyLafon37 avatar Aug 29 '19 21:08 CharlyLafon37

I too am curious if there's any update on this issue in 2020. Looking into solutions for unit testing withContext(Dispatchers.IO/Default) calls in my library that doesn't involve dependency injecting it into code where I don't actually want users to be able to provide their own dispatcher.

bungoboingo avatar Apr 06 '20 21:04 bungoboingo

Why don't you want users to provide your own dispatcher? Any solution here would basically be just another way to replace default dispatcher (just limited to the test library that could be used in production by your users).

matejdro avatar Apr 07 '20 07:04 matejdro

You can still use dependency injection without exposing to your consumers. E.g. an internal primary constructor that takes a Dispatcher, and a public secondary constructor that provides a default value. Or a public interface with internal implementation and a public factory function.

zach-klippenstein avatar Apr 07 '20 12:04 zach-klippenstein

@zach-klippenstein This solution does not help me with hardcoded withContext(Dispatchers.IO/Default) in libraries I have a dependency on

CharlyLafon37 avatar Apr 07 '20 15:04 CharlyLafon37

Another potential approach to this problem, which still avoids overriding global singletons, would be to bake something like what is proposed in this article into the standard library, and change Dispatchers.* to instead simply indicate which of the DispatcherProvider-provided dispatchers to use (e.g. withContext(Dispatchers.IO) means lookup coroutineContext[DispatcherProvider].io and use that). Then any libraries that take a coroutine context automatically have configurable dispatchers. I'm sure there are still libraries that this still wouldn't solve for, but it feels less hacky for those that it would.

zach-klippenstein avatar May 14 '20 18:05 zach-klippenstein

This approach still requires you to have access to the scope/context. If library creates its scope internally, you cannot access that.

matejdro avatar May 14 '20 18:05 matejdro

This issue keeps coming up for me. Is there anything we can do to help move it along?

BenTilbrook avatar Jan 07 '21 00:01 BenTilbrook

This is (partially?) fixed now in 1.6.0-RC with the new API for testing.

  • Using withContext(Dispatchers.IO) { ... } in runTest (which is a replacement for runBlockingTest) will wait for a minute for the code in the block to execute, so there won't be a near-instant crash. withContext(Dispatchers.IO) { delay(100) } will work just fine, for example.
  • However, the delays are not skipped in Dispatchers.IO, Dispatchers.Default, and in general dispatchers that are not integrated with the test module. So, withContext(Dispatchers.IO) { delay(100_000) } will spin for a minute before crashing the test.

I'm not sure how much of a problem the remaining limitation is in practice: is it often that third-party code delays for long enough to notice? So, not closing this issue yet. Please share your thoughts and feedback!

dkhalanskyjb avatar Dec 08 '21 11:12 dkhalanskyjb

We do have retry handling and other code that needs to work around limitations of underlying libraries with delay and those can happen inside non-Main dispatchers. It would be really great to have the possibility to not just set the main dispatcher but really all dispatchers. Currently we still depend on a wrapper (e.g. dispatchers.io instead of Dispatchers.IO) to have full control and a well-defined test environment. Does anything speak against just providing a simple Dispatchers.setAll(dispatcher)?

Also, it would be really awesome to have some utility to make setting up coroutine-capable unit tests trivially easy. Ideally, the default behavior in unit tests would automatically switch all dispatchers and the test lib would provide a global singleton default testScope. If anyone needs more control it's still possible to modify the default setup and use custom testScopes.

If having nice default behavior automatically is impossible, then it would be great to have something like a CoroutineTest base class or delegate or a test rule that can be added to any existing test class in as little code as possible to get the nice defaults. Anything that doesn't require repeating that boilerplate and learning unnecessary details just to get started would be great.

wkornewald avatar Dec 30 '21 08:12 wkornewald

You can still use dependency injection without exposing to your consumers. E.g. an internal primary constructor that takes a Dispatcher, and a public secondary constructor that provides a default value. Or a public interface with internal implementation and a public factory function.

Could anyone explain more about what he is saying for me?

sgc109 avatar May 04 '22 14:05 sgc109

This is (partially?) fixed now in 1.6.0-RC with the new API for testing.

  • Using withContext(Dispatchers.IO) { ... } in runTest (which is a replacement for runBlockingTest) will wait for a minute for the code in the block to execute, so there won't be a near-instant crash. withContext(Dispatchers.IO) { delay(100) } will work just fine, for example.
  • However, the delays are not skipped in Dispatchers.IO, Dispatchers.Default, and in general dispatchers that are not integrated with the test module. So, withContext(Dispatchers.IO) { delay(100_000) } will spin for a minute before crashing the test.

I'm not sure how much of a problem the remaining limitation is in practice: is it often that third-party code delays for long enough to notice? So, not closing this issue yet. Please share your thoughts and feedback!

@dkhalanskyjb AFAIS the main problem is still there, which is that using hard-coded Dispatchers.IO makes your tests slow and non-deterministic. It's a bummer that injecting manually the dispatchers is still the way to go, also indicated by the Android guidelines as a good practice to follow. I'm a library developer and share the pain of doing it. Writing classes with an injected dispatcher is not great but ok. Writing static functions with an injected dispatcher is ugly. With the upcoming context receivers and the ability to write more and more effectful static functions, I feel that not having a standardized solution provided by kotlinx.coroutines for properly injecting the dispatcher is weird. WDYT?

Exploiting the coroutineContext in a solution similar to the one provided by @zach-klippenstein seems the most natural way of decoupling the dispatchers IMHO.

Example of the current problem as far as I see that:

class DispatchersTest {

    private val dispatcher = UnconfinedTestDispatcher()

    @Test
    fun `a non-deterministic test`(): Unit = runTest {
        spinHardcoded() // Sometimes fails and slow
    }

    @Test
    fun `a deterministic test`(): Unit = runTest(dispatcher) {
        spinInjected(dispatcher) // Deterministic and fast
    }

    suspend fun spinHardcoded() =
        withContext(Dispatchers.IO) { spinTheWheel() }

    suspend fun spinInjected(dispatcher: CoroutineDispatcher) =
        withContext(dispatcher) { spinTheWheel() }

    suspend fun spinTheWheel() = coroutineScope {
        var state: Int
        repeat(100000) { index ->
            launch {
                state = index
                check(state == index)
            }
        }
    }

}

francescotescari avatar Oct 19 '22 08:10 francescotescari

Not a very illustrating example. spinTheWheel, if it was part of the production code, is obviously buggy, so it's correct for the test to fail: for tests to be useful, they must reflect what's happening in production. That's what we're testing, the purpose of the process is not just to spin the CPU a bit and be satisfied with 15068/15068 tests being green.

So, our goal is twofold:

  1. Make it difficult to write tests that pass when production fails,
  2. Make it convenient to write tests that pass when production passes.

Making Dispatchers.IO deterministic in tests by default goes against the first goal, as your example shows, which is what holds us back. If we only recognized the second goal, then sure, you're right, not having to inject something is always more convenient.

Another issue with mocking Dispatchers.IO by default (or semi-default): there are some libraries that use Dispatchers.IO that don't expect to be mocked, (especially by a single-threaded dispatcher, which is another set of restrictions). For example, and this is a common occurrence, let's say you connect to some database in your tests. While most tests should probably mock the database calls, it's sometimes much more convenient (in cases of complex queries) and not any less robust to spawn an instance of some in-memory database just for running the tests. The following code becomes incorrect when run in an injected Dispatchers.IO:

val connection = try {
  withTimeout(20.seconds) {
    database.connect()
  }
} catch (e: TimeoutCancellationException) {
  throw IllegalStateException("Couldn't connect to the database")
}

The reason is, those 20 seconds will pass instantly with the test framework, so the code will always time out. We would need a way for the author to say "I need to use Dispatchers.IO here, exactly the multithreaded version, the one without the delay-skipping, please don't mock it". Besides, in any case, is it a good idea to shift the burden on the author of this specific code so that they have to account for the delay skipping? We would be trading one group of disgruntled library authors who now have to make Dispatchers.IO explicitly injectable for another, those who would have to deal with the users of their library complaining that, with the mocking of Dispatchers.IO by the test framework, they see no way to have their code work well.

dkhalanskyjb avatar Oct 19 '22 11:10 dkhalanskyjb

Thank you for your answer! What you say makes sense to me. Let me just share that in my company (Android) in unit tests the dispatcher is always injected and mocked withh TestDispatcher (even IO and Default) and this seems to be aligned with what the guidelines of Android suggest (1, 2) and also seems to be a different approach compared to:

Making Dispatchers.IO deterministic in tests by default goes against the first goal, as your example shows, which is what holds us back.

Regarding the database example issue, I see the problem and the problem it could be with third-party libraries.

francescotescari avatar Oct 19 '22 17:10 francescotescari

This is (partially?) fixed now in 1.6.0-RC with the new API for testing.

  • Using withContext(Dispatchers.IO) { ... } in runTest (which is a replacement for runBlockingTest) will wait for a minute for the code in the block to execute, so there won't be a near-instant crash. withContext(Dispatchers.IO) { delay(100) } will work just fine, for example.
  • However, the delays are not skipped in Dispatchers.IO, Dispatchers.Default, and in general dispatchers that are not integrated with the test module. So, withContext(Dispatchers.IO) { delay(100_000) } will spin for a minute before crashing the test.

I'm not sure how much of a problem the remaining limitation is in practice: is it often that third-party code delays for long enough to notice? So, not closing this issue yet. Please share your thoughts and feedback!

Thank you very much, this hack solve my issue

mash6699 avatar Feb 24 '23 05:02 mash6699