Changes LGTM, although a regression test would be nice. This is a behavior change (semver-major) that gets us in line with what Chrome does.

Agree with @cjihrig. A test is necessary. Otherwise LGTM though

@cjihrig @evanlucas the only test I can imagine is one that checks internal _times map length (size) on timeEnd call (or checks the particular key was removed from the map). Does such checks of module's internal behaviour are suitable?

yea I would say just call console.time a few times and then verify the size of this._times and then call console.timeEnd a few times and do another assert

I have added a test which verifies that timeEnd() calls doesn't leave any new links in the map.

Should probably use strictEqual() in the test. Other than that, LGTM if the CI is happy.

@cjihrig yep!

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

@cjihrig @evanlucas Can you explain why this is semver-major?

Currently, you can reuse a timer, for example after each iteration of a loop. With this change you would have to create a new timer each time.

@cjihrig do you mean the cases like this:

console.time('label');
console.timeEnd('label');
console.timeEnd('label');
console.timeEnd('label');

Yes. I'd be willing to bet there are people out there relying on that behavior.

To be honest I've never thought about such usecase. At least Blink's implementation of console.time() doesn't work this way: undefined is returned after first timeEnd() call.

To be honest I've never thought about such usecase. At least Blink's implementation of console.time() doesn't work this way: undefined is returned after first timeEnd() call.

Note: we don't follow the spec anyways. See: #3473

@Fishrock123 did we decide to move forward with this change?

1. The spec says that the timer should be stopped.
2. Both Chrome and Firefox stop the timer, and don't output anything on subsequent console.timeEnd calls.
3. The current implementation actually leaks memory if one creates and stops a significant amount of timers.

+1 for this change, a semver-major though.

@narqo could you rebase this please.

🆙

LGTM

We may also want to stop throwing on console.timeEnd calls with inexistant labels but that can be done in another PR.

Thanks! Landed in a5cce79