TestCoroutineDispatcher swallows exceptions

[manuelvicnt]

@julioyg found an interesting bug(?) regarding the TestCoroutineDispatcher.

The fact that runBlockingTest uses async to run the test body (TestBuilders.runBlockingTest#L49), by the time it tries to throw an exception, the test has already completed.

Problems:

1. Silent exceptions
   If the coroutine inside a function throws an exception, the test passes and the exception is thrown silently to the console.

2. Tests that expect an exception to be thrown
   Consider the following scenario. Since we're running this in a blocking way, the expectation is that the exception is thrown, but it is not.

class MyViewModel(): ViewModel() {
   fun throwException() {
        viewModelScope.launch {
            throw Exception("Not found")
        }
    }
}

class MyViewModelTest {

   @get:Rule
   var coroutinesTestRule = CoroutinesTestRule()

   @Test
   fun testExceptionThrown() = coroutinesTestRule.testDispatcher.runBlockingTest {
        val assertionThrown = false
        try {
            val viewModel = MyViewModel()
            viewModel.throwException()
        } catch(e: Exception) {
            assertionThrown = true
        }    
        assertTrue(assertionThrown)
   }
}

[manuelvicnt]

cc @objcode

[objcode]

Ah so this one is a bit weird - since you're not passing the exception handler from the TestCoroutineScope the exception is handled by viewModelScope (which I don't believe will surface it).

This might be more properly a bug against the viewModelScope implementation (allow the injection of parent scope) WDYT?

[objcode]

Two options here:

1. Add test-injection of uncaught exception handler to viewModelScope for a specific viewModel
2. Add a global uncaught exception handler on CoroutineScope (CoroutineScope.addGlobalUncaughtExceptionHandler) - this mirrors the thread APIs. However, it's really quite global (and note that it's not normal to set the thread version in test code because it breaks test parallelism)

[yigit]

I've not thought about the whole problem but there is a flaw in that test code.

class MyViewModel(): ViewModel() {
   fun throwException() {
        viewModelScope.launch {
            throw Exception("Not found")
        }
    }
}

You are calling a regular function and launching inside it. It is async, there is no reason for your test to wait for it to finish.

I don't think this has anything to do w/ uncaught exceptions. That test would never really work as intended.

For the view-model though, might be necessary for us to provide some side channels to access / swap the context / scope.

[julioyg]

I guess the problem is that the tests passes (printing the exception in the log) but the code in production crashes because the exception is not handled.

[objcode]

Agreed - this should definitely cause a test (or at least the test runner from a global uncaught exception) to fault via some mechanism.

[manuelvicnt]

I don't completely agree with the fact that the test is flawed @yigit . It's true that the example is taken to extremes but the same way you test the happy path of a coroutine, you should be able to test the sad path.

We've been testing happy paths forever. e.g. using LiveDataTestUtil to wait for an element to be present. The sad path should be as easy to test as the happy path.

[objcode]

Agreed, this does seem to violate surface expectations despite being "working as intended." Given how prevalent non-configurable test exception handlers have been in the current API designs (e.g. viewModelScope and liveData {}) it's likely to assume that this will be a recurring problem both in public APIs and normal usage.

Notably the following two things are true:

1. Since the TestCoroutineDispatcher ensures that the coroutine completes prior to the test, it is known that the exception happens during test execution.
2. Uncaught exceptions in a thread are silently logged by this JUnit4 configuration.

I'm starting to lean towards installing a Thread.setDefaultUncaughtExceptionHandler inside of runBlockingTest when executed on the JVM.

Pros:

• Doesn't introduce a CoroutineScope.globalUncaughtException handler (I'm hesitant about adding another global without a clear well understood use case that can't be solved by Thread's uncaught exception handler)
• Meets developer expectations of what happens when an exception is thrown during test execution.
• No dependency on threading CoroutineContext correctly through tests and code under test to get the correct behavior

Cons:

• Tests may not run in parallel
• This won't solve the async {} and never await issue, though that may be considered a unsupported use case.

