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

Add TimeSource implementation to TestCoroutineScheduler #3087

Closed
wants to merge 1 commit into from
Closed

Add TimeSource implementation to TestCoroutineScheduler #3087

wants to merge 1 commit into from

Conversation

hfhbd
Copy link
Contributor

@hfhbd hfhbd commented Dec 15, 2021

Fixes #3086

I didn't inherit TestCoroutineScheduler from TimeSource, because you rarely use TestCoroutineScheduler.markNow() but rather use its timeSource to inject it in your time based components.
Also the method elapsedNow from TimeSource is quite confusing in a Scheduler. What happened when calling elapsedNow with its job? (Nothing...)

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

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

Nicely implemented! It was a TODO not because it was bothersome, but because we didn't want to decide immediately whether to deprecate currentTime in favor of manipulating time marks. On one hand, measureTime, markTime, etc., are more structured than manipulating currentTime's Long values, but on the other hand, TimeSource is still an experimental API, and also, a lot of tests explicitly compare currentTime with specific Long values.

However, your approach provides the functionality without railroading us into some particular API. If we decide after all to make TestCoroutineScheduler itself a TimeSource, we can just easily deprecate timeSource in favor of this and be done.

kotlinx-coroutines-test/common/src/TestScope.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-test/common/src/TestScope.kt Outdated Show resolved Hide resolved
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

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

LGTM. @qwwdfsad, what do you think about these changes?

@hfhbd
Copy link
Contributor Author

hfhbd commented Dec 15, 2021

Loosely related: Kotlin/kotlinx-datetime#164 to handle this new delay skipping behavior when using datetime.Clock as well

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.

Conceptually, I have no objections. Thanks for the listed use-case in the issue!

/**
* Returns the [TimeSource] representation of the virtual time of this scheduler.
*/
@ExperimentalTime
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it is worth marking it as ExperimentalCoroutinesApi as well in case we'll decide to implement TimeSource interface when it's stable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point!

Copy link
Contributor Author

@hfhbd hfhbd Dec 15, 2021

Choose a reason for hiding this comment

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

Sure, added

* @see TestCoroutineScheduler.timeSource
*/
@ExperimentalTime
public val TestScope.testTimeSource: TimeSource get() = testScheduler.timeSource
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't it be timeSource or schedulerTimeSource?
Also lacks ExperimentalCoroutinesApi

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb Dec 15, 2021

Choose a reason for hiding this comment

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

It was initially timeSource, but I advised against it because seeing just a bare timeSource in the middle of a test wolud be disheartening, I assumed. schedulerTimeSource is not much better than testScheduler.timeSource, so I think it's either testTimeSource or no extension on TestScope at all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Works for me, thanks for the explanation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added annotation as well

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

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

Thanks!

@dkhalanskyjb
Copy link
Collaborator

Merged manually: d5a66e0

@hfhbd hfhbd deleted the hfhbd/timeSource branch December 16, 2021 09:43
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

3 participants