-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -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() } | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
|
@@ -196,6 +196,7 @@ internal const val DEFAULT_DISPATCH_TIMEOUT_MS = 60_000L | |||||||
internal suspend fun <T: AbstractCoroutine<Unit>> runTestCoroutine( | ||||||||
qwwdfsad marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
coroutine: T, | ||||||||
dispatchTimeoutMs: Long, | ||||||||
tryGetCompletionCause: T.() -> Throwable?, | ||||||||
testBody: suspend T.() -> Unit, | ||||||||
cleanup: () -> List<Throwable>, | ||||||||
) { | ||||||||
|
@@ -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 } | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
} catch (e: Throwable) { | ||||||||
completing = false | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of |
||||||||
null | ||||||||
} | ||||||||
var message = "After waiting for $dispatchTimeoutMs ms" | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||
} | ||||||||
} | ||||||||
} | ||||||||
|
There was a problem hiding this comment.
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