Looking at prior work (e.g. ReactiveX/RxJava#5234), this sort of global is a common solution to async code + tests + exceptions on the JVM.

This code would look very similar to the TestCoroutineUncaughtExceptionHandler, but only be installed on the JVM via expect/actual and used inside of runBlockingTest. Should this be easy to disable for a specific invocation of runBlockingTest that prefers to run in parallel?

@Test
fun testWithExceptions() = runBlockingTest {
    // installed by default on the JVM
}

@Test
fun testWithoutExceptions_canBeParallelExecuted() = runBlockingTest(handleThreadExceptions=false) {
   // not installed on the JVM
}

Thought: I do not think it's a good idea to expose this as a public interface since it's not coroutine-specific (and easily recreated). Developers using TestCoroutineDispatcher or TestCoroutineScope to manually manage coroutines without runBlockingTest should also install their own appropriate solution.

Alternative: Provide a public interface to give a way to do this without runBlockingTest, it would look something like this:

@Test
fun testFoo() {
    rethrowAllUncaughtExceptions {
        // do stuff with TCD/TCS
    }
}

[ZacSweers]

I'm a little unfamiliar with coroutines internals, but after eyeballing https://github.com/Kotlin/kotlinx.coroutines/blob/69c26dfbcefc24c66aadc586f9b5129894516bee/kotlinx-coroutines-core/jvm/src/CoroutineExceptionHandlerImpl.kt I think the RxJava solutions in the linked thread would work here too. Hoping I'm understanding the thread above is about uncaught exceptions silently failing, and not race threading race conditions where the test is finishing before the async work does (forgive me if it is!).

The problem here is with JUnit, not any of the solutions itself (though maybe there's cancellation exceptions being bubbled up that are inherent to doing concurrency in tests, I unfortunately wasn't sure if that was happening in the examples above). JUnit's default exception handler will silently accept and disregard exceptions, which is why the test doesn't fail.

There's a few solutions

Test rule

You could install a test rule that installs a global errors hook on test start and removes it on test end. It would record unhandled errors and, in successful tests, check to make sure they're empty.

At Uber we installed an "RxErrorsRule" into the base test class that every test in the Android monorepo used, and it worked extremely well. There was no opt-in or opt-out, just flat enabled for everyone. There was an API on the rule to "take" expected exceptions during the test.

This relies on having the ability to install a global errors hook (not sure if coroutines has a global solution like this)

Example implementation: https://github.com/uber/AutoDispose/blob/master/test-utils/src/main/java/com/uber/autodispose/test/RxErrorsRule.java

Global run listener

This doesn't work in gradle currently (or at least I've been woefully unsuccessful), but JUnit has a higher level API for "run listeners" where you could install a global exception handler. OkHttp had an implementation of this prior to its move to gradle: https://github.com/square/okhttp/blob/9042a2c2436c7acfd384f2726a5c1a84ae1145f8/okhttp-testing-support/src/main/java/okhttp3/testing/InstallUncaughtExceptionHandlerListener.java

I tried making it work in gradle here but never got much progress https://github.com/ZacSweers/okhttp/tree/z/testingJunitFoundation

Localized

Basically @objcode's example above:

@Test
fun testFoo() {
    rethrowAllUncaughtExceptions {
        // do stuff with TCD/TCS
    }
}

Is opt-in, creates a custom uncaught handler for the scope of that execution.

---

I'd recommend the base test class + rule approach for maximum flexibility (having an API to expect exceptions), and frankly feasibility since the runlistener approach wasn't working in gradle.

fwiw - I think the blanket global handler is a cleaner solution since it doesn't have an API. Having an API to expect them sometimes led to tests misusing it to assert implementation details/control flow rather than just use it sparingly for unavoidable cases. I had started trying to do a general case via adapting the okhttp runlistener into a general test rule at Uber before I left, but never finished. Kinda of the opinion that JUnit should do that in general, but 🤷‍♂.

[julioyg]

Yeah, thanks, I guess then it's nothing to do with coroutines but the way JUnit works, thanks for that.

I believe for my use case I could create that rule to install setDefaultUncaughtExceptionHandler before tests when we need it, as I'm not using async in my code, but I guess we will only do that if we expect that code to error, my concern then is: what if we don't expect it to error?

Don't really know what the best option would it be tho... Dispatchers.setMain() (I know that's part of the coroutines lib) seems to do the job to override the dispatcher... maybe the option of a CoroutineScope.globalUncaughtException is the best idea? 🤷‍♂

[manuelvicnt]

Thread.setDefaultUncaughtExceptionHandler doesn't do anything when installed. You can print the exception in a nicer way if you want but the test will silently pass anyway.

Re CoroutineScope.globalUncaughtException, there will be a lot of thoughts to put into this if it becomes a thing such as: how global is the globalUncaughtException, is this only for tests?, etc.

I like the idea of combining the Global run listener into a Rule as @ZacSweers suggested but seems like it's not an easy task? What's the issue with Gradle?

[ZakTaccardi]

I don't use viewModelScope, but I believe it will not be a child scope of the scope emitted by runBlockingTest { }, which creates this problem?

I avoid using ViewModelScope directly and allow injection of my own CoroutineScope into every ViewModel of mine to avoid issues like this.

[audkar]

Why ViewModel uses SupervisorJob instead of Job here?

[yigit]

@audkar , we debated a lot about whether to use a supervisor or not, at then end, decided to go w/ it because VM cannot destroy itself so in the case where VM is still alive but the scope is cancelled, it would be a very confusing inconsistent situation.
also, developer is likely to launch unrelated stuff in that scope, just because one is cancelled usually does not mean desire to cancel the other one.

@ZakTaccardi VM uses the Main dispatcher so i think it would work fine as long as you swap the main dispatcher w/ the test dispatcher. I've not tried though.