Skip to content
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

What is the preferred way of testing when the code being tested launches a never-ending coroutine? #3283

Closed
azabost opened this issue May 13, 2022 · 5 comments

Comments

@azabost
Copy link

azabost commented May 13, 2022

What is the preferred way of unit testing classes which internally launch a never-ending coroutine such as SharedFlow collection?

Let’s say:

class MyClass(coroutineScope: CoroutineScope, flowToCollect: Flow<Int>) {
    
    var lastObservedResult: Int? = null

    init {
        coroutineScope.launch {
            flowToCollect.collect { lastObservedResult = it }
        }
    }
}

If I use runTest and pass the created TestScope then the test is going to fail after some time because there is a running coroutine.

@Test
fun testMyClass() = runTest {
    MyClass(this, flow)
    // do something here, make some assertions etc.
    // at the end, the test is going to fail because of the running coroutine
}
After waiting for 60000 ms, the test coroutine is not completing, there were active child jobs (...)

So should I create another scope instead? Like this, for example?

val dispatcher = UnconfinedTestDispatcher()

@Test
fun testMyClass() = runTest(dispatcher) {
    val additionalScope = CoroutineScope(dispatcher)

    MyClass(additionalScope, flow)
    // do something here, make some assertions etc.

    additionalScope.close() // Is this necessary, btw? Is the launched coroutine going to leak after the test is finished or something?
}

I'm trying to figure out what are the best practices in such cases.

@dkhalanskyjb
Copy link
Collaborator

Creating a scope for neverending jobs and cancelling it in the end is the recommended way. You may be interested in this as well, it looks fairly similar to your question: #1531 tl;dr: runTest requires the coroutines to complete because a running coroutine is a resource leak in production code, which we want to help eliminate. An analogy: if an object stays in memory forever because it's a singleton, it's perfectly fine, but an object wasting memory because something references it needlessly is a resource leak.

@hrach
Copy link
Contributor

hrach commented May 13, 2022

We did it this way: we have injectable CoroutineScope (appScope~GlobalScope) for app-lifetime jobs, this caused us issues in our unit tests, so we had to rewrite runBlockingTest somehow like this. (appScope is injected through constructor to the tested class)

  private val testScope: TestScope = TestScope()
  protected val appScope: CoroutineScope = CoroutineScope(testScope.coroutineContext + SupervisorJob())

  protected fun runBlockingTest(
    body: suspend TestScope.() -> Unit,
  ): TestResult = testScope.runTest {
    body()
    appScope.coroutineContext[Job.Key]!!.cancelAndJoin()
  }

@azabost
Copy link
Author

azabost commented May 13, 2022

@dkhalanskyjb Thanks for such a quick response 🙏 I supposed this is going to be the answer. However, I must admit that I somewhat agree with the last comment here. It seems like a lot of unnecessary hassle to create and close an additional scope in these cases, but I understand there must have been some good reasons for the current design of runTest/TestScope behavior.

One of my questions was hidden in the code snippet so let me repeat it because I feel like it wasn't answered yet: is it really necessary to call additionalScope.close() each time? In other words: is the launched coroutine somehow "leaked" after the test is finished, i.e. when I run thousands of tests like that without calling additionalScope.close(), will I get thousands of dangling coroutines in memory even though the tests which launched them have already finished running?

@dkhalanskyjb
Copy link
Collaborator

In the provided code, when the dispatcher is mocked, no, the coroutines will not leak, there's no global state related to the test framework.

In general, though, it's absolutely possible to have thousands of dangling coroutines; the simplest way to get this is to do GlobalScope.launch { something infinite }. Test runners like JUnit don't typically spawn a whole JVM instance per test, so any coroutines spawned in Dispatchers.Default, Dispatchers.IO, or any other dispatcher that uses the global thread pool, will run while the other tests execute. So, in case your code launches some coroutines with the non-mocked built-in dispatchers, you probably want to cancel them at the end of the test.

@azabost
Copy link
Author

azabost commented May 13, 2022

Thanks for the explanation 👍 I guess we can close this issue now 🙂

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

No branches or pull requests

3 participants