Skip to content

Update exception handling in the test module #2953

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

Jump to bottom
Merged
merged 5 commits into from
Nov 1, 2021
Merged

Update exception handling in the test module #2953

merged 5 commits into from
Nov 1, 2021

Conversation

dkhalanskyjb
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb commented Sep 24, 2021 •
edited
Loading

This change provides the means to more easily define what to do with unhandled exceptions during tests.

@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-exception-handling branch from 04ff300 to 4f1d7df Compare October 4, 2021 08:10
@dkhalanskyjb dkhalanskyjb changed the base branch from develop to coroutines-test-dispatchers October 18, 2021 09:20
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-exception-handling branch from 4f1d7df to 68832cc Compare October 18, 2021 13:42
@dkhalanskyjb dkhalanskyjb changed the title WIP: fix exception handling in the test module Update exception handling in the test module Oct 18, 2021
@dkhalanskyjb dkhalanskyjb marked this pull request as ready for review October 18, 2021 16:16
@dkhalanskyjb dkhalanskyjb requested a review from qwwdfsad October 18, 2021 16:20
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-dispatchers branch from 5fd866a to d8ae171 Compare October 25, 2021 12:41
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-exception-handling branch from d70d5e2 to d651dec Compare October 25, 2021 12:45
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-dispatchers branch from d8ae171 to 80e076f Compare October 26, 2021 07:29
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-exception-handling branch from d651dec to f158e36 Compare October 26, 2021 07:39
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-dispatchers branch 2 times, most recently from b7f4216 to 015b06c Compare October 27, 2021 09:11
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-exception-handling branch 2 times, most recently from 2a12be4 to 6b589e2 Compare October 27, 2021 10:12
@qwwdfsad qwwdfsad self-requested a review October 27, 2021 15:02
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-dispatchers branch from 015b06c to e821121 Compare October 28, 2021 11:55
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-exception-handling branch from 6b589e2 to 11c3712 Compare October 28, 2021 12:06
qwwdfsad
qwwdfsad requested changes Oct 28, 2021
View reviewed changes
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.

For the record: the final decision was to hide TestExceptionHandler from public API and prohibit any custom coroutine exceptions handlers.

The proposed API has unwanted pitfalls and we do not have enough driving use-cases to provide it in its current form

@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-dispatchers branch from e821121 to 7765d76 Compare October 29, 2021 08:06
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-exception-handling branch from 11c3712 to 5ae073d Compare October 29, 2021 09:03
@dkhalanskyjb dkhalanskyjb requested a review from qwwdfsad October 29, 2021 09:11
qwwdfsad
qwwdfsad reviewed Oct 29, 2021
View reviewed changes
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.

Otherwise looks good

@dkhalanskyjb
Copy link
Collaborator Author

Forbidding passing a CoroutineExceptionHandler to createTestCoroutineScope turned out not so straightforward after all. I managed to make it work, but have doubts about the lengths to which I had to go. Could you please review the latest commit?

@dkhalanskyjb dkhalanskyjb requested a review from qwwdfsad November 1, 2021 09:54
qwwdfsad
qwwdfsad approved these changes Nov 1, 2021
View reviewed changes
kotlinx-coroutines-test/common/src/TestCoroutineScope.kt
CoroutineExceptionHandler { _, throwable ->
val ownExceptionHandler = run {
val lock = SynchronizedObject()
object : AbstractCoroutineContextElement(CoroutineExceptionHandler), TestCoroutineScopeExceptionHandler {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The overall approach looks good

@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-dispatchers branch from 7765d76 to 6a33516 Compare November 1, 2021 13:36
Base automatically changed from coroutines-test-dispatchers to coroutines-test-virtualtime November 1, 2021 13:49
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-exception-handling branch from 9e5e8f8 to 9f635e9 Compare November 1, 2021 13:53
@dkhalanskyjb dkhalanskyjb merged commit dd687c6 into coroutines-test-virtualtime Nov 1, 2021
@dkhalanskyjb dkhalanskyjb deleted the coroutines-test-exception-handling branch November 1, 2021 13:57
dkhalanskyjb added a commit that referenced this pull request Nov 17, 2021
@dkhalanskyjb
Update exception handling in the test module (#2953)
cf63d45
dkhalanskyjb added a commit that referenced this pull request Nov 17, 2021
@dkhalanskyjb
Update exception handling in the test module (#2953)
a1e3262
dkhalanskyjb added a commit that referenced this pull request Nov 19, 2021
@dkhalanskyjb
Update exception handling in the test module (#2953)
ca6bf03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qwwdfsad qwwdfsad qwwdfsad approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dkhalanskyjb @qwwdfsad