Skip to content
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

console: update time formatting #29629

Closed
wants to merge 3 commits into from
Closed

console: update time formatting #29629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1dcb3d1
console: update time formatting
BridgeAR Sep 4, 2019
1c5e1b1
fixup: reformat for easier comparison
BridgeAR Sep 23, 2019
7f9d5e7
fixup: fix tests
BridgeAR Sep 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 30 additions & 14 deletions lib/internal/console/constructor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// The Console constructor is not actually used to construct the global
// console. It's exported for backwards compatibility.

const { Object, ObjectPrototype, Reflect } = primordials;
const { Object, ObjectPrototype, Reflect, Math } = primordials;

const { trace } = internalBinding('trace_events');
const {
Expand Down Expand Up @@ -533,22 +533,38 @@ function timeLogImpl(self, name, label, data) {
return true;
}

function pad(value) {
return `${value}`.padStart(2, '0');
}

function formatTime(ms) {
let value = ms;
let unit = 'ms';

if (ms >= kHour) {
value = ms / kHour;
unit = 'h';
} else if (ms >= kMinute) {
value = ms / kMinute;
unit = 'min';
} else if (ms >= kSecond) {
value = ms / kSecond;
unit = 's';
let hours = 0;
let minutes = 0;
let seconds = 0;

if (ms >= kSecond) {
if (ms >= kMinute) {
if (ms >= kHour) {
hours = Math.floor(ms / kHour);
ms = ms % kHour;
}
minutes = Math.floor(ms / kMinute);
ms = ms % kMinute;
}
seconds = ms / kSecond;
}

if (hours !== 0 || minutes !== 0) {
[seconds, ms] = seconds.toFixed(3).split('.');
const res = hours !== 0 ? `${hours}:${pad(minutes)}` : minutes;
return `${res}:${pad(seconds)}.${ms} (${hours !== 0 ? 'h:m' : ''}m:ss.mmm)`;
}

if (seconds !== 0) {
return `${seconds.toFixed(3)}s`;
}

return value.toFixed(3) + unit;
return `${Number(ms.toFixed(3))}ms`;
}

const keyKey = 'Key';
Expand Down
17 changes: 8 additions & 9 deletions test/parallel/test-console-formatTime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@ require('../common');
const { formatTime } = require('internal/console/constructor');
const assert = require('assert');

const test1 = formatTime(100);
const test2 = formatTime(1500);
const test3 = formatTime(60300);
const test4 = formatTime(4000000);

assert.strictEqual(test1, '100.000ms');
assert.strictEqual(test2, '1.500s');
assert.strictEqual(test3, '1.005min');
assert.strictEqual(test4, '1.111h');
assert.strictEqual(formatTime(100.0096), '100.01ms');
assert.strictEqual(formatTime(100.0115), '100.011ms');
assert.strictEqual(formatTime(1500.04), '1.500s');
assert.strictEqual(formatTime(1000.056), '1.000s');
Copy link
Member

Choose a reason for hiding this comment

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

I would keep these consistent with the higher numbers, to be honest...

e.g.

Suggested change
assert.strictEqual(formatTime(1000.056), '1.000s');
assert.strictEqual(formatTime(1000.056), '1.000 (ss.mmm)');

Copy link
Member Author

Choose a reason for hiding this comment

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

Seconds and milliseconds are the most common ones. People will rarely reach the higher values and it seems more straight forward to me to keep s and ms as common cases.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I understand the reasoning, I just disagree :) I'd rather opt for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

I'm going to remove the author ready label out of caution while this gets sorted, but feel free to re-add it if that's overly-cautious of me.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasnell is your comment blocking? I would rather close this PR than changing the common cases (even though I believe that this PR does improve the overall output).

Copy link
Member

Choose a reason for hiding this comment

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

Not blocking but I definitely don't consider the inconsistency to be ideal.

assert.strictEqual(formatTime(60300.3), '1:00.300 (m:ss.mmm)');
assert.strictEqual(formatTime(4000457.4), '1:06:40.457 (h:mm:ss.mmm)');
assert.strictEqual(formatTime(3601310.4), '1:00:01.310 (h:mm:ss.mmm)');
assert.strictEqual(formatTime(3213601017.6), '892:40:01.018 (h:mm:ss.mmm)');
Copy link
Member

Choose a reason for hiding this comment

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

so we're outputting the literal string "hh:mm:as.mmm"? i think this format is standard enough that we don't have to do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess most people will know about it but I added it to leave no doubt. I would like to support people who are not yet that experienced as well.

32 changes: 16 additions & 16 deletions test/parallel/test-console.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,24 +246,24 @@ assert.ok(strings[0].includes('foo: { bar: { baz:'));
assert.ok(strings[0].includes('quux'));
assert.ok(strings.shift().includes('quux: true'));

assert.ok(/^label: \d+\.\d{3}(ms|s|min|h)$/.test(strings.shift().trim()));
assert.ok(/^__proto__: \d+\.\d{3}(ms|s|min|h)$/.test(strings.shift().trim()));
assert.ok(/^constructor: \d+\.\d{3}(ms|s|min|h)$/.test(strings.shift().trim()));
assert.ok(/^hasOwnProperty: \d+\.\d{3}(ms|s|min|h)$/.test(strings.shift().trim()));
assert.ok(/^label: \d+(\.\d{1,3})?(ms|s)$/.test(strings.shift().trim()));
assert.ok(/^__proto__: \d+(\.\d{1,3})?(ms|s)$/.test(strings.shift().trim()));
assert.ok(/^constructor: \d+(\.\d{1,3})?(ms|s)$/.test(strings.shift().trim()));
assert.ok(/^hasOwnProperty: \d+(\.\d{1,3})?(ms|s)$/.test(strings.shift().trim()));

// Verify that console.time() coerces label values to strings as expected
assert.ok(/^: \d+\.\d{3}(ms|s|min|h)$/.test(strings.shift().trim()));
assert.ok(/^\[object Object\]: \d+\.\d{3}(ms|s|min|h)$/.test(strings.shift().trim()));
assert.ok(/^\[object Object\]: \d+\.\d{3}(ms|s|min|h)$/.test(strings.shift().trim()));
assert.ok(/^null: \d+\.\d{3}(ms|s|min|h)$/.test(strings.shift().trim()));
assert.ok(/^default: \d+\.\d{3}(ms|s|min|h)$/.test(strings.shift().trim()));
assert.ok(/^default: \d+\.\d{3}(ms|s|min|h)$/.test(strings.shift().trim()));
assert.ok(/^NaN: \d+\.\d{3}(ms|s|min|h)$/.test(strings.shift().trim()));

assert.ok(/^log1: \d+\.\d{3}(ms|s|min|h)$/.test(strings.shift().trim()));
assert.ok(/^log1: \d+\.\d{3}(ms|s|min|h) test$/.test(strings.shift().trim()));
assert.ok(/^log1: \d+\.\d{3}(ms|s|min|h) {} \[ 1, 2, 3 ]$/.test(strings.shift().trim()));
assert.ok(/^log1: \d+\.\d{3}(ms|s|min|h)$/.test(strings.shift().trim()));
assert.ok(/^: \d+(\.\d{1,3})?(ms|s)$/.test(strings.shift().trim()));
assert.ok(/^\[object Object\]: \d+(\.\d{1,3})?(ms|s)$/.test(strings.shift().trim()));
assert.ok(/^\[object Object\]: \d+(\.\d{1,3})?(ms|s)$/.test(strings.shift().trim()));
assert.ok(/^null: \d+(\.\d{1,3})?(ms|s)$/.test(strings.shift().trim()));
assert.ok(/^default: \d+(\.\d{1,3})?(ms|s)$/.test(strings.shift().trim()));
assert.ok(/^default: \d+(\.\d{1,3})?(ms|s)$/.test(strings.shift().trim()));
assert.ok(/^NaN: \d+(\.\d{1,3})?(ms|s)$/.test(strings.shift().trim()));

assert.ok(/^log1: \d+(\.\d{1,3})?(ms|s)$/.test(strings.shift().trim()));
assert.ok(/^log1: \d+(\.\d{1,3})?(ms|s) test$/.test(strings.shift().trim()));
assert.ok(/^log1: \d+(\.\d{1,3})?(ms|s) {} \[ 1, 2, 3 ]$/.test(strings.shift().trim()));
assert.ok(/^log1: \d+(\.\d{1,3})?(ms|s)$/.test(strings.shift().trim()));

// Make sure that we checked all strings
assert.strictEqual(strings.length, 0);
Expand Down