From d36304d921275bf556ad57fe8acddc54dbad0dd3 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 8 Jan 2019 17:22:22 -0600 Subject: [PATCH 1/7] core: special case AppManifest as baseArtifact --- lighthouse-core/gather/gather-runner.js | 45 +++++++++++++++++-- lighthouse-core/gather/gatherers/manifest.js | 26 +---------- lighthouse-core/gather/gatherers/start-url.js | 29 +++++------- types/artifacts.d.ts | 2 + types/gatherer.d.ts | 1 + 5 files changed, 58 insertions(+), 45 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 70c2c7a48746..cd5b602f3569 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -6,10 +6,11 @@ 'use strict'; const log = require('lighthouse-logger'); -const LHError = require('../lib/lh-error'); -const URL = require('../lib/url-shim'); +const manifestParser = require('../lib/manifest-parser.js'); +const LHError = require('../lib/lh-error.js'); +const URL = require('../lib/url-shim.js'); const NetworkRecorder = require('../lib/network-recorder.js'); -const constants = require('../config/constants'); +const constants = require('../config/constants.js'); const Driver = require('../gather/driver.js'); // eslint-disable-line no-unused-vars @@ -419,6 +420,7 @@ class GatherRunner { HostUserAgent: (await options.driver.getBrowserVersion()).userAgent, NetworkUserAgent: '', // updated later BenchmarkIndex: 0, // updated later + AppManifest: null, // updated later traces: {}, devtoolsLogs: {}, settings: options.settings, @@ -427,6 +429,41 @@ class GatherRunner { }; } + /** + * Uses the debugger protocol to fetch the manifest from within the context of + * the target page, reusing any credentials, emulation, etc, already established + * there. + * + * Returns the parsed manifest or null if the page had no manifest. If the manifest + * was unparseable as JSON, manifest.value will be undefined and manifest.warning + * will have the reason. See manifest-parser.js for more information. + * + * @param {LH.Gatherer.PassContext} passContext + * @return {Promise} + */ + static async getAppManifest(passContext) { + if (passContext.baseArtifacts.AppManifest) return passContext.baseArtifacts.AppManifest; + + const BOM_LENGTH = 3; + const BOM_FIRSTCHAR = 65279; + + const manifestPromise = passContext.driver.getAppManifest(); + /** @type {Promise} */ + const timeoutPromise = new Promise(resolve => setTimeout(resolve, 3000)); + + const response = await Promise.race([manifestPromise, timeoutPromise]); + if (!response) { + return null; + } + + const isBomEncoded = response.data.charCodeAt(0) === BOM_FIRSTCHAR; + if (isBomEncoded) { + response.data = Buffer.from(response.data).slice(BOM_LENGTH).toString(); + } + + return manifestParser(response.data, response.url, passContext.url); + } + /** * @param {Array} passes * @param {{driver: Driver, requestedUrl: string, settings: LH.Config.Settings}} options @@ -456,6 +493,7 @@ class GatherRunner { url: options.requestedUrl, settings: options.settings, passConfig, + baseArtifacts, // *pass() functions and gatherers can push to this warnings array. LighthouseRunWarnings: baseArtifacts.LighthouseRunWarnings, }; @@ -467,6 +505,7 @@ class GatherRunner { } await GatherRunner.beforePass(passContext, gathererResults); await GatherRunner.pass(passContext, gathererResults); + baseArtifacts.AppManifest = await GatherRunner.getAppManifest(passContext); const passData = await GatherRunner.afterPass(passContext, gathererResults); // Save devtoolsLog, but networkRecords are discarded and not added onto artifacts. diff --git a/lighthouse-core/gather/gatherers/manifest.js b/lighthouse-core/gather/gatherers/manifest.js index ff4268aff656..6b9a068ff014 100644 --- a/lighthouse-core/gather/gatherers/manifest.js +++ b/lighthouse-core/gather/gatherers/manifest.js @@ -6,40 +6,18 @@ 'use strict'; const Gatherer = require('./gatherer'); -const manifestParser = require('../../lib/manifest-parser'); -const BOM_LENGTH = 3; -const BOM_FIRSTCHAR = 65279; /** - * Uses the debugger protocol to fetch the manifest from within the context of - * the target page, reusing any credentials, emulation, etc, already established - * there. The artifact produced is the fetched string, if any, passed through + * The artifact produced is the fetched manifest string, if any, passed through * the manifest parser. */ class Manifest extends Gatherer { /** - * Returns the parsed manifest or null if the page had no manifest. If the manifest - * was unparseable as JSON, manifest.value will be undefined and manifest.warning - * will have the reason. See manifest-parser.js for more information. * @param {LH.Gatherer.PassContext} passContext * @return {Promise} */ async afterPass(passContext) { - const manifestPromise = passContext.driver.getAppManifest(); - /** @type {Promise} */ - const timeoutPromise = new Promise(resolve => setTimeout(resolve, 3000)); - - const response = await Promise.race([manifestPromise, timeoutPromise]); - if (!response) { - return null; - } - - const isBomEncoded = response.data.charCodeAt(0) === BOM_FIRSTCHAR; - if (isBomEncoded) { - response.data = Buffer.from(response.data).slice(BOM_LENGTH).toString(); - } - - return manifestParser(response.data, response.url, passContext.url); + return passContext.baseArtifacts.AppManifest; } } diff --git a/lighthouse-core/gather/gatherers/start-url.js b/lighthouse-core/gather/gatherers/start-url.js index 1d42543acb82..e6a429f4e5e0 100644 --- a/lighthouse-core/gather/gatherers/start-url.js +++ b/lighthouse-core/gather/gatherers/start-url.js @@ -6,7 +6,6 @@ 'use strict'; const Gatherer = require('./gatherer'); -const manifestParser = require('../../lib/manifest-parser'); /** @typedef {import('../driver.js')} Driver */ @@ -16,27 +15,21 @@ class StartUrl extends Gatherer { * @param {LH.Gatherer.PassContext} passContext * @return {Promise} */ - afterPass(passContext) { - const driver = passContext.driver; - return driver.goOnline(passContext) - .then(() => driver.getAppManifest()) - .then(response => driver.goOffline().then(() => response)) - .then(response => response && manifestParser(response.data, response.url, passContext.url)) - .then(manifest => { - const startUrlInfo = this._readManifestStartUrl(manifest); - if (startUrlInfo.isReadFailure) { - return {statusCode: -1, explanation: startUrlInfo.reason}; - } + async afterPass(passContext) { + const manifest = passContext.baseArtifacts.AppManifest; + const startUrlInfo = this._readManifestStartUrl(manifest); + if (startUrlInfo.isReadFailure) { + return {statusCode: -1, explanation: startUrlInfo.reason}; + } - return this._attemptManifestFetch(passContext.driver, startUrlInfo.startUrl); - }).catch(() => { - return {statusCode: -1, explanation: 'Unable to fetch start URL via service worker.'}; - }); + return this._attemptStartURLFetch(passContext.driver, startUrlInfo.startUrl).catch(() => { + return {statusCode: -1, explanation: 'Unable to fetch start URL via service worker.'}; + }); } /** * Read the parsed manifest and return failure reasons or the startUrl - * @param {?{value?: {start_url: {value: string, warning?: string}}, warning?: string}} manifest + * @param {LH.Artifacts['Manifest']} manifest * @return {{isReadFailure: true, reason: string}|{isReadFailure: false, startUrl: string}} */ _readManifestStartUrl(manifest) { @@ -61,7 +54,7 @@ class StartUrl extends Gatherer { * @param {string} startUrl * @return {Promise<{statusCode: number, explanation: string}>} */ - _attemptManifestFetch(driver, startUrl) { + _attemptStartURLFetch(driver, startUrl) { // Wait up to 3s to get a matched network request from the fetch() to work const timeoutPromise = new Promise(resolve => setTimeout( diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index d4ccf5024057..9fae731eb0ff 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -29,6 +29,8 @@ declare global { NetworkUserAgent: string; /** The benchmark index that indicates rough device class. */ BenchmarkIndex: number; + /** The application manifest if one was fetched. */ + AppManifest: Artifacts.Manifest | null; /** A set of page-load traces, keyed by passName. */ traces: {[passName: string]: Trace}; /** A set of DevTools debugger protocol records, keyed by passName. */ diff --git a/types/gatherer.d.ts b/types/gatherer.d.ts index db4755b36939..78e6c711ce77 100644 --- a/types/gatherer.d.ts +++ b/types/gatherer.d.ts @@ -20,6 +20,7 @@ declare global { options?: object; /** Push to this array to add top-level warnings to the LHR. */ LighthouseRunWarnings: Array; + baseArtifacts: BaseArtifacts; } export interface LoadData { From 6073f9945035d9c9856102a57f8cf93b1413abc4 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Wed, 9 Jan 2019 19:44:12 -0600 Subject: [PATCH 2/7] just feedback --- lighthouse-core/gather/driver.js | 33 ++++++++++++++++--------- lighthouse-core/gather/gather-runner.js | 28 +++++---------------- types/artifacts.d.ts | 4 +-- 3 files changed, 29 insertions(+), 36 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index aae4a4339a38..7dcc4e1dba31 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -410,19 +410,28 @@ class Driver { /** * @return {Promise<{url: string, data: string}|null>} */ - getAppManifest() { - return this.sendCommand('Page.getAppManifest') - .then(response => { - // We're not reading `response.errors` however it may contain critical and noncritical - // errors from Blink's manifest parser: - // https://chromedevtools.github.io/debugger-protocol-viewer/tot/Page/#type-AppManifestError - if (!response.data) { - // If the data is empty, the page had no manifest. - return null; - } + async getAppManifest() { + this.setNextProtocolTimeout(3000); + const response = await this.sendCommand('Page.getAppManifest'); + let data = response.data; + + // We're not reading `response.errors` however it may contain critical and noncritical + // errors from Blink's manifest parser: + // https://chromedevtools.github.io/debugger-protocol-viewer/tot/Page/#type-AppManifestError + if (!data) { + // If the data is empty, the page had no manifest. + return null; + } - return /** @type {Required} */ (response); - }); + const BOM_LENGTH = 3; + const BOM_FIRSTCHAR = 65279; + const isBomEncoded = data.charCodeAt(0) === BOM_FIRSTCHAR; + + if (isBomEncoded) { + data = Buffer.from(data).slice(BOM_LENGTH).toString(); + } + + return {...response, data}; } /** diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index cd5b602f3569..09ab453181d0 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -420,7 +420,7 @@ class GatherRunner { HostUserAgent: (await options.driver.getBrowserVersion()).userAgent, NetworkUserAgent: '', // updated later BenchmarkIndex: 0, // updated later - AppManifest: null, // updated later + WebAppManifest: null, // updated later traces: {}, devtoolsLogs: {}, settings: options.settings, @@ -441,26 +441,10 @@ class GatherRunner { * @param {LH.Gatherer.PassContext} passContext * @return {Promise} */ - static async getAppManifest(passContext) { - if (passContext.baseArtifacts.AppManifest) return passContext.baseArtifacts.AppManifest; - - const BOM_LENGTH = 3; - const BOM_FIRSTCHAR = 65279; - - const manifestPromise = passContext.driver.getAppManifest(); - /** @type {Promise} */ - const timeoutPromise = new Promise(resolve => setTimeout(resolve, 3000)); - - const response = await Promise.race([manifestPromise, timeoutPromise]); - if (!response) { - return null; - } - - const isBomEncoded = response.data.charCodeAt(0) === BOM_FIRSTCHAR; - if (isBomEncoded) { - response.data = Buffer.from(response.data).slice(BOM_LENGTH).toString(); - } - + static async getWebAppManifest(passContext) { + if (passContext.baseArtifacts.WebAppManifest) return passContext.baseArtifacts.WebAppManifest; + const response = await passContext.driver.getAppManifest(); + if (!response) return null; return manifestParser(response.data, response.url, passContext.url); } @@ -505,7 +489,7 @@ class GatherRunner { } await GatherRunner.beforePass(passContext, gathererResults); await GatherRunner.pass(passContext, gathererResults); - baseArtifacts.AppManifest = await GatherRunner.getAppManifest(passContext); + baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext); const passData = await GatherRunner.afterPass(passContext, gathererResults); // Save devtoolsLog, but networkRecords are discarded and not added onto artifacts. diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 9fae731eb0ff..5ddbb7457098 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -29,8 +29,8 @@ declare global { NetworkUserAgent: string; /** The benchmark index that indicates rough device class. */ BenchmarkIndex: number; - /** The application manifest if one was fetched. */ - AppManifest: Artifacts.Manifest | null; + /** Parsed version of the page's Web App Manifest, or null if none found.. */ + WebAppManifest: Artifacts.Manifest | null; /** A set of page-load traces, keyed by passName. */ traces: {[passName: string]: Trace}; /** A set of DevTools debugger protocol records, keyed by passName. */ From 09869b0af8ca55cf815a5fdbd2f9e12d4c2a5158 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Wed, 9 Jan 2019 21:01:39 -0600 Subject: [PATCH 3/7] fix tests --- lighthouse-core/gather/gather-runner.js | 4 ++ lighthouse-core/test/gather/driver-test.js | 30 +++++++++++++ lighthouse-core/test/gather/fake-driver.js | 3 ++ .../test/gather/gather-runner-test.js | 45 +++++++++++++++++++ .../test/gather/gatherers/manifest-test.js | 6 +-- types/gatherer.d.ts | 1 + 6 files changed, 84 insertions(+), 5 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 09ab453181d0..826cd3aaf3db 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -442,7 +442,10 @@ class GatherRunner { * @return {Promise} */ static async getWebAppManifest(passContext) { + // If we already have a manifest, return it. if (passContext.baseArtifacts.WebAppManifest) return passContext.baseArtifacts.WebAppManifest; + // If we're not on the first pass and we didn't already have a manifest, don't try again. + if (!passContext.isFirstPass) return null; const response = await passContext.driver.getAppManifest(); if (!response) return null; return manifestParser(response.data, response.url, passContext.url); @@ -472,6 +475,7 @@ class GatherRunner { let isFirstPass = true; for (const passConfig of passes) { const passContext = { + isFirstPass, driver: options.driver, // If the main document redirects, we'll update this to keep track url: options.requestedUrl, diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index 3229755d6533..37935582fe67 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -318,6 +318,36 @@ describe('Browser Driver', () => { assert.equal(sendCommandParams[0], undefined); }); }); + + describe('.getAppManifest', () => { + it('should return null when no manifest', async () => { + const sendCommand = jest.spyOn(connection, 'sendCommand'); + sendCommand.mockResolvedValueOnce({data: undefined, url: '/manifest'}); + const result = await driverStub.getAppManifest(); + expect(result).toEqual(null); + }); + + it('should return the manifest', async () => { + const manifest = {name: 'The App'}; + const sendCommand = jest.spyOn(connection, 'sendCommand'); + sendCommand.mockResolvedValueOnce({data: JSON.stringify(manifest), url: '/manifest'}); + const result = await driverStub.getAppManifest(); + expect(result).toEqual({data: JSON.stringify(manifest), url: '/manifest'}); + }); + + it('should handle BOM-encoded manifest', async () => { + const fs = require('fs'); + const manifestWithoutBOM = fs.readFileSync(__dirname + '/../fixtures/manifest.json') + .toString(); + const manifestWithBOM = fs.readFileSync(__dirname + '/../fixtures/manifest-bom.json') + .toString(); + + const sendCommand = jest.spyOn(connection, 'sendCommand'); + sendCommand.mockResolvedValueOnce({data: manifestWithBOM, url: '/manifest'}); + const result = await driverStub.getAppManifest(); + expect(result).toEqual({data: manifestWithoutBOM, url: '/manifest'}); + }); + }); }); describe('Multiple tab check', () => { diff --git a/lighthouse-core/test/gather/fake-driver.js b/lighthouse-core/test/gather/fake-driver.js index 75ce1e2f3eca..860877240391 100644 --- a/lighthouse-core/test/gather/fake-driver.js +++ b/lighthouse-core/test/gather/fake-driver.js @@ -22,6 +22,9 @@ const fakeDriver = { getBenchmarkIndex() { return Promise.resolve(125.2); }, + getAppManifest() { + return Promise.resolve(null); + }, connect() { return Promise.resolve(); }, diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 68c2076e109b..212ed1755f9d 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -1110,4 +1110,49 @@ describe('GatherRunner', function() { }); }); }); + + describe('.getWebAppManifest', () => { + const MANIFEST_URL = 'https://example.com/manifest.json'; + let passContext; + + beforeEach(() => { + passContext = { + isFirstPass: true, + url: 'https://example.com/index.html', + baseArtifacts: {}, + driver: fakeDriver, + }; + }); + + it('should pass through manifest when null', async () => { + const getAppManifest = jest.spyOn(fakeDriver, 'getAppManifest'); + getAppManifest.mockResolvedValueOnce(null); + const result = await GatherRunner.getWebAppManifest(passContext); + expect(result).toEqual(null); + }); + + it('should parse the manifest when found', async () => { + const manifest = {name: 'App'}; + const getAppManifest = jest.spyOn(fakeDriver, 'getAppManifest'); + getAppManifest.mockResolvedValueOnce({data: JSON.stringify(manifest), url: MANIFEST_URL}); + const result = await GatherRunner.getWebAppManifest(passContext); + expect(result).toHaveProperty('raw', JSON.stringify(manifest)); + expect(result.value).toMatchObject({ + name: {value: 'App', raw: 'App'}, + start_url: {value: passContext.url, raw: undefined}, + }); + }); + + it('should return previous result when already set', async () => { + passContext.baseArtifacts.WebAppManifest = {value: {name: 'App'}}; + const result = await GatherRunner.getWebAppManifest(passContext); + expect(result).toEqual({value: {name: 'App'}}); + }); + + it('should not try to fetch on 2nd/3rd passes', async () => { + passContext.isFirstPass = false; + const result = await GatherRunner.getWebAppManifest(passContext); + expect(result).toEqual(null); + }); + }); }); diff --git a/lighthouse-core/test/gather/gatherers/manifest-test.js b/lighthouse-core/test/gather/gatherers/manifest-test.js index a22813ff9321..c4d3db89c85a 100644 --- a/lighthouse-core/test/gather/gatherers/manifest-test.js +++ b/lighthouse-core/test/gather/gatherers/manifest-test.js @@ -24,11 +24,7 @@ describe('Manifest gatherer', () => { return manifestGather.afterPass({ driver: { getAppManifest() { - return Promise.resolve({ - data: '{}', - errors: [], - url: EXAMPLE_MANIFEST_URL, - }); + return Promise.resolve(); }, }, url: EXAMPLE_DOC_URL, diff --git a/types/gatherer.d.ts b/types/gatherer.d.ts index 78e6c711ce77..499083fb0fa1 100644 --- a/types/gatherer.d.ts +++ b/types/gatherer.d.ts @@ -12,6 +12,7 @@ import Driver = require('../lighthouse-core/gather/driver'); declare global { module LH.Gatherer { export interface PassContext { + isFirstPass: boolean url: string; driver: Driver; disableJavaScript?: boolean; From 633829ebf812309f1f1fb428df095abc5e2e5238 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Wed, 9 Jan 2019 21:02:41 -0600 Subject: [PATCH 4/7] todo --- lighthouse-core/gather/gatherers/manifest.js | 5 +- .../test/gather/gatherers/manifest-test.js | 124 ------------------ 2 files changed, 2 insertions(+), 127 deletions(-) delete mode 100644 lighthouse-core/test/gather/gatherers/manifest-test.js diff --git a/lighthouse-core/gather/gatherers/manifest.js b/lighthouse-core/gather/gatherers/manifest.js index 6b9a068ff014..5d0f72cbde4f 100644 --- a/lighthouse-core/gather/gatherers/manifest.js +++ b/lighthouse-core/gather/gatherers/manifest.js @@ -8,8 +8,7 @@ const Gatherer = require('./gatherer'); /** - * The artifact produced is the fetched manifest string, if any, passed through - * the manifest parser. + * TODO(phulce): delete this file and move usages over to WebAppManifest */ class Manifest extends Gatherer { /** @@ -17,7 +16,7 @@ class Manifest extends Gatherer { * @return {Promise} */ async afterPass(passContext) { - return passContext.baseArtifacts.AppManifest; + return passContext.baseArtifacts.WebAppManifest; } } diff --git a/lighthouse-core/test/gather/gatherers/manifest-test.js b/lighthouse-core/test/gather/gatherers/manifest-test.js deleted file mode 100644 index c4d3db89c85a..000000000000 --- a/lighthouse-core/test/gather/gatherers/manifest-test.js +++ /dev/null @@ -1,124 +0,0 @@ -/** - * @license Copyright 2016 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'; - -/* eslint-env jest */ - -const ManifestGather = require('../../../gather/gatherers/manifest'); -const assert = require('assert'); -let manifestGather; - -const EXAMPLE_MANIFEST_URL = 'https://example.com/manifest.json'; -const EXAMPLE_DOC_URL = 'https://example.com/index.html'; - -describe('Manifest gatherer', () => { - // Reset the Gatherer before each test. - beforeEach(() => { - manifestGather = new ManifestGather(); - }); - - it('returns an artifact', () => { - return manifestGather.afterPass({ - driver: { - getAppManifest() { - return Promise.resolve(); - }, - }, - url: EXAMPLE_DOC_URL, - }).then(artifact => { - assert.ok(typeof artifact === 'object'); - }); - }); - - it('returns an artifact when manifest is saved as BOM UTF-8', () => { - const fs = require('fs'); - const manifestWithoutBOM = fs.readFileSync(__dirname + '/../../fixtures/manifest.json') - .toString(); - const manifestWithBOM = fs.readFileSync(__dirname + '/../../fixtures/manifest-bom.json') - .toString(); - - const getDriver = (data) => { - return { - driver: { - getAppManifest() { - return Promise.resolve({ - data, - errors: [], - url: EXAMPLE_MANIFEST_URL, - }); - }, - }, - url: EXAMPLE_DOC_URL, - }; - }; - - const promises = []; - promises.push(manifestGather.afterPass(getDriver(manifestWithBOM)) - .then(manifest => { - assert.strictEqual(manifest.raw, Buffer.from(manifestWithBOM).slice(3).toString()); - assert.strictEqual(manifest.value.name.value, 'Example App'); - assert.strictEqual(manifest.value.short_name.value, 'ExApp'); - }) - ); - - promises.push(manifestGather.afterPass(getDriver(manifestWithoutBOM)) - .then(manifest => { - assert.strictEqual(manifest.raw, manifestWithoutBOM); - assert.strictEqual(manifest.value.name.value, 'Example App'); - assert.strictEqual(manifest.value.short_name.value, 'ExApp'); - }) - ); - - return Promise.all(promises); - }); - - it('throws an error when unable to retrieve the manifest', () => { - return manifestGather.afterPass({ - driver: { - getAppManifest() { - return Promise.reject( - new Error(`Unable to retrieve manifest at ${EXAMPLE_MANIFEST_URL}.`) - ); - }, - }, - }).then( - _ => assert.ok(false), - err => assert.ok(err.message.includes(EXAMPLE_MANIFEST_URL))); - }); - - it('returns null when the page had no manifest', () => { - return manifestGather.afterPass({ - driver: { - getAppManifest() { - return Promise.resolve(null); - }, - }, - }).then(artifact => { - assert.strictEqual(artifact, null); - }); - }); - - it('creates a manifest object for valid manifest content', () => { - const data = JSON.stringify({ - name: 'App', - }); - return manifestGather.afterPass({ - driver: { - getAppManifest() { - return Promise.resolve({ - errors: [], - data, - url: EXAMPLE_MANIFEST_URL, - }); - }, - }, - url: EXAMPLE_DOC_URL, - }).then(artifact => { - assert.ok(typeof artifact.value === 'object'); - assert.strictEqual(artifact.explanation, undefined); - }); - }); -}); From 5cca8a90d9fbb1be0c229ac86c28f46f5c3f0760 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Wed, 9 Jan 2019 21:17:28 -0600 Subject: [PATCH 5/7] cleanup start url --- lighthouse-core/gather/gatherers/start-url.js | 2 +- .../test/gather/gatherers/start-url-test.js | 86 +++++++++++-------- 2 files changed, 51 insertions(+), 37 deletions(-) diff --git a/lighthouse-core/gather/gatherers/start-url.js b/lighthouse-core/gather/gatherers/start-url.js index e6a429f4e5e0..c7751d22494a 100644 --- a/lighthouse-core/gather/gatherers/start-url.js +++ b/lighthouse-core/gather/gatherers/start-url.js @@ -16,7 +16,7 @@ class StartUrl extends Gatherer { * @return {Promise} */ async afterPass(passContext) { - const manifest = passContext.baseArtifacts.AppManifest; + const manifest = passContext.baseArtifacts.WebAppManifest; const startUrlInfo = this._readManifestStartUrl(manifest); if (startUrlInfo.isReadFailure) { return {statusCode: -1, explanation: startUrlInfo.reason}; diff --git a/lighthouse-core/test/gather/gatherers/start-url-test.js b/lighthouse-core/test/gather/gatherers/start-url-test.js index df9b0fd3710f..3868ec62541c 100644 --- a/lighthouse-core/test/gather/gatherers/start-url-test.js +++ b/lighthouse-core/test/gather/gatherers/start-url-test.js @@ -7,9 +7,9 @@ /* eslint-env jest */ -const StartUrlGatherer = require('../../../gather/gatherers/start-url'); +const StartUrlGatherer = require('../../../gather/gatherers/start-url.js'); +const parseManifest = require('../../../lib/manifest-parser.js'); const assert = require('assert'); -const tracingData = require('../../fixtures/traces/network-records.json'); const mockDriver = { goOffline() { @@ -28,18 +28,25 @@ const wrapSendCommand = (mockDriver, url, status = 200, fromServiceWorker = fals cb({response: {status, url, fromServiceWorker}}); }; - mockDriver.getAppManifest = () => { - return Promise.resolve({ - data: '{"start_url": "' + url + '"}', - errors: [], - url, - }); - }; - return mockDriver; }; describe('Start-url gatherer', () => { + let baseArtifacts; + + function createArtifactsWithURL(url) { + return {WebAppManifest: {value: {start_url: {value: url}}}}; + } + + beforeEach(() => { + jest.useFakeTimers(); + baseArtifacts = {WebAppManifest: null}; + }); + + afterEach(() => { + jest.advanceTimersByTime(5000); + }); + it('returns an artifact set to -1 when offline loading fails', () => { const startUrlGatherer = new StartUrlGatherer(); const startUrlGathererWithQueryString = new StartUrlGatherer(); @@ -58,23 +65,26 @@ describe('Start-url gatherer', () => { return mockDriver; }; - const options = { + const passContext = { url: 'https://do-not-match.com/', + baseArtifacts, driver: throwOnEvaluate(wrapSendCommand(mockDriver, 'https://do-not-match.com/', -1)), }; - const optionsWithQueryString = { + const passContextWithFragment = { + baseArtifacts, url: 'https://ifixit-pwa.appspot.com/?history', driver: throwOnEvaluate(wrapSendCommand(mockDriver, 'https://ifixit-pwa.appspot.com/?history', -1)), }; - const optionsWithResponseNotFromSW = { + const passContextWithResponseNotFromSW = { url: 'https://do-not-match.com/', + baseArtifacts: createArtifactsWithURL('https://do-not-match.com/'), driver: wrapSendCommand(mockDriver, 'https://do-not-match.com/', 200), }; return Promise.all([ - startUrlGatherer.afterPass(options), - startUrlGathererWithQueryString.afterPass(optionsWithQueryString), - startUrlGathererWithResponseNotFromSW.afterPass(optionsWithResponseNotFromSW), + startUrlGatherer.afterPass(passContext), + startUrlGathererWithQueryString.afterPass(passContextWithFragment), + startUrlGathererWithResponseNotFromSW.afterPass(passContextWithResponseNotFromSW), ]).then(([artifact, artifactWithQueryString, artifactWithResponseNotFromSW]) => { assert.equal(artifact.statusCode, -1); assert.ok(artifact.explanation, 'did not set debug string'); @@ -89,18 +99,20 @@ describe('Start-url gatherer', () => { it('returns an artifact set to 200 when offline loading from service worker succeeds', () => { const startUrlGatherer = new StartUrlGatherer(); const startUrlGathererWithFragment = new StartUrlGatherer(); - const options = { + const passContext = { url: 'https://ifixit-pwa.appspot.com/', + baseArtifacts: createArtifactsWithURL('https://ifixit-pwa.appspot.com/'), driver: wrapSendCommand(mockDriver, 'https://ifixit-pwa.appspot.com/', 200, true), }; - const optionsWithQueryString = { + const passContextWithFragment = { url: 'https://ifixit-pwa.appspot.com/#/history', + baseArtifacts: createArtifactsWithURL('https://ifixit-pwa.appspot.com/#/history'), driver: wrapSendCommand(mockDriver, 'https://ifixit-pwa.appspot.com/#/history', 200, true), }; return Promise.all([ - startUrlGatherer.afterPass(options, tracingData), - startUrlGathererWithFragment.afterPass(optionsWithQueryString, tracingData), + startUrlGatherer.afterPass(passContext), + startUrlGathererWithFragment.afterPass(passContextWithFragment), ]).then(([artifact, artifactWithFragment]) => { assert.equal(artifact.statusCode, 200); assert.equal(artifactWithFragment.statusCode, 200); @@ -110,14 +122,13 @@ describe('Start-url gatherer', () => { it('returns a explanation when manifest cannot be found', () => { const driver = Object.assign({}, mockDriver); const startUrlGatherer = new StartUrlGatherer(); - const options = { + const passContext = { + baseArtifacts, url: 'https://ifixit-pwa.appspot.com/', driver, }; - driver.getAppManifest = () => Promise.resolve(null); - - return startUrlGatherer.afterPass(options, tracingData) + return startUrlGatherer.afterPass(passContext) .then(artifact => { assert.equal(artifact.explanation, `No usable web app manifest found on page.`); @@ -127,17 +138,19 @@ describe('Start-url gatherer', () => { it('returns a explanation when manifest cannot be parsed', () => { const driver = Object.assign({}, mockDriver); const startUrlGatherer = new StartUrlGatherer(); - const options = { + const passContext = { + baseArtifacts, url: 'https://ifixit-pwa.appspot.com/', driver, }; - driver.getAppManifest = () => Promise.resolve({ - data: 'this is invalid', - url: 'https://ifixit-pwa.appspot.com/manifest.json', - }); + baseArtifacts.WebAppManifest = parseManifest( + 'this is invalid', + 'https://ifixit-pwa.appspot.com/manifest.json', + passContext.url + ); - return startUrlGatherer.afterPass(options, tracingData) + return startUrlGatherer.afterPass(passContext) .then(artifact => { assert.equal(artifact.explanation, `Error fetching web app manifest: ERROR: file isn't valid JSON: ` + @@ -145,16 +158,17 @@ describe('Start-url gatherer', () => { }); }); - it('times out when a start_url is too slow to respond', () => { + it('times out when a start_url is too slow to respond', async () => { const startUrlGatherer = new StartUrlGatherer(); - const options = { + const passContext = { url: 'https://ifixit-pwa.appspot.com/', + baseArtifacts: createArtifactsWithURL('https://ifixit-pwa.appspot.com/'), driver: wrapSendCommand(mockDriver, ''), }; - return startUrlGatherer.afterPass(options, tracingData) - .then(artifact => { - assert.equal(artifact.explanation, 'Timed out waiting for fetched start_url.'); - }); + const resultPromise = startUrlGatherer.afterPass(passContext); + jest.advanceTimersByTime(5000); + const artifact = await resultPromise; + assert.equal(artifact.explanation, 'Timed out waiting for fetched start_url.'); }); }); From 78ba22dd183bd202bac639156bc8fb9ad6fc20a9 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Wed, 16 Jan 2019 20:34:51 -0600 Subject: [PATCH 6/7] feedback --- lighthouse-core/gather/gather-runner.js | 11 ++++------- lighthouse-core/gather/gatherers/start-url.js | 3 ++- lighthouse-core/test/gather/driver-test.js | 18 ++++++++++-------- .../test/gather/gather-runner-test.js | 13 ------------- types/artifacts.d.ts | 2 +- types/gatherer.d.ts | 1 - 6 files changed, 17 insertions(+), 31 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 826cd3aaf3db..eb7c1f74595b 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -439,13 +439,9 @@ class GatherRunner { * will have the reason. See manifest-parser.js for more information. * * @param {LH.Gatherer.PassContext} passContext - * @return {Promise} + * @return {Promise} */ static async getWebAppManifest(passContext) { - // If we already have a manifest, return it. - if (passContext.baseArtifacts.WebAppManifest) return passContext.baseArtifacts.WebAppManifest; - // If we're not on the first pass and we didn't already have a manifest, don't try again. - if (!passContext.isFirstPass) return null; const response = await passContext.driver.getAppManifest(); if (!response) return null; return manifestParser(response.data, response.url, passContext.url); @@ -475,7 +471,6 @@ class GatherRunner { let isFirstPass = true; for (const passConfig of passes) { const passContext = { - isFirstPass, driver: options.driver, // If the main document redirects, we'll update this to keep track url: options.requestedUrl, @@ -493,7 +488,9 @@ class GatherRunner { } await GatherRunner.beforePass(passContext, gathererResults); await GatherRunner.pass(passContext, gathererResults); - baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext); + if (isFirstPass) { + baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext); + } const passData = await GatherRunner.afterPass(passContext, gathererResults); // Save devtoolsLog, but networkRecords are discarded and not added onto artifacts. diff --git a/lighthouse-core/gather/gatherers/start-url.js b/lighthouse-core/gather/gatherers/start-url.js index c7751d22494a..a1a09099da47 100644 --- a/lighthouse-core/gather/gatherers/start-url.js +++ b/lighthouse-core/gather/gatherers/start-url.js @@ -29,7 +29,7 @@ class StartUrl extends Gatherer { /** * Read the parsed manifest and return failure reasons or the startUrl - * @param {LH.Artifacts['Manifest']} manifest + * @param {LH.Artifacts.Manifest|null} manifest * @return {{isReadFailure: true, reason: string}|{isReadFailure: false, startUrl: string}} */ _readManifestStartUrl(manifest) { @@ -55,6 +55,7 @@ class StartUrl extends Gatherer { * @return {Promise<{statusCode: number, explanation: string}>} */ _attemptStartURLFetch(driver, startUrl) { + // TODO(phulce): clean up this setTimeout once the response has happened // Wait up to 3s to get a matched network request from the fetch() to work const timeoutPromise = new Promise(resolve => setTimeout( diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index 37935582fe67..11e511dc9af4 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -53,7 +53,7 @@ function createOnceMethodResponse(method, response) { sendCommandMockResponses.set(method, response); } -connection.sendCommand = function(command, params) { +function fakeSendCommand(command, params) { sendCommandParams.push({command, params}); if (sendCommandMockResponses.has(command)) { @@ -108,12 +108,13 @@ connection.sendCommand = function(command, params) { default: throw Error(`Stub not implemented: ${command}`); } -}; +} /* eslint-env jest */ describe('Browser Driver', () => { beforeEach(() => { + connection.sendCommand = fakeSendCommand; sendCommandParams = []; sendCommandMockResponses.clear(); }); @@ -321,16 +322,15 @@ describe('Browser Driver', () => { describe('.getAppManifest', () => { it('should return null when no manifest', async () => { - const sendCommand = jest.spyOn(connection, 'sendCommand'); - sendCommand.mockResolvedValueOnce({data: undefined, url: '/manifest'}); + connection.sendCommand = jest.fn().mockResolvedValueOnce({data: undefined, url: '/manifest'}); const result = await driverStub.getAppManifest(); expect(result).toEqual(null); }); it('should return the manifest', async () => { const manifest = {name: 'The App'}; - const sendCommand = jest.spyOn(connection, 'sendCommand'); - sendCommand.mockResolvedValueOnce({data: JSON.stringify(manifest), url: '/manifest'}); + connection.sendCommand = jest.fn() + .mockResolvedValueOnce({data: JSON.stringify(manifest), url: '/manifest'}); const result = await driverStub.getAppManifest(); expect(result).toEqual({data: JSON.stringify(manifest), url: '/manifest'}); }); @@ -342,8 +342,8 @@ describe('Browser Driver', () => { const manifestWithBOM = fs.readFileSync(__dirname + '/../fixtures/manifest-bom.json') .toString(); - const sendCommand = jest.spyOn(connection, 'sendCommand'); - sendCommand.mockResolvedValueOnce({data: manifestWithBOM, url: '/manifest'}); + connection.sendCommand = jest.fn() + .mockResolvedValueOnce({data: manifestWithBOM, url: '/manifest'}); const result = await driverStub.getAppManifest(); expect(result).toEqual({data: manifestWithoutBOM, url: '/manifest'}); }); @@ -352,7 +352,9 @@ describe('Browser Driver', () => { describe('Multiple tab check', () => { beforeEach(() => { + connection.sendCommand = fakeSendCommand; sendCommandParams = []; + sendCommandMockResponses.clear(); }); it('will pass if there are no current service workers', () => { diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 212ed1755f9d..0a6570960c93 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -1117,7 +1117,6 @@ describe('GatherRunner', function() { beforeEach(() => { passContext = { - isFirstPass: true, url: 'https://example.com/index.html', baseArtifacts: {}, driver: fakeDriver, @@ -1142,17 +1141,5 @@ describe('GatherRunner', function() { start_url: {value: passContext.url, raw: undefined}, }); }); - - it('should return previous result when already set', async () => { - passContext.baseArtifacts.WebAppManifest = {value: {name: 'App'}}; - const result = await GatherRunner.getWebAppManifest(passContext); - expect(result).toEqual({value: {name: 'App'}}); - }); - - it('should not try to fetch on 2nd/3rd passes', async () => { - passContext.isFirstPass = false; - const result = await GatherRunner.getWebAppManifest(passContext); - expect(result).toEqual(null); - }); }); }); diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 5ddbb7457098..b69150b0ed3c 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -29,7 +29,7 @@ declare global { NetworkUserAgent: string; /** The benchmark index that indicates rough device class. */ BenchmarkIndex: number; - /** Parsed version of the page's Web App Manifest, or null if none found.. */ + /** Parsed version of the page's Web App Manifest, or null if none found. */ WebAppManifest: Artifacts.Manifest | null; /** A set of page-load traces, keyed by passName. */ traces: {[passName: string]: Trace}; diff --git a/types/gatherer.d.ts b/types/gatherer.d.ts index 499083fb0fa1..78e6c711ce77 100644 --- a/types/gatherer.d.ts +++ b/types/gatherer.d.ts @@ -12,7 +12,6 @@ import Driver = require('../lighthouse-core/gather/driver'); declare global { module LH.Gatherer { export interface PassContext { - isFirstPass: boolean url: string; driver: Driver; disableJavaScript?: boolean; From bc6dfd5b5a3d34591f85beed2be953b8f14d6657 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Thu, 17 Jan 2019 09:15:36 -0600 Subject: [PATCH 7/7] merge fixes --- lighthouse-core/test/gather/driver-test.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index 2634077a79c1..9dc52c17540b 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -117,10 +117,11 @@ function sendCommandStub(command, params) { /* eslint-env jest */ let driverStub; +let connectionStub; beforeEach(() => { - const connection = new Connection(); - connection.sendCommand = sendCommandStub; - driverStub = new Driver(connection); + connectionStub = new Connection(); + connectionStub.sendCommand = sendCommandStub; + driverStub = new Driver(connectionStub); }); describe('Browser Driver', () => { @@ -332,14 +333,15 @@ describe('Browser Driver', () => { describe('.getAppManifest', () => { it('should return null when no manifest', async () => { - connection.sendCommand = jest.fn().mockResolvedValueOnce({data: undefined, url: '/manifest'}); + connectionStub.sendCommand = jest.fn() + .mockResolvedValueOnce({data: undefined, url: '/manifest'}); const result = await driverStub.getAppManifest(); expect(result).toEqual(null); }); it('should return the manifest', async () => { const manifest = {name: 'The App'}; - connection.sendCommand = jest.fn() + connectionStub.sendCommand = jest.fn() .mockResolvedValueOnce({data: JSON.stringify(manifest), url: '/manifest'}); const result = await driverStub.getAppManifest(); expect(result).toEqual({data: JSON.stringify(manifest), url: '/manifest'}); @@ -352,7 +354,7 @@ describe('Browser Driver', () => { const manifestWithBOM = fs.readFileSync(__dirname + '/../fixtures/manifest-bom.json') .toString(); - connection.sendCommand = jest.fn() + connectionStub.sendCommand = jest.fn() .mockResolvedValueOnce({data: manifestWithBOM, url: '/manifest'}); const result = await driverStub.getAppManifest(); expect(result).toEqual({data: manifestWithoutBOM, url: '/manifest'});