-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
qwwdfsad
reviewed
Feb 14, 2023
@@ -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() { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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()
orlaunch { some non-suspending action }; runCurrent()
. This could be replaced withjob.join()
, or even better, when we fix Dispatchers.setMain does not implement a correct Dispatchers.Main.immediate #3298, many of suchlaunch
calls will just execute undispatched, and this won't even be needed. So, I think that time control shouldn't be so prominent. - 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: callingadvanceUntilIdle
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. - 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
approved these changes
Feb 15, 2023
dkhalanskyjb
force-pushed
the
test-remove-experimentality-markers
branch
from
February 21, 2023 14:40
1145514
to
5ecec90
Compare
dkhalanskyjb
force-pushed
the
test-remove-experimentality-markers
branch
from
February 22, 2023 12:36
5ecec90
to
30b28f7
Compare
aSemy
added a commit
to aSemy/kotest
that referenced
this pull request
Jun 8, 2024
`TestDispatcher` used to be experimental, but it is not any more Kotlin/kotlinx.coroutines#3622
github-merge-queue bot
pushed a commit
to kotest/kotest
that referenced
this pull request
Jun 8, 2024
…l code (#4070) `TestScope` and `TestDispatcher` used to be experimental in Coroutines, but it is not any more Kotlin/kotlinx.coroutines#3622 Leaving the annotation on `testCoroutineScheduler` leads to unnecessary `@OptIn`s in user's projects.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.