Skip to content

Make UncompletedTestCoroutines more verbose #3071

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.

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 4 commits into from
Dec 10, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ internal expect suspend inline fun recoverAndThrow(exception: Throwable): Nothin
* The opposite of [recoverStackTrace].
* It is guaranteed that `unwrap(recoverStackTrace(e)) === e`
*/
@PublishedApi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please leave a comment that it is only used from other module tests, so when the time comes we know it can be safely deleted/reverted

internal expect fun <E: Throwable> unwrap(exception: E): E

internal expect class StackTraceElement
Expand Down
24 changes: 22 additions & 2 deletions kotlinx-coroutines-test/common/src/TestBuilders.kt
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ public fun TestScope.runTest(
): TestResult = asSpecificImplementation().let {
it.enter()
createTestResult {
runTestCoroutine(it, dispatchTimeoutMs, testBody) { it.leave() }
runTestCoroutine(it, dispatchTimeoutMs, TestScopeImpl::tryGetCompletionCause, testBody) { it.leave() }
}
}

Expand Down Expand Up @@ -196,6 +196,7 @@ internal const val DEFAULT_DISPATCH_TIMEOUT_MS = 60_000L
internal suspend fun <T: AbstractCoroutine<Unit>> runTestCoroutine(
coroutine: T,
dispatchTimeoutMs: Long,
tryGetCompletionCause: T.() -> Throwable?,
testBody: suspend T.() -> Unit,
cleanup: () -> List<Throwable>,
) {
Expand Down Expand Up @@ -228,7 +229,26 @@ internal suspend fun <T: AbstractCoroutine<Unit>> runTestCoroutine(
// we expect these and will instead throw a more informative exception just below.
emptyList()
}.throwAll()
throw UncompletedCoroutinesError("The test coroutine was not completed after waiting for $dispatchTimeoutMs ms")
var completing: Boolean
val completionCause = try {
coroutine.tryGetCompletionCause().also { completing = true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
coroutine.tryGetCompletionCause().also { completing = true }
// Protected JobSupport.completionCause lifted in TestCoroutineScope and TestScope
coroutine.tryGetCompletionCause().also { completing = true }

} catch (e: Throwable) {
completing = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of try-catch, it should be enough to check coroutine.isCancelled.
The same goes for completing

null
}
var message = "After waiting for $dispatchTimeoutMs ms"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to follow our overall approach and extract slow-path to a separate method

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But this whole code fragment is the slow path, don't you agree?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, then maybe it's worth extracting the whole onTimeout body?

if (!completing)
message += ", the test coroutine is not completing"
val activeChildren = coroutine.children.filter { it.isActive }.toList()
if (activeChildren.isNotEmpty())
message += ", there were active child jobs: $activeChildren"
if (completing && activeChildren.isEmpty()) {
// some sort of race condition? write something generic.
message += ", the test coroutine was not completed"
}
val error = UncompletedCoroutinesError(message)
completionCause?.let { cause -> error.addSuppressed(cause) }
throw error
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions kotlinx-coroutines-test/common/src/TestScope.kt
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,9 @@ internal class TestScopeImpl(context: CoroutineContext) :
}
}

/** Throws an exception if the coroutine is not completing. */
fun tryGetCompletionCause(): Throwable? = completionCause

override fun toString(): String =
"TestScope[" + (if (finished) "test ended" else if (entered) "test started" else "test not started") + "]"
}
Expand Down
54 changes: 54 additions & 0 deletions kotlinx-coroutines-test/common/test/RunTestTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package kotlinx.coroutines.test

import kotlinx.coroutines.*
import kotlinx.coroutines.internal.*
import kotlinx.coroutines.flow.*
import kotlin.coroutines.*
import kotlin.test.*
Expand Down Expand Up @@ -97,6 +98,59 @@ class RunTestTest {
}
}

/** Tests that, on timeout, the names of the active coroutines are listed,
* whereas the names of the completed ones are not. */
@Test
@NoJs
@NoNative
fun testListingActiveCoroutinesOnTimeout(): TestResult {
val name1 = "GoodUniqueName"
val name2 = "BadUniqueName"
return testResultMap({
try {
it()
fail("unreached")
} catch (e: UncompletedCoroutinesError) {
assertTrue((e.message ?: "").contains(name1))
assertFalse((e.message ?: "").contains(name2))
}
}) {
runTest(dispatchTimeoutMs = 10) {
launch(CoroutineName(name1)) {
CompletableDeferred<Unit>().await()
}
launch(CoroutineName(name2)) {
}
}
}
}

/** Tests that the [UncompletedCoroutinesError] suppresses an exception with which the coroutine is completing. */
@Test
fun testFailureWithPendingCoroutine() = testResultMap({
try {
it()
fail("unreached")
} catch (e: UncompletedCoroutinesError) {
@Suppress("INVISIBLE_MEMBER")
val suppressed = unwrap(e).suppressedExceptions
assertEquals(1, suppressed.size)
assertIs<TestException>(suppressed[0]).also {
assertEquals("A", it.message)
}
}
}) {
runTest(dispatchTimeoutMs = 10) {
launch {
withContext(NonCancellable) {
awaitCancellation()
}
}
yield()
throw TestException("A")
}
}

/** Tests that real delays can be accounted for with a large enough dispatch timeout. */
@Test
fun testRunTestWithLargeTimeout() = runTest(dispatchTimeoutMs = 5000) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
package kotlinx.coroutines.test

import kotlinx.coroutines.*
import kotlinx.coroutines.selects.*
import kotlin.coroutines.*
import kotlin.jvm.*

Expand Down Expand Up @@ -137,9 +136,9 @@ public fun runTestWithLegacyScope(
): TestResult {
if (context[RunningInRunTest] != null)
throw IllegalStateException("Calls to `runTest` can't be nested. Please read the docs on `TestResult` for details.")
val testScope = TestBodyCoroutine<Unit>(createTestCoroutineScope(context + RunningInRunTest))
val testScope = TestBodyCoroutine(createTestCoroutineScope(context + RunningInRunTest))
return createTestResult {
runTestCoroutine(testScope, dispatchTimeoutMs, testBody) {
runTestCoroutine(testScope, dispatchTimeoutMs, TestBodyCoroutine::tryGetCompletionCause, testBody) {
try {
testScope.cleanup()
emptyList()
Expand Down Expand Up @@ -169,9 +168,9 @@ public fun TestCoroutineScope.runTest(
block: suspend TestCoroutineScope.() -> Unit
): TestResult = runTestWithLegacyScope(coroutineContext, dispatchTimeoutMs, block)

private class TestBodyCoroutine<T>(
private class TestBodyCoroutine(
private val testScope: TestCoroutineScope,
) : AbstractCoroutine<T>(testScope.coroutineContext, initParentJob = true, active = true), TestCoroutineScope {
) : AbstractCoroutine<Unit>(testScope.coroutineContext, initParentJob = true, active = true), TestCoroutineScope {

override val testScheduler get() = testScope.testScheduler

Expand All @@ -187,4 +186,7 @@ private class TestBodyCoroutine<T>(
)

fun cleanup() = testScope.cleanupTestCoroutines()

/** Throws an exception if the coroutine is not completing. */
fun tryGetCompletionCause(): Throwable? = completionCause
}