Compute timestamp Using Date.now (without deriving from duration) #1431
Conversation
|
@charlierudolph @davidjgoss are we ready to cut a 7.0.0 after this PR is merged? If so, I'd love to do that tomorrow. If not - what's missing? |
| return { | ||
| seconds, | ||
| nanos, | ||
| // TODO: Remove. It's impossible to convert timestamps to durations and vice-versa |
There was a problem hiding this comment.
What is preventing us from doing this now?
There was a problem hiding this comment.
Nothing in particular - just didn't want to go down the rabbit hole. My thinking is that we need a more thorough review/overhaul of how we handle wall time vs monotonic time in a separate PR, and this can be addressed then.
There was a problem hiding this comment.
Okay yeah, so the stopwatch concept I guess becomes unnecessary and we should probably be able to infer any durations we want by finding the started/finished messages and using their timestamps.
(I'm happy to pick that up BTW since this is borne of my mistake.)
There was a problem hiding this comment.
We should use monotonic timestamps to calculate duration values and wall time to calculate timestamp values.
In JavaScript, wall time (CLOCK_REALTIME) is used in Date.now(). Monotonic time (CLOCK_MONOTONIC) is used in performance.now() (available on window in browsers and from perf_hooks in Node.js).
What's important to realise is that wall time can go backwards. In other words, Date.now() is unsuitable for durations. It's only suitable for timestamps. On the flip side, performance.now() is the right choice for durations, and the wrong choice for timestamps.
The only reason we're calculating the test run duration with wall time is that I found it difficult to do it with the current stopwatch. Now that I think of it, it's probably not that hard.
I think all of this will be easier if we drop the duration and durations modules and implement our own. It's also a good way to understand how to implement a stopwatch and a clock.
I propose the following API for a (wall) clock:
type Timestamp = number
type Clock = () => Timestamp
const clock: Clock = ...
const timestamp = clock()And a stop watch:
type Duration = number
type StartStopwatch = () => StopStopwatch
type StopStopwatch = () => Duration
const startStopwatch: StartStopwatch = ...
const stopStopwatch = startStopWatch()
doSomeWork()
const duration = stopStopwatch()I started this in #1430 but I abandoned it in favour of this PR because I went too far down a rabbit hole. I'd like to give it another try though. Or maybe you want to have a stab at it @davidjgoss?
|
@aslakhellesoy I think we're all set. Except maybe #1425 would be good - I'm going to carry on with that this evening so will check back here later. |
…cumber#1431) This fixes a few bugs with time/duration handling: * The timestamp of testRunStarted was 0. It's been fixed to be an actual timestamp (Date.now). * The timestamp of testRunFinshed was not a timestamp, but a duration camouflaged as a timestamp. * The stopwatch.timestamp() method no longer calculates the timestamp from a duration (which is impossible). Instead it uses Date.now() * Previously the reports test run duration was based on the testRunFinished timestamp alone, which didn't make sense. A timestamp is not a duration. Instead we calculate it using the difference of the testRunStarted and testRunFinished timestamps We think there is still some work to do to clean up the timestamp/duration code, but this should be done in a separate PR. This PR simply focuses on getting correct values into the messages. Co-authored-by: Aslak Hellesøy <[email protected]>
This fixes a few bugs with time/duration handling
We think there is still some work to do to clean up the timestamp/duration code, but this should be done in a separate PR. This PR simply focuses on getting correct values into the messages.