Better handle the exceptions from child coroutines in runTest
#2995
Conversation
|
Should it be fixed that way
I actually did spend quite some time attempting to eliminate
`SupervisorJob`, but in the end, decided against it.
Consider this code:
```kotlin
fun testFoo() = runTest {
launch {
throw TestException()
}
delay(10)
}
```
If we use a plain `Job`, `runTest` will fail with
`CancellationException`. Normally, it would be expected, but here, the
whole test will fail with that exception, which works against
`runTest` not getting in the way.
We could special-case this by treating `CancellationException`
differently, deprioritizing it in the test output. However, also
consider this:
```kotlin
fun testBar() = runTest {
var f = false
launch {
delay(200)
if (f != true)
throw TestException()
}
withTimeout(100) {
delay(1000)
f = true
}
}
```
Here, we would be deprioritizing `TimeoutCancellationException`,
which, in my opinion, is incorrect.
We could special-case *that*… but then a question arises: do we want
to consider cancellation exceptions other than
`TimeoutCancellationException` safe? Is `TimeoutCancellationException`
a very special thing, the only cancellation exception that's not just
a support mechanism for structured concurrency? If so, it's a big
conceptual shift that affects the project as a whole. If not, then why
the special-casing?
Instead, we can avoid the whole question by using the following
intuition: failures in the test body take priority over failures in
the spawned coroutines. This agrees with using `SupervisorJob`.
|
dkhalanskyjb
force-pushed
the
coroutines-test-exception-handling
branch
from
d70d5e2
to
d651dec
Compare
dkhalanskyjb
force-pushed
the
coroutines-test-cancel-on-failure
branch
from
ae524a7
to
35aa581
Compare
| } catch (e: UncompletedCoroutinesError) { | ||
| // it's normal that some jobs are not completed if the test body has failed, won't clutter the output | ||
| } catch (e: Throwable) { | ||
| e.printStackTrace() |
There was a problem hiding this comment.
Is it a leftover or deliberate code?
There was a problem hiding this comment.
A tricky question! I deliberately left it as is so we could discuss it.
cleanupTestCoroutines, too, prints a bunch of exceptions to the console if there's more than one, only throwing a single one. It would be consistent here to print the only thrown exception, too, if it turns out that there's a more important exception (the one with which the testScope completed).
If I understood correctly, you advise to, instead, get the exception with which testScope completed, add a suppressed exception from the cleanup, and then just throw it, right? But then, for consistency, we would also need the other exceptions from cleanupTestCoroutines to be suppressed and not printed, right? This would somewhat complicate the API, and I'm not sure what would the benefits be, other than the ability to better test this test module (validating printed exceptions is a pain).
There was a problem hiding this comment.
The main benefits here are programmatic access for any test frameworks -- exception aggregation, logging on test failures on CI and so on.
Printing the error straight to the console seems neither controllable nor usable.
dkhalanskyjb
changed the title
runTest
dkhalanskyjb
changed the title
runTestrunTest
dkhalanskyjb
force-pushed
the
coroutines-test-exception-handling
branch
from
d651dec
to
f158e36
Compare
dkhalanskyjb
force-pushed
the
coroutines-test-cancel-on-failure
branch
from
f184e40
to
eee46c5
Compare
| @@ -169,16 +179,16 @@ public fun runTest( | |||
| ): TestResult { | |||
| if (context[RunningInRunTest] != null) | |||
| throw IllegalStateException("Calls to `runTest` can't be nested. Please read the docs on `TestResult` for details.") | |||
| val testScope = createTestCoroutineScope(context + RunningInRunTest()) | |||
| val testScope = TestBodyCoroutine<Unit>(createTestCoroutineScope(context + RunningInRunTest())) | |||
There was a problem hiding this comment.
[nit] RunningInRunTest probably can be object with prettified toString
| } catch (e: UncompletedCoroutinesError) { | ||
| // it's normal that some jobs are not completed if the test body has failed, won't clutter the output | ||
| } catch (e: Throwable) { | ||
| e.printStackTrace() |
There was a problem hiding this comment.
The main benefits here are programmatic access for any test frameworks -- exception aggregation, logging on test failures on CI and so on.
Printing the error straight to the console seems neither controllable nor usable.
dkhalanskyjb
force-pushed
the
coroutines-test-cancel-on-failure
branch
from
eee46c5
to
48bf565
Compare
dkhalanskyjb
changed the base branch from
coroutines-test-exception-handling
to
coroutines-test-await-other-dispatchers
Fixes #1910