Fix js tests (#2824)

* Properly detect asynchronous tests that try to use `runTest` multiple times

* Refactor multi-shot tests

    * Wrap them in 'withContext(Job)' instead of 'runTest' to be properly working on JS
    * Skip few way too slow for JS tests only on JS platform

Fixes #2820

Author: qwwdfsad

kotlinx-coroutines-core/common/test/CancelledParentAttachTest.kt

@@ -11,75 +11,105 @@ import kotlin.test.*
 class CancelledParentAttachTest : TestBase() {
 
     @Test
-    fun testAsync() = CoroutineStart.values().forEach(::testAsyncCancelledParent)
+    fun testAsync() = runTest {
+        CoroutineStart.values().forEach { testAsyncCancelledParent(it) }
+    }
 
-    private fun testAsyncCancelledParent(start: CoroutineStart) =
-        runTest({ it is CancellationException }) {
-            cancel()
-            expect(1)
-            val d = async<Int>(start = start) { 42 }
-            expect(2)
-            d.invokeOnCompletion {
-                finish(3)
-                reset()
+    private suspend fun testAsyncCancelledParent(start: CoroutineStart) {
+        try {
+            withContext(Job()) {
+                cancel()
+                expect(1)
+                val d = async<Int>(start = start) { 42 }
+                expect(2)
+                d.invokeOnCompletion {
+                    finish(3)
+                    reset()
+                }
             }
+            expectUnreached()
+        } catch (e: CancellationException) {
+            // Expected
         }
+    }
 
     @Test
-    fun testLaunch() = CoroutineStart.values().forEach(::testLaunchCancelledParent)
+    fun testLaunch() = runTest {
+        CoroutineStart.values().forEach { testLaunchCancelledParent(it) }
+    }
 
-    private fun testLaunchCancelledParent(start: CoroutineStart) =
-        runTest({ it is CancellationException }) {
-            cancel()
-            expect(1)
-            val d = launch(start = start) { }
-            expect(2)
-            d.invokeOnCompletion {
-                finish(3)
-                reset()
+    private suspend fun testLaunchCancelledParent(start: CoroutineStart) {
+        try {
+            withContext(Job()) {
+                cancel()
+                expect(1)
+                val d = launch(start = start) { }
+                expect(2)
+                d.invokeOnCompletion {
+                    finish(3)
+                    reset()
+                }
             }
+            expectUnreached()
+        } catch (e: CancellationException) {
+            // Expected
         }
+    }
 
     @Test
-    fun testProduce() =
-        runTest({ it is CancellationException }) {
-            cancel()
-            expect(1)
-            val d = produce<Int> { }
-            expect(2)
-            (d as Job).invokeOnCompletion {
-                finish(3)
-                reset()
-            }
+    fun testProduce() = runTest({ it is CancellationException }) {
+        cancel()
+        expect(1)
+        val d = produce<Int> { }
+        expect(2)
+        (d as Job).invokeOnCompletion {
+            finish(3)
+            reset()
         }
+    }
 
     @Test
-    fun testBroadcast() = CoroutineStart.values().forEach(::testBroadcastCancelledParent)
+    fun testBroadcast() = runTest {
+        CoroutineStart.values().forEach { testBroadcastCancelledParent(it) }
+    }
 
-    private fun testBroadcastCancelledParent(start: CoroutineStart) =
-        runTest({ it is CancellationException }) {
-            cancel()
-            expect(1)
-            val bc = broadcast<Int>(start = start) {}
-            expect(2)
-            (bc as Job).invokeOnCompletion {
-                finish(3)
-                reset()
+    private suspend fun testBroadcastCancelledParent(start: CoroutineStart) {
+        try {
+            withContext(Job()) {
+                cancel()
+                expect(1)
+                val bc = broadcast<Int>(start = start) {}
+                expect(2)
+                (bc as Job).invokeOnCompletion {
+                    finish(3)
+                    reset()
+                }
             }
+            expectUnreached()
+        } catch (e: CancellationException) {
+            // Expected
         }
+    }
 
     @Test
-    fun testScopes() {
-        testScope { coroutineScope {  } }
-        testScope { supervisorScope {  } }
-        testScope { flowScope {  } }
-        testScope { withTimeout(Long.MAX_VALUE) {  } }
-        testScope { withContext(Job()) {  } }
-        testScope { withContext(CoroutineName("")) {  } }
+    fun testScopes() = runTest {
+        testScope { coroutineScope { } }
+        testScope { supervisorScope { } }
+        testScope { flowScope { } }
+        testScope { withTimeout(Long.MAX_VALUE) { } }
+        testScope { withContext(Job()) { } }
+        testScope { withContext(CoroutineName("")) { } }
     }
 
-    private inline fun testScope(crossinline block: suspend () -> Unit) = runTest({ it is CancellationException }) {
-        cancel()
-        block()
+    private suspend inline fun testScope(crossinline block: suspend () -> Unit) {
+        try {
+            withContext(Job()) {
+                cancel()
+                block()
+            }
+            expectUnreached()
+        } catch (e: CancellationException) {
+            // Expected
+        }
     }
 }

kotlinx-coroutines-core/common/test/TestBase.common.kt

@@ -14,6 +14,14 @@ public expect val isStressTest: Boolean
 public expect val stressTestMultiplier: Int
 
 public expect open class TestBase constructor() {
+    /*
+     * In common tests we emulate parameterized tests
+     * by iterating over parameters space in the single @Test method.
+     * This kind of tests is too slow for JS and does not fit into
+     * the default Mocha timeout, so we're using this flag to bail-out
+     * and run such tests only on JVM and K/N.
+     */
+    public val isBoundByJsTestTimeout: Boolean
     public fun error(message: Any, cause: Throwable? = null): Nothing
     public fun expect(index: Int)
     public fun expectUnreached()

kotlinx-coroutines-core/common/test/channels/ChannelUndeliveredElementTest.kt

@@ -10,17 +10,19 @@ import kotlin.test.*
 
 class ChannelUndeliveredElementTest : TestBase() {
     @Test
-    fun testSendSuccessfully() = runAllKindsTest { kind ->
-        val channel = kind.create<Resource> { it.cancel() }
-        val res = Resource("OK")
-        launch {
-            channel.send(res)
+    fun testSendSuccessfully() = runTest {
+        runAllKindsTest { kind ->
+            val channel = kind.create<Resource> { it.cancel() }
+            val res = Resource("OK")
+            launch {
+                channel.send(res)
+            }
+            val ok = channel.receive()
+            assertEquals("OK", ok.value)
+            assertFalse(res.isCancelled) // was not cancelled
+            channel.close()
+            assertFalse(res.isCancelled) // still was not cancelled
         }
-        val ok = channel.receive()
-        assertEquals("OK", ok.value)
-        assertFalse(res.isCancelled) // was not cancelled
-        channel.close()
-        assertFalse(res.isCancelled) // still was not cancelled
     }
 
     @Test
@@ -86,21 +88,23 @@ class ChannelUndeliveredElementTest : TestBase() {
     }
 
     @Test
-    fun testSendToClosedChannel() = runAllKindsTest { kind ->
-        val channel = kind.create<Resource> { it.cancel() }
-        channel.close() // immediately close channel
-        val res = Resource("OK")
-        assertFailsWith<ClosedSendChannelException> {
-            channel.send(res) // send fails to closed channel, resource was not delivered
+    fun testSendToClosedChannel() = runTest {
+        runAllKindsTest { kind ->
+            val channel = kind.create<Resource> { it.cancel() }
+            channel.close() // immediately close channel
+            val res = Resource("OK")
+            assertFailsWith<ClosedSendChannelException> {
+                channel.send(res) // send fails to closed channel, resource was not delivered
+            }
+            assertTrue(res.isCancelled)
         }
-        assertTrue(res.isCancelled)
     }
 
-    private fun runAllKindsTest(test: suspend CoroutineScope.(TestChannelKind) -> Unit) {
+    private suspend fun runAllKindsTest(test: suspend CoroutineScope.(TestChannelKind) -> Unit) {
         for (kind in TestChannelKind.values()) {
             if (kind.viaBroadcast) continue // does not support onUndeliveredElement
             try {
-                runTest {
+                withContext(Job()) {
                     test(kind)
                 }
             } catch(e: Throwable) {

kotlinx-coroutines-core/common/test/flow/sharing/SharedFlowTest.kt

@@ -439,7 +439,8 @@ class SharedFlowTest : TestBase() {
     }
 
     @Test
-    fun testDifferentBufferedFlowCapacities() {
+    fun testDifferentBufferedFlowCapacities() = runTest {
+        if (isBoundByJsTestTimeout) return@runTest // Too slow for JS, bounded by 2 sec. default JS timeout
         for (replay in 0..10) {
             for (extraBufferCapacity in 0..5) {
                 if (replay == 0 && extraBufferCapacity == 0) continue // test only buffered shared flows
@@ -456,7 +457,7 @@ class SharedFlowTest : TestBase() {
         }
     }
 
-    private fun testBufferedFlow(sh: MutableSharedFlow<Int>, replay: Int) = runTest {
+    private suspend fun testBufferedFlow(sh: MutableSharedFlow<Int>, replay: Int) = withContext(Job()) {
         reset()
         expect(1)
         val n = 100 // initially emitted to fill buffer
@@ -678,6 +679,7 @@ class SharedFlowTest : TestBase() {
 
     @Test
     fun testStateFlowModel() = runTest {
+        if (isBoundByJsTestTimeout) return@runTest // Too slow for JS, bounded by 2 sec. default JS timeout
         val stateFlow = MutableStateFlow<Data?>(null)
         val expect = modelLog(stateFlow)
         val sharedFlow = MutableSharedFlow<Data?>(
@@ -795,4 +797,4 @@ class SharedFlowTest : TestBase() {
         job.join()
         finish(5)
     }
-}
+}

kotlinx-coroutines-core/js/test/TestBase.kt

@@ -10,9 +10,11 @@ public actual val isStressTest: Boolean = false
 public actual val stressTestMultiplier: Int = 1
 
 public actual open class TestBase actual constructor() {
+    public actual val isBoundByJsTestTimeout = true
     private var actionIndex = 0
     private var finished = false
     private var error: Throwable? = null
+    private var lastTestPromise: Promise<*>? = null
 
     /**
      * Throws [IllegalStateException] like `error` in stdlib, but also ensures that the test will not
@@ -70,7 +72,6 @@ public actual open class TestBase actual constructor() {
         finished = false
     }
 
-    // todo: The dynamic (promise) result is a work-around for missing suspend tests, see KT-22228
     @Suppress("ACTUAL_FUNCTION_WITH_DEFAULT_ARGUMENTS")
     public actual fun runTest(
         expected: ((Throwable) -> Boolean)? = null,
@@ -79,7 +80,29 @@ public actual open class TestBase actual constructor() {
     ): dynamic {
         var exCount = 0
         var ex: Throwable? = null
-        return GlobalScope.promise(block = block, context = CoroutineExceptionHandler { context, e ->
+        /*
+         * This is an additional sanity check against `runTest` mis-usage on JS.
+         * The only way to write an async test on JS is to return Promise from the test function.
+         * _Just_ launching promise and returning `Unit` won't suffice as the underlying test framework
+         * won't be able to detect an asynchronous failure in a timely manner.
+         * We cannot detect such situations, but we can detect the most common erroneous pattern
+         * in our code base, an attempt to use multiple `runTest` in the same `@Test` method,
+         * which typically is a premise to the same error:
+         * ```
+         * @Test
+         * fun incorrectTestForJs() { // <- promise is not returned
+         *     for (parameter in parameters) {
+         *         runTest {
+         *             runTestForParameter(parameter)
+         *         }
+         *     }
+         * }
+         * ```
+         */
+        if (lastTestPromise != null) {
+            error("Attempt to run multiple asynchronous test within one @Test method")
+        }
+        val result = GlobalScope.promise(block = block, context = CoroutineExceptionHandler { context, e ->
             if (e is CancellationException) return@CoroutineExceptionHandler // are ignored
             exCount++
             when {
@@ -102,6 +125,8 @@ public actual open class TestBase actual constructor() {
             error?.let { throw it }
             check(actionIndex == 0 || finished) { "Expecting that 'finish(...)' was invoked, but it was not" }
         }
+        lastTestPromise = result
+        return result
     }
 }
 

kotlinx-coroutines-core/jvm/test/TestBase.kt

@@ -50,6 +50,7 @@ public val stressTestMultiplierCbrt = cbrt(stressTestMultiplier.toDouble()).roun
  * ```
  */
 public actual open class TestBase actual constructor() {
+    public actual val isBoundByJsTestTimeout = false
     private var actionIndex = AtomicInteger()
     private var finished = AtomicBoolean()
     private var error = AtomicReference<Throwable>()

kotlinx-coroutines-core/native/test/TestBase.kt

@@ -8,6 +8,7 @@ public actual val isStressTest: Boolean = false
 public actual val stressTestMultiplier: Int = 1
 
 public actual open class TestBase actual constructor() {
+    public actual val isBoundByJsTestTimeout = false
     private var actionIndex = 0
     private var finished = false
     private var error: Throwable? = null