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

Remove some ExperimentalCoroutinesApi markers in the test module #3622

Merged
merged 2 commits into from
Feb 22, 2023

Conversation

dkhalanskyjb
Copy link
Contributor

No description provided.

@@ -129,7 +127,6 @@ public class TestCoroutineScheduler : AbstractCoroutineContextElement(TestCorout
/**
* Runs the tasks that are scheduled to execute at this moment of virtual time.
*/
@ExperimentalCoroutinesApi
public fun runCurrent() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please elaborate on why TestScope counterparts of stabilized functions (e.g. TestScope.runCurrent) are left experimental?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Time control is heavily misused: see most examples in https://grep.app/search?q=advanceUntilIdle. There are almost always alternative ways to express the same intent without the magic of "and here we stop until we magically know that there's no more work to be done". Almost all examples of using time control in the wild are just of the form launch { ... }; advanceUntilIdle() or launch { some non-suspending action }; runCurrent(). This could be replaced with job.join(), or even better, when we fix Dispatchers.setMain does not implement a correct Dispatchers.Main.immediate #3298, many of such launch calls will just execute undispatched, and this won't even be needed. So, I think that time control shouldn't be so prominent.
  2. When one does want such "stop the world, I want one particular execution, and I am going to get it" magic, the time control does not provide this behavior in case of external dispatches. You only get the behavior you asked for when you only use the test scheduler and nothing else. TestScope doesn't highlight this on its own: calling advanceUntilIdle in the middle of the test, one could easily forget that waiting for a network request to return a value is also considered to be idleness. So, I think time control should be syntactically linked to the test dispatcher.
  3. All that said, I've seen actual use cases where time control significantly simplifies tests. This is all in fairly advanced cases, so the users know what they're doing and can afford to search a bit for time control capabilities. So, I think that, sufficiently hidden from the public eye, time control can be useful and we should stabilize it for the scheduler.

@qwwdfsad qwwdfsad self-requested a review February 15, 2023 12:26
Base automatically changed from coroutines-test-timeout to develop February 21, 2023 14:35
@dkhalanskyjb dkhalanskyjb force-pushed the test-remove-experimentality-markers branch from 1145514 to 5ecec90 Compare February 21, 2023 14:40
@dkhalanskyjb dkhalanskyjb force-pushed the test-remove-experimentality-markers branch from 5ecec90 to 30b28f7 Compare February 22, 2023 12:36
@dkhalanskyjb dkhalanskyjb merged commit bf03c48 into develop Feb 22, 2023
@dkhalanskyjb dkhalanskyjb deleted the test-remove-experimentality-markers branch February 22, 2023 12:36
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