-
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: cap byte-efficiency-audit scores to a max of 1 #14921
Changes from all commits
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 |
---|---|---|
|
@@ -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
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. 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 ( |
||
}); | ||
|
||
it('should throw on invalid graph', () => { | ||
assert.throws(() => { | ||
ByteEfficiencyAudit.createAuditProduct({ | ||
|
@@ -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', () => { | ||
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.
(there's one |
||
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); | ||
}); | ||
}); | ||
}); |
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.
added this comment because my first thought was "why isn't
wastedMs
being passed through aMath.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.