8000 console: display timeEnd with suitable time unit by Xstoudi · Pull Request #29251 · nodejs/node · GitHub
[go: up one dir, main page]
Skip to content

console: display timeEnd with suitable time unit #29251

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.

Sign up for GitHub

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

Jump to bottom
Closed
Xstoudi wants to merge 9 commits into from
Closed

console: display timeEnd with suitable time unit #29251

Xstoudi wants to merge 9 commits into from

Conversation

@Xstoudi
Copy link
Contributor
@Xstoudi Xstoudi commented Aug 21, 2019 •
edited
Loading

When timeEnd function is called, display result with a suitable
time unit instead of a big amount of milliseconds.

Refs: #29099

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the console Issues and PRs related to the console subsystem. label Aug 21, 2019
cjihrig
cjihrig reviewed Aug 21, 2019
View reviewed changes
Copy link
Contributor
@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Can you please add tests.

@Xstoudi
Copy link
Contributor Author
Xstoudi commented Aug 21, 2019

I'm aware that the PR needs tests but I'm a little bit afraid of writing tests incorrectly regarding the fact that tests with timers can be flaky. Can someone give me a clue about how to achieve this correctly?

@cjihrig
Copy link
Contributor
cjihrig commented Aug 21, 2019

Since testing this would require large timers, (which won't work in the CI) to be able to test all the code paths here you can try a few things. You could monkey patch process.hrtime(). You could also create a separate function with all of this logic and export it (since this is an internal module) for testing.

devsnek
devsnek reviewed Aug 21, 2019
View reviewed changes
lib/internal/console/constructor.js Outdated
Copy link
Member
@devsnek devsnek Aug 21, 2019

Choose a reason for hiding this comment

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

i think we should always show the milliseconds

Copy link
Member
@targos targos Aug 22, 2019

Choose a reason for hiding this comment

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

I'm not convinced. even in case it's a few seconds, I don't think we need more than millisecond precision.

Copy link
Member
@devsnek devsnek Aug 22, 2019

Choose a reason for hiding this comment

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

if we don't include milliseconds can we remove the toFixed?

Copy link
Contributor Author
@Xstoudi Xstoudi Aug 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

time and timeEnd couple seems to be here to allow developers to get an idea of which time scale does a part of their code take to run. I think it's usefull to know whether the code takes 500ms, 1, 1min or 1h to run but I don't see any case where the difference between 1h and 1.0000000000000001h is important. People that need high precision can still use process.hrtime.

Copy link
Member
@BridgeAR BridgeAR Sep 3, 2019

Choose a reason for hiding this comment

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

I am in favor of changing the representation in general but I think it would have been good to keep the milliseconds. I think it would have been ideal to visualize it as: n1h, n2m, n3s and n4ms (n1 - n4 as value) while each leading part would be optional as long as all former ones are zero. Right now you have to calculate in a fraction of an hour or minutes and that is not very intuitive.

targos
targos reviewed Aug 22, 2019
View reviewed changes
@nodejs-github-bot
Copy link
Collaborator
nodejs-github-bot commented Aug 22, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/25181/

@nodejs-github-bot
Copy link
Collaborator
nodejs-github-bot commented Aug 22, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/25184/

@targos
Copy link
Member
targos commented Aug 22, 2019

@Trott I'm not sure what you force-pushed? I helped @Xstoudi to prepare this PR but I never touched git myself and he doesn't have access to the main repo

@targos
Copy link
Member
targos commented Aug 22, 2019

This pull request now has two more commits. I don't understand how they arrived here 😕