console: console.time() should not reset a timer when it exists #20442

devsnek · 2018-05-01T01:02:46Z

Fixes: #20440
Refs: https://console.spec.whatwg.org/#time

@domfarolino

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

devsnek · 2018-05-01T01:03:43Z

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

lpinca · 2018-05-01T06:27:26Z

Makes sense, add a test?

cjihrig

LGTM. A test should be added. Also, maybe we should use the same message as Chrome?

devsnek · 2018-05-01T12:42:20Z

@cjihrig I made the message based on a warning that timeEnd might emit, better to keep it similar within the program imo

TimothyGu · 2018-05-02T00:41:55Z

test/parallel/test-console.js

+console.time('test');
+const time = console._times.get('test');
+setTimeout(() => {
+  console.time('test');


Check process warning as well?

do we have a mechanism for that?

Yes, common.expectWarning().

common.expectWarning( 'Warning', "Label 'test' already exists for console.time()", common.noWarnCode );

lpinca · 2018-05-02T05:52:53Z

test/parallel/test-console.js

+// make sure calling time twice without timeEnd doesn't reset the timer.
+console.time('test');
+const time = console._times.get('test');
+setTimeout(() => {


Can we get rid of the timeout?

ita just to make sure we have a noticeable difference if something goes wrong, I'm tired of flakey tests from too small timeouts etc

lpinca · 2018-05-02T05:53:28Z

test/parallel/test-console.js

+const time = console._times.get('test');
+setTimeout(() => {
+  console.time('test');
+  assert.deepStrictEqual(time, console._times.get('test'));


Can you please swap the arguments?

BridgeAR · 2018-05-02T12:30:05Z

test/parallel/test-console.js

+  console.time('test');
+  assert.deepStrictEqual(time, console._times.get('test'));
+  console.timeEnd('test');
+}, 10);


A timeout of 1 should already be sufficient.

devsnek · 2018-05-02T12:58:43Z

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

devsnek · 2018-05-02T15:13:23Z

does anyone know how to restart a build? i see people doing that now but i can't find any UI for it

BridgeAR · 2018-05-02T15:19:17Z

test/parallel/test-console.js

+    'Label \'test\' already exists for console.time()',
+    common.noWarnCode);
+  console.timeEnd('test');
+}, 11);


I guess it was meant to be 1?

BridgeAR · 2018-05-02T15:52:14Z

@devsnek

devsnek · 2018-05-02T16:41:33Z

ohhh i thought you could restart a specific build on a specific node, thanks

devsnek · 2018-05-02T16:53:08Z

@lpinca @BridgeAR please let me know if i haven't addressed your comments fully

lpinca · 2018-05-02T17:35:13Z

test/parallel/test-console.js

+setTimeout(() => {
+  console.time('test');
+  assert.deepStrictEqual(console._times.get('test'), time);
+  common.expectWarning(


I'm a bit surprised this actually works, shouldn't it be set before calling console.time('test')?

it's set before the second call, which is when it should emit

the warning emit is probably async, so it's run after this whole block...

Ok that makes sense, I would still move it before the second call though.

devsnek · 2018-05-03T03:53:53Z

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

devsnek · 2018-05-03T18:42:09Z

landed in a598264

Notable changes: * Build * FreeBSD 10 is no longer supported. [#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * `crypto` * PEM-level encryption is now supported. [#23151](#23151) * An API for key pair generation has been added. [#22660](#22660) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * `http2` * An event will be emitted when a `PING` frame is received. [#23009](#23009) * Support for the `ORIGIN` frame has been added. [#22956](#22956) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Promises * A new `multipleResolves` event will be emitted when a Promise is resolved (or rejected) more than once. [#22218](#22218) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[nodejs#22617](nodejs#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [nodejs#21316](nodejs#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [nodejs#21649](nodejs#21649) * `console.time()` will no longer reset a timer if it already exists. [nodejs#20442](nodejs#20442) * Dependencies * V8 has been updated to 7.0. [nodejs#22754](nodejs#22754) * `fs` * The `fs.read()` method now requires a callback. [nodejs#22146](nodejs#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[nodejs#20735](nodejs#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [nodejs#20270](nodejs#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [nodejs#22951](nodejs#22951) * Internal * Windows performance-counter support has been removed. [nodejs#22485](nodejs#22485) * The `--expose-http2` command-line option has been removed. [nodejs#20887](nodejs#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [nodejs#20002](nodejs#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [nodejs#22281](nodejs#22281) * `util.inspect()` output size is limited to 128 MB by default. [nodejs#22756](nodejs#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [nodejs#21914](nodejs#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](nodejs/node#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](nodejs/node#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](nodejs/node#20442) * Dependencies * V8 has been updated to 7.0. [#22754](nodejs/node#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](nodejs/node#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](nodejs/node#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](nodejs/node#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](nodejs/node#22951) * Internal * Windows performance-counter support has been removed. [#22485](nodejs/node#22485) * The `--expose-http2` command-line option has been removed. [#20887](nodejs/node#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](nodejs/node#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](nodejs/node#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](nodejs/node#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](nodejs/node#21914)

devsnek added semver-major PRs that contain breaking changes and should be released in the next major version. console Issues and PRs related to the console subsystem. labels May 1, 2018

cjihrig approved these changes May 1, 2018

View reviewed changes

jasnell approved these changes May 1, 2018

View reviewed changes

devsnek force-pushed the fix/console-time-existing-label branch from 3242652 to d74f436 Compare May 1, 2018 17:32

Fishrock123 approved these changes May 1, 2018

View reviewed changes

TimothyGu approved these changes May 2, 2018

View reviewed changes

lpinca reviewed May 2, 2018

View reviewed changes

BridgeAR reviewed May 2, 2018

View reviewed changes

devsnek force-pushed the fix/console-time-existing-label branch from d74f436 to 8cc6d53 Compare May 2, 2018 12:58

BridgeAR reviewed May 2, 2018

View reviewed changes

devsnek force-pushed the fix/console-time-existing-label branch from 8cc6d53 to 4637099 Compare May 2, 2018 15:30

lpinca reviewed May 2, 2018

View reviewed changes

console: console.time() should not reset a timer when it exists

b9f0272

devsnek force-pushed the fix/console-time-existing-label branch from 4637099 to b9f0272 Compare May 3, 2018 03:53

devsnek closed this May 3, 2018

devsnek deleted the fix/console-time-existing-label branch May 3, 2018 18:42

devsnek added dont-land-on-v4.x labels May 3, 2018

targos removed dont-land-on-v10.x labels Jun 8, 2018

jasnell mentioned this pull request Oct 2, 2018

11.0.0 proposal #23230

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

console: console.time() should not reset a timer when it exists #20442

console: console.time() should not reset a timer when it exists #20442

devsnek commented May 1, 2018

devsnek commented May 1, 2018

lpinca commented May 1, 2018

cjihrig left a comment

devsnek commented May 1, 2018

TimothyGu May 2, 2018

devsnek May 2, 2018

lpinca May 2, 2018 •

edited

Loading

lpinca May 2, 2018

devsnek May 2, 2018

lpinca May 2, 2018

BridgeAR May 2, 2018

devsnek commented May 2, 2018

devsnek commented May 2, 2018

BridgeAR May 2, 2018

BridgeAR commented May 2, 2018

devsnek commented May 2, 2018

devsnek commented May 2, 2018

lpinca May 2, 2018

devsnek May 2, 2018

refack May 2, 2018

lpinca May 2, 2018

devsnek commented May 3, 2018

devsnek commented May 3, 2018

console: console.time() should not reset a timer when it exists #20442

console: console.time() should not reset a timer when it exists #20442

Conversation

devsnek commented May 1, 2018

Checklist

devsnek commented May 1, 2018

lpinca commented May 1, 2018

cjihrig left a comment

Choose a reason for hiding this comment

devsnek commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented May 2, 2018

devsnek commented May 2, 2018

Choose a reason for hiding this comment

BridgeAR commented May 2, 2018

devsnek commented May 2, 2018

devsnek commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented May 3, 2018

devsnek commented May 3, 2018

lpinca May 2, 2018 •

edited

Loading