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

Improve TestCoroutineScope #2975

Merged

Conversation

dkhalanskyjb
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb commented Oct 11, 2021

@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-coroutineScope branch 2 times, most recently from 3e872b4 to f90ba4c Compare October 13, 2021 15:01
kotlinx-coroutines-test/common/src/TestCoroutineScope.kt Outdated Show resolved Hide resolved
}
this ?: TestCoroutineExceptionHandler()
}
val job: Job
Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with the changes in general.
What puzzles me is the overall sense of passing your own job to TestCoroutineScope that does not complete when the scope semantically completes.

Let's not prevent my comment from merging it though, I'll return to it later (it's much easier to review the whole picture again when one has all the details incorporated)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The point of this approach was to avoid breakage with an unclear migration path. It's possible that someone shared a Job among several TestCoroutineScope instances, which would break if we were to complete the passed job on each cleanup.

However, I did not encounter such usage in the wild—in fact, the only instances of someone passing a Job to TestCoroutineScope that I observed were to work around the bug where, before, TestCoroutineScope was constructed without one.

Another approach to fix this particular problem would be to treat the passed job as the parent. However, it could also break some code.

Not much depends on this particular choice, so maybe we can reach a resolution right in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the things consistent then:

  • TestCoroutineScope provides its own job when it's missing from the context or uses the existing ones. It's perfectly consistent with our CoroutineScope() builder (and MainScope())
  • runTest coroutine uses the job from the scope as a parent, no matter whether it is the default one or a user-defined one. Also consistent with our builders (e.g. runBlocking or withContext)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean that, given how unlikely breakage would be, we need to avoid discerning between a user-supplied Job and the one TestCoroutineScope creates on its own?

* Add more detailed documentation;
* Move most verification logic from `runBlockingTest` to
  `cleanupTestCoroutines`
Fixes #1749
* Complete the scope's job if a new job was created for it
Fixes #1772
@qwwdfsad qwwdfsad self-requested a review October 27, 2021 15:00
@qwwdfsad
Copy link
Member

Otherwise looks good and is good to go

@dkhalanskyjb dkhalanskyjb merged commit 22e8c7d into coroutines-test-virtualtime Oct 28, 2021
@dkhalanskyjb dkhalanskyjb deleted the coroutines-test-coroutineScope branch October 28, 2021 12:10
dkhalanskyjb added a commit that referenced this pull request Nov 17, 2021
* Add more detailed documentation;
* Move most verification logic from `runBlockingTest` to
  `cleanupTestCoroutines`
Fixes #1749
dkhalanskyjb added a commit that referenced this pull request Nov 17, 2021
* Add more detailed documentation;
* Move most verification logic from `runBlockingTest` to
  `cleanupTestCoroutines`
Fixes #1749
dkhalanskyjb added a commit that referenced this pull request Nov 19, 2021
* Add more detailed documentation;
* Move most verification logic from `runBlockingTest` to
  `cleanupTestCoroutines`
Fixes #1749
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