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.

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: cap byte-efficiency-audit scores to a max of 1 #14921

Merged
merged 1 commit into from
Mar 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion core/audits/byte-efficiency/byte-efficiency-audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,14 @@ const WASTED_MS_FOR_SCORE_OF_ZERO = 5000;
class ByteEfficiencyAudit extends Audit {
/**
* Creates a score based on the wastedMs value using linear interpolation between control points.
* A negative wastedMs is scored as 1, assuming time is not being wasted with respect to the
* opportunity being measured.
*
* @param {number} wastedMs
* @return {number}
*/
static scoreForWastedMs(wastedMs) {
if (wastedMs === 0) {
if (wastedMs <= 0) {
return 1;
} else if (wastedMs < WASTED_MS_FOR_AVERAGE) {
return linearInterpolation(0, 1, WASTED_MS_FOR_AVERAGE, 0.75, wastedMs);
Expand Down Expand Up @@ -214,6 +216,8 @@ class ByteEfficiencyAudit extends Audit {

const wastedBytes = results.reduce((sum, item) => sum + item.wastedBytes, 0);

// `wastedMs` may be negative, if making the opportunity change could be detrimental.
// This is useful information in the LHR and should be preserved.
Comment on lines +219 to +220
Copy link
Member Author

Choose a reason for hiding this comment

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

added this comment because my first thought was "why isn't wastedMs being passed through a Math.max(0, wastedMs)?", but it's actually very useful information to know that the opportunity would be negative, so I wanted to make that explicit to future readers of the code. Happy to move/bikeshed.

let wastedMs;
if (gatherContext.gatherMode === 'navigation') {
if (!graph) throw Error('Page dependency graph should always be computed in navigation mode');
Expand Down
26 changes: 26 additions & 0 deletions core/test/audits/byte-efficiency/byte-efficiency-audit-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,17 @@ describe('Byte efficiency base audit', () => {
assert.ok(failingResult.score < 0.5, 'scores failing wastedMs');
});

it('should score negative wastedMs as perfect', () => {
const negativeResult = ByteEfficiencyAudit.createAuditProduct({
headings: baseHeadings,
items: [{url: 'http://example.com/', wastedBytes: -1 * 1000}],
}, graph, simulator, {gatherMode: 'timespan'});

assert.equal(negativeResult.score, 1);
assert.ok(negativeResult.numericValue < 0);
assert.equal(negativeResult.numericValue, negativeResult.details.overallSavingsMs);
Comment on lines +147 to +148
Copy link
Member Author

Choose a reason for hiding this comment

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

explicit test asserting that we want the negative result in the LHR even though it won't appear in the HTML report once it's tucked into the passing audits section (displayValue is used then)

});

it('should throw on invalid graph', () => {
assert.throws(() => {
ByteEfficiencyAudit.createAuditProduct({
Expand Down Expand Up @@ -398,4 +409,19 @@ describe('Byte efficiency base audit', () => {
const result = await MockAudit.audit(artifacts, {settings, computedCache});
expect(result.details.overallSavingsMs).toBeCloseTo(575, 1);
});

describe('#scoreForWastedMs', () => {
Copy link
Member Author

@brendankenny brendankenny Mar 22, 2023

Choose a reason for hiding this comment

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

scoreForWastedMs is imported and used independently of audits extending ByteEfficiencyAudit in at least prioritize-lcp-image, so it felt worth it to test separately as well.

(there's one wastedMs assertion for every segment and every control point in the piecewise scoring function)

it('scores wastedMs values', () => {
expect(ByteEfficiencyAudit.scoreForWastedMs(-50)).toBe(1);
expect(ByteEfficiencyAudit.scoreForWastedMs(0)).toBe(1);
expect(ByteEfficiencyAudit.scoreForWastedMs(240)).toBe(0.8);
expect(ByteEfficiencyAudit.scoreForWastedMs(300)).toBe(0.75);
expect(ByteEfficiencyAudit.scoreForWastedMs(390)).toBe(0.7);
expect(ByteEfficiencyAudit.scoreForWastedMs(750)).toBe(0.5);
expect(ByteEfficiencyAudit.scoreForWastedMs(1_175)).toBe(0.45);
expect(ByteEfficiencyAudit.scoreForWastedMs(5_000)).toBe(0);
expect(ByteEfficiencyAudit.scoreForWastedMs(10_000)).toBe(0);
expect(ByteEfficiencyAudit.scoreForWastedMs(Number.MAX_VALUE)).toBe(0);
});
});
});