-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
report(redirects): reformat results, incl all requests and wasted time, #3492
Changes from 3 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 |
---|---|---|
|
@@ -9,41 +9,54 @@ const Audit = require('../../audits/redirects.js'); | |
const assert = require('assert'); | ||
|
||
/* eslint-env mocha */ | ||
const FAILING_REDIRECTS = { | ||
const FAILING_THREE_REDIRECTS = { | ||
startTime: 17, | ||
url: 'http://exampel.com/', | ||
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. typo example.com? 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 was deliberate. :) |
||
redirects: [ | ||
{ | ||
endTime: 1, | ||
responseReceivedTime: 5, | ||
startTime: 0, | ||
url: 'http://example.com/', | ||
}, | ||
{ | ||
endTime: 16, | ||
responseReceivedTime: 14, | ||
startTime: 11, | ||
url: 'https://example.com/', | ||
}, | ||
{ | ||
endTime: 17, | ||
responseReceivedTime: 15, | ||
startTime: 12, | ||
url: 'https://m.example.com/', | ||
}, | ||
], | ||
}; | ||
|
||
const FAILING_TWO_REDIRECTS = { | ||
startTime: 446.286, | ||
url: 'http://lisairish.com/', | ||
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. perhaps better to just use example.com as an url so it's consistent with the rest? Or is this a marketing stunt for your mom's website? 😆 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. Well i tested this with these actual URLs, which have this set of redirects setup. |
||
redirects: [ | ||
{ | ||
startTime: 445.648, | ||
url: 'https://lisairish.com/', | ||
}, | ||
{ | ||
startTime: 445.757, | ||
url: 'https://www.lisairish.com/', | ||
}, | ||
], | ||
}; | ||
|
||
const SUCCESS_ONE_REDIRECT = { | ||
startTime: 0.7, | ||
startTime: 136.383, | ||
url: 'https://lisairish.com/', | ||
redirects: [{ | ||
endTime: 0.7, | ||
responseReceivedTime: 5, | ||
startTime: 0, | ||
url: 'https://example.com/', | ||
startTime: 135.873, | ||
url: 'https://www.lisairish.com/', | ||
}], | ||
}; | ||
|
||
const SUCCESS_NOREDIRECT = {}; | ||
const SUCCESS_NOREDIRECT = { | ||
startTime: 135.873, | ||
url: 'https://www.google.com/', | ||
redirects: [], | ||
}; | ||
|
||
const mockArtifacts = (mockChain) => { | ||
return { | ||
|
@@ -60,19 +73,28 @@ const mockArtifacts = (mockChain) => { | |
}; | ||
|
||
describe('Performance: Redirects audit', () => { | ||
it('fails when more than one redirect detected', () => { | ||
return Audit.audit(mockArtifacts(FAILING_REDIRECTS)).then(output => { | ||
it('fails when 3 redirects detected', () => { | ||
return Audit.audit(mockArtifacts(FAILING_THREE_REDIRECTS)).then(output => { | ||
assert.equal(output.score, 0); | ||
assert.equal(output.details.items.length, 4); | ||
assert.equal(output.rawValue, 17000); | ||
}); | ||
}); | ||
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. nit: add newline |
||
it('fails when 2 redirects detected', () => { | ||
return Audit.audit(mockArtifacts(FAILING_TWO_REDIRECTS)).then(output => { | ||
assert.equal(output.score, 65); | ||
assert.equal(output.details.items.length, 3); | ||
assert.equal(output.rawValue, 6000); | ||
assert.equal(Math.round(output.rawValue), 638); | ||
}); | ||
}); | ||
|
||
it('passes when one redirect detected', () => { | ||
return Audit.audit(mockArtifacts(SUCCESS_ONE_REDIRECT)).then(output => { | ||
// If === 1 redirect, perfect score is expected, regardless of latency | ||
assert.equal(output.score, 100); | ||
assert.equal(output.details.items.length, 1); | ||
assert.equal(output.rawValue, 0); | ||
// We will still generate a table and show wasted time | ||
assert.equal(output.details.items.length, 2); | ||
assert.equal(Math.round(output.rawValue), 510); | ||
}); | ||
}); | ||
|
||
|
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.
if
totalWastedMs === 0
scoreForWastedMs doesn't already give you a 100?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.
With the new logic you can get
totalWastedMs > 0
and still get a passing grade. Take http://www.lisairish.com which i was seeing it take 500ms to redirect to https://www.lisairish.com. We should still tell them about the 500ms even if we pass them.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.
could we add a passing test case for a resource with 0 redirects too? I had to pause for a second on this, alternatively switch to
redirectRequests.length <= 2
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.
already have passing test case for 0 redirects.
changed to
<= 2
.