-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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.
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
core(lhr): support printf displayValues #5099
Changes from 4 commits
6e8ae64
dc4fc72
bbb8c35
7af9dd0
8208002
37fbd53
0abca9b
790158b
2d69e89
141103d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,40 @@ class Util { | |
return PASS_THRESHOLD; | ||
} | ||
|
||
static get MS_DISPLAY_VALUE() { | ||
return `%10d${NBSP}ms`; | ||
} | ||
|
||
/** | ||
* @param {string|Array<string|number>=} displayValue | ||
* @return {string} | ||
*/ | ||
static formatDisplayValue(displayValue) { | ||
if (typeof displayValue === 'string' || typeof displayValue === 'undefined') { | ||
return displayValue || ''; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'd prefer being super clear if (typeof displayValue === 'string') return displayValue;
if (typeof displayValue === 'undefined') return ''; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
} | ||
|
||
const template = /** @type {string} */ displayValue.shift(); | ||
if (typeof template !== 'string') { | ||
// First value should always be the format string, but we don't want to fail to build | ||
// a report, return a placeholder. | ||
return 'UNKNOWN'; | ||
} | ||
|
||
let output = template; | ||
while (displayValue.length) { | ||
const replacement = /** @type {number|string} */ (displayValue.shift()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. assert.equal(format(['%s']), '%s', 'not enough replacements');
assert.equal(format(['%s', 'a', 'a']), 'a', 'extra replacements'); i think we may want to console.warn() in these cases. Allow them, but raise a warning flag. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just switch to regular indexing and avoid these shift assertions? Or could be a reduce There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
output = output.replace(/%([0-9.]+)?(d|s)/, match => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this allows more than one decimal point? and no support for granularity when it's a string? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. correct if you mess up your format string it'll put There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant should it be more like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, tweaked it a bit |
||
const granularity = Number(match.match(/[0-9.]+/)) || 1; | ||
return match === '%s' ? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. assert.equal(format(['%10s', 123.456]), '120'); currently this does pass, fwiw. shoudl we also throw a warning if you are matching 's' and have a granularity? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. modified regex to not match this 👍 |
||
replacement.toLocaleString() : | ||
(Math.round(Number(replacement) / granularity) * granularity).toLocaleString(); | ||
}); | ||
} | ||
|
||
return output; | ||
} | ||
|
||
/** | ||
* Convert a score to a rating label. | ||
* @param {number} score | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
|
||
const Audit = require('../../audits/estimated-input-latency'); | ||
const Runner = require('../../runner'); | ||
const Util = require('../../report/html/renderer/util'); | ||
const assert = require('assert'); | ||
const options = Audit.defaultOptions; | ||
|
||
|
@@ -29,7 +30,7 @@ describe('Performance: estimated-input-latency audit', () => { | |
return Audit.audit(artifacts, {options, settings}).then(output => { | ||
assert.equal(output.debugString, undefined); | ||
assert.equal(Math.round(output.rawValue * 10) / 10, 17.1); | ||
assert.equal(output.displayValue, '17\xa0ms'); | ||
assert.equal(Util.formatDisplayValue(output.displayValue), '17\xa0ms'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. test the real object here like |
||
assert.equal(output.score, 1); | ||
}); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
|
||
const FMPAudit = require('../../audits/first-meaningful-paint.js'); | ||
const Audit = require('../../audits/audit.js'); | ||
const Util = require('../../report/html/renderer/util'); | ||
const assert = require('assert'); | ||
const options = FMPAudit.defaultOptions; | ||
const trace = require('../fixtures/traces/progressive-app-m60.json'); | ||
|
@@ -26,7 +27,7 @@ describe('Performance: first-meaningful-paint audit', () => { | |
const fmpResult = await FMPAudit.audit(artifacts, context); | ||
|
||
assert.equal(fmpResult.score, 1); | ||
assert.equal(fmpResult.displayValue, '780\xa0ms'); | ||
assert.equal(Util.formatDisplayValue(fmpResult.displayValue), '780\xa0ms'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same with these |
||
assert.equal(fmpResult.rawValue, 783.328); | ||
}); | ||
|
||
|
@@ -39,7 +40,7 @@ describe('Performance: first-meaningful-paint audit', () => { | |
const fmpResult = await FMPAudit.audit(artifacts, context); | ||
|
||
assert.equal(fmpResult.score, 0.79); | ||
assert.equal(fmpResult.displayValue, '2,850\xa0ms'); | ||
assert.equal(Util.formatDisplayValue(fmpResult.displayValue), '2,850\xa0ms'); | ||
assert.equal(Math.round(fmpResult.rawValue), 2851); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
|
||
const Interactive = require('../../audits/interactive.js'); | ||
const Runner = require('../../runner.js'); | ||
const Util = require('../../report/html/renderer/util'); | ||
const assert = require('assert'); | ||
const options = Interactive.defaultOptions; | ||
|
||
|
@@ -33,7 +34,7 @@ describe('Performance: interactive audit', () => { | |
return Interactive.audit(artifacts, {options, settings}).then(output => { | ||
assert.equal(output.score, 1); | ||
assert.equal(Math.round(output.rawValue), 1582); | ||
assert.equal(output.displayValue, '1,580\xa0ms'); | ||
assert.equal(Util.formatDisplayValue(output.displayValue), '1,580\xa0ms'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and these :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
}); | ||
}); | ||
|
||
|
@@ -51,7 +52,7 @@ describe('Performance: interactive audit', () => { | |
return Interactive.audit(artifacts, {options, settings}).then(output => { | ||
assert.equal(output.score, 0.97); | ||
assert.equal(Math.round(output.rawValue), 2712); | ||
assert.equal(output.displayValue, '2,710\xa0ms'); | ||
assert.equal(Util.formatDisplayValue(output.displayValue), '2,710\xa0ms'); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,4 +53,16 @@ describe('util helpers', () => { | |
assert.equal(Util.calculateRating(0.80), 'pass'); | ||
assert.equal(Util.calculateRating(1.00), 'pass'); | ||
}); | ||
|
||
it('formats display values', () => { | ||
const format = (...args) => Util.formatDisplayValue(...args); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't need to rest and then spread since they're all arrays below? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
assert.equal(format(undefined), ''); | ||
assert.equal(format([1]), 'UNKNOWN'); | ||
assert.equal(format(['%s %s', 'Hello', 'Paul']), 'Hello Paul'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hello formatDisplayValue There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
assert.equal(format(['%s%', 99.9]), '99.9%'); | ||
assert.equal(format(['%d%', 99.9]), '100%'); | ||
assert.equal(format(['%s ms', 12345.678]), '12,345.678 ms'); | ||
assert.equal(format(['%10d ms', 12345.678]), '12,350 ms'); | ||
assert.equal(format(['%.01d ms', 12345.678]), '12,345.68 ms'); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: switch conditional above to testing
wastedKb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done