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

Implement new test dispatchers #2986

Merged

Conversation

dkhalanskyjb
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb commented Oct 14, 2021

Defines two test dispatchers:

  • StandardTestDispatcher, which, combined with runTest, gives an illusion of an event loop;
  • UnconfinedTestDispatcher, which is like Dispatchers.Unconfined, but skips delays.

By default, StandardTestDispatcher is used due to the somewhat chaotic execution order of Dispatchers.Unconfined. TestCoroutineDispatcher is deprecated.

Fixes #1626
Fixes #1742
Fixes #2082
Fixes #2102
Fixes #2405
Fixes #2462

@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-await-other-dispatchers branch 5 times, most recently from 98102de to 608783e Compare October 15, 2021 12:33
@dkhalanskyjb dkhalanskyjb marked this pull request as ready for review October 15, 2021 15:45
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-await-other-dispatchers branch from 608783e to d694912 Compare October 25, 2021 12:34
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-await-other-dispatchers branch from d694912 to 77d524f Compare October 26, 2021 07:29
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-await-other-dispatchers branch from 77d524f to 78e5855 Compare October 27, 2021 08:07
@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-await-other-dispatchers branch from 15d1dec to a8e43d6 Compare October 28, 2021 11:41
* scheduler in order for the tasks to run.
*/
@ExperimentalCoroutinesApi
public class UnconfinedTestDispatcher(
Copy link
Member

Choose a reason for hiding this comment

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

I do not recall our final decision regarding this, maybe we've discussed it and decided not to do:

Are we going to add runTestUnconfined for people to gracefully migrate from runBlockingTest and to have a counterpart for runTest?

Copy link
Member

Choose a reason for hiding this comment

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

Also, we can then bulk-replace deprecated runBlockingTest in our tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are we going to add runTestUnconfined for people to gracefully migrate from runBlockingTest

We didn't decide on this in our discussion. However, checking more closely how Unconfined behaves, I discovered that it does not, in fact, enter launch and async blocks eagerly. The only reliable behavior I got from it is that launching a coroutine with Unconfined leads to all dispatches to that coroutine from non-unconfined dispatchers to happen eagerly. Launching the whole test under UnconfinedTestDispatcher leads to haphazard execution. I still decided to keep it given how useful the unconfined dispatcher is when child coroutines are launched with it.

So, it may turn out that we'll need what is now the TestCoroutineDispatcher after all, just with a different name, and probably with slightly different behavior.

Also, we can then bulk-replace deprecated runBlockingTest in our tests

I don't think we should do that. The only tests that use runBlockingTest are the legacy ones that ensure that we didn't break a lot of existing code during the transition. I checked the tests that use runBlockingTest and, for each one that I found useful, I added the corresponding version in terms of the new primitives.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(This is a test to see if I can still write in this discussion; a Github bug could prevent me)

*
* Additionally, [name] can be set to distinguish each dispatcher instance when debugging.
*
* @see StandardTestDispatcher for a more predictable [TestDispatcher] that, however, requires interacting with the
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's worth to avoid such caution, especially if we would like to promote standard dispatcher (and, well, runTest in general) as the default one

Copy link
Member

Choose a reason for hiding this comment

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

Also, it may be good to state that such dispatcher is intended to be used in advanced scenarios and show one with a code example to prevent people from using it in a new code where it is not necessary and to warn that this may be not the best migration path from runBlockingTest

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Base automatically changed from coroutines-test-await-other-dispatchers to coroutines-test-virtualtime November 1, 2021 13:26
@dkhalanskyjb dkhalanskyjb merged commit 776250b into coroutines-test-virtualtime Nov 1, 2021
@dkhalanskyjb dkhalanskyjb deleted the coroutines-test-dispatchers branch November 1, 2021 13:49
dkhalanskyjb added a commit that referenced this pull request Nov 17, 2021
Defines two test dispatchers:
* StandardTestDispatcher, which, combined with runTest,
  gives an illusion of an event loop;
* UnconfinedTestDispatcher, which is like
  Dispatchers.Unconfined, but skips delays.

By default, StandardTestDispatcher is used due to the somewhat
chaotic execution order of Dispatchers.Unconfined.
TestCoroutineDispatcher is deprecated.

Fixes #1626
Fixes #1742
Fixes #2082
Fixes #2102
Fixes #2405
Fixes #2462
dkhalanskyjb added a commit that referenced this pull request Nov 17, 2021
Defines two test dispatchers:
* StandardTestDispatcher, which, combined with runTest,
  gives an illusion of an event loop;
* UnconfinedTestDispatcher, which is like
  Dispatchers.Unconfined, but skips delays.

By default, StandardTestDispatcher is used due to the somewhat
chaotic execution order of Dispatchers.Unconfined.
TestCoroutineDispatcher is deprecated.

Fixes #1626
Fixes #1742
Fixes #2082
Fixes #2102
Fixes #2405
Fixes #2462
dkhalanskyjb added a commit that referenced this pull request Nov 19, 2021
Defines two test dispatchers:
* StandardTestDispatcher, which, combined with runTest,
  gives an illusion of an event loop;
* UnconfinedTestDispatcher, which is like
  Dispatchers.Unconfined, but skips delays.

By default, StandardTestDispatcher is used due to the somewhat
chaotic execution order of Dispatchers.Unconfined.
TestCoroutineDispatcher is deprecated.

Fixes #1626
Fixes #1742
Fixes #2082
Fixes #2102
Fixes #2405
Fixes #2462
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