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

polish: show audits with debug string, don't fail loadfast4pwa on network latencies, works-offline change #2294

Merged
merged 8 commits into from
May 25, 2017
Merged
Show file tree
Hide file tree
Changes from 2 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
24 changes: 9 additions & 15 deletions lighthouse-core/audits/load-fast-enough-for-pwa.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class LoadFastEnough4Pwa extends Audit {
category: 'PWA',
name: 'load-fast-enough-for-pwa',
description: 'Page load is fast enough on 3G',
helpText: 'Satisfied if the Time To Interactive duration is shorter than 10 seconds, as defined by the [PWA Baseline Checklist](https://developers.google.com/web/progressive-web-apps/checklist). Network throttling is required (specifically: RTT latencies >= 150 RTT are expected).',
helpText: 'Satisfied if First Interactive is less than 10 seconds, as defined by the [PWA Baseline Checklist](https://developers.google.com/web/progressive-web-apps/checklist). Network throttling is required (specifically: RTT latencies >= 150 RTT are expected).',
requiredArtifacts: ['traces', 'devtoolsLogs']
};
}
Expand All @@ -63,12 +63,6 @@ class LoadFastEnough4Pwa extends Audit {
return;
}

// Disregard requests with an invalid start time, (H2 request start times are sometimes less
// than issue time and even negative which throws off timing)
if (record._startTime < record._issueTime) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

lets keep this


// Use DevTools' definition of Waiting latency: https://github.com/ChromeDevTools/devtools-frontend/blob/66595b8a73a9c873ea7714205b828866630e9e82/front_end/network/RequestTimingView.js#L164
const latency = record._timing.receiveHeadersEnd - record._timing.sendEnd;
const latencyInfo = {
Expand Down Expand Up @@ -109,22 +103,22 @@ class LoadFastEnough4Pwa extends Audit {
{key: 'latency', itemType: 'text', text: 'Latency (ms)'},
], firstRequestLatencies);

if (!areLatenciesAll3G) {
if (!isFast) {
return {
rawValue: false,
// eslint-disable-next-line max-len
debugString: `First Interactive was found at ${timeToFirstInteractive.toLocaleString()}, however, the network request latencies were not sufficiently realistic, so the performance measurements cannot be trusted.`,
extendedInfo,
details
debugString: `First Interactive was at ${timeToFirstInteractive.toLocaleString()}. More details in the "Performance" section.`,
extendedInfo
};
}

if (!isFast) {
if (!areLatenciesAll3G) {
return {
rawValue: false,
rawValue: true,
// eslint-disable-next-line max-len
debugString: `Under 3G conditions, First Interactive was at ${timeToFirstInteractive.toLocaleString()}. More details in the "Performance" section.`,
extendedInfo
debugString: `First Interactive was found at ${timeToFirstInteractive.toLocaleString()}, however, the network request latencies were not sufficiently realistic, so the performance measurements cannot be trusted.`,
extendedInfo,
details
};
}

Expand Down
10 changes: 6 additions & 4 deletions lighthouse-core/audits/works-offline.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,16 @@ class WorksOffline extends Audit {
*/
static audit(artifacts) {
let debugString;
const passed = artifacts.Offline === 200;
if (!URL.equalWithExcludedFragments(artifacts.URL.initialUrl, artifacts.URL.finalUrl)) {
debugString = 'WARNING: You may be failing this check because your test URL ' +
`(${artifacts.URL.initialUrl}) was redirected to "${artifacts.URL.finalUrl}". ` +
'Try testing the second URL directly.';
const explanation = passed ? 'Your test URL' :
Copy link
Member

Choose a reason for hiding this comment

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

do we want to show this at all if they passed? If they passed, it means that the initialUrl successfully redirected to finalUrl and finalUrl successfully loaded, so it seems like everything is good in that case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

had that originally actually but it makes some sense that we flag that we don't know for sure if the original URL they wanted can be loaded offline, but the message is still a little off, I'll adjust it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

eh you know what, I'll just hide it if it passed, we don't have any actionable information for them in that scenario anyway since LH won't test it

'You may be failing this check because your test URL';
debugString = `WARNING: ${explanation} (${artifacts.URL.initialUrl}) was redirected to ` +
`"${artifacts.URL.finalUrl}". Try testing the second URL directly to verify .`;
}

return {
rawValue: artifacts.Offline === 200,
rawValue: passed,
debugString
};
}
Expand Down
3 changes: 2 additions & 1 deletion lighthouse-core/report/v2/renderer/category-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,8 @@ class CategoryRenderer {

const manualAudits = category.audits.filter(audit => audit.result.manual);
const nonManualAudits = category.audits.filter(audit => !manualAudits.includes(audit));
const passedAudits = nonManualAudits.filter(audit => audit.score === 100);
const passedAudits = nonManualAudits.filter(audit => audit.score === 100 &&
!audit.result.debugString);
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

add this to the renders the passed audits test, maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ya done

const nonPassedAudits = nonManualAudits.filter(audit => !passedAudits.includes(audit));

for (const audit of nonPassedAudits) {
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ describe('PWA: load-fast-enough-for-pwa audit', () => {
});
});

it('fails a good TTI value with no throttling', () => {
it('warns on a good TTI value with no throttling', () => {
// latencies are very short
const mockNetworkRecords = [
{_timing: {sendEnd: 0, receiveHeadersEnd: 50}, finished: true, _url: 'https://google.com/'},
Expand All @@ -62,7 +62,7 @@ describe('PWA: load-fast-enough-for-pwa audit', () => {
{_timing: {sendEnd: 0, receiveHeadersEnd: 50}, finished: true, _url: 'https://google.com/b'},
];
return FastPWAAudit.audit(generateArtifacts(5000, mockNetworkRecords)).then(result => {
assert.equal(result.rawValue, false);
assert.equal(result.rawValue, true);
assert.ok(result.debugString.includes('network request latencies'));
assert.ok(result.details, 'contains details when latencies were not realistic');
});
Expand Down