TestCoroutineDispatcher swallows exceptions #1205
Comments
|
cc @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? |
|
Two options here:
|
|
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. |
|
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. |
|
Agreed - this should definitely cause a test (or at least the test runner from a global uncaught exception) to fault via some mechanism. |
|
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 |
|
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:
I'm starting to lean towards installing a Pros:
Cons:
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 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 Alternative: Provide a public interface to give a way to do this without |
|
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 ruleYou 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 listenerThis 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 LocalizedBasically @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 🤷♂. |
|
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 Don't really know what the best option would it be tho... |
|
Re 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? |
|
I don't use I avoid using |
|
Why |
|
@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. @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. |
Sorry but I don't follow. In what case this would happen if
Hmmm. Developers who uses coroutines should know how scopes works. Silently catching exceptions isn't expected behavior IMO. And this ticket itself shows that it is not expected behavior. |
|
not sure what you mean by that, we do not catch exceptions silently. If you run the code above, app will immediately crash on a real exception vs a cancelling coroutine will not crash the app as you can cancel a coroutine maybe to just start another work because it is not valid anymore. |
|
So what we get here? |
|
+1 to the bug found by @julioyg and @manuelvicnt, seeing similar behaviour here. Why is the exception logged but not re-thrown to fail the test? |
|
Just an FYI - the |
|
Will application crash if you execute such code on JVM (not Android)? I don't think that JVM Application will crash. Only error message will be printed out. https://pl.kotl.in/G8VIg_Ys4 |
|
why does
Does the coroutines library come with a default exception handler when run in the JVM? |
|
is this the final way to write a test case, for error case?
|
Hi @yigit I did a very quick test here adding this to a helloWorld-viewModel: And used this With that in mind, I wonder if it would be a valid approach for the AndroidX / ViewModel team to release a |
|
Sorry, people! I have not found |
|
This was causing us issues for a long time and today I've decided to fix it. Observation: From my brief testing, this is not a fault of Also, the implementation of Also, it does not matter whether Also, it does not matter what dispatcher is used. For Now, the very minimal reproducible example would look like class CoroutineExceptionTest {
@Test
fun checkScope() {
val scope = CoroutineScope(Dispatchers.Unconfined)
scope.launch {
throw Exception("Oh noes")
}
}
}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: @Test
fun checkThread() {
val job = thread {
throw IllegalStateException("why does this exception not fail the test?")
}
job.join()
}prints but also passes the test. Attempting to I was not lucky using a Solution 1As mentioned in the same answer
Indeed, running @Test
fun checkAsync() {
val scope = CoroutineScope(Dispatchers.Unconfined)
val deferred = scope.async {
throw Exception("Oh noes")
}
runBlocking {
deferred.await()
}
}will fail the test with the expected exception, but now we have another problem - how to propagate the Solution 2We can install a @Test
fun checkDefaultUncaughtExceptionHandler() {
// TODO: Create a Rule and reset the DefaultUncaughtExceptionHandler
// to the original value so that it does not leak to other tests.
Thread.setDefaultUncaughtExceptionHandler { thread, throwable ->
throw throwable
}
val scope = CoroutineScope(Dispatchers.Unconfined)
scope.launch {
throw Exception("Oh noes")
}
}This will crash fail the test. Can be extracted into a Solution 3Although we cannot fail the test directly from a @Test
fun checkCoroutineExceptionHandler() {
var failed: Throwable? = null
val handler = CoroutineExceptionHandler { _, throwable ->
failed = throwable
throw throwable
}
val scope = CoroutineScope(Dispatchers.Unconfined)
scope.launch(handler) {
throw Exception("Oh noes")
}
failed?.let { throw it }
}Requires to pass a Solution 4We can achieve the same thing as in solution 3 in a much cleaner way using experimental API @Test
fun checkTestCoroutineExceptionHandler() {
val testHandler = TestCoroutineExceptionHandler()
val scope = CoroutineScope(Dispatchers.Unconfined)
scope.launch(testHandler) {
throw Exception("Oh noes")
}
testHandler.cleanupTestCoroutines()
}Solution 5Similarly to solution 4, we can use an experimental @Test
fun checkTestCoroutineScope() {
val scope = TestCoroutineScope()
scope.launch {
throw Exception("Oh noes")
}
scope.cleanupTestCoroutines()
} |
|
If you want to just use a JUnit 4 test rule or JUnit 5 test extension in your tests, you can find it in this gist |
|
@Antimonit thanks for this response, it's really helpful However, I'm experiencing an issue with Solution 2, I'm not quite sure if it's something related to coroutine's mechanics or a straight up bug. Running exact same snippet as provided by You, leaves me with I can work around it by changing the code to something like that: Is this an expected behaviour? My Kotlin/coroutines versions: |
|
Seeing this happen with class FooViewModelTest {
@get:Rule
var coroutinesTestRule = CoroutinesTestRule() // StandardTestDispatcher
@Test
fun shouldFailButDoesnt() = runTest {
val viewModel = FooViewModel()
viewModel.throwException()
runCurrent()
}
}
// ViewModel
class FooViewModel(): ViewModel() {
fun throwException() {
viewModelScope.launch {
throw Exception("Oops!")
}
}
}Should we be creating Edit: let me know if this should be created as part of a separate issue since it is |
|
|
|
@ephemient, not quite. In general, the reproducer looks like this: @Test
fun unreportedException() = runTest {
val scope = CoroutineScope(EmptyCoroutineContext)
scope.launch {
throw RuntimeException()
}
}It has several aspects:
The problem is that the exceptions are reported via the structured concurrency mechanism: coroutines that have a chain of inheritance with the Passing the @Test
fun reportedException() = runTest {
val scope = CoroutineScope(coroutineContext[CoroutineExceptionHandler]!!)
val job = scope.launch {
throw RuntimeException()
}
job.join()
}Some of the solutions listed by @Antimonit still hold up: Solution 1: @Test
fun reportedException() = runTest {
val scope = CoroutineScope(EmptyCoroutineContext)
val deferred = scope.async {
throw RuntimeException()
}
deferred.await()
}Solution 2 (JVM only): @Test
fun reportedException() = runTest {
Thread.setDefaultUncaughtExceptionHandler { thread, throwable ->
throw throwable
} // can be extracted into a rule
val scope = CoroutineScope(StandardTestDispatcher(testScheduler))
scope.launch {
throw RuntimeException()
}
}Solution 3 is also fine, but in the presence of the guaranteed The recommended way when possible is to use structured concurrency, that is, to ensure that the scopes you create are children of |
Wouldn't the solution be to add And then in test: |
|
If you're going down that path, I'd rather something like import viewModelScope as viewModelScope_
class MyViewModel(
viewModelScope: CoroutineScope? = null,
) {
val viewModelScope = viewModelScope ?: viewModelScope_so that you can't accidentally use the wrong |
|
In any case, this wouldn't help the (for example, third-party) code that hardcodes using Also, even if you control all the code, whether or not the solution is well-structured depends on which A much easier approach would not use structured concurrency and only pass the |
|
Thanks @dkhalanskyjb for the explanations I was confused about this:
These exceptions (in Is this a larger problem where if one is using some 3rd party library with |
|
@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 To put it simply by using |
|
In case it's helpful, here's the helper method that we wrote to replace import androidx.lifecycle.ViewModel
import kotlinx.coroutines.test.TestScope
import kotlinx.coroutines.test.runTest
import kotlin.coroutines.CoroutineContext
import kotlin.coroutines.EmptyCoroutineContext
/**
* The default [runTest] will swallow thrown [Throwable]s that occur within
* [ViewModel].viewModelScope.launch calls. Thus, you may have [Throwable]s being thrown
* and and yet passing unit tests. This helper method works around the issue by listening to the
* Thread's UncaughtExceptionHandler and propagating the exception to fail the unit test.
*
* [This has been a known issue for the last 3 years.](https://github.com/Kotlin/kotlinx.coroutines/issues/1205)
*/
fun runReliableTest(
context: CoroutineContext = EmptyCoroutineContext,
dispatchTimeoutMs: Long = 30_000L,
testBody: suspend TestScope.() -> Unit
) = runTest(context, dispatchTimeoutMs) {
// Capture exceptions that may occur in `testBody()` since `runTest` will swallow
// them if the exception occurs within another scope even though all dispatchers
// should run without any delays - https://github.com/Kotlin/kotlinx.coroutines/issues/1205
var error: Throwable? = null
Thread.setDefaultUncaughtExceptionHandler { _, throwable -> error = throwable }
// Run our actual test code
testBody()
// Throw exception here, otherwise it will be wrapped and hard to read if thrown from
// within `Thread.setDefaultUncaughtExceptionHandler`
error?.let { throw it }
} |
|
(@dkhalanskyjb is on the vacation right now, so expect delayed responses here) |
|
Going back in the thread, came across this example where the exception would be printed but passes the test: @Test
fun checkThread() {
val job = thread {
throw IllegalStateException("why does this exception not fail the test?")
}
job.join()
}Looks like |
|
@drinkthestars Yes, I tested locally and import org.junit.Test
import kotlin.concurrent.thread
class ThreadTest {
@Test
fun checkThread() = runReliableTest {
val job = thread {
throw IllegalStateException("why does this exception not fail the test?")
}
job.join()
}
} |
|
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. |
|
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, #1205 (comment) 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
Not true. In the listed cases,
The solution that @mhernand40 very helpfully linked us to in #1205 (comment) is more robust: even if exceptions happen after the test already finished, they will still be reported, which is not the case with
this still can be improved a bit: /**
* Runs a function, making sure that the exceptions that happen throughout its execution get reported.
*/
fun<T> runReliably(
testBody: () -> T,
): T {
val oldHandler = Thread.getDefaultUncaughtExceptionHandler()
val errors = Vector<Throwable>()
try {
Thread.setDefaultUncaughtExceptionHandler { _, throwable -> errors.add(throwable) }
val result = try {
testBody()
} catch (testError: Throwable) {
for (e in errors) {
testError.addSuppressed(e)
}
throw testError
}
errors.firstOrNull()?.apply {
errors.drop(1).forEach { addSuppressed(it) }
throw this
}
return result
} finally {
Thread.setDefaultUncaughtExceptionHandler(oldHandler)
}
}
/**
* Run a coroutine test, making sure the uncaught exceptions that happen during the execution of the test do get reported.
*/
fun runReliableTest(
context: CoroutineContext = EmptyCoroutineContext,
dispatchTimeoutMs: Long = 30_000L,
testBody: suspend TestScope.() -> Unit
) = runReliably {
runTest(context, dispatchTimeoutMs, testBody)
}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 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). |
When when a `Job` doesn't have a parent, failures in it can not be reported via structured concurrency. Instead, the mechanism of unhandled exceptions is used. If there is a `CoroutineExceptionHandler` in the coroutine context or registered as a `ServiceLoader` service, this gets notified, and otherwise, something platform-specific happens: on Native, the program crashes, and on the JVM, by default, the exception is just logged, though this is configurable via `Thread.setUncaughtExceptionHandler`. With tests on the JVM, this is an issue: we want exceptions not simply *logged*, we want them to fail the test, and this extends beyond just coroutines. However, JUnit does not override the uncaught exception handler, and uncaught exceptions do just get logged: <https://stackoverflow.com/questions/36648317/how-to-capture-all-uncaucht-exceptions-on-junit-tests> This is a problem with a widely-used `viewModelScope` on Android. This is a scope without a parent, and so the exceptions in it are uncaught. On Android, such uncaught exceptions crash the app by default, but in tests, they just get logged. Clearly, without overriding the behavior of uncaught exceptions, the tests are lacking. This can be solved on the test writers' side via `setUncaughtExceptionHandler`, but one has to remember to do that. In this commit, we attempt to solve this issue for the overwhelming majority of users. To that end, when the test framework is used, we collect the uncaught exceptions and report them at the end of a test. This approach is marginally less robust than `setUncaughtExceptionHandler`: if an exception happened after the last test using `kotlinx-coroutines-test`, it won't get reported, for example. `CoroutineExceptionHandler` is designed in such a way that, when it is used in a coroutine context, its presence means that the exceptions are safe in its care and will not be propagated further, but when used as a service, it has no such property. We, however, know that our `CoroutineExceptionHandler` reports the exceptions properly and they don't need to be further logged, and so we had to extend the behavior of mechanism for uncaught exception handling so that it the handler throws a new kind of exception if the exception was processed successfully. Also, because there's no `ServiceLoader` mechanism on JS or Native, we had to refactor the whole uncaught exception handling mechanism a bit in the same vein as we had to adapt the `Main` dispatcher to `Dispatchers.setMain`: by introducing internal setter APIs that services have to call manually to register in. Fixes #1205 as thoroughly as we can given the circumstances.
The solution for #1205 may be undesirable for those who already have their own solution, like setting the default exception handlers. In this case, there's a usability issue without the corresponding benefit: instead of all tests being ran and the exceptions from them being reported, unrelated tests may fail, making looking for problems more difficult. This is probably a very rare issue, so we don't provide public API for that, instead introducing a need-to-know internal variable.
The solution for #1205 may be undesirable for those who already have their own solution, like setting the default exception handlers. In this case, there's a usability issue without the corresponding benefit: instead of all tests being ran and the exceptions from them being reported, unrelated tests may fail, making looking for problems more difficult. This is probably a very rare issue, so we don't provide public API for that, instead introducing a need-to-know internal variable.
This is a hacky workaround for the change in kotlinx.coroutines 1.7.0 that causes tests that throw exceptions to fail. Previously test methods that threw exceptions would not prevent tests from passing, which was a bug in kotlinx.coroutines that has now been fixed. However, significant number of our tests are currently failing because of this change, because we have bugs in the implementation of our test classes, usually related to the way we instantiate the view models in tests. See the following issue for more details: Kotlin/kotlinx.coroutines#1205. The workaround coming with this commit is taken from the related PR: Kotlin/kotlinx.coroutines#3736 and is a solution suggested by JetBrains to disable the new behavior using non-public API until we fix our tests. This should not be considered a long-term solution, rather a temporary hack.
This is a hacky workaround for the change in kotlinx.coroutines 1.7.0 that causes tests that throw exceptions to fail. Previously test methods that threw exceptions would not prevent tests from passing, which was a bug in kotlinx.coroutines that has now been fixed. However, significant number of our tests are currently failing because of this change, because we have bugs in the implementation of our test classes, usually related to the way we instantiate the view models in tests. See the following issue for more details: Kotlin/kotlinx.coroutines#1205. The workaround coming with this commit is taken from the related PR: Kotlin/kotlinx.coroutines#3736 and is a solution suggested by JetBrains to disable the new behavior using non-public API until we fix our tests. This should not be considered a long-term solution, rather a temporary hack.
and others
@julioyg found an interesting bug(?) regarding the TestCoroutineDispatcher.
The fact that
runBlockingTestusesasyncto 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.
The text was updated successfully, but these errors were encountered: