WIP: fix exception handling in the test module

Author: dkhalanskyjb

kotlinx-coroutines-test/api/kotlinx-coroutines-test.api

@@ -34,15 +34,16 @@ public final class kotlinx/coroutines/test/TestCoroutineDispatcher : kotlinx/cor
 	public fun toString ()Ljava/lang/String;
 }
 
-public final class kotlinx/coroutines/test/TestCoroutineExceptionHandler : kotlin/coroutines/AbstractCoroutineContextElement, kotlinx/coroutines/CoroutineExceptionHandler, kotlinx/coroutines/test/UncaughtExceptionCaptor {
+public final class kotlinx/coroutines/test/TestCoroutineExceptionHandler : kotlin/coroutines/AbstractCoroutineContextElement, kotlinx/coroutines/CoroutineExceptionHandler {
 	public fun <init> ()V
-	public fun cleanupTestCoroutines ()V
-	public fun getUncaughtExceptions ()Ljava/util/List;
+	public final fun cleanupTestCoroutines ()V
+	public final fun getUncaughtExceptions ()Ljava/util/List;
 	public fun handleException (Lkotlin/coroutines/CoroutineContext;Ljava/lang/Throwable;)V
 }
 
-public abstract interface class kotlinx/coroutines/test/TestCoroutineScope : kotlinx/coroutines/CoroutineScope, kotlinx/coroutines/test/DelayController, kotlinx/coroutines/test/UncaughtExceptionCaptor {
+public abstract interface class kotlinx/coroutines/test/TestCoroutineScope : kotlinx/coroutines/CoroutineScope, kotlinx/coroutines/test/DelayController {
 	public abstract fun cleanupTestCoroutines ()V
+	public abstract fun getUncaughtExceptions ()Ljava/util/List;
 }
 
 public final class kotlinx/coroutines/test/TestCoroutineScopeKt {

kotlinx-coroutines-test/src/TestBuilders.kt

@@ -43,15 +43,20 @@ import kotlin.coroutines.*
  */
 @ExperimentalCoroutinesApi // Since 1.2.1, tentatively till 1.3.0
 public fun runBlockingTest(context: CoroutineContext = EmptyCoroutineContext, testBody: suspend TestCoroutineScope.() -> Unit) {
-    val (safeContext, dispatcher) = context.checkArguments()
+    val (safeContext, dispatcher) = context.checkTestScopeArguments()
     val startingJobs = safeContext.activeJobs()
     val scope = TestCoroutineScope(safeContext)
-    val deferred = scope.async {
-        scope.testBody()
-    }
-    dispatcher.advanceUntilIdle()
-    deferred.getCompletionExceptionOrNull()?.let {
-        throw it
+    try {
+        val deferred = scope.async {
+            scope.testBody()
+        }
+        dispatcher.advanceUntilIdle()
+        deferred.getCompletionExceptionOrNull()?.let {
+            throw it
+        }
+    } catch (e: CancellationException) {
+        scope.cleanupTestCoroutines()
+        throw e
     }
     scope.cleanupTestCoroutines()
     val endingJobs = safeContext.activeJobs()
@@ -79,20 +84,13 @@ public fun TestCoroutineScope.runBlockingTest(block: suspend TestCoroutineScope.
 public fun TestCoroutineDispatcher.runBlockingTest(block: suspend TestCoroutineScope.() -> Unit): Unit =
     runBlockingTest(this, block)
 
-private fun CoroutineContext.checkArguments(): Pair<CoroutineContext, DelayController> {
+internal fun CoroutineContext.checkTestScopeArguments(): Pair<CoroutineContext, DelayController> {
     // TODO optimize it
     val dispatcher = get(ContinuationInterceptor).run {
         this?.let { require(this is DelayController) { "Dispatcher must implement DelayController: $this" } }
         this ?: TestCoroutineDispatcher()
     }
-
-    val exceptionHandler =  get(CoroutineExceptionHandler).run {
-        this?.let {
-            require(this is UncaughtExceptionCaptor) { "coroutineExceptionHandler must implement UncaughtExceptionCaptor: $this" }
-        }
-        this ?: TestCoroutineExceptionHandler()
-    }
-
-    val job = get(Job) ?: SupervisorJob()
+    val exceptionHandler = get(CoroutineExceptionHandler) ?: TestCoroutineExceptionHandler()
+    val job = get(Job) ?: Job()
     return Pair(this + dispatcher + exceptionHandler + job, dispatcher as DelayController)
 }

kotlinx-coroutines-test/src/TestCoroutineExceptionHandler.kt

@@ -10,7 +10,7 @@ import kotlin.coroutines.*
 /**
  * Access uncaught coroutine exceptions captured during test execution.
  */
-@ExperimentalCoroutinesApi // Since 1.2.1, tentatively till 1.3.0
+@Deprecated("Consider whether a plain `CoroutineExceptionHandler` would suffice. If not, please report your use case at https://github.com/Kotlin/kotlinx.coroutines/issues.", level = DeprecationLevel.ERROR)
 public interface UncaughtExceptionCaptor {
     /**
      * List of uncaught coroutine exceptions.
@@ -33,27 +33,37 @@ public interface UncaughtExceptionCaptor {
 
 /**
  * An exception handler that captures uncaught exceptions in tests.
+ *
+ * In order to work as intended, this exception handler requires that the [Job] of the test coroutine scope is a
+ * [SupervisorJob].
  */
-@ExperimentalCoroutinesApi // Since 1.2.1, tentatively till 1.3.0
+// @Deprecated("In order to work correctly, this exception handler requires that " +
+
+//     "the test coroutine scope's Job is a SupervisorJob, which may lead to tests running indefinitely " +
+//     "instead of quickly crashing. Please consider specifying another `CoroutineExceptionHandler`", level = DeprecationLevel.WARNING)
 public class TestCoroutineExceptionHandler :
-    AbstractCoroutineContextElement(CoroutineExceptionHandler), UncaughtExceptionCaptor, CoroutineExceptionHandler
+    AbstractCoroutineContextElement(CoroutineExceptionHandler), CoroutineExceptionHandler
 {
     private val _exceptions = mutableListOf<Throwable>()
+    private var _coroutinesCleanedUp = false
 
-    /** @suppress **/
+    @Suppress("INVISIBLE_MEMBER")
     override fun handleException(context: CoroutineContext, exception: Throwable) {
         synchronized(_exceptions) {
+            if (_coroutinesCleanedUp) {
+                handleCoroutineExceptionImpl(context, exception)
+                return
+            }
             _exceptions += exception
         }
     }
 
-    /** @suppress **/
-    override val uncaughtExceptions: List<Throwable>
+    public val uncaughtExceptions: List<Throwable>
         get() = synchronized(_exceptions) { _exceptions.toList() }
 
-    /** @suppress **/
-    override fun cleanupTestCoroutines() {
+    public fun cleanupTestCoroutines() {
         synchronized(_exceptions) {
+            _coroutinesCleanedUp = true
             val exception = _exceptions.firstOrNull() ?: return
             // log the rest
             _exceptions.drop(1).forEach { it.printStackTrace() }

kotlinx-coroutines-test/src/TestCoroutineScope.kt

@@ -11,29 +11,56 @@ import kotlin.coroutines.*
  * A scope which provides detailed control over the execution of coroutines for tests.
  */
 @ExperimentalCoroutinesApi // Since 1.2.1, tentatively till 1.3.0
-public interface TestCoroutineScope: CoroutineScope, UncaughtExceptionCaptor, DelayController {
+public interface TestCoroutineScope: CoroutineScope, DelayController {
     /**
-     * Call after the test completes.
-     * Calls [UncaughtExceptionCaptor.cleanupTestCoroutines] and [DelayController.cleanupTestCoroutines].
+     * Is being called after a test completes.
+     *
+     * Calls [DelayController.cleanupTestCoroutines].
+     *
+     * If the [CoroutineExceptionHandler] is a [TestCoroutineExceptionHandler], its
+     * [TestCoroutineExceptionHandler.cleanupTestCoroutines] behavior is performed:
+     * the first exception in [uncaughtExceptions] is rethrown, and all the other exceptions are
+     * printed using [Throwable.printStackTrace].
      *
      * @throws Throwable the first uncaught exception, if there are any uncaught exceptions.
      * @throws UncompletedCoroutinesError if any pending tasks are active, however it will not throw for suspended
      * coroutines.
      */
     public override fun cleanupTestCoroutines()
+
+    /**
+     * List of uncaught coroutine exceptions.
+     *
+     * Exceptions are only collected in this list if [TestCoroutineExceptionHandler] is in the test context and this
+     * scope's job is a [SupervisorJob].
+     * Otherwise, the failure of one child cancels all the others and the scope itself.
+     *
+     * The returned list is a copy of the exceptions caught during execution.
+     * During [cleanupTestCoroutines] the first element of this list is rethrown if it is not empty.
+     */
+    @Deprecated(
+        "This list is only populated if `TestCoroutineExceptionHandler` in the test context, and so can be " +
+            "easily misused. It is only present for backward compatibility and will be removed in the subsequent " +
+            "releases. If you need to check the list of exceptions, please consider  your own" +
+            "`CoroutineExceptionHandler`.",
+        level = DeprecationLevel.WARNING)
+    public val uncaughtExceptions: List<Throwable>
 }
 
 private class TestCoroutineScopeImpl (
     override val coroutineContext: CoroutineContext
 ):
     TestCoroutineScope,
-    UncaughtExceptionCaptor by coroutineContext.uncaughtExceptionCaptor,
     DelayController by coroutineContext.delayController
 {
     override fun cleanupTestCoroutines() {
-        coroutineContext.uncaughtExceptionCaptor.cleanupTestCoroutines()
+        (coroutineContext[CoroutineExceptionHandler] as? TestCoroutineExceptionHandler)?.cleanupTestCoroutines()
         coroutineContext.delayController.cleanupTestCoroutines()
     }
+
+    override val uncaughtExceptions: List<Throwable>
+        get() = (coroutineContext[CoroutineExceptionHandler] as? TestCoroutineExceptionHandler)?.uncaughtExceptions
+            ?: emptyList()
 }
 
 /**
@@ -46,21 +73,8 @@ private class TestCoroutineScopeImpl (
  */
 @Suppress("FunctionName")
 @ExperimentalCoroutinesApi // Since 1.2.1, tentatively till 1.3.0
-public fun TestCoroutineScope(context: CoroutineContext = EmptyCoroutineContext): TestCoroutineScope {
-    var safeContext = context
-    if (context[ContinuationInterceptor] == null) safeContext += TestCoroutineDispatcher()
-    if (context[CoroutineExceptionHandler] == null) safeContext += TestCoroutineExceptionHandler()
-    return TestCoroutineScopeImpl(safeContext)
-}
-
-private inline val CoroutineContext.uncaughtExceptionCaptor: UncaughtExceptionCaptor
-    get() {
-        val handler = this[CoroutineExceptionHandler]
-        return handler as? UncaughtExceptionCaptor ?: throw IllegalArgumentException(
-            "TestCoroutineScope requires a UncaughtExceptionCaptor such as " +
-                "TestCoroutineExceptionHandler as the CoroutineExceptionHandler"
-        )
-    }
+public fun TestCoroutineScope(context: CoroutineContext = EmptyCoroutineContext): TestCoroutineScope =
+    TestCoroutineScopeImpl(context.checkTestScopeArguments().first)
 
 private inline val CoroutineContext.delayController: DelayController
     get() {

kotlinx-coroutines-test/test/TestCoroutineScopeTest.kt

@@ -9,14 +9,6 @@ import org.junit.Test
 import kotlin.test.*
 
 class TestCoroutineScopeTest {
-    @Test
-    fun whenGivenInvalidExceptionHandler_throwsException() {
-        val handler = CoroutineExceptionHandler {  _, _ -> Unit }
-        assertFails {
-            TestCoroutineScope(handler)
-        }
-    }
-
     @Test
     fun whenGivenInvalidDispatcher_throwsException() {
         assertFails {

kotlinx-coroutines-test/test/TestRunBlockingTest.kt

@@ -288,13 +288,6 @@ class TestRunBlockingTest {
         }
     }
 
-    @Test(expected = java.lang.IllegalArgumentException::class)
-    fun runBlockingTestBuilder_throwsOnBadHandler() {
-        runBlockingTest(CoroutineExceptionHandler { _, _ -> Unit} ) {
-
-        }
-    }
-
     @Test
     fun pauseDispatcher_disablesAutoAdvance_forCurrent() = runBlockingTest {
         var mutable = 0
@@ -389,9 +382,4 @@ class TestRunBlockingTest {
     fun testOverrideExceptionHandler() = runBlockingTest(exceptionHandler) {
         assertSame(coroutineContext[CoroutineExceptionHandler], exceptionHandler)
     }
-
-    @Test(expected = IllegalArgumentException::class)
-    fun testOverrideExceptionHandlerError() = runBlockingTest(CoroutineExceptionHandler { _, _ -> }) {
-        fail("Unreached")
-    }
 }