Skip to content

Commit

Permalink
Rollback runTest timeout to 60 seconds, configure it with getenv (#3945)
Browse files Browse the repository at this point in the history
This commit attempts to fix #3800.
The first part of the fix, reverting the timeout to 60 seconds, is
successful.
The second part, allowing global configuration, is only provided
for the JVM so far.

On JVM, Native, and Node JS, it's possible to use environment
variables to communicate data to the process, and it could in
theory be the solution, but it doesn't seem to interoperate well
with Gradle.

The best attempt so far was to use this:
```kotlin
tasks.withType(AbstractTestTask::class).all {
    if (this is ProcessForkOptions) {
        environment("kotlinx.coroutines.test.default_timeout", "1ms")
    }
}
```

Unfortunately, only `jvmTest` implements `ProcessForkOptions`.

Without a clear way to configure the `runTest` timeout in Gradle
builds for all targets, we only support this via system properties
on the JVM until a better mechanism appears.
  • Loading branch information
dkhalanskyjb committed Nov 30, 2023
1 parent d5c0eff commit 9a98eab
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 7 deletions.
35 changes: 29 additions & 6 deletions kotlinx-coroutines-test/common/src/TestBuilders.kt
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import kotlin.jvm.*
import kotlin.time.*
import kotlin.time.Duration.Companion.milliseconds
import kotlin.time.Duration.Companion.seconds
import kotlinx.coroutines.internal.*

/**
* A test result.
Expand Down Expand Up @@ -122,8 +121,14 @@ public expect class TestResult
*
* #### Timing out
*
* There's a built-in timeout of 10 seconds for the test body. If the test body doesn't complete within this time,
* then the test fails with an [AssertionError]. The timeout can be changed by setting the [timeout] parameter.
* There's a built-in timeout of 60 seconds for the test body. If the test body doesn't complete within this time,
* then the test fails with an [AssertionError]. The timeout can be changed for each test separately by setting the
* [timeout] parameter.
*
* Additionally, setting the `kotlinx.coroutines.test.default_timeout` system property on the
* JVM to any string that can be parsed using [Duration.parse] (like `1m`, `30s` or `1500ms`) will change the default
* timeout to that value for all tests whose [timeout] is not set explicitly; setting it to anything else will throw an
* exception every time [runTest] is invoked.
*
* On timeout, the test body is cancelled so that the test finishes. If the code inside the test body does not
* respond to cancellation, the timeout will not be able to make the test execution stop.
Expand Down Expand Up @@ -157,7 +162,7 @@ public expect class TestResult
*/
public fun runTest(
context: CoroutineContext = EmptyCoroutineContext,
timeout: Duration = DEFAULT_TIMEOUT,
timeout: Duration = DEFAULT_TIMEOUT.getOrThrow(),
testBody: suspend TestScope.() -> Unit
): TestResult {
check(context[RunningInRunTest] == null) {
Expand Down Expand Up @@ -301,7 +306,7 @@ public fun runTest(
* Performs [runTest] on an existing [TestScope]. See the documentation for [runTest] for details.
*/
public fun TestScope.runTest(
timeout: Duration = DEFAULT_TIMEOUT,
timeout: Duration = DEFAULT_TIMEOUT.getOrThrow(),
testBody: suspend TestScope.() -> Unit
): TestResult = asSpecificImplementation().let { scope ->
scope.enter()
Expand Down Expand Up @@ -421,8 +426,15 @@ internal const val DEFAULT_DISPATCH_TIMEOUT_MS = 60_000L

/**
* The default timeout to use when running a test.
*
* It's not just a [Duration] but a [Result] so that every access to [runTest]
* throws the same clear exception if parsing the environment variable failed.
* Otherwise, the parsing error would only be thrown in one tests, while the
* other ones would get an incomprehensible `NoClassDefFoundError`.
*/
internal val DEFAULT_TIMEOUT = 10.seconds
private val DEFAULT_TIMEOUT: Result<Duration> = runCatching {
systemProperty("kotlinx.coroutines.test.default_timeout", Duration::parse, 60.seconds)
}

/**
* Run the [body][testBody] of the [test coroutine][coroutine], waiting for asynchronous completions for at most
Expand Down Expand Up @@ -571,6 +583,17 @@ internal fun throwAll(head: Throwable?, other: List<Throwable>) {

internal expect fun dumpCoroutines()

private fun <T: Any> systemProperty(
name: String,
parse: (String) -> T,
default: T,
): T {
val value = systemPropertyImpl(name) ?: return default
return parse(value)
}

internal expect fun systemPropertyImpl(name: String): String?

@Deprecated(
"This is for binary compatibility with the `runTest` overload that existed at some point",
level = DeprecationLevel.HIDDEN
Expand Down
2 changes: 2 additions & 0 deletions kotlinx-coroutines-test/js/src/TestBuilders.kt
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import kotlin.js.*
@Suppress("ACTUAL_WITHOUT_EXPECT", "ACTUAL_TYPE_ALIAS_TO_CLASS_WITH_DECLARATION_SITE_VARIANCE")
public actual typealias TestResult = Promise<Unit>

internal actual fun systemPropertyImpl(name: String): String? = null

internal actual fun createTestResult(testProcedure: suspend CoroutineScope.() -> Unit): TestResult =
GlobalScope.promise {
testProcedure()
Expand Down
7 changes: 7 additions & 0 deletions kotlinx-coroutines-test/jvm/src/TestBuildersJvm.kt
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@ internal actual fun createTestResult(testProcedure: suspend CoroutineScope.() ->
}
}

internal actual fun systemPropertyImpl(name: String): String? =
try {
System.getProperty(name)
} catch (e: SecurityException) {
null
}

internal actual fun dumpCoroutines() {
@Suppress("INVISIBLE_REFERENCE", "INVISIBLE_MEMBER")
if (DebugProbesImpl.isInstalled) {
Expand Down
3 changes: 2 additions & 1 deletion kotlinx-coroutines-test/native/src/TestBuilders.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

package kotlinx.coroutines.test
import kotlinx.coroutines.*
import kotlin.native.concurrent.*

@Suppress("ACTUAL_WITHOUT_EXPECT")
public actual typealias TestResult = Unit
Expand All @@ -15,4 +14,6 @@ internal actual fun createTestResult(testProcedure: suspend CoroutineScope.() ->
}
}

internal actual fun systemPropertyImpl(name: String): String? = null

internal actual fun dumpCoroutines() { }

0 comments on commit 9a98eab

Please sign in to comment.