Skip to content
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

Make UncompletedTestCoroutines more verbose #3071

Merged
merged 4 commits into from
Dec 10, 2021

Conversation

dkhalanskyjb
Copy link
Collaborator

Fixes #3066
Fixes #3069

Copy link
Collaborator

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Overall looks good

@@ -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

val completionCause = try {
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

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 }

completing = false
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?

@dkhalanskyjb dkhalanskyjb merged commit 17d23e6 into develop Dec 10, 2021
@dkhalanskyjb dkhalanskyjb deleted the coroutines-test-verbose-uncompleted-coroutines branch December 10, 2021 07:35
woainikk pushed a commit that referenced this pull request Dec 14, 2021
* The error is more verbose: it lists the active children.
* If the test coroutine is already finishing with an error, that error
  is added to the timeout notification as suppressed.
* The timeout is considered more important, so now, if there were
  any uncaught exceptions, they don't take precedence over it but
  are suppressed by it.

Fixes #3066
Fixes #3069
yorickhenning pushed a commit to yorickhenning/kotlinx.coroutines that referenced this pull request Jan 28, 2022
* The error is more verbose: it lists the active children.
* If the test coroutine is already finishing with an error, that error
  is added to the timeout notification as suppressed.
* The timeout is considered more important, so now, if there were
  any uncaught exceptions, they don't take precedence over it but
  are suppressed by it.

Fixes Kotlin#3066
Fixes Kotlin#3069
pablobaxter pushed a commit to pablobaxter/kotlinx.coroutines that referenced this pull request Sep 14, 2022
* The error is more verbose: it lists the active children.
* If the test coroutine is already finishing with an error, that error
  is added to the timeout notification as suppressed.
* The timeout is considered more important, so now, if there were
  any uncaught exceptions, they don't take precedence over it but
  are suppressed by it.

Fixes Kotlin#3066
Fixes Kotlin#3069
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants