Kotlin/kotlinx.coroutines

Exceptions being swallowed during tests / UncaughtExceptionsBeforeTest

SnyersK opened this issue · 5 comments

Describe the bug

When replacing the Main Dispatcher with a TestDispatcher, tests are not failing as expected.
I've provided an example below which showcases the issue.

When the Main Dispatcher is replaced, and I don't use runTest, as it doesn't seem necessary, exceptions during that test get swallowed, and the test completes successfully.

There's some inconsistent behaviour though, and I'm not sure why:

  • Sometimes the exception seems to be swallowed completely, and even a subsequent test does not fail. (1 & 2)
  • Sometimes a subsequent test which does use runTest fails with an UncaughtExceptionsBeforeTest (4 & 5)

I'm not sure if this is intended behaviour.
If it is, I suppose the main issue I have, is that it's not clear that runTest should be used for all tests when the Main Dispatcher is replaced with a TestDispatcher;
Even if we're not actively interacting with Coroutines.
(no time control / not launching any jobs / not calling suspending functions ...)

One way this can become an issue without it being noticed:
Given a class with corresponding tests which already replaces the Main Scheduler via setup/teardown or a test rule

if a method of this class, which was not launching any coroutine job before, is changed to launch a coroutine,
the existing test(s) won't be using runTest.
This may result in exceptions being thrown, but the test not failing.
Best case scenario, a later test fails, giving enough information to fix the actual cause.
Worst case scenario, this exception is not thrown anywhere at all, potentially leaving a bug in the code which should have gotten caught by the tests.

Provide a Reproducer

interface Dependency {
    fun getSomething(): Boolean
}

private class Example(
    private val dependency: Dependency
) {

    private val coroutineScope by lazy {
        CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate)
    }

    fun doSomething(fail: Boolean) {
        coroutineScope.launch {
            if (fail) throw Exception("fail")
        }
    }

    fun doSomethingWithDependency() {
        coroutineScope.launch {
            dependency.getSomething()
        }
    }
}

@FixMethodOrder(MethodSorters.NAME_ASCENDING)
class ExampleTest {

    @get:Rule
    val mockkRule = MockKRule(this)

    private val dependencyMock: Dependency = mockk()
    private val sut = Example(dependencyMock)

    @Before
    fun setUp() {
        Dispatchers.setMain(UnconfinedTestDispatcher())
    }

    @After
    fun tearDown() {
        Dispatchers.resetMain()
    }

    @Test
    fun `1 should fail but does not`() {
        sut.doSomething(fail = true)
    }

    @Test
    fun `2 succeeds - so above error is fully swallowed`() = runTest {
        sut.doSomething(fail = false)
    }

    @Test
    fun `3 fails as expected`() = runTest {
        sut.doSomething(fail = true)
    }

    @Test
    fun `4 should fail but does not`() {
        sut.doSomethingWithDependency()

        verify { dependencyMock.getSomething() }
    }

    @Test
    fun `5 should succeed but does not - due to above error`() = runTest {
        // kotlinx.coroutines.test.UncaughtExceptionsBeforeTest: There were uncaught exceptions before the test started. Please avoid this, as such exceptions are also reported in a platform-dependent manner so that they are not lost.
        every { dependencyMock.getSomething() } returns true

        sut.doSomethingWithDependency()

        verify { dependencyMock.getSomething() }
    }
}

There's a potential for the documentation to be improved, but yes, it's not enough to replace the Main dispatcher with a TestDispatcher: someone has to actually execute the tasks on that dispatcher (that is, something should emulate the event loop of the main thread), and runTest is the one doing it. It's certainly not enough to just override the main dispatcher, even if your coroutines don't throw exceptions: some tasks spawned on the main thread just won't run.

In theory, you can use a TestDispatcher with a TestCoroutineScheduler separately from runTest, but this is a very advanced use case that requires a deep understanding, so their docs certainly should mention runTest.

Would there be a way to detect if a test is not using runTest when required, and throw an exception, so it's immediately clear that this is missing?

(Or an existing test needs to be updated, after a change)

I don't know if the TestDispatcher can perhaps check if it's running in a TestScope, for example?

Unfortunately, no: injecting a TestDispatcher before a TestScope exists or runTest is run is an important pattern. Dependency injection is typically initialized before the test is started.

Ah yes, I understand now.
Classes create their own / use another CoroutineScope.
There's no link between the TestScope and the coroutines launched from it's scope.

I've found that apparently there's a Detekt rule to check if tests which interact with coroutines make use of runTest.
Will need to try this out.

https://github.com/detekt/detekt/blob/f8bf79b24baebd10fda01ffe2ef586cf8bc1fff1/detekt-core/src/main/resources/default-detekt-config.yml#L175-L178

It's safe to assume that, as a general rule of thumb:

all of the tests in a test class which replaces the Main scheduler; and injects the TestDispatcher for all of the other dispatchers; should use runTest, to avoid running into potential (future) issues, even if the test itself is not directly interacting with a coroutine / using the time methods, right?

So even if a test just calls a method which is not launching any coroutine at all, it would be safer to just use runTest regardless?