console: display timeEnd with suitable time unit #29251

Xstoudi · 2019-08-21T16:48:16Z

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

Checklist

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

cjihrig

Can you please add tests.

Xstoudi · 2019-08-21T17:00:38Z

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 · 2019-08-21T17:04:53Z

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 · 2019-08-21T21:52:29Z

lib/internal/console/constructor.js

+    unit = 's';
+  }
+
+  return value.toFixed(3) + unit;


i think we should always show the milliseconds

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

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

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.

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.

test/parallel/test-console.js

nodejs-github-bot · 2019-08-22T10:01:15Z

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

nodejs-github-bot · 2019-08-22T13:29:29Z

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

targos · 2019-08-22T14:54:43Z

@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 · 2019-08-22T14:55:45Z

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

Trott · 2019-08-23T04:08:28Z

@Trott I'm not sure what you force-pushed?

@targos You can see what I force-pushed out of master at 94e47b8 and e460838. I commented on those commits, which I guess somehow are also in this branch? I didn't force push anything in this branch. I force-pushed to master branch to clear out those two commits from master.

Trott · 2019-08-23T04:09:07Z

It's possible that those two commits got added to this branch as part of a rebase against master in the time when those commits were on master. Perhaps they can/should be force-pushed out of this branch.

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

Extract the format time part to be able to test it.

Change regexp of test-console to accept other possible units and create test for formatTime

Tests in test-console.js expect 3 floating points.

Assert that returnes value as 3 floating points

Edit the file to fulfill the linter requirements

Hour, minute and second equivalent in ms are now constants.

Xstoudi · 2019-08-23T14:58:15Z

I removed the documentation is changed or added checkbox because I think it doesn't require a change in the documentation. Let me know if it's not the case.

nodejs-github-bot · 2019-08-23T20:57:07Z

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

When timeEnd function is called, display result with a suitable time unit instead of a big amount of milliseconds. Refs: #29099 PR-URL: #29251 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

addaleax · 2019-08-25T14:47:53Z

Landed in 66043e1, thanks for the PR! 🎉

BridgeAR · 2019-09-03T13:49:26Z

This feels like a semver-major change to me. I can imagine tests hooking into output to verify a specific min/max delay (there are definitely more straight forward ways to do that but who knows).

When timeEnd function is called, display result with a suitable time unit instead of a big amount of milliseconds. Refs: #29099 PR-URL: #29251 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

sam-github · 2019-09-03T22:37:04Z

I agree, though not strongly. If anyone really feels this is not major and should land in 12.x, feel free to remove the semver-major label, as far as I'm concerned.

Notable changes: - **assert**: - do not repeat .throws() code (Ruben Bridgewater) [#28263](#28263) - wrap validation function errors (Ruben Bridgewater) [#28263](#28263) - fix generatedMessage property (Ruben Bridgewater) [#28263](#28263) - improve class instance errors (Ruben Bridgewater) [#28263](#28263) - **benchmark**: - use test/common/tmpdir consistently (João Reis) [#28858](#28858) - **build**: - make full-icu the default for releases (Richard Lau) [#29887](#29887) - update minimum Xcode version for macOS (Michael Dawson) [#29622](#29622) - **child_process**: - runtime deprecate \_channel (cjihrig) [#27949](#27949) - simplify spawn argument parsing (cjihrig) [#27854](#27854) - **console**: - display timeEnd with suitable time unit (Xavier Stouder) [#29251](#29251) - **deps**: - patch V8 to 7.8.279.14 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.12 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.10 (Myles Borins) [#29694](#29694) - update V8's postmortem script (cjihrig) [#29694](#29694) - V8: cherry-pick 716875d (Myles Borins) [#29694](#29694) - update V8 to 7.8.279.9 (Myles Borins) [#29694](#29694) - V8: cherry-pick b33af60 (Michaël Zasso) [#28016](#28016) - update V8 to 7.6.303.28 (Michaël Zasso) [#28016](#28016) - **domain**: - error handler runs outside of its domain (Julien Gilli) [#26211](#26211) - **fs**: - make FSWatcher.start private (Lucas Holmquist) [#29905](#29905) - add runtime deprecate for file stream open() (Robert Nagy) [#29061](#29061) - allow int64 offset in fs.write/writeSync/fd.write (Zach Bjornson) [#26572](#26572) - use IsSafeJsInt instead of IsNumber for ftruncate (Zach Bjornson) [#26572](#26572) - allow int64 offset in fs.read/readSync/fd.read (Zach Bjornson) [#26572](#26572) - close file descriptor of promisified truncate (João Reis) [#28858](#28858) - **http**: - do not emit end after aborted (Robert Nagy) [#27984](#27984) - don't emit 'data' after 'error' (Robert Nagy) [#28711](#28711) - remove legacy parser (Anna Henningsen) [#29589](#29589) - throw if 'host' agent header is not a string value (Giorgos Ntemiris) [#29568](#29568) - replace superfluous connection property with getter/setter (Robert Nagy) [#29015](#29015) - fix test where aborted should not be emitted (Robert Nagy) [#20077](#20077) - remove default 'timeout' listener on upgrade (Luigi Pinca) [#26030](#26030) - **http, http2**: - remove default server timeout (Ali Ijaz Sheikh) [#27558](#27558) - **http2**: - remove security revert flags (Anna Henningsen) [#29141](#29141) - remove callback-based padding (Anna Henningsen) [#29144](#29144) - **lib**: - rename validateInteger to validateSafeInteger (Zach Bjornson) [#26572](#26572) - correct error.errno to always be numeric (Joyee Cheung) [#28140](#28140) - no need to strip BOM or shebang for scripts (Refael Ackermann) [#27375](#27375) - rework logic of stripping BOM+Shebang from commonjs (Gus Caplan) [#27768](#27768) - **module**: - runtime deprecate createRequireFromPath() (cjihrig) [#27951](#27951) - **readline**: - error on falsy values for callback (Sam Roberts) [#28109](#28109) - **repl**: - close file descriptor of history file (João Reis) [#28858](#28858) - **src**: - bring 425 status code name into accordance with RFC 8470 (Sergei Osipov) [#29880](#29880) - update NODE\_MODULE\_VERSION to 79 (Myles Borins) [#29694](#29694) - update NODE\_MODULE\_VERSION to 78 (Michaël Zasso) [#28918](#28918) - add error codes to errors thrown in C++ (Yaniv Friedensohn) [#27700](#27700) - use non-deprecated overload of V8::SetFlagsFromString (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 77 (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 74 (Refael Ackermann) [#27375](#27375) - make process.env.TZ setter clear tz cache (Ben Noordhuis) [#20026](#20026) - enable V8's WASM trap handlers (Gus Caplan) [#27246](#27246) - **stream**: - throw unhandled error for readable with autoDestroy (Robert Nagy) [#29806](#29806) - always invoke callback before emitting error (Robert Nagy) [#29293](#29293) - invoke callback before emitting error always (Robert Nagy) [#29293](#29293) - do not flush destroyed writable (Robert Nagy) [#29028](#29028) - don't emit finish on error (Robert Nagy) [#28979](#28979) - disallow stream methods on finished stream (Robert Nagy) [#28687](#28687) - do not emit after 'error' (Robert Nagy) [#28708](#28708) - fix destroy() behavior (Robert Nagy) [#29058](#29058) - simplify `.pipe()` and `.unpipe()` in Readable (Weijia Wang) [#28583](#28583) - **tools**: - patch V8 to run on older XCode versions (Ujjwal Sharma) [#29694](#29694) - update V8 gypfiles (Michaël Zasso) [#29694](#29694) - support full-icu by default (Steven R. Loomis) [#29522](#29522) - **util**: validate formatWithOptions inspectOptions (Ruben Bridgewater) [#29824](#29824) PR-URL: #29504

Notable changes: * assert: * If the validation function passed to `assert.throws()` or `assert.rejects()` returns a value other than `true`, an assertion error will be thrown instead of the original error to highlight the programming mistake (Ruben Bridgewater). #28263 * If a constructor function is passed to validate the instance of errors thrown in `assert.throws()` or `assert.reject()`, an assertion error will be thrown instead of the original error (Ruben Bridgewater). #28263 * build: * Node.js releases are now built with default full-icu support. This means that all locales supported by ICU are now included and Intl-related APIs may return different values than before (Richard Lau). #29887 * The minimum Xcode version supported for macOS was increased to 10. It is still possible to build Node.js with Xcode 8 but this may no longer be the case in a future v13.x release (Michael Dawson). #29622 * child_process: * `ChildProcess._channel` (DEP0129) is now a Runtime deprecation (cjihrig). #27949 * console: * The output `console.timeEnd()` and `console.timeLog()` will now automatically select a suitable time unit instead of always using milliseconds (Xavier Stouder). #29251 * deps: * The V8 engine was updated to version 7.8. This includes performance improvements to object destructuring, memory usage and WebAssembly startup time (Myles Borins). #29694) * domain: * The domain's error handler is now executed with the active domain set to the domain's parent to prevent inner recursion (Julien Gilli). #26211 * fs: * The undocumented method `FSWatcher.prototype.start()` was removed (Lucas Holmquist). #29905 * Calling the `open()` method on a `ReadStream` or `WriteStream` now emits a runtime deprecation warning. The methods are supposed to be internal and should not be called by user code (Robert Nagy). #29061 * `fs.read/write`, `fs.readSync/writeSync` and `fd.read/write` now accept any safe integer as their `offset` parameter. The value of `offset` is also no longer coerced, so a valid type must be passed to the functions (Zach Bjornson). #26572 * http: * Aborted requests no longer emit the `end` or `error` events after `aborted` (Robert Nagy). #27984 #20077 * Data will no longer be emitted after a socket error (Robert Nagy). #28711 * The legacy HTTP parser (previously available under the `--http-parser=legacy` flag) was removed (Anna Henningsen). #29589 * The `host` option for HTTP requests is now validated to be a string value (Giorgos Ntemiris). #29568 * The `request.connection` and `response.connection` properties are now runtime deprecated. The equivalent `request.socket` and `response.socket` should be used instead (Robert Nagy). #29015 * http, http2: * The default server timeout was removed (Ali Ijaz Sheikh). #27558 * Brought 425 status code name into accordance with RFC 8470. The name changed from "Unordered Collection" to "Too Early" (Sergei Osipov). #29880 * lib: * The `error.errno` property will now always be a number. To get the string value, use `error.code` instead (Joyee Cheung). #28140 * module: * `module.createRequireFromPath()` is deprecated. Use `module.createRequire()` instead (cjihrig). #27951 * src: * Changing the value of `process.env.TZ` will now clear the tz cache. This affects the default time zone used by methods such as `Date.prototype.toString` (Ben Noordhuis). #20026 * stream: * The timing and behavior of streams was consolidated for a number of edge cases. Please look at the individual commits below for more information. PR-URL: #29504

Since PR nodejs#29251 is merged into the master, the implementation of `console.timeEnd` is not to print the milliseconds on the stdout but instead print a more readable time unit on the console. For example if the total time comes out to be 3456 milliseconds, it will be printed as 3.45 seconds so that it is much more readable and consistent. Referenced PR: nodejs#29251 Fixes: nodejs#35011

Since PR #29251 is merged into master, the implementation of `console.timeEnd` is not to print the milliseconds on the stdout but instead print a more readable time unit on the console. For example if the total time comes out to be 3456 milliseconds, it will be printed as 3.45 seconds so that it is much more readable and consistent. PR-URL: #35027 Fixes: #35011 Refs: #29251 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Since PR nodejs#29251 is merged into master, the implementation of `console.timeEnd` is not to print the milliseconds on the stdout but instead print a more readable time unit on the console. For example if the total time comes out to be 3456 milliseconds, it will be printed as 3.45 seconds so that it is much more readable and consistent. PR-URL: nodejs#35027 Fixes: nodejs#35011 Refs: nodejs#29251 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added the console Issues and PRs related to the console subsystem. label Aug 21, 2019

cjihrig reviewed Aug 21, 2019

View reviewed changes

addaleax approved these changes Aug 21, 2019

View reviewed changes

devsnek reviewed Aug 21, 2019

View reviewed changes

targos reviewed Aug 22, 2019

View reviewed changes

test/parallel/test-console.js Show resolved Hide resolved

targos approved these changes Aug 22, 2019

View reviewed changes

cjihrig approved these changes Aug 22, 2019

View reviewed changes

Trott force-pushed the master branch from e460838 to 775048d Compare August 22, 2019 13:47

Xstoudi added 9 commits August 23, 2019 11:11

console: display timeEnd with suitable time unit

b00bdf6

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

console: extract formatTime and export it for test

c4eb2d6

Extract the format time part to be able to test it.

test: adapt tests

dc97305

Change regexp of test-console to accept other possible units and create test for formatTime

console: don't trim formatTime value before returning it

106bbd4

Tests in test-console.js expect 3 floating points.

test: adapt formatTime tests to previous commit

510ef86

Assert that returnes value as 3 floating points

console: fix linter

57bbc6c

Edit the file to fulfill the linter requirements

test: fix linter

c5063d0

Edit the file to fulfill the linter requirements

console: move magic numbers to constants

368e5d7

Hour, minute and second equivalent in ms are now constants.

test: add blank line

da4e02f

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 24, 2019

addaleax closed this Aug 25, 2019

BridgeAR mentioned this pull request Sep 3, 2019

v12.10.0 proposal #29429

Merged

sam-github added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 3, 2019

BethGriggs mentioned this pull request Sep 9, 2019

2019-10-22, Version 13.0.0 (Current) #29504

Merged

BridgeAR mentioned this pull request Sep 20, 2019

console: update time formatting #29629

Closed

4 tasks

Whobeu mentioned this pull request Sep 1, 2020

doc: console.time() Does Not Document PR #29251 Changes #35011

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

console: display timeEnd with suitable time unit #29251

console: display timeEnd with suitable time unit #29251

Xstoudi commented Aug 21, 2019 •

edited

Loading

cjihrig left a comment

Xstoudi commented Aug 21, 2019

cjihrig commented Aug 21, 2019

devsnek Aug 21, 2019

targos Aug 22, 2019

devsnek Aug 22, 2019

Xstoudi Aug 23, 2019 •

edited

Loading

BridgeAR Sep 3, 2019

nodejs-github-bot commented Aug 22, 2019

nodejs-github-bot commented Aug 22, 2019

targos commented Aug 22, 2019

targos commented Aug 22, 2019

Trott commented Aug 23, 2019

Trott commented Aug 23, 2019

Xstoudi commented Aug 23, 2019 •

edited

Loading

nodejs-github-bot commented Aug 23, 2019

addaleax commented Aug 25, 2019

BridgeAR commented Sep 3, 2019

sam-github commented Sep 3, 2019

console: display timeEnd with suitable time unit #29251

console: display timeEnd with suitable time unit #29251

Conversation

Xstoudi commented Aug 21, 2019 • edited Loading

Checklist

cjihrig left a comment

Choose a reason for hiding this comment

Xstoudi commented Aug 21, 2019

cjihrig commented Aug 21, 2019

devsnek Aug 21, 2019

Choose a reason for hiding this comment

targos Aug 22, 2019

Choose a reason for hiding this comment

devsnek Aug 22, 2019

Choose a reason for hiding this comment

Xstoudi Aug 23, 2019 • edited Loading

Choose a reason for hiding this comment

BridgeAR Sep 3, 2019

Choose a reason for hiding this comment

nodejs-github-bot commented Aug 22, 2019

nodejs-github-bot commented Aug 22, 2019

targos commented Aug 22, 2019

targos commented Aug 22, 2019

Trott commented Aug 23, 2019

Trott commented Aug 23, 2019

Xstoudi commented Aug 23, 2019 • edited Loading

nodejs-github-bot commented Aug 23, 2019

addaleax commented Aug 25, 2019

BridgeAR commented Sep 3, 2019

sam-github commented Sep 3, 2019

Xstoudi commented Aug 21, 2019 •

edited

Loading

Xstoudi Aug 23, 2019 •

edited

Loading

Xstoudi commented Aug 23, 2019 •

edited

Loading