From 31a2639c737cf5f29cfdfd4b085f2f2703db9d7c Mon Sep 17 00:00:00 2001 From: Shane Exterkamp Date: Tue, 16 Oct 2018 12:55:11 -0700 Subject: [PATCH 1/5] Added multiple output strings for pwa load audit. --- .../audits/load-fast-enough-for-pwa.js | 18 ++++++++++----- .../audits/load-fast-enough-for-pwa-test.js | 23 +++++++++++++++++++ 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/lighthouse-core/audits/load-fast-enough-for-pwa.js b/lighthouse-core/audits/load-fast-enough-for-pwa.js index c7c73cea6440..c276e6a4959f 100644 --- a/lighthouse-core/audits/load-fast-enough-for-pwa.js +++ b/lighthouse-core/audits/load-fast-enough-for-pwa.js @@ -16,6 +16,9 @@ const Audit = require('./audit'); const mobileThrottling = require('../config/constants').throttling.mobileSlow4G; const Interactive = require('../gather/computed/metrics/interactive.js'); +const displayValueText = `Interactive at %d\xa0s`; +const displayValueTextWithOverride = `Interactive on simulated mobile network at %d\xa0s`; + // Maximum TTI to be considered "fast" for PWA baseline checklist // https://developers.google.com/web/progressive-web-apps/checklist const MAXIMUM_TTI = 10 * 1000; @@ -48,11 +51,14 @@ class LoadFastEnough4Pwa extends Audit { // If throttling was default devtools or lantern slow 4G throttling, then reuse the given settings // Otherwise, we'll force the usage of lantern slow 4G. const settingOverrides = {throttlingMethod: 'simulate', throttling: mobileThrottling}; - const settings = - context.settings.throttlingMethod !== 'provided' && - isDeepEqual(context.settings.throttling, mobileThrottling) - ? context.settings - : Object.assign({}, context.settings, settingOverrides); + + // Override settings for interactive if provided throttling was used and network + // throttling was not consistent with standard `mobile network throttling` + const override = context.settings.throttlingMethod === 'provided' && + !isDeepEqual(context.settings.throttling, mobileThrottling); + + const settings = !override? context.settings: + Object.assign({}, context.settings, settingOverrides); const metricComputationData = {trace, devtoolsLog, settings}; const tti = await Interactive.request(metricComputationData, context); @@ -64,7 +70,7 @@ class LoadFastEnough4Pwa extends Audit { /** @type {string|undefined} */ let explanation; if (!score) { - displayValue = [`Interactive at %d\xa0s`, tti.timing / 1000]; + displayValue = [override?displayValueTextWithOverride: displayValueText, tti.timing / 1000]; explanation = 'Your page loads too slowly and is not interactive within 10 seconds. ' + 'Look at the opportunities and diagnostics in the "Performance" section to learn how to ' + 'improve.'; diff --git a/lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js b/lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js index 734f86e8e94a..e6d54d86073d 100644 --- a/lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js +++ b/lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js @@ -78,4 +78,27 @@ describe('PWA: load-fast-enough-for-pwa audit', () => { expect(result.rawValue).toBeGreaterThan(2000); expect(Math.round(result.rawValue)).toMatchSnapshot(); }); + + + it('override with simulated result fails a bad simulated TTI value', async () => { + const topLevelTasks = [ + {ts: 1000, duration: 1000}, + {ts: 3000, duration: 1000}, + {ts: 5000, duration: 1000}, + {ts: 9000, duration: 1000}, + {ts: 12000, duration: 1000}, + {ts: 14900, duration: 1000}, + ]; + const longTrace = createTestTrace({navigationStart: 0, traceEnd: 20000, topLevelTasks}); + + const artifacts = { + traces: {defaultPass: longTrace}, + devtoolsLogs: {defaultPass: devtoolsLog}, + }; + + const settings = {throttlingMethod: 'provided', throttling: {rttMs: 40, throughput: 100000}}; + const result = await FastPWAAudit.audit(artifacts, {settings, computedCache: new Map()}); + expect(result.displayValue).toContain('Interactive on simulated mobile network at %d\xa0s'); + expect(result.rawValue).toBeGreaterThan(10000); + }); }); From 8cf1376591f17133b34add8e699db39c51a3b3c1 Mon Sep 17 00:00:00 2001 From: Shane Exterkamp Date: Mon, 22 Oct 2018 17:37:00 -0700 Subject: [PATCH 2/5] Reveresed settings ternary. --- lighthouse-core/audits/load-fast-enough-for-pwa.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/audits/load-fast-enough-for-pwa.js b/lighthouse-core/audits/load-fast-enough-for-pwa.js index c276e6a4959f..3b2157bdd292 100644 --- a/lighthouse-core/audits/load-fast-enough-for-pwa.js +++ b/lighthouse-core/audits/load-fast-enough-for-pwa.js @@ -57,8 +57,8 @@ class LoadFastEnough4Pwa extends Audit { const override = context.settings.throttlingMethod === 'provided' && !isDeepEqual(context.settings.throttling, mobileThrottling); - const settings = !override? context.settings: - Object.assign({}, context.settings, settingOverrides); + const settings = override ? Object.assign({}, context.settings, settingOverrides): + context.settings; const metricComputationData = {trace, devtoolsLog, settings}; const tti = await Interactive.request(metricComputationData, context); From 7abf33f316d36edefeefd048c3ae8a3190e14ebb Mon Sep 17 00:00:00 2001 From: Shane Exterkamp Date: Tue, 23 Oct 2018 09:56:28 -0700 Subject: [PATCH 3/5] Fixed override check and added test to make sure it is consistent. --- lighthouse-core/audits/load-fast-enough-for-pwa.js | 10 ++++++---- .../test/audits/load-fast-enough-for-pwa-test.js | 10 ++++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/lighthouse-core/audits/load-fast-enough-for-pwa.js b/lighthouse-core/audits/load-fast-enough-for-pwa.js index 3b2157bdd292..197e50772f31 100644 --- a/lighthouse-core/audits/load-fast-enough-for-pwa.js +++ b/lighthouse-core/audits/load-fast-enough-for-pwa.js @@ -54,10 +54,12 @@ class LoadFastEnough4Pwa extends Audit { // Override settings for interactive if provided throttling was used and network // throttling was not consistent with standard `mobile network throttling` - const override = context.settings.throttlingMethod === 'provided' && - !isDeepEqual(context.settings.throttling, mobileThrottling); + const override = context.settings.throttlingMethod === 'provided' || + !isDeepEqual(context.settings.throttling, mobileThrottling); - const settings = override ? Object.assign({}, context.settings, settingOverrides): + const displayValueFinal = override?displayValueTextWithOverride: displayValueText; + + const settings = override ? Object.assign({}, context.settings, settingOverrides): context.settings; const metricComputationData = {trace, devtoolsLog, settings}; @@ -70,7 +72,7 @@ class LoadFastEnough4Pwa extends Audit { /** @type {string|undefined} */ let explanation; if (!score) { - displayValue = [override?displayValueTextWithOverride: displayValueText, tti.timing / 1000]; + displayValue = [displayValueFinal, tti.timing / 1000]; explanation = 'Your page loads too slowly and is not interactive within 10 seconds. ' + 'Look at the opportunities and diagnostics in the "Performance" section to learn how to ' + 'improve.'; diff --git a/lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js b/lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js index e6d54d86073d..96645353365e 100644 --- a/lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js +++ b/lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js @@ -79,6 +79,16 @@ describe('PWA: load-fast-enough-for-pwa audit', () => { expect(Math.round(result.rawValue)).toMatchSnapshot(); }); + it('overrides when throttling is modified but method is not provided', async () => { + const artifacts = { + traces: {defaultPass: trace}, + devtoolsLogs: {defaultPass: devtoolsLog}, + }; + + const settings = {throttlingMethod: 'devtools', throttling: {rttMs: 40, throughput: 100000}}; + const result = await FastPWAAudit.audit(artifacts, {settings, computedCache: new Map()}); + expect(result.rawValue).toBeGreaterThan(2000); + }); it('override with simulated result fails a bad simulated TTI value', async () => { const topLevelTasks = [ From d929208d1c85be54db7e06d419ff2b42f149f01b Mon Sep 17 00:00:00 2001 From: Shane Exterkamp Date: Wed, 24 Oct 2018 18:25:43 -0700 Subject: [PATCH 4/5] Feedback on comments and linting. --- lighthouse-core/audits/load-fast-enough-for-pwa.js | 4 ++-- .../test/audits/load-fast-enough-for-pwa-test.js | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lighthouse-core/audits/load-fast-enough-for-pwa.js b/lighthouse-core/audits/load-fast-enough-for-pwa.js index 197e50772f31..345d85c7e613 100644 --- a/lighthouse-core/audits/load-fast-enough-for-pwa.js +++ b/lighthouse-core/audits/load-fast-enough-for-pwa.js @@ -52,12 +52,12 @@ class LoadFastEnough4Pwa extends Audit { // Otherwise, we'll force the usage of lantern slow 4G. const settingOverrides = {throttlingMethod: 'simulate', throttling: mobileThrottling}; - // Override settings for interactive if provided throttling was used and network + // Override settings for interactive if provided throttling was used or network // throttling was not consistent with standard `mobile network throttling` const override = context.settings.throttlingMethod === 'provided' || !isDeepEqual(context.settings.throttling, mobileThrottling); - const displayValueFinal = override?displayValueTextWithOverride: displayValueText; + const displayValueFinal = override ? displayValueTextWithOverride : displayValueText; const settings = override ? Object.assign({}, context.settings, settingOverrides): context.settings; diff --git a/lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js b/lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js index 96645353365e..939ef87564d5 100644 --- a/lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js +++ b/lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js @@ -75,11 +75,11 @@ describe('PWA: load-fast-enough-for-pwa audit', () => { const settings = {throttlingMethod: 'provided', throttling: {rttMs: 40, throughput: 100000}}; const result = await FastPWAAudit.audit(artifacts, {settings, computedCache: new Map()}); - expect(result.rawValue).toBeGreaterThan(2000); + expect(result.rawValue).toBeGreaterThan(2000); // If not overridden this would be 1582 expect(Math.round(result.rawValue)).toMatchSnapshot(); }); - it('overrides when throttling is modified but method is not provided', async () => { + it('overrides when throttling is modified but method is not "provided"', async () => { const artifacts = { traces: {defaultPass: trace}, devtoolsLogs: {defaultPass: devtoolsLog}, @@ -87,10 +87,10 @@ describe('PWA: load-fast-enough-for-pwa audit', () => { const settings = {throttlingMethod: 'devtools', throttling: {rttMs: 40, throughput: 100000}}; const result = await FastPWAAudit.audit(artifacts, {settings, computedCache: new Map()}); - expect(result.rawValue).toBeGreaterThan(2000); + expect(result.rawValue).toBeGreaterThan(2000); // If not overridden this would be 1582 }); - it('override with simulated result fails a bad simulated TTI value', async () => { + it('overrides when throttling is "provided" and fails the simulated TTI value', async () => { const topLevelTasks = [ {ts: 1000, duration: 1000}, {ts: 3000, duration: 1000}, From ada0e4774fee2fc888df0311ff491631d10616cf Mon Sep 17 00:00:00 2001 From: Shane Exterkamp Date: Thu, 25 Oct 2018 11:52:55 -0700 Subject: [PATCH 5/5] Fixed infix lint error. --- lighthouse-core/audits/load-fast-enough-for-pwa.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/audits/load-fast-enough-for-pwa.js b/lighthouse-core/audits/load-fast-enough-for-pwa.js index 345d85c7e613..d447a8035d4a 100644 --- a/lighthouse-core/audits/load-fast-enough-for-pwa.js +++ b/lighthouse-core/audits/load-fast-enough-for-pwa.js @@ -59,7 +59,7 @@ class LoadFastEnough4Pwa extends Audit { const displayValueFinal = override ? displayValueTextWithOverride : displayValueText; - const settings = override ? Object.assign({}, context.settings, settingOverrides): + const settings = override ? Object.assign({}, context.settings, settingOverrides) : context.settings; const metricComputationData = {trace, devtoolsLog, settings};