-
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
Add TTI < 10s audit for PWA #1840
Changes from 6 commits
17f51ad
08a9a89
c333c8a
544095f
0311c22
2e0004a
b35ac3c
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 |
---|---|---|
@@ -0,0 +1,89 @@ | ||
/** | ||
* @license Copyright 2017 Google Inc. All Rights Reserved. | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | ||
*/ | ||
|
||
'use strict'; | ||
|
||
/** @fileoverview | ||
* This audit evaluates if a page's load performance is fast enough for it to be considered a PWA. | ||
* We are doublechecking that the network requests were throttled (or slow on their own) | ||
* Afterwards, we report if the TTI is less than 10 seconds. | ||
*/ | ||
|
||
const Audit = require('./audit'); | ||
const TTIMetric = require('./time-to-interactive'); | ||
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. we should really just make a "page timing" computed artifact and move both FMP and TTI in there 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. Yeah I have need for it now too in OffscreenImages thanks to @brendankenny obnoxiously useful suggestion :P 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. TBH i think audits depending on audits is almost the same as computed artifacts depending on eachother. and i'm kinda less interested in making one big "page timing" one, as it'd have to run TTI tracing model stuff even if you only want FMP. 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. but now don't we have to run the TTI tracing model stuff 3x instead of just once? 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. gotta rerun parts of it, but the tracing model is a comptued artifact. i profiled a run and reruning the TTI.audit() probably is like 3ms max, but we have like 6000ms of main thread time total. 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. calling into another audit really is an architecture smell, but we can fix that in a follow up |
||
const Emulation = require('../lib/emulation'); | ||
|
||
const Formatter = require('../report/formatter'); | ||
|
||
// Maximum TTI to be considered "fast" for PWA baseline checklist | ||
// https://developers.google.com/web/progressive-web-apps/checklist | ||
const MAXIMUM_TTI = 10 * 1000; | ||
|
||
class LoadFastEnough4Pwa extends Audit { | ||
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. also hideous :P |
||
/** | ||
* @return {!AuditMeta} | ||
*/ | ||
static get meta() { | ||
return { | ||
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).', | ||
requiredArtifacts: ['traces', 'networkRecords'] | ||
}; | ||
} | ||
|
||
/** | ||
* @param {!Artifacts} artifacts | ||
* @return {!AuditResult} | ||
*/ | ||
static audit(artifacts) { | ||
const networkRecords = artifacts.networkRecords[Audit.DEFAULT_PASS]; | ||
const allRequestLatencies = networkRecords.map(record => { | ||
if (!record._timing) return Infinity; | ||
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. why default with 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. because if there is no timing data on a request then we don't care about it. we don't want to consider it for this analysis. i'm also happy to filter netRecords to only the ones with timing first. 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. or alternatively the 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. definitely don't want to depend on implicit |
||
// Use DevTools' definition of Waiting latency: https://github.com/ChromeDevTools/devtools-frontend/blob/66595b8a73a9c873ea7714205b828866630e9e82/front_end/network/RequestTimingView.js#L164 | ||
return record._timing.receiveHeadersEnd - record._timing.sendEnd; | ||
}); | ||
|
||
const latency3gMin = Emulation.settings.TYPICAL_MOBILE_THROTTLING_METRICS.latency - 10; | ||
const areLatenciesAll3G = allRequestLatencies.every(val => val > latency3gMin); | ||
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. if you don't include 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. incidentally, this is a really cool way of checking that it's a sufficiently slow connection :) |
||
|
||
return TTIMetric.audit(artifacts).then(ttiResult => { | ||
const timeToInteractive = ttiResult.extendedInfo.value.timings.timeToInteractive; | ||
const isFast = timeToInteractive < MAXIMUM_TTI; | ||
|
||
const extendedInfo = { | ||
formatter: Formatter.SUPPORTED_FORMATS.NULL, | ||
value: {areLatenciesAll3G, allRequestLatencies, isFast, timeToInteractive} | ||
}; | ||
|
||
if (!areLatenciesAll3G) { | ||
return { | ||
rawValue: false, | ||
// eslint-disable-next-line max-len | ||
debugString: `The Time To Interactive was found at ${ttiResult.displayValue}, however, the network request latencies were not sufficiently realistic, so the performance measurements cannot be trusted.`, | ||
extendedInfo | ||
}; | ||
} | ||
|
||
if (!isFast) { | ||
return { | ||
rawValue: false, | ||
// eslint-disable-next-line max-len | ||
debugString: `Under 3G conditions, the Time To Interactive was at ${ttiResult.displayValue}. More details in the "Performance" section.`, | ||
extendedInfo | ||
}; | ||
} | ||
|
||
return { | ||
rawValue: true, | ||
extendedInfo | ||
}; | ||
}); | ||
} | ||
} | ||
|
||
module.exports = LoadFastEnough4Pwa; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
/** | ||
* @license Copyright 2017 Google Inc. All Rights Reserved. | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | ||
*/ | ||
|
||
'use strict'; | ||
|
||
const FastPWAAudit = require('../../audits/load-fast-enough-for-pwa'); | ||
const TTIAudit = require('../../audits/time-to-interactive'); | ||
const Audit = require('../../audits/audit.js'); | ||
const assert = require('assert'); | ||
|
||
function generateTTIResults(ttiValue) { | ||
const ttiResult = { | ||
rawValue: ttiValue, | ||
extendedInfo: { | ||
value: { | ||
timings: { | ||
timeToInteractive: ttiValue | ||
} | ||
} | ||
} | ||
}; | ||
return Promise.resolve.bind(Promise, ttiResult); | ||
} | ||
|
||
function generateArtifacts(networkRecords = []) { | ||
return { | ||
networkRecords: { | ||
[Audit.DEFAULT_PASS]: networkRecords | ||
}, | ||
traces: { | ||
[Audit.DEFAULT_PASS]: {traceEvents: []} | ||
} | ||
}; | ||
} | ||
|
||
/* eslint-env mocha */ | ||
describe('PWA: load-fast-enough-for-pwa audit', () => { | ||
// monkeypatch TTI to for a more focused test | ||
let origTTI; | ||
beforeEach(() => origTTI = TTIAudit.audit); | ||
afterEach(() => TTIAudit.audit = origTTI); | ||
|
||
it('returns boolean based on TTI value', () => { | ||
TTIAudit.audit = generateTTIResults(5000); | ||
return FastPWAAudit.audit(generateArtifacts()).then(result => { | ||
assert.equal(result.rawValue, true, 'fixture trace is not passing audit'); | ||
}).catch(err => { | ||
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. don't need |
||
assert.ok(false, err); | ||
}); | ||
}); | ||
|
||
it('fails a bad TTI value', () => { | ||
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. add a quick test for the network case? 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. quick you say.. 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. added! |
||
TTIAudit.audit = generateTTIResults(15000); | ||
return FastPWAAudit.audit(generateArtifacts()).then(result => { | ||
assert.equal(result.rawValue, false, 'not failing a long TTI value'); | ||
assert.ok(result.debugString); | ||
}).catch(err => { | ||
assert.ok(false, err); | ||
}); | ||
}); | ||
|
||
it('fails a good TTI value with no throttling', () => { | ||
TTIAudit.audit = generateTTIResults(5000); | ||
// latencies are very short | ||
const mockNetworkRecords = [ | ||
{_timing: {sendEnd: 0, receiveHeadersEnd: 50}}, | ||
{_timing: {sendEnd: 0, receiveHeadersEnd: 75}}, | ||
{ }, | ||
{_timing: {sendEnd: 0, receiveHeadersEnd: 50}}, | ||
]; | ||
return FastPWAAudit.audit(generateArtifacts(mockNetworkRecords)).then(result => { | ||
assert.equal(result.rawValue, false); | ||
assert.ok(result.debugString.includes('network request latencies')); | ||
}).catch(err => { | ||
assert.ok(false, err); | ||
}); | ||
}); | ||
|
||
|
||
it('passes a good TTI value and WITH throttling', () => { | ||
TTIAudit.audit = generateTTIResults(5000); | ||
// latencies are very long | ||
const mockNetworkRecords = [ | ||
{_timing: {sendEnd: 0, receiveHeadersEnd: 250}}, | ||
{_timing: {sendEnd: 0, receiveHeadersEnd: 175}}, | ||
{ }, | ||
{_timing: {sendEnd: 0, receiveHeadersEnd: 250}}, | ||
]; | ||
return FastPWAAudit.audit(generateArtifacts(mockNetworkRecords)).then(result => { | ||
assert.equal(result.rawValue, true); | ||
assert.strictEqual(result.debugString, undefined); | ||
}).catch(err => { | ||
assert.ok(false, err); | ||
}); | ||
}); | ||
}); |
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.
this is hideous
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.
but for reals, I think we should have one style or the other, and not mix them based on who wrote the file :)
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.
i am very happy to change the codebase in one full sweep.
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.
sounds perfect for another PR :P