-
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 8 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,50 @@ 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 === 'undefined') return ''; | ||
if (typeof displayValue === 'string') return displayValue; | ||
|
||
const replacementRegex = /%([0-9]*(\.[0-9]+)?d|s)/; | ||
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; | ||
for (const replacement of displayValue) { | ||
if (!replacementRegex.test(output)) { | ||
// eslint-disable-next-line no-console | ||
console.warn('Too many replacements given'); | ||
break; | ||
} | ||
|
||
output = output.replace(replacementRegex, match => { | ||
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(); | ||
}); | ||
} | ||
|
||
if (replacementRegex.test(output)) { | ||
// eslint-disable-next-line no-console | ||
console.warn('Not enough replacements given'); | ||
} | ||
|
||
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'); | ||
}); | ||
}); | ||
}); |
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