Refactor ClientLoader.clientTests (#4548)

* Disable mocha timeout * Use Duration for timeout in test utils * Rename 'ktor-junit' -> 'ktor-test-base' * Add multiplatform implementation of runTestWithData * Use runTestWithData in ClientLoader * Decrease timeouts
ktorio · Dec 17, 2024 · e810eaa · e810eaa
1 parent e18c901
commit e810eaa
Show file tree

Hide file tree

Showing 44 changed files with 549 additions and 136 deletions.
diff --git a/build.gradle.kts b/build.gradle.kts
@@ -29,7 +29,7 @@ apply(from = "gradle/verifier.gradle")
 extra["skipPublish"] = mutableListOf(
     "ktor-server-test-suites",
     "ktor-server-tests",
-    "ktor-junit",
+    "ktor-test-base",
 )
 
 // Point old artifact to new location
@@ -48,7 +48,7 @@ val disabledExplicitApiModeProjects = listOf(
     "ktor-server-test-suites",
     "ktor-server-tests",
     "ktor-client-content-negotiation-tests",
-    "ktor-junit"
+    "ktor-test-base"
 )
 
 apply(from = "gradle/compatibility.gradle")

diff --git a/buildSrc/src/main/kotlin/JsConfig.kt b/buildSrc/src/main/kotlin/JsConfig.kt
@@ -2,13 +2,11 @@
  * Copyright 2014-2024 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
  */
 
-import internal.*
-import org.gradle.api.*
-import org.gradle.internal.extensions.stdlib.*
-import org.gradle.kotlin.dsl.*
-import org.jetbrains.kotlin.gradle.targets.js.dsl.*
-import java.io.*
-import kotlin.toString
+import internal.capitalized
+import internal.libs
+import org.gradle.api.Project
+import org.gradle.kotlin.dsl.invoke
+import org.jetbrains.kotlin.gradle.targets.js.dsl.KotlinJsSubTargetDsl
 
 fun Project.configureJs() {
     kotlin {
@@ -34,7 +32,8 @@ fun Project.configureJs() {
 internal fun KotlinJsSubTargetDsl.useMochaForTests() {
     testTask {
         useMocha {
-            timeout = "10000"
+            // Disable timeout as we use individual timeouts for tests
+            timeout = "0"
         }
     }
 }
@@ -43,7 +42,7 @@ internal fun KotlinJsSubTargetDsl.useKarmaForTests() {
     testTask {
         useKarma {
             useChromeHeadless()
-            useConfigDirectory(File(project.rootProject.projectDir, "karma"))
+            useConfigDirectory(project.rootProject.file("karma"))
         }
     }
 }

diff --git a/gradle/compatibility.gradle b/gradle/compatibility.gradle
@@ -12,7 +12,7 @@ apiValidation {
         'ktor-client-plugins',
         'ktor-server-test-suites',
         "ktor-server-test-base",
-        'ktor-junit',
+        'ktor-test-base',
     ].toSet()
 
     def projects = [].toSet()

diff --git a/karma/chrome_bin.js b/karma/chrome_bin.js
@@ -21,7 +21,8 @@ config.set({
     "client": {
         captureConsole: true,
         "mocha": {
-            timeout: 10000
+            // Disable timeout as we use individual timeouts for tests
+            timeout: 0
         }
     }
 });

diff --git a/ktor-client/ktor-client-cio/build.gradle.kts b/ktor-client/ktor-client-cio/build.gradle.kts
@@ -24,7 +24,7 @@ kotlin {
         jvmTest {
             dependencies {
                 api(project(":ktor-network:ktor-network-tls:ktor-network-tls-certificates"))
-                api(project(":ktor-shared:ktor-junit"))
+                api(project(":ktor-shared:ktor-test-base"))
                 implementation(libs.mockk)
             }
         }

diff --git a/ktor-client/ktor-client-cio/jvm/test/io/ktor/client/engine/cio/CIORequestTest.kt b/ktor-client/ktor-client-cio/jvm/test/io/ktor/client/engine/cio/CIORequestTest.kt
@@ -12,11 +12,11 @@ import io.ktor.client.statement.*
 import io.ktor.client.tests.utils.*
 import io.ktor.http.*
 import io.ktor.http.content.*
-import io.ktor.junit.coroutines.*
 import io.ktor.server.engine.*
 import io.ktor.server.netty.*
 import io.ktor.server.response.*
 import io.ktor.server.routing.*
+import io.ktor.test.junit.coroutines.*
 import io.mockk.mockkStatic
 import io.mockk.verify
 import kotlinx.coroutines.delay

diff --git a/ktor-client/ktor-client-cio/jvm/test/io/ktor/client/engine/cio/ConnectErrorsTest.kt b/ktor-client/ktor-client-cio/jvm/test/io/ktor/client/engine/cio/ConnectErrorsTest.kt
@@ -11,7 +11,7 @@ import io.ktor.client.plugins.*
 import io.ktor.client.request.*
 import io.ktor.client.statement.*
 import io.ktor.http.*
-import io.ktor.junit.coroutines.*
+import io.ktor.test.junit.coroutines.*
 import io.ktor.network.tls.certificates.*
 import io.ktor.server.application.*
 import io.ktor.server.engine.*

diff --git a/ktor-client/ktor-client-tests/build.gradle.kts b/ktor-client/ktor-client-tests/build.gradle.kts
@@ -17,8 +17,7 @@ kotlin.sourceSets {
         dependencies {
             api(project(":ktor-client:ktor-client-mock"))
             api(project(":ktor-test-dispatcher"))
-            api(libs.kotlin.test)
-            api(libs.kotlinx.coroutines.test)
+            api(project(":ktor-shared:ktor-test-base"))
         }
     }
     commonTest {
@@ -46,7 +45,7 @@ kotlin.sourceSets {
             api(project(":ktor-server:ktor-server-plugins:ktor-server-auth"))
             api(project(":ktor-server:ktor-server-plugins:ktor-server-websockets"))
             api(project(":ktor-shared:ktor-serialization:ktor-serialization-kotlinx"))
-            api(project(":ktor-shared:ktor-junit"))
+            api(project(":ktor-shared:ktor-test-base"))
             api(libs.logback.classic)
             implementation(libs.kotlinx.coroutines.debug)
         }

diff --git a/ktor-client/ktor-client-tests/common/src/io/ktor/client/tests/utils/ClientLoader.kt b/ktor-client/ktor-client-tests/common/src/io/ktor/client/tests/utils/ClientLoader.kt
@@ -1,12 +1,12 @@
 /*
- * Copyright 2014-2019 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
+ * Copyright 2014-2024 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
  */
 
 package io.ktor.client.tests.utils
 
 import io.ktor.client.engine.*
+import io.ktor.test.*
 import kotlinx.coroutines.test.TestResult
-import kotlinx.coroutines.test.runTest
 import kotlin.time.Duration
 import kotlin.time.Duration.Companion.minutes
 
@@ -15,6 +15,8 @@ internal expect val platformName: String
 internal expect fun platformDumpCoroutines()
 internal expect fun platformWaitForAllCoroutines()
 
+private typealias ClientTestFailure = TestFailure<HttpClientEngineFactory<*>>
+
 /**
  * Helper interface to test client.
  */
@@ -26,39 +28,38 @@ abstract class ClientLoader(private val timeout: Duration = 1.minutes) {
         skipEngines: List<String> = emptyList(),
         onlyWithEngine: String? = null,
         retries: Int = 1,
+        timeout: Duration = this.timeout,
         block: suspend TestClientBuilder<HttpClientEngineConfig>.() -> Unit
-    ): TestResult = runTest(timeout = timeout) {
+    ): TestResult {
         val skipPatterns = skipEngines.map(SkipEnginePattern::parse)
-
-        val failures: List<TestFailure> = enginesToTest.mapNotNull { engineFactory ->
-            val engineName = engineFactory.engineName
-
-            if (shouldRun(engineName, skipPatterns, onlyWithEngine)) {
-                try {
-                    println("Run test with engine $engineName")
-                    // run test here
-                    performTestWithEngine(engineFactory, this@ClientLoader, retries, block)
-                    null // engine test passed
-                } catch (cause: Throwable) {
-                    // engine test failed, save failure to report after run for every engine.
-                    TestFailure(engineName, cause)
-                }
-            } else {
-                println("Skipping test with engine $engineName")
-                null // engine skipped
-            }
+        val (selectedEngines, skippedEngines) = enginesToTest
+            .partition { shouldRun(it.engineName, skipPatterns, onlyWithEngine) }
+        if (skippedEngines.isNotEmpty()) println("Skipped engines: ${skippedEngines.joinToString { it.engineName }}")
+
+        return runTestWithData(
+            selectedEngines,
+            timeout = timeout,
+            retries = retries,
+            handleFailures = ::aggregatedAssertionError,
+        ) { (engine, retry) ->
+            val retrySuffix = if (retry > 0) " [$retry]" else ""
+            println("Run test with engine ${engine.engineName}$retrySuffix")
+            performTestWithEngine(engine, this@ClientLoader, block)
         }
+    }
 
-        if (failures.isNotEmpty()) {
-            val message = buildString {
-                appendLine("Test failed for engines: ${failures.map { it.engineName }}")
-                failures.forEach {
-                    appendLine("Test failed for engine '$platformName:${it.engineName}' with:")
-                    appendLine(it.cause.stackTraceToString().prependIndent("  "))
-                }
+    private fun aggregatedAssertionError(failures: List<ClientTestFailure>): Nothing {
+        val message = buildString {
+            val engineNames = failures.map { it.data.engineName }
+            if (failures.size > 1) {
+                appendLine("Test failed for engines: ${engineNames.joinToString()}")
+            }
+            failures.forEachIndexed { index, (cause, _) ->
+                appendLine("Test failed for engine '$platformName:${engineNames[index]}' with:")
+                appendLine(cause.stackTraceToString().prependIndent("  "))
             }
-            throw AssertionError(message)
         }
+        throw AssertionError(message)
     }
 
     private fun shouldRun(
@@ -132,5 +133,3 @@ private data class SkipEnginePattern(
         }
     }
 }
-
-private class TestFailure(val engineName: String, val cause: Throwable)
diff --git a/ktor-client/ktor-client-tests/common/src/io/ktor/client/tests/utils/CommonClientTestUtils.kt b/ktor-client/ktor-client-tests/common/src/io/ktor/client/tests/utils/CommonClientTestUtils.kt
@@ -6,10 +6,11 @@ package io.ktor.client.tests.utils
 
 import io.ktor.client.*
 import io.ktor.client.engine.*
+import io.ktor.test.*
 import io.ktor.utils.io.core.*
 import kotlinx.coroutines.*
-import kotlinx.coroutines.test.runTest
-import kotlin.time.Duration.Companion.milliseconds
+import kotlin.time.Duration
+import kotlin.time.Duration.Companion.minutes
 
 /**
  * Web url for tests.
@@ -31,29 +32,27 @@ const val TCP_SERVER: String = "http://127.0.0.1:8082"
  */
 fun testWithEngine(
     engine: HttpClientEngine,
-    timeoutMillis: Long = 60 * 1000L,
+    timeout: Duration = 1.minutes,
     retries: Int = 1,
     block: suspend TestClientBuilder<*>.() -> Unit
-) = testWithClient(HttpClient(engine), timeoutMillis, retries, block)
+) = testWithClient(HttpClient(engine), timeout, retries, block)
 
 /**
  * Perform test with selected [client].
  */
 private fun testWithClient(
     client: HttpClient,
-    timeout: Long,
+    timeout: Duration,
     retries: Int,
     block: suspend TestClientBuilder<HttpClientEngineConfig>.() -> Unit
-) = runTest(timeout = timeout.milliseconds) {
+) = runTestWithData(listOf(client), timeout = timeout, retries = retries) {
     val builder = TestClientBuilder<HttpClientEngineConfig>().also { it.block() }
 
-    retryTest(retries) {
-        concurrency(builder.concurrency) { threadId ->
-            repeat(builder.repeatCount) { attempt ->
-                @Suppress("UNCHECKED_CAST")
-                client.config { builder.config(this as HttpClientConfig<HttpClientEngineConfig>) }
-                    .use { client -> builder.test(TestInfo(threadId, attempt), client) }
-            }
+    concurrency(builder.concurrency) { threadId ->
+        repeat(builder.repeatCount) { attempt ->
+            @Suppress("UNCHECKED_CAST")
+            client.config { builder.config(this as HttpClientConfig<HttpClientEngineConfig>) }
+                .use { client -> builder.test(TestInfo(threadId, attempt), client) }
         }
     }
 
@@ -66,18 +65,17 @@ private fun testWithClient(
 fun <T : HttpClientEngineConfig> testWithEngine(
     factory: HttpClientEngineFactory<T>,
     loader: ClientLoader? = null,
-    timeoutMillis: Long = 60L * 1000L,
+    timeout: Duration = 1.minutes,
     retries: Int = 1,
     block: suspend TestClientBuilder<T>.() -> Unit
-) = runTest(timeout = timeoutMillis.milliseconds) {
-    performTestWithEngine(factory, loader, retries, block)
+) = runTestWithData(listOf(factory), timeout = timeout, retries = retries) {
+    performTestWithEngine(factory, loader, block)
 }
 
 @OptIn(DelicateCoroutinesApi::class)
 suspend fun <T : HttpClientEngineConfig> performTestWithEngine(
     factory: HttpClientEngineFactory<T>,
     loader: ClientLoader? = null,
-    retries: Int = 1,
     block: suspend TestClientBuilder<T>.() -> Unit
 ) {
     val builder = TestClientBuilder<T>().apply { block() }
@@ -89,45 +87,31 @@ suspend fun <T : HttpClientEngineConfig> performTestWithEngine(
         }
     }
 
-    retryTest(retries) {
-        withContext(Dispatchers.Default.limitedParallelism(1)) {
-            concurrency(builder.concurrency) { threadId ->
-                repeat(builder.repeatCount) { attempt ->
-                    val client = HttpClient(factory, block = builder.config)
+    withContext(Dispatchers.Default.limitedParallelism(1)) {
+        concurrency(builder.concurrency) { threadId ->
+            repeat(builder.repeatCount) { attempt ->
+                val client = HttpClient(factory, block = builder.config)
 
-                    client.use {
-                        builder.test(TestInfo(threadId, attempt), it)
-                    }
+                client.use {
+                    builder.test(TestInfo(threadId, attempt), it)
+                }
 
-                    try {
-                        val job = client.coroutineContext[Job]!!
-                        while (job.isActive) {
-                            yield()
-                        }
-                    } catch (cause: Throwable) {
-                        client.cancel("Test failed", cause)
-                        throw cause
-                    } finally {
-                        builder.after(client)
+                try {
+                    val job = client.coroutineContext[Job]!!
+                    while (job.isActive) {
+                        yield()
                     }
+                } catch (cause: Throwable) {
+                    client.cancel("Test failed", cause)
+                    throw cause
+                } finally {
+                    builder.after(client)
                 }
             }
         }
     }
 }
 
-internal suspend fun <T> retryTest(attempts: Int, block: suspend () -> T): T {
-    var currentAttempt = 0
-    while (true) {
-        try {
-            return block()
-        } catch (cause: Throwable) {
-            if (currentAttempt >= attempts) throw cause
-            currentAttempt++
-        }
-    }
-}
-
 private suspend fun concurrency(level: Int, block: suspend (Int) -> Unit) {
     coroutineScope {
         List(level) {

diff --git a/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ContentTest.kt b/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ContentTest.kt
@@ -43,7 +43,7 @@ val testArrays = testSize.map {
     makeArray(it)
 }
 
-class ContentTest : ClientLoader(timeout = 5.minutes) {
+class ContentTest : ClientLoader() {
 
     @Test
     fun testGetFormData() = clientTests {

diff --git a/...client/ktor-client-tests/common/test/io/ktor/client/tests/plugins/ServerSentEventsTest.kt b/...client/ktor-client-tests/common/test/io/ktor/client/tests/plugins/ServerSentEventsTest.kt
@@ -1,5 +1,5 @@
 /*
- * Copyright 2014-2023 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
+ * Copyright 2014-2024 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
  */
 
 package io.ktor.client.tests.plugins
@@ -25,9 +25,8 @@ import kotlin.coroutines.CoroutineContext
 import kotlin.coroutines.resume
 import kotlin.coroutines.suspendCoroutine
 import kotlin.test.*
-import kotlin.time.Duration.Companion.minutes
 
-class ServerSentEventsTest : ClientLoader(timeout = 2.minutes) {
+class ServerSentEventsTest : ClientLoader() {
 
     @Test
     fun testExceptionIfSseIsNotInstalled() = testSuspend {

diff --git a/ktor-client/ktor-client-tests/jvm/src/io/ktor/client/tests/utils/TestWithKtor.kt b/ktor-client/ktor-client-tests/jvm/src/io/ktor/client/tests/utils/TestWithKtor.kt
@@ -6,8 +6,8 @@ package io.ktor.client.tests.utils
 
 import ch.qos.logback.classic.Level
 import ch.qos.logback.classic.Logger
-import io.ktor.junit.coroutines.*
 import io.ktor.server.engine.*
+import io.ktor.test.junit.coroutines.*
 import org.junit.jupiter.api.AfterEach
 import org.junit.jupiter.api.BeforeEach
 import org.slf4j.LoggerFactory