-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
fix: display test duration even if time is mocked out #7264
Conversation
flow-typed/convert-hrtime.js
Outdated
|
||
declare module 'convert-hrtime' { | ||
declare export default ( | ||
hrtime: [number, number], |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
@@ -240,7 +241,12 @@ export const callAsyncCircusFn = ( | |||
|
|||
export const getTestDuration = (test: TestEntry): ?number => { | |||
const {startedAt} = test; | |||
return startedAt ? Date.now() - startedAt : null; |
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.
Previously it was missing completely when time was fakes as startedAt === 0
which was falsy. using hrtime
it changed to always say 0ms
. The babel plugin ensures we reference the real process.hrtime
now
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.
Isn't the millisecond resolution from Date.now
enough or is there any other reason to use process.hrtime
?
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.
In theory it's more accurate, although I don't think that matters in practice.
Any reason to prefer Date
?
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.
If we don't need a more accurate precision than milliseconds we can stick to Date. I'd prefer it for simplicity.
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.
Changed back, but fixed the startedAt === 0
case
Codecov Report
@@ Coverage Diff @@
## master #7264 +/- ##
==========================================
+ Coverage 66.52% 66.53% +<.01%
==========================================
Files 241 241
Lines 9308 9310 +2
Branches 6 5 -1
==========================================
+ Hits 6192 6194 +2
Misses 3113 3113
Partials 3 3
Continue to review full report at Codecov.
|
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.
Look good. Thanks!
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
As discussed in #7259. I'm not sure why Jasmine does not have the same issue. I don't care enough to dig though, this fixes it for Circus 🙂
Test plan
Integration test added.