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 asClock converter #164

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hfhbd
Copy link
Contributor

@hfhbd hfhbd commented Dec 15, 2021

Usecase: Using the coroutines TestCoroutinesScheduler timeSource to sync skipped delays with this Clock

@Test
fun integrationTest() = runTest {
    val clock = testTimeSource.toClock()
    assertEquals(Instant.fromEpochSeconds(0), clock.now())
    delay(1.seconds)
    assertEquals(Instant.fromEpochSeconds(1), clock.now())
}

@dkhalanskyjb
Copy link
Contributor

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 Instant values after calls to delay? If so, what does that code do?

@hfhbd
Copy link
Contributor Author

hfhbd commented Dec 20, 2021

Almost every high level datetime usage:
For example storing instants in your database with Exposed: https://github.com/JetBrains/Exposed/tree/master/exposed-kotlin-datetime. The kotlin Instant is converted to Java Instant, which is converted to Java.Sql.Timestamp. Without syncing the Clock, the Clock and the TimeSource does not match when storing an Instant, delaying and fetching it again.

real world usage:
Store the list of daily events with a starttime in your database.

  1. Set clock to epoch (test) or current (production)
  2. Load the events in the db from a file (test) or user provided file (production)
  3. Get all events for the current day
  4. Next day, mock (test) or sleep/wait (production)
  5. Get all events for the next day
  6. Compare the fetched results with the expectation

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 testTimeSource and this converter you can finally mock the internal clock for RFC compliant headers (which needs UTC Instants). currently, you need to use runBlocking { delay() } and wait for server clock changes.
So getting a delay-synced TimeSource via testTimeSource is nice for lower level apis, but for syncing DateTime APIs, you need a Clock converter.

@hfhbd
Copy link
Contributor Author

hfhbd commented Dec 20, 2021

BTW regarding TestTimeSource: Even if you use this mockable TimeSource, how can you convert it to a Clock? This is the real issue, not syncing the TimeSource with coroutines. Syncing it with coroutines is nice and works out of the box with testTimeSource. But you cannot connect a TimeSource with a Clock.

@dkhalanskyjb
Copy link
Contributor

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, TimeSource just isn't a Clock in the sense that it produces Instants, which are defined as numbers of seconds since Jan 1, 1970. For example, consider TimeSource.Monotonic, which should be always monotonic and is typically linked to the time elapsed since system boot. You convert this time source to Clock:

val currentTime = Clock.System.now() // instant1
val clock = TimeSource.Monotonic.toClock(currentTime) // instant2

Initially, the difference between what Clock.System.now() reports and what clock reports is instant2 - instant1, which should be negligible. However, let's say your system clock gets corrected at some point. For example, my personal laptop has a fast and very imprecise clock, and so it has to sync daily via NTP, and by each sync, about two seconds are subtracted. In this case, after the sync, Clock.System.now() adapts to the new system clock, so it may look like its measurements went back in time somewhat. In fact, they simply became more precise. On the other hand, clock, being monotonic, will not move back, it will keep those two seconds.

If a program using clock runs for about a month on my laptop, then Clock.System.now() and clock will be off by a whole minute.

Also, monotonic clocks are not defined to be precise. Not sure how we implement this in Kotlin, but Linux implements the POSIX CLOCK_MONOTONIC in a way that discards the time spent when the system was suspended. So, if Kotlin uses CLOCK_MONOTONIC for its TimeSource.Monotonic, then, on Linux, after a night of sleep, clock and Clock.System.now() would differ by ~9 hours.

@dkhalanskyjb
Copy link
Contributor

BTW regarding TestTimeSource

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.

@hfhbd
Copy link
Contributor Author

hfhbd commented Dec 20, 2021

Yes, this is absolutely correct... Honestly I know this case, because simple speaking, TimeSource provides only some TimeMarks ("a number"), which has absolutely no contracts regarding precision and increasing. In fact, it has well-known side effects, like you mentioned.
In production, you should not use this current implementation, and this converter at all.

I would/will only use this converter function for testing, either TestTimeSource or TestScope.testTimeSource, and never for production.
At least, this should be clear in the docs, or even better limit it to testing code only.
One possibility I know is some kind of marker interface: TestingTimeSource: TimeSource and define the converter on this interface. For supporting TestScope.testingTimeSource, this means, it requires adding this marker interface to the stdlib.
Alternative adding a custom annotation eg @TestingOnly, this would not require some new interfaces, but is ugly.

@hfhbd
Copy link
Contributor Author

hfhbd commented Dec 20, 2021

If only interaction between coroutines and datetime was the issue, we could maybe imagine some other solution.

Sorry, I just want to add something :D Sure, we could add a TestScope.testClock: Clock to coroutines-test, but this would require adding datetime lib as dependency to coroutines-test.

With this solution, this converter function could be limited to TestTimeSource. (Although I don't like limiting a function to a specific implementation, this prevents using this function from a custom MockTimeSource)

@dkhalanskyjb
Copy link
Contributor

My reply was to your point about storing Instant values:

For example storing instants in your database with Exposed: https://github.com/JetBrains/Exposed/tree/master/exposed-kotlin-datetime. The kotlin Instant is converted to Java Instant, which is converted to Java.Sql.Timestamp. Without syncing the Clock, the Clock and the TimeSource does not match when storing an Instant, delaying and fetching it again.

I thought you meant this as something to do in production. Maybe I misinterpreted something. Could you explain this scenario more specifically?

@hfhbd
Copy link
Contributor Author

hfhbd commented Dec 20, 2021

At some point, providing a sample project sounds simpler :D
I will do it later, but now I can give you this demo code only:

// 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 Events to test, the duration arithmetic and this test code is quite complex.

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 Clock as parameter would allow you to simplify inject the clock. On possibility would be creating a custom TestClock, but using the already existing TestTimeSource and convert it sounds better. Futhermore this converter also allows you to support other TimeSources as well.

@hfhbd
Copy link
Contributor Author

hfhbd commented Dec 20, 2021

Here is a sample project: https://github.com/hfhbd/events
There are 5 different implementations testing the EventController, from runBlocking using Clock.System to TestTimeSource.toClock() usage. Additionally, there is also a runTest implementation using delay.

@hfhbd
Copy link
Contributor Author

hfhbd commented Jan 10, 2022

@dkhalanskyjb
This extension should indeed only be used in tests.

So how do we make this limitation clear?

  • rename the function fun TimeSource.toTestClock(): Clock or similar
  • rename the function fun TestClock(timeSource: TimeSource): Clock or similar
  • move it to a separate datetime-test module
  • (Add an annotation @TestingOnly)

@hfhbd
Copy link
Contributor Author

hfhbd commented Dec 29, 2022

What about adding it to coroutines-test instead?
This would require adding datetime as a dependency to the test library too.

While the use-case TimeSource.toClock also affects the stdlib TimeSource, (so you can also use it with TestTimeSouce and other custom implementations), I think the most use-case is using toClock inside runTest { testTimeSource.toClock() }.

core/common/src/Clock.kt Outdated Show resolved Hide resolved
* parameter.
*/
@ExperimentalTime
public fun TimeSource.toClock(offset: Instant = Instant.fromEpochSeconds(0)): Clock = object : Clock {
Copy link
Member

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?

Copy link
Contributor Author

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.

@hfhbd hfhbd changed the title Add TimeSource toClock converter Add TimeSource asClock converter Apr 29, 2023
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ilya-g If you plan to merge #271 first, I would move these tests to the ClockTimeSourceTest.

@kevincianfarini
Copy link

kevincianfarini commented Feb 12, 2024

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 TestScheduler.

@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 testTimeSource like the following:

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 delayUntilNextCalendarDay. In my opinion, one of the purposes of kotlinx-datetime is to make reasoning about those kinds of scenarios easier.

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.

@kevincianfarini
Copy link

kevincianfarini commented Feb 12, 2024

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, TimeSource just isn't a Clock in the sense that it produces Instants, which are defined as numbers of seconds since Jan 1, 1970. For example, consider TimeSource.Monotonic, which should be always monotonic and is typically linked to the time elapsed since system boot. You convert this time source to Clock:

Regarding the point about this functionality only ever being available under test, it seems like there's a few use cases for people wanting Clock testing support. I raised this in the Kotlin Slack a few months ago.

If we wanted to we could include this facility in akotlinx-datetime-test artifact alongside some other Clock testing support for people to use. Maybe it could include:

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.

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

4 participants