-
Notifications
You must be signed in to change notification settings - Fork 101
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 asClock converter #164
base: master
Are you sure you want to change the base?
Conversation
1dee30a
to
b544b69
Compare
Let's discuss the use case. Why do you need this synchronization? Is TestTimeSource not enough and there's some interplay between the datetime code and the coroutines code that requires observing |
Almost every high level datetime usage: real world usage:
Next usecase is Ktor: Currently, it is not possible mock Ktors clock (not related to this issue), but I am currently developing the switch to kotlinx-datetime. With |
BTW regarding TestTimeSource: Even if you use this mockable |
Ok, the second case is clear, thanks: for testing the behavior of a system that stores/logs/sends timestamps, this is indeed useful, and will work fine. However, the first use case, as well as this change in general, makes me uneasy. The reason is that, well, val currentTime = Clock.System.now() // instant1
val clock = TimeSource.Monotonic.toClock(currentTime) // instant2 Initially, the difference between what If a program using Also, monotonic clocks are not defined to be precise. Not sure how we implement this in Kotlin, but Linux implements the POSIX |
I was asking specifically about the use case of interacting with coroutines. If only interaction between coroutines and datetime was the issue, we could maybe imagine some other solution. I was interested in why coroutines exactly. You already answered this, so nevermind. |
Yes, this is absolutely correct... Honestly I know this case, because simple speaking, I would/will only use this converter function for testing, either |
Sorry, I just want to add something :D Sure, we could add a With this solution, this converter function could be limited to |
My reply was to your point about storing
I thought you meant this as something to do in production. Maybe I misinterpreted something. Could you explain this scenario more specifically? |
At some point, providing a sample project sounds simpler :D // current implementation
class EventController(val db: exposed.Database) {
fun store(eventName: String, at: Instant) = db.save(...)
fun getUpcomingEvents(now: Instant = Clock.System.now): List<Event> = db.fetch(where = {
Event.at greaterThan now
}).map(Event::create)
}
// testing
val testDB = ...
val controller = EventController(testDB)
assertEmpty(controller.getUpcomingEvents(now = Instant.fromEpochSeconds(0)))
val instant1 = Instant.fromEpochSeconds(42)
controller.store("Foo", instant1)
assertNotEmpty(controller.getUpcomingEvents(now = instant1 - 10.seconds))
assertEmpty(controller.getUpcomingEvents(now = instant1 + 10.seconds)) But with this implementation, you need to use Instants everywhere. In testing, you need to add/subtract the duration. With longer/more This code would be nicer: // current implementation
class EventController(val db: exposed.Database, val clock: Clock = Clock.System) {
fun store(eventName: String, at: Instant) = db.save(...)
fun getUpcomingEvents(): List<Event> = db.fetch(where = {
Event.at greaterThan clock.now()
}).map(Event::create)
}
// testing
val testDB = ...
val testClock = ??? // TestTimeSource().toClock() would be nice
val controller = EventController(testDB, testClock)
assertEmpty(controller.getUpcomingEvents())
val instant1 = testClock.now() + 5.seconds
controller.store("Foo", instant1)
assertNotEmpty(controller.getUpcomingEvents())
testClock += 10.seconds
assertEmpty(controller.getUpcomingEvents()) So using a |
Here is a sample project: https://github.com/hfhbd/events |
@dkhalanskyjb So how do we make this limitation clear?
|
What about adding it to While the use-case |
core/common/src/Clock.kt
Outdated
* parameter. | ||
*/ | ||
@ExperimentalTime | ||
public fun TimeSource.toClock(offset: Instant = Instant.fromEpochSeconds(0)): Clock = object : Clock { |
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.
Have you considered some other default value for offset
, e.g. Clock.System.now()
, or no default value at all?
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.
I did. The main use-case is converting the testTimeSource of coroutines to a Clock for high-level datetime usage, like Instant, with coroutines sync support, like delay
. I prefer deterministic values for easier testing, like assertEquals or using test fixtures. With this use-case, I wouldn't use Clock.System.now()
using the current changing time.
I am open to having no default value, but this would require passing an offset each time using this converter and if you want to customize the offset, you can already do it by passing your offset explicitly. So I don't see a reason to remove a convenience default value.
I used the start of the epoch, because, well, it is the start and it's commonly used for test fixtures.
I want to chime in here that I regularly use similar functionality. I currently have a definition similar to this PR in my codebase, but it's closely tied to a kotlinx-coroutines @OptIn(ExperimentalCoroutinesApi::class)
private class TestClock(private val scheduler: TestCoroutineScheduler) : Clock {
override fun now() = Instant.fromEpochMilliseconds(scheduler.currentTime)
}
/**
* Acquire a [Clock] from [TestScope] that reflects the time of the underlying
* [TestCoroutineScheduler]. Advances in virtual time of the test scheduler via
* calls like delay will be reflected in the values this clock returns.
*
* [TestCoroutineScheduler.currentTime] is by default set to the UNIX epoch,
* January 1, 1970 00:00. This can be adjusted by passing in a different
* [initialReference].
*/
@OptIn(ExperimentalCoroutinesApi::class)
fun TestScope.virtualTimeClock(
initialReference: Instant = Instant.fromEpochMilliseconds(0)
): Clock {
advanceTimeBy(initialReference - Instant.fromEpochMilliseconds(0))
return TestClock(testScheduler)
} Usages of this class are particularly useful for asserting that you delay until a specific moment in time, and not as useful for determining for how long your code delayed. For example, rather than asserting that a certain amount of time passes with val clock = virtualTimeClock()
val duration = testTimeSource.measureTime {
clock.delayUntilNextCalendarDay()
}
assertEquals(expected = 24.hours, actual = duration) I find it to be more robust to express it like the following: val todayMidnight = LocalDateTime(/* omitted */)
val tomorrowMidnight = LocalDateTime(/* omitted */)
val timeZone = TimeZone.of("Europe/London")
val clock = virtualTimeClock(todayMidnight.toInstant(timeZone))
clock.delayUntilNextCalendarDay()
assertEquals(tomorrowMidnight.toInstant(timeZone), clock.now()) The former example makes the erroneous assumption that the duration between today and tomorrow will be 24 hours, when it won't be for every period in time prior to calling In the second example, the intricacies of timezone conversions is handled by kotlinx-datetime and I am indeed able to reason about this scenario much easier! This of course isn't completely fault tolerant because of ambiguities converting between local time and UTC time, but it's a lot safer than the first example. |
Regarding the point about this functionality only ever being available under test, it seems like there's a few use cases for people wanting If we wanted to we could include this facility in a public fun TimeSource.asTestClock(offset: Instant = Instant.fromEpochSeconds(0)): Clock = object : Clock {
private val startMark: TimeMark = markNow()
override fun now() = offset + startMark.elapsedNow()
}
class StaticTestClock(private val now: Instant) {
override fun now(): Instant = now
}
class DynamicTestClock(val times: ArrayDeque<Instant>) {
override fun now(): Instant = times.removeFirst()
} If the maintainers of this repository are amenable to this idea, I'm happy to raise a PR. |
core/common/src/Clock.kt
Outdated
@ExperimentalTime | ||
public fun TimeSource.asClock(offset: Instant = Instant.fromEpochSeconds(0)): Clock = object : Clock { |
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.
We've deliberated internally about this function, and it does seem useful. A couple of notes:
- The name
offset
leaves the impression that it's somehow related toUtcOffset
, but it's not. We arrived at the nameorigin
instead. - Having a default parameter here leaves the wrong impression.
TimeSource.Monotonic.asClock()
seems benign, but starts returning dates far in the past.Clock.System.now()
would be less surprising, but it's nondeterministic, which is a bad fit for tests (which we'd expect to be the most common use case).
core/common/src/Clock.kt
Outdated
* into different [Instant]s iff the time of the [TimeSource] was increased. To sync different [Clock]s, use the [offset] | ||
* parameter. | ||
*/ | ||
@ExperimentalTime |
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.
Looks like, after all this time, this is no longer needed!
Co-authored-by: Dmitry Khalanskiy <[email protected]>
Usecase: Using the coroutines TestCoroutinesScheduler timeSource to sync skipped delays with this Clock