kotlinx.coroutines: TestCoroutineDispatcher swallows exceptions
@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:
-
Silent exceptions If the coroutine inside a function throws an exception, the test passes and the exception is thrown silently to the console.
-
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)
}
}
About this issue
- Original URL
- State: closed
- Created 5 years ago
- Reactions: 39
- Comments: 41 (6 by maintainers)
Commits related to this issue
- Attempt to report uncaught exceptions in the test module When when a `Job` doesn't have a parent, failures in it can not be reported via structured concurrency. Instead, the mechanism of unhandled ex... — committed to Kotlin/kotlinx.coroutines by dkhalanskyjb 2 years ago
- Attempt to report uncaught exceptions in the test module (#3449) When when a `Job` doesn't have a parent, failures in it can not be reported via structured concurrency. Instead, the mechanism of un... — committed to Kotlin/kotlinx.coroutines by dkhalanskyjb a year ago
- Support disabling reporting of uncaught exceptions in tests The solution for #1205 may be undesirable for those who already have their own solution, like setting the default exception handlers. In th... — committed to Kotlin/kotlinx.coroutines by dkhalanskyjb a year ago
- Support disabling reporting of uncaught exceptions in tests (#3736) The solution for #1205 may be undesirable for those who already have their own solution, like setting the default exception handl... — committed to Kotlin/kotlinx.coroutines by dkhalanskyjb a year ago
- Restore behavior of tests passing despite exceptions thrown This is a hacky workaround for the change in kotlinx.coroutines 1.7.0 that causes tests that throw exceptions to fail. Previously test meth... — committed to woocommerce/woocommerce-android by samiuelson 7 months ago
- Restore behavior of tests passing despite exceptions thrown This is a hacky workaround for the change in kotlinx.coroutines 1.7.0 that causes tests that throw exceptions to fail. Previously test meth... — committed to woocommerce/woocommerce-android by samiuelson 7 months ago
I see some discontent in this discussion and so feel the need to clarify: in general, this problem is not with our library. There may be some specific cases where we could improve the situation (and we want to do so), which is why the issue is still not closed, but in general, this is certainly not our responsibility.
For example, https://github.com/Kotlin/kotlinx.coroutines/issues/1205#issuecomment-1209788685 here, our library is not used at all, but the issue is still present. Here’s a discussion of the same issue for JUnit in general: https://stackoverflow.com/questions/36648317/how-to-capture-all-uncaucht-exceptions-on-junit-tests
When JUnit runs methods marked with
@Test, it guarantees that the test will crash if the code inside the test body crashes. However, when you askGlobalScopeto execute some code, that code does not execute in the test body. Instead, the code is just being run on some other thread and doesn’t know how to notify anyone about the exception. So, this thing is called: https://docs.oracle.com/javase/7/docs/api/java/lang/Thread.UncaughtExceptionHandler.html, but by default (Java’s, not ours), it just logs the exception instead of crashing the program.Not true. In the listed cases,
runTestdoes not lose exceptions, it just does not go out of its way to grab every exception flying through the system. So, Java’s default behavior of logging the exception is what happens instead.The solution that @mhernand40 very helpfully linked us to in https://github.com/Kotlin/kotlinx.coroutines/issues/1205#issuecomment-1232015300 is more robust: even if exceptions happen after the test already finished, they will still be reported, which is not the case with
runReliableTest. However, if you eitherUncaughtExceptionHandlerthat other tests use,this still can be improved a bit:
This has the advantage that, if several exceptions happen, all of them get reported, and also, the exception handler gets reset after the test; without that, the uncaught exceptions from subsequent tests that don’t use this function will not even be logged.
I should note that providing
runReliableTestinstead ofrunTestby default is certainly not something we should do. For example, look here: https://grep.app/search?q=setDefaultUncaughtExceptionHandler A lot of people set the default uncaught exception handler to something else that makes sense for their use case. Silently overriding something that they rely on, in a coroutine test framework, just because JUnit does not do it, is out of the question.I see a way right now to improve this experience, and that is to detect from the core coroutine library that the test module is used, and in this case, collect all the uncaught exceptions in addition to reporting them to the default uncaught exception handler, and then, when running tests, check if some exceptions were collected. This would automagically make the majority of tests shown here catch problems as intended, I think, but this needs research and careful testing (testing a test framework is also good entertainment, highly recommend).
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.
This was causing us issues for a long time and today I’ve decided to fix it.
Observation: Whenever an exception is thrown within
viewModelScope.launchduring a test, the exception is intercepted, logged as an error, but does not fail the test. The test either fails at some point later due to an inconsistent state or in worse cases it even passes the test. In our case, this mainly pertains to exceptions caused by incorrect test logic, such asio.mockk.MockKException: no answer found for: Xxx, etc.From my brief testing, this is not a fault of
TestCoroutineDispatcher.runBlockingTestas the same behavior can be observed withrunBlockingTest(the one without receiver),runBlocking(that does not automatically progress time), or without any blocking whatsoever (withMainorMain.immediatedispatcher).Also, the implementation of
viewModelScopeis not at fault. This can be replicated even without the use ofViewModelat all. Running withGlobalScopeor customCoroutineScope(context)will exhibit the same behavior.Also, it does not matter whether
Job()orSupervisorJob()is used as the context of the coroutine scope.Also, it does not matter what dispatcher is used. For
MainandMain.immediatedispatchers, I did useCoroutinesTestRule(). For non-Maindispatchers, I used some form ofrunBlockinganddelay(1000)to make sure the coroutine finishes before the test finishes.Now, the very minimal reproducible example would look like
Executing the test, you can see the stacktrace in the log like
but the test passes nonetheless.
As mentioned by the great Roman Elizarov here, the same behavior can be observer when using plain threads:
prints
but also passes the test.
Attempting to
job.join()the job does not fail the test either.I was not lucky using a
CoroutineExceptionHandlereither.Solution 1
As mentioned in the same answer
Indeed, running
will fail the test with the expected exception, but now we have another problem - how to propagate the
Defferedback to test so that we canawait()it. This can be quite intrusive in an existing project and I don’t even know how to solve this issue if we launch a coroutine in a constructor of a class that does not have a return value.Solution 2
We can install a
DefaultUncaughtExceptionHandler.This will crash fail the test. Can be extracted into a
Rulewithout the need to modify existing code. Exception stacktrace reports a wrapped exception though and can change the expected result of other, well-behaved tests. This is a similar approach as employed inokhttp3.testing.InstallUncaughtExceptionHandlerListener.Solution 3
Although we cannot fail the test directly from a
CoroutineExceptionHandler, we can keep the exception around and assert that no exception was thrown in our coroutine scope.Requires to pass a
handleras a context to the top-levellaunch. Reports the expected exception only, no wrapper exception. A somewhat similar approach is used inautodispose2.test.RxErrorsRule.Solution 4
We can achieve the same thing as in solution 3 in a much cleaner way using experimental API
Solution 5
Similarly to solution 4, we can use an experimental
TestCoroutineScope. In addition toTestCoroutineExceptionHandler, theTestCoroutineScopealso usesTestCoroutineDispatcherunder the hood - the same one provided byCoroutinesTestRule.(@dkhalanskyjb is on the vacation right now, so expect delayed responses here)
In case it’s helpful, here’s the helper method that we wrote to replace
runTestwithrunReliableTest:Thanks @dkhalanskyjb for the explanations
I was confused about this:
These exceptions (in
viewModelScope/GlobalScope/someOtherScope) would otherwise crash during regular execution - what is it about not inheriting fromTestScopethat’s preventing them from “crashing” the unit tests?Is this a larger problem where if one is using some 3rd party library with
thirdPartyScope, then unit tests can’t ordinarily catch cases where exceptions get thrown inthirdPartyScope? If so, and if one is doing something withCoroutineExceptionHandlerto get exceptions to fail tests (or some other strategy), then it would have to be the norm for all unit tests using athirdPartyScope- doesn’t seem trivial.I don’t use
viewModelScope, but I believe it will not be a child scope of the scope emitted byrunBlockingTest { }, which creates this problem?I avoid using
ViewModelScopedirectly and allow injection of my ownCoroutineScopeinto everyViewModelof mine to avoid issues like this.I’ve not thought about the whole problem but there is a flaw in that test code.
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.
FWIW, there are still plenty of other areas outside of Coroutines where unhandled exceptions can be thrown and the unit test still “passes” even though errors may be logged to the console. I worked around this by applying the suggestion from this article and it’s working great. It surfaced a decent number of unit tests that were “passing” but throwing unhandled exceptions.
@drinkthestars That’s a good way to put it:
Can we ask for some clarification on @drinkthestars’s point? A basic view model test has become rather complex and our team is wondering how we can have reliable tests without writing all our view models to inject a scope.
The approaches here seem out of date with Google’s documentation because, if I’m understanding correctly, the approach here is that shouldn’t use
viewModelScopedirectly to have reliable tests. However, Google’s docs state we should useDispatchers.setMainto overrideviewModelScope- https://developer.android.com/kotlin/coroutines/test#setting-main-dispatcherTo put it simply by using
runTest- exceptions are swallowed where they otherwise would not be. CanrunTestnot introduce the swallowing of exceptions?In any case, this wouldn’t help the (for example, third-party) code that hardcodes using
viewModelScope, even with the name-shadowing trick, because this would not go through a dynamic dispatch, and instead will statically resolve to the extension value.Also, even if you control all the code, whether or not the solution is well-structured depends on which
coroutineContextyou pass: to do this properly, you would need to establish aJobhierarchy between the passed context andTestScope. Also, you would need to be careful to pass aSupervisorJob, as otherwise, you would change the behavior ofviewModelScope.A much easier approach would not use structured concurrency and only pass the
coroutineContext[CoroutineExceptionHandler]!!as thecoroutineContextin your examples. This would lead to the exceptions being reported.@ephemient, not quite.
Dispatchers.Main.immediateis aTestDispatcher, it just doesn’t have the correct properties. Fixing #3298 would fix this particular example, but not every case. So, the issue is still present, that’s why we didn’t close this one.In general, the reproducer looks like this:
It has several aspects:
jointhe newly-created coroutines, or inject the test dispatcher to ensure their completion.The problem is that the exceptions are reported via the structured concurrency mechanism: coroutines that have a chain of inheritance with the
TestScopedo have their exceptions reported, and those that don’t, don’t. So,GlobalScope.launch { throw RuntimeException("...") }, for example, will not be caught by us.Passing the
CoroutineExceptionHandlerto the coroutine looks ugly, but it helps:Some of the solutions listed by @Antimonit still hold up:
Solution 1:
Solution 2 (JVM only):
Solution 3 is also fine, but in the presence of the guaranteed
CoroutineExceptionHandlerinrunTest, it should be easier to just access that.The recommended way when possible is to use structured concurrency, that is, to ensure that the scopes you create are children of
TestScope. It doesn’t look like it’s possible withviewModelScope, however.Seeing this happen with
1.6.0andrunTestas well:Should we be creating
ExceptionHandlers somewhere to mitigate this?Edit: let me know if this should be created as part of a separate issue since it is
runTest.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?