Skip to content

Use Duration in runTest builders as timeout by default #3483

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 21, 2022
Merged

Use Duration in runTest builders as timeout by default #3483

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ca1e061
Use Duration in runTest builders as timeout by default
hfhbd Oct 15, 2022
adcd834
Add Kdoc
hfhbd Oct 17, 2022
bfe0b91
Fix api
hfhbd Oct 17, 2022
5b6c527
Fix kdoc
hfhbd Oct 21, 2022
ef7b2f7
Fix overload
hfhbd Oct 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion kotlinx-coroutines-test/api/kotlinx-coroutines-test.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ public final class kotlinx/coroutines/test/TestBuildersKt {
public static final fun runTest (Lkotlinx/coroutines/test/TestScope;JLkotlin/jvm/functions/Function2;)V
public static synthetic fun runTest$default (Lkotlin/coroutines/CoroutineContext;JLkotlin/jvm/functions/Function2;ILjava/lang/Object;)V
public static synthetic fun runTest$default (Lkotlinx/coroutines/test/TestCoroutineScope;JLkotlin/jvm/functions/Function2;ILjava/lang/Object;)V
public static synthetic fun runTest$default (Lkotlinx/coroutines/test/TestScope;JLkotlin/jvm/functions/Function2;ILjava/lang/Object;)V
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unpleasant that this overload has disappeared from the resulting binary. What I think we should do instead is to make the Duration overload not have a default argument, and change the Long overload back so that it has it. From the programmer's perspective, it doesn't matter, as the behavior stays exactly the same, but preserving compatibility is nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, although it is experimental api.
But is there a bug in bcv? I removed the default value from runTest(Long), but there is no synthetic runTest$default function without TestScope receiver.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the source compatibility did break at least, causing the tests to fail.

Copy link
Contributor Author

@hfhbd hfhbd Oct 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which tests? I still guess, there is some bug in bcv (or I don't get binary compatibility).
Before:

  • runTest(long: default)
  • TestScope.runTest(long: default)

Now:

  • runTest(duration: default)
  • runTest(long: no default) <- I removed this default, but there is no removed line in .api
  • TestScope.runTest(duration: no default)
  • TestScope.runTest(long: default) <- added back the default

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which tests?

The tests in general didn't even compile for me, they were unable to find a runTest overload that could be invoked just as runTest { ... }.

I removed this default, but there is no removed line in .api

It's my understanding that having at least one optional parameter creates a single $default overload that allows passing all the parameters in the Object. So, since the context parameter still had a default value, the overload was created, and the binary compatibility was preserved. @qwwdfsad, could you confirm or deny this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of, except that the trailing Object parameter is never used, it's just a rudiment that we are unlikely to ever use.

public static final fun runTest-8Mi8wO0 (Lkotlin/coroutines/CoroutineContext;JLkotlin/jvm/functions/Function2;)V
public static final fun runTest-8Mi8wO0 (Lkotlinx/coroutines/test/TestScope;JLkotlin/jvm/functions/Function2;)V
public static synthetic fun runTest-8Mi8wO0$default (Lkotlin/coroutines/CoroutineContext;JLkotlin/jvm/functions/Function2;ILjava/lang/Object;)V
public static synthetic fun runTest-8Mi8wO0$default (Lkotlinx/coroutines/test/TestScope;JLkotlin/jvm/functions/Function2;ILjava/lang/Object;)V
public static final fun runTestWithLegacyScope (Lkotlin/coroutines/CoroutineContext;JLkotlin/jvm/functions/Function2;)V
public static synthetic fun runTestWithLegacyScope$default (Lkotlin/coroutines/CoroutineContext;JLkotlin/jvm/functions/Function2;ILjava/lang/Object;)V
}
Expand Down
56 changes: 41 additions & 15 deletions kotlinx-coroutines-test/common/src/TestBuilders.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import kotlinx.coroutines.*
import kotlinx.coroutines.selects.*
import kotlin.coroutines.*
import kotlin.jvm.*
import kotlin.time.*
import kotlin.time.Duration.Companion.milliseconds

/**
* A test result.
Expand Down Expand Up @@ -41,9 +43,9 @@ public expect class TestResult
* @Test
* fun exampleTest() = runTest {
* val deferred = async {
* delay(1_000)
* delay(1.seconds)
* async {
* delay(1_000)
* delay(1.seconds)
* }.await()
* }
*
Expand Down Expand Up @@ -99,9 +101,9 @@ public expect class TestResult
* fun exampleTest() = runTest {
* val elapsed = TimeSource.Monotonic.measureTime {
* val deferred = async {
* delay(1_000) // will be skipped
* delay(1.seconds) // will be skipped
* withContext(Dispatchers.Default) {
* delay(5_000) // Dispatchers.Default doesn't know about TestCoroutineScheduler
* delay(5.seconds) // Dispatchers.Default doesn't know about TestCoroutineScheduler
* }
* }
* deferred.await()
Expand Down Expand Up @@ -131,7 +133,7 @@ public expect class TestResult
*
* In the general case, if there are active jobs, it's impossible to detect if they are going to complete eventually due
* to the asynchronous nature of coroutines. In order to prevent tests hanging in this scenario, [runTest] will wait
* for [dispatchTimeoutMs] milliseconds (by default, 60 seconds) from the moment when [TestCoroutineScheduler] becomes
* for [dispatchTimeout] (by default, 60 seconds) from the moment when [TestCoroutineScheduler] becomes
* idle before throwing [AssertionError]. If some dispatcher linked to [TestCoroutineScheduler] receives a
* task during that time, the timer gets reset.
*
Expand All @@ -146,32 +148,55 @@ public expect class TestResult
@ExperimentalCoroutinesApi
public fun runTest(
context: CoroutineContext = EmptyCoroutineContext,
dispatchTimeoutMs: Long = DEFAULT_DISPATCH_TIMEOUT_MS,
dispatchTimeout: Duration = DEFAULT_DISPATCH_TIMEOUT,
testBody: suspend TestScope.() -> Unit
): TestResult {
if (context[RunningInRunTest] != null)
throw IllegalStateException("Calls to `runTest` can't be nested. Please read the docs on `TestResult` for details.")
return TestScope(context + RunningInRunTest).runTest(dispatchTimeoutMs, testBody)
return TestScope(context + RunningInRunTest).runTest(dispatchTimeout, testBody)
}

@ExperimentalCoroutinesApi
public fun runTest(
context: CoroutineContext = EmptyCoroutineContext,
dispatchTimeoutMs: Long,
testBody: suspend TestScope.() -> Unit
): TestResult = runTest(
context = context,
dispatchTimeout = dispatchTimeoutMs.milliseconds,
testBody = testBody
)

/**
* Performs [runTest] on an existing [TestScope].
*/
@ExperimentalCoroutinesApi
public fun TestScope.runTest(
dispatchTimeoutMs: Long = DEFAULT_DISPATCH_TIMEOUT_MS,
dispatchTimeout: Duration = DEFAULT_DISPATCH_TIMEOUT,
testBody: suspend TestScope.() -> Unit
): TestResult = asSpecificImplementation().let {
it.enter()
createTestResult {
runTestCoroutine(it, dispatchTimeoutMs, TestScopeImpl::tryGetCompletionCause, testBody) {
runTestCoroutine(it, dispatchTimeout, TestScopeImpl::tryGetCompletionCause, testBody) {
backgroundScope.cancel()
testScheduler.advanceUntilIdleOr { false }
it.leave()
}
}
}

/**
* Performs [runTest] on an existing [TestScope].
*/
@ExperimentalCoroutinesApi
public fun TestScope.runTest(
dispatchTimeoutMs: Long,
testBody: suspend TestScope.() -> Unit
): TestResult = runTest(
dispatchTimeout = dispatchTimeoutMs.milliseconds,
testBody = testBody
)

/**
* Runs [testProcedure], creating a [TestResult].
*/
Expand All @@ -189,10 +214,11 @@ internal object RunningInRunTest : CoroutineContext.Key<RunningInRunTest>, Corou
/** The default timeout to use when waiting for asynchronous completions of the coroutines managed by
* a [TestCoroutineScheduler]. */
internal const val DEFAULT_DISPATCH_TIMEOUT_MS = 60_000L
internal val DEFAULT_DISPATCH_TIMEOUT = DEFAULT_DISPATCH_TIMEOUT_MS.milliseconds

/**
* Run the [body][testBody] of the [test coroutine][coroutine], waiting for asynchronous completions for at most
* [dispatchTimeoutMs] milliseconds, and performing the [cleanup] procedure at the end.
* [dispatchTimeout], and performing the [cleanup] procedure at the end.
*
* [tryGetCompletionCause] is the [JobSupport.completionCause], which is passed explicitly because it is protected.
*
Expand All @@ -201,7 +227,7 @@ internal const val DEFAULT_DISPATCH_TIMEOUT_MS = 60_000L
*/
internal suspend fun <T: AbstractCoroutine<Unit>> CoroutineScope.runTestCoroutine(
coroutine: T,
dispatchTimeoutMs: Long,
dispatchTimeout: Duration,
tryGetCompletionCause: T.() -> Throwable?,
testBody: suspend T.() -> Unit,
cleanup: () -> List<Throwable>,
Expand Down Expand Up @@ -258,8 +284,8 @@ internal suspend fun <T: AbstractCoroutine<Unit>> CoroutineScope.runTestCoroutin
scheduler.onDispatchEvent {
// we received knowledge that `scheduler` observed a dispatch event, so we reset the timeout
}
onTimeout(dispatchTimeoutMs) {
handleTimeout(coroutine, dispatchTimeoutMs, tryGetCompletionCause, cleanup)
onTimeout(dispatchTimeout) {
handleTimeout(coroutine, dispatchTimeout, tryGetCompletionCause, cleanup)
}
}
} finally {
Expand All @@ -284,7 +310,7 @@ internal suspend fun <T: AbstractCoroutine<Unit>> CoroutineScope.runTestCoroutin
*/
private inline fun<T: AbstractCoroutine<Unit>> handleTimeout(
coroutine: T,
dispatchTimeoutMs: Long,
dispatchTimeout: Duration,
tryGetCompletionCause: T.() -> Throwable?,
cleanup: () -> List<Throwable>,
) {
Expand All @@ -296,7 +322,7 @@ private inline fun<T: AbstractCoroutine<Unit>> handleTimeout(
}
val activeChildren = coroutine.children.filter { it.isActive }.toList()
val completionCause = if (coroutine.isCancelled) coroutine.tryGetCompletionCause() else null
var message = "After waiting for $dispatchTimeoutMs ms"
var message = "After waiting for $dispatchTimeout"
if (completionCause == null)
message += ", the test coroutine is not completing"
if (activeChildren.isNotEmpty())
Expand Down
3 changes: 2 additions & 1 deletion kotlinx-coroutines-test/jvm/src/migration/TestBuildersDeprecated.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ package kotlinx.coroutines.test
import kotlinx.coroutines.*
import kotlin.coroutines.*
import kotlin.jvm.*
import kotlin.time.Duration.Companion.milliseconds

/**
* Executes a [testBody] inside an immediate execution dispatcher.
Expand Down Expand Up @@ -164,7 +165,7 @@ public fun runTestWithLegacyScope(
throw IllegalStateException("Calls to `runTest` can't be nested. Please read the docs on `TestResult` for details.")
val testScope = TestBodyCoroutine(createTestCoroutineScope(context + RunningInRunTest))
return createTestResult {
runTestCoroutine(testScope, dispatchTimeoutMs, TestBodyCoroutine::tryGetCompletionCause, testBody) {
runTestCoroutine(testScope, dispatchTimeoutMs.milliseconds, TestBodyCoroutine::tryGetCompletionCause, testBody) {
try {
testScope.cleanup()
emptyList()
Expand Down