From eefff12f55d6901c374310817a39c505286067eb Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Mon, 22 Apr 2019 17:05:17 -0700 Subject: [PATCH 1/9] split run() and runPass() --- lighthouse-core/gather/gather-runner.js | 75 +++++++++++++++---------- lighthouse-core/lib/stack-collector.js | 8 ++- 2 files changed, 49 insertions(+), 34 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index f7efa385fa02..1447e803492d 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -460,11 +460,10 @@ class GatherRunner { baseArtifacts.BenchmarkIndex = await options.driver.getBenchmarkIndex(); await GatherRunner.setupDriver(driver, options); - // Run each pass let isFirstPass = true; for (const passConfig of passes) { const passContext = { - driver: options.driver, + driver, // If the main document redirects, we'll update this to keep track url: options.requestedUrl, settings: options.settings, @@ -474,49 +473,38 @@ class GatherRunner { LighthouseRunWarnings: baseArtifacts.LighthouseRunWarnings, }; - await driver.setThrottling(options.settings, passConfig); if (!isFirstPass) { // Already on blank page if driver was just set up. await GatherRunner.loadBlank(driver, passConfig.blankPage); } - await GatherRunner.beforePass(passContext, gathererResults); - await GatherRunner.pass(passContext, gathererResults); - if (isFirstPass) { - // Fetch the manifest, if it exists. Currently must be fetched before gatherers' `afterPass`. - baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext); - } - - const passData = await GatherRunner.afterPass(passContext, gathererResults); + const passResults = await GatherRunner.runPass(passContext); + Object.assign(gathererResults, passResults); + // TODO(bckenny): move into collect base or whatever? if (isFirstPass) { - baseArtifacts.Stacks = await stacksGatherer(passContext); - } - - // Save devtoolsLog, but networkRecords are discarded and not added onto artifacts. - baseArtifacts.devtoolsLogs[passConfig.passName] = passData.devtoolsLog; + // Copy redirected URL to artifact in the first pass only. + baseArtifacts.URL.finalUrl = passContext.url; - const userAgentEntry = passData.devtoolsLog.find(entry => - entry.method === 'Network.requestWillBeSent' && - !!entry.params.request.headers['User-Agent'] - ); + // Fetch the manifest, if it exists. Currently must be fetched before `start-url` gatherer. + baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext); - if (userAgentEntry && !baseArtifacts.NetworkUserAgent) { - // @ts-ignore - guaranteed to exist by the find above - baseArtifacts.NetworkUserAgent = userAgentEntry.params.request.headers['User-Agent']; - } + baseArtifacts.Stacks = await stacksGatherer(driver); - // If requested by config, save pass's trace. - if (passData.trace) { - baseArtifacts.traces[passConfig.passName] = passData.trace; - } + const devtoolsLog = baseArtifacts.devtoolsLogs[passConfig.passName]; + const userAgentEntry = devtoolsLog.find(entry => + entry.method === 'Network.requestWillBeSent' && + !!entry.params.request.headers['User-Agent'] + ); + if (userAgentEntry) { + // @ts-ignore - guaranteed to exist by the find above + baseArtifacts.NetworkUserAgent = userAgentEntry.params.request.headers['User-Agent']; + } - if (isFirstPass) { - // Copy redirected URL to artifact in the first pass only. - baseArtifacts.URL.finalUrl = passContext.url; isFirstPass = false; } } + const resetStorage = !options.settings.disableStorageReset; if (resetStorage) await driver.clearDataForOrigin(options.requestedUrl); await GatherRunner.disposeDriver(driver); @@ -527,6 +515,31 @@ class GatherRunner { throw err; } } + + /** + * Starting from about:blank, load the page and run gatherers for this pass. + * @param {LH.Gatherer.PassContext} passContext + * @return {Promise>} + */ + static async runPass(passContext) { + const passConfig = passContext.passConfig; + await passContext.driver.setThrottling(passContext.settings, passConfig); + + /** @type {Partial} */ + const gathererResults = {}; + + await GatherRunner.beforePass(passContext, gathererResults); + await GatherRunner.pass(passContext, gathererResults); + const passData = await GatherRunner.afterPass(passContext, gathererResults); + + // Save devtoolsLog and trace (if requested by passConfig). + const baseArtifacts = passContext.baseArtifacts; + baseArtifacts.devtoolsLogs[passConfig.passName] = passData.devtoolsLog; + if (passData.trace) baseArtifacts.traces[passConfig.passName] = passData.trace; + + // TODO(bckenny): actually collect here + return gathererResults; + } } module.exports = GatherRunner; diff --git a/lighthouse-core/lib/stack-collector.js b/lighthouse-core/lib/stack-collector.js index 984d8b1dca60..fe45a027889e 100644 --- a/lighthouse-core/lib/stack-collector.js +++ b/lighthouse-core/lib/stack-collector.js @@ -17,6 +17,8 @@ const fs = require('fs'); const libDetectorSource = fs.readFileSync( require.resolve('js-library-detector/library/libraries.js'), 'utf8'); +/** @typedef {import('./../gather/driver.js')} Driver */ + /** @typedef {false | {version: string|null}} JSLibraryDetectorTestResult */ /** * @typedef JSLibraryDetectorTest @@ -66,17 +68,17 @@ async function detectLibraries() { } /** - * @param {LH.Gatherer.PassContext} passContext + * @param {Driver} driver * @return {Promise} */ -async function collectStacks(passContext) { +async function collectStacks(driver) { const expression = `(function () { ${libDetectorSource}; return (${detectLibraries.toString()}()); })()`; /** @type {JSLibrary[]} */ - const jsLibraries = await passContext.driver.evaluateAsync(expression); + const jsLibraries = await driver.evaluateAsync(expression); return jsLibraries.map(lib => ({ detector: /** @type {'js'} */ ('js'), From 91eb99655cf10ec36af898e16db4928eb3456a38 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Mon, 22 Apr 2019 18:30:00 -0700 Subject: [PATCH 2/9] split beforePass and pass --- lighthouse-core/gather/driver.js | 12 +-- lighthouse-core/gather/gather-runner.js | 93 ++++++++++++------- .../test/gather/gather-runner-test.js | 24 ++--- 3 files changed, 76 insertions(+), 53 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 38fe383209c8..bdbfb2b3f709 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -1476,15 +1476,15 @@ class Driver { } /** - * Emulate internet disconnection. + * Clear the network cache on disk and in memory. * @return {Promise} */ - cleanBrowserCaches() { + async cleanBrowserCaches() { // Wipe entire disk cache - return this.sendCommand('Network.clearBrowserCache') - // Toggle 'Disable Cache' to evict the memory cache - .then(_ => this.sendCommand('Network.setCacheDisabled', {cacheDisabled: true})) - .then(_ => this.sendCommand('Network.setCacheDisabled', {cacheDisabled: false})); + await this.sendCommand('Network.clearBrowserCache'); + // Toggle 'Disable Cache' to evict the memory cache + await this.sendCommand('Network.setCacheDisabled', {cacheDisabled: true}); + await this.sendCommand('Network.setCacheDisabled', {cacheDisabled: false}); } /** diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 1447e803492d..52a53b84fc48 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -89,12 +89,22 @@ class GatherRunner { * @return {Promise} */ static async loadPage(driver, passContext) { + const gatherers = passContext.passConfig.gatherers; + const status = { + msg: 'Loading page & waiting for onload', + id: `lh:gather:loadPage-${passContext.passConfig.passName}`, + args: [gatherers.map(g => g.instance.name).join(', ')], + }; + log.time(status); + const finalUrl = await driver.gotoURL(passContext.url, { waitForFCP: passContext.passConfig.recordTrace, waitForLoad: true, passContext, }); passContext.url = finalUrl; + + log.timeEnd(status); } /** @@ -176,15 +186,15 @@ class GatherRunner { } /** - * Calls beforePass() on gatherers before tracing - * has started and before navigation to the target page. + * Initialize network settings for the pass, e.g. throttling, blocked URLs, + * and manual request headers. * @param {LH.Gatherer.PassContext} passContext - * @param {Partial} gathererResults * @return {Promise} */ - static async beforePass(passContext, gathererResults) { - const bpStatus = {msg: `Running beforePass methods`, id: `lh:gather:beforePass`}; - log.time(bpStatus, 'verbose'); + static async setupPassNetwork(passContext) { + const passConfig = passContext.passConfig; + await passContext.driver.setThrottling(passContext.settings, passConfig); + const blockedUrls = (passContext.passConfig.blockedUrlPatterns || []) .concat(passContext.settings.blockedUrlPatterns || []); @@ -193,6 +203,18 @@ class GatherRunner { // neccessary at the beginning of the next pass. await passContext.driver.blockUrlPatterns(blockedUrls); await passContext.driver.setExtraHTTPHeaders(passContext.settings.extraHeaders); + } + + /** + * Calls beforePass() on gatherers before tracing + * has started and before navigation to the target page. + * @param {LH.Gatherer.PassContext} passContext + * @param {Partial} gathererResults + * @return {Promise} + */ + static async beforePass(passContext, gathererResults) { + const bpStatus = {msg: `Running beforePass methods`, id: `lh:gather:beforePass`}; + log.time(bpStatus, 'verbose'); for (const gathererDefn of passContext.passConfig.gatherers) { const gatherer = gathererDefn.instance; @@ -212,41 +234,34 @@ class GatherRunner { } /** - * Navigates to requested URL and then runs pass() on gatherers while trace - * (if requested) is still being recorded. + * Beging recording devtoolsLog and trace (if requested). * @param {LH.Gatherer.PassContext} passContext - * @param {Partial} gathererResults * @return {Promise} */ - static async pass(passContext, gathererResults) { - const driver = passContext.driver; - const config = passContext.passConfig; - const settings = passContext.settings; - const gatherers = config.gatherers; - - const recordTrace = config.recordTrace; - const isPerfRun = !settings.disableStorageReset && recordTrace && config.useThrottling; - - const status = { - msg: 'Loading page & waiting for onload', - id: `lh:gather:loadPage-${passContext.passConfig.passName}`, - args: [gatherers.map(g => g.instance.name).join(', ')], - }; - log.time(status); + static async beginRecording(passContext) { + const {driver, passConfig, settings} = passContext; - // Clear disk & memory cache if it's a perf run - if (isPerfRun) await driver.cleanBrowserCaches(); // Always record devtoolsLog await driver.beginDevtoolsLog(); - // Begin tracing if requested by config. - if (recordTrace) await driver.beginTrace(settings); - // Navigate. - await GatherRunner.loadPage(driver, passContext); - log.timeEnd(status); + if (passConfig.recordTrace) { + await driver.beginTrace(settings); + } + } + + /** + * Run pass() on gatherers. + * @param {LH.Gatherer.PassContext} passContext + * @param {Partial} gathererResults + * @return {Promise} + */ + static async pass(passContext, gathererResults) { + const config = passContext.passConfig; + const gatherers = config.gatherers; const pStatus = {msg: `Running pass methods`, id: `lh:gather:pass`}; log.time(pStatus, 'verbose'); + for (const gathererDefn of gatherers) { const gatherer = gathererDefn.instance; // Abuse the passContext to pass through gatherer options @@ -263,7 +278,7 @@ class GatherRunner { gathererResults[gatherer.name] = gathererResult; await artifactPromise.catch(() => {}); } - log.timeEnd(status); + log.timeEnd(pStatus); } @@ -522,13 +537,21 @@ class GatherRunner { * @return {Promise>} */ static async runPass(passContext) { - const passConfig = passContext.passConfig; - await passContext.driver.setThrottling(passContext.settings, passConfig); - /** @type {Partial} */ const gathererResults = {}; + const {driver, settings, passConfig} = passContext; + const isPerfPass = !settings.disableStorageReset && passConfig.recordTrace && passConfig.useThrottling; + // On about:blank. + await GatherRunner.setupPassNetwork(passContext); + if (isPerfPass) await driver.cleanBrowserCaches(); // Clear disk & memory cache if it's a perf run await GatherRunner.beforePass(passContext, gathererResults); + await GatherRunner.beginRecording(passContext); + + // Navigate. + await GatherRunner.loadPage(driver, passContext); + + // On test page. await GatherRunner.pass(passContext, gathererResults); const passData = await GatherRunner.afterPass(passContext, gathererResults); diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 36ab4f962d67..d6ca0224fefc 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -102,7 +102,9 @@ describe('GatherRunner', function() { const passContext = { requestedUrl: url1, settings: {}, - passConfig: {}, + passConfig: { + gatherers: [], + }, }; return GatherRunner.loadPage(driver, passContext).then(_ => { @@ -420,7 +422,7 @@ describe('GatherRunner', function() { receivedUrlPatterns = params.urls; }); - return GatherRunner.beforePass({ + return GatherRunner.setupPassNetwork({ driver, settings: { blockedUrlPatterns: ['http://*.evil.com', '.jpg', '.woff2'], @@ -441,7 +443,7 @@ describe('GatherRunner', function() { receivedUrlPatterns = params.urls; }); - return GatherRunner.beforePass({ + return GatherRunner.setupPassNetwork({ driver, settings: {}, passConfig: {gatherers: []}, @@ -459,7 +461,7 @@ describe('GatherRunner', function() { 'x-men': 'wolverine', }; - return GatherRunner.beforePass({ + return GatherRunner.setupPassNetwork({ driver, settings: { extraHeaders: headers, @@ -471,7 +473,7 @@ describe('GatherRunner', function() { )); }); - it('tells the driver to begin tracing', () => { + it('tells the driver to begin tracing', async () => { let calledTrace = false; const driver = { beginTrace() { @@ -494,9 +496,8 @@ describe('GatherRunner', function() { }; const settings = {}; - return GatherRunner.pass({driver, passConfig, settings}, {TestGatherer: []}).then(_ => { - assert.equal(calledTrace, true); - }); + await GatherRunner.beginRecording({driver, passConfig, settings}, {TestGatherer: []}); + assert.equal(calledTrace, true); }); it('tells the driver to end tracing', () => { @@ -524,7 +525,7 @@ describe('GatherRunner', function() { }); }); - it('tells the driver to begin devtoolsLog collection', () => { + it('tells the driver to begin devtoolsLog collection', async () => { let calledDevtoolsLogCollect = false; const driver = { beginDevtoolsLog() { @@ -543,9 +544,8 @@ describe('GatherRunner', function() { }; const settings = {}; - return GatherRunner.pass({driver, passConfig, settings}, {TestGatherer: []}).then(_ => { - assert.equal(calledDevtoolsLogCollect, true); - }); + await GatherRunner.beginRecording({driver, passConfig, settings}, {TestGatherer: []}); + assert.equal(calledDevtoolsLogCollect, true); }); it('tells the driver to end devtoolsLog collection', () => { From e661dfcc24239db7043dc971845f375f98eba381 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Tue, 23 Apr 2019 12:51:01 -0700 Subject: [PATCH 3/9] split afterPass --- lighthouse-core/gather/gather-runner.js | 175 +++++++++++------- .../test/gather/gather-runner-test.js | 4 +- 2 files changed, 106 insertions(+), 73 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 52a53b84fc48..026a05fc37a5 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -205,6 +205,54 @@ class GatherRunner { await passContext.driver.setExtraHTTPHeaders(passContext.settings.extraHeaders); } + /** + * Beging recording devtoolsLog and trace (if requested). + * @param {LH.Gatherer.PassContext} passContext + * @return {Promise} + */ + static async beginRecording(passContext) { + const {driver, passConfig, settings} = passContext; + + // Always record devtoolsLog + await driver.beginDevtoolsLog(); + + if (passConfig.recordTrace) { + await driver.beginTrace(settings); + } + } + + /** + * End recording devtoolsLog and trace (if requested). + * @param {LH.Gatherer.PassContext} passContext + * @return {Promise} + */ + static async endRecording(passContext) { + const {driver, passConfig} = passContext; + + let trace; + if (passConfig.recordTrace) { + const status = {msg: 'Retrieving trace', id: `lh:gather:getTrace`}; + log.time(status); + trace = await driver.endTrace(); + log.timeEnd(status); + } + + const status = { + msg: 'Retrieving devtoolsLog & network records', + id: `lh:gather:getDevtoolsLog`, + }; + log.time(status); + const devtoolsLog = driver.endDevtoolsLog(); + const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog); + log.timeEnd(status); + + return { + networkRecords, + devtoolsLog, + trace, + }; + } + /** * Calls beforePass() on gatherers before tracing * has started and before navigation to the target page. @@ -233,22 +281,6 @@ class GatherRunner { log.timeEnd(bpStatus); } - /** - * Beging recording devtoolsLog and trace (if requested). - * @param {LH.Gatherer.PassContext} passContext - * @return {Promise} - */ - static async beginRecording(passContext) { - const {driver, passConfig, settings} = passContext; - - // Always record devtoolsLog - await driver.beginDevtoolsLog(); - - if (passConfig.recordTrace) { - await driver.beginTrace(settings); - } - } - /** * Run pass() on gatherers. * @param {LH.Gatherer.PassContext} passContext @@ -287,51 +319,15 @@ class GatherRunner { * afterPass() on gatherers with trace data passed in. Promise resolves with * object containing trace and network data. * @param {LH.Gatherer.PassContext} passContext + * @param {LH.Gatherer.LoadData} loadData * @param {Partial} gathererResults - * @return {Promise} + * @return {Promise} */ - static async afterPass(passContext, gathererResults) { - const driver = passContext.driver; + static async afterPass(passContext, loadData, gathererResults) { const config = passContext.passConfig; const gatherers = config.gatherers; - let trace; - if (config.recordTrace) { - const status = {msg: 'Retrieving trace', id: `lh:gather:getTrace`}; - log.time(status); - trace = await driver.endTrace(); - log.timeEnd(status); - } - - const status = { - msg: 'Retrieving devtoolsLog & network records', - id: `lh:gather:getDevtoolsLog`, - }; - log.time(status); - const devtoolsLog = driver.endDevtoolsLog(); - const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog); - log.timeEnd(status); - - let pageLoadError = GatherRunner.getPageLoadError(passContext.url, networkRecords); - // If the driver was offline, a page load error is expected, so do not save it. - if (!driver.online) pageLoadError = undefined; - - if (pageLoadError) { - log.error('GatherRunner', pageLoadError.message, passContext.url); - passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage); - } - - // Expose devtoolsLog, networkRecords, and trace (if present) to gatherers - /** @type {LH.Gatherer.LoadData} */ - const passData = { - networkRecords, - devtoolsLog, - trace, - }; - const apStatus = {msg: `Running afterPass methods`, id: `lh:gather:afterPass`}; - // Disable throttling so the afterPass analysis isn't throttled - await driver.setThrottling(passContext.settings, {useThrottling: false}); log.time(apStatus, 'verbose'); for (const gathererDefn of gatherers) { @@ -344,12 +340,8 @@ class GatherRunner { // Add gatherer options to the passContext. passContext.options = gathererDefn.options || {}; - - // If there was a pageLoadError, fail every afterPass with it rather than bail completely. - const artifactPromise = pageLoadError ? - Promise.reject(pageLoadError) : - // Wrap gatherer response in promise, whether rejected or not. - Promise.resolve().then(_ => gatherer.afterPass(passContext, passData)); + const artifactPromise = Promise.resolve() + .then(_ => gatherer.afterPass(passContext, loadData)); const gathererResult = gathererResults[gatherer.name] || []; gathererResult.push(artifactPromise); @@ -358,9 +350,24 @@ class GatherRunner { log.timeEnd(status); } log.timeEnd(apStatus); + } - // Resolve on tracing data using passName from config. - return passData; + /** + * @param {LH.Gatherer.PassContext} passContext + * @param {LHError} pageLoadError + * @param {Partial} gathererResults + * @return {Promise} + */ + static async fillWithPageLoadErrors(passContext, pageLoadError, gathererResults) { + // Fail every gatherer's afterPass with the pageLoadError. + for (const gathererDefn of passContext.passConfig.gatherers) { + const gatherer = gathererDefn.instance; + const artifactPromise = Promise.reject(pageLoadError); + const gathererResult = gathererResults[gatherer.name] || []; + gathererResult.push(artifactPromise); + gathererResults[gatherer.name] = gathererResult; + await artifactPromise.catch(() => {}); + } } /** @@ -531,6 +538,16 @@ class GatherRunner { } } + /** + * Returns whether this pass should be considered to be measuring performance. + * @param {LH.Gatherer.PassContext} passContext + * @return {boolean} + */ + static isPerfPass(passContext) { + const {settings, passConfig} = passContext; + return !settings.disableStorageReset && passConfig.recordTrace && passConfig.useThrottling; + } + /** * Starting from about:blank, load the page and run gatherers for this pass. * @param {LH.Gatherer.PassContext} passContext @@ -539,27 +556,43 @@ class GatherRunner { static async runPass(passContext) { /** @type {Partial} */ const gathererResults = {}; - const {driver, settings, passConfig} = passContext; - const isPerfPass = !settings.disableStorageReset && passConfig.recordTrace && passConfig.useThrottling; + const {driver, passConfig} = passContext; // On about:blank. await GatherRunner.setupPassNetwork(passContext); + const isPerfPass = GatherRunner.isPerfPass(passContext); if (isPerfPass) await driver.cleanBrowserCaches(); // Clear disk & memory cache if it's a perf run await GatherRunner.beforePass(passContext, gathererResults); - await GatherRunner.beginRecording(passContext); // Navigate. + await GatherRunner.beginRecording(passContext); await GatherRunner.loadPage(driver, passContext); - - // On test page. await GatherRunner.pass(passContext, gathererResults); - const passData = await GatherRunner.afterPass(passContext, gathererResults); + const loadData = await GatherRunner.endRecording(passContext); + + // Disable throttling so the afterPass analysis isn't throttled + await driver.setThrottling(passContext.settings, {useThrottling: false}); // Save devtoolsLog and trace (if requested by passConfig). const baseArtifacts = passContext.baseArtifacts; - baseArtifacts.devtoolsLogs[passConfig.passName] = passData.devtoolsLog; - if (passData.trace) baseArtifacts.traces[passConfig.passName] = passData.trace; + baseArtifacts.devtoolsLogs[passConfig.passName] = loadData.devtoolsLog; + if (loadData.trace) baseArtifacts.traces[passConfig.passName] = loadData.trace; + + // Check for any load errors (but if the driver was offline, ignore the expected error). + let pageLoadError = GatherRunner.getPageLoadError(passContext.url, loadData.networkRecords); + if (!driver.online) pageLoadError = undefined; + + // If no error, finish the afterPass and return. + if (!pageLoadError) { + await GatherRunner.afterPass(passContext, loadData, gathererResults); + // TODO(bckenny): actually collect here + return gathererResults; + } + // Move out of here? + log.error('GatherRunner', pageLoadError.message, passContext.url); + passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage); + await GatherRunner.fillWithPageLoadErrors(passContext, pageLoadError, gathererResults); // TODO(bckenny): actually collect here return gathererResults; } diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index d6ca0224fefc..38b30e3cdba2 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -519,7 +519,7 @@ describe('GatherRunner', function() { ], }; - return GatherRunner.afterPass({url, driver, passConfig}, {TestGatherer: []}).then(passData => { + return GatherRunner.endRecording({url, driver, passConfig}).then(passData => { assert.equal(calledTrace, true); assert.equal(passData.trace, fakeTraceData); }); @@ -568,7 +568,7 @@ describe('GatherRunner', function() { ], }; - return GatherRunner.afterPass({url, driver, passConfig}, {TestGatherer: []}).then(passData => { + return GatherRunner.endRecording({url, driver, passConfig}).then(passData => { assert.equal(calledDevtoolsLogCollect, true); assert.strictEqual(passData.devtoolsLog[0], fakeDevtoolsMessage); }); From a518aa5418cb34f0755bf38c15c7765487ff7e58 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Thu, 9 May 2019 18:09:42 -0700 Subject: [PATCH 4/9] more --- lighthouse-core/gather/gather-runner.js | 94 +++++++++---------------- 1 file changed, 32 insertions(+), 62 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 026a05fc37a5..628a33066a9a 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -13,7 +13,7 @@ const URL = require('../lib/url-shim.js'); const NetworkRecorder = require('../lib/network-recorder.js'); const constants = require('../config/constants.js'); -const Driver = require('../gather/driver.js'); // eslint-disable-line no-unused-vars +/** @typedef {import('../gather/driver.js')} Driver */ /** @typedef {import('./gatherers/gatherer.js').PhaseResult} PhaseResult */ /** @@ -23,44 +23,9 @@ const Driver = require('../gather/driver.js'); // eslint-disable-line no-unused- * @typedef {Record>>} GathererResults */ /** @typedef {Array<[keyof GathererResults, GathererResults[keyof GathererResults]]>} GathererResultsEntries */ + /** * Class that drives browser to load the page and runs gatherer lifecycle hooks. - * Execution sequence when GatherRunner.run() is called: - * - * 1. Setup - * A. driver.connect() - * B. GatherRunner.setupDriver() - * i. assertNoSameOriginServiceWorkerClients - * ii. retrieve and save userAgent - * iii. beginEmulation - * iv. enableRuntimeEvents/enableAsyncStacks - * v. evaluateScriptOnLoad rescue native Promise from potential polyfill - * vi. register a performance observer - * vii. register dialog dismisser - * viii. clearDataForOrigin - * - * 2. For each pass in the config: - * A. GatherRunner.beforePass() - * i. navigate to about:blank - * ii. Enable network request blocking for specified patterns - * iii. all gatherers' beforePass() - * B. GatherRunner.pass() - * i. cleanBrowserCaches() (if it's a perf run) - * ii. beginDevtoolsLog() - * iii. beginTrace (if requested) - * iv. GatherRunner.loadPage() - * a. navigate to options.url (and wait for onload) - * v. all gatherers' pass() - * C. GatherRunner.afterPass() - * i. endTrace (if requested) & endDevtoolsLog & endThrottling - * ii. all gatherers' afterPass() - * - * 3. Teardown - * A. clearDataForOrigin - * B. GatherRunner.disposeDriver() - * C. collect all artifacts and return them - * i. collectArtifacts() from completed passes on each gatherer - * ii. add trace and devtoolsLog data */ class GatherRunner { /** @@ -462,6 +427,33 @@ class GatherRunner { return manifestParser(response.data, response.url, passContext.url); } + /** + * Populates the important base artifacts from a fully loaded test page. + * @param {LH.Gatherer.PassContext} passContext + */ + static async collectBaseArtifacts(passContext) { + const baseArtifacts = passContext.baseArtifacts; + + // Copy redirected URL to artifact in the first pass only. + baseArtifacts.URL.finalUrl = passContext.url; + + // Fetch the manifest, if it exists. Currently must be fetched before `start-url` gatherer. + baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext); + + // TODO(bckenny): move back to passContext + baseArtifacts.Stacks = await stacksGatherer(passContext.driver); + + const devtoolsLog = baseArtifacts.devtoolsLogs[passContext.passConfig.passName]; + const userAgentEntry = devtoolsLog.find(entry => + entry.method === 'Network.requestWillBeSent' && + !!entry.params.request.headers['User-Agent'] + ); + if (userAgentEntry) { + // @ts-ignore - guaranteed to exist by the find above + baseArtifacts.NetworkUserAgent = userAgentEntry.params.request.headers['User-Agent']; + } + } + /** * @param {Array} passes * @param {{driver: Driver, requestedUrl: string, settings: LH.Config.Settings}} options @@ -495,34 +487,11 @@ class GatherRunner { LighthouseRunWarnings: baseArtifacts.LighthouseRunWarnings, }; - if (!isFirstPass) { - // Already on blank page if driver was just set up. - await GatherRunner.loadBlank(driver, passConfig.blankPage); - } - const passResults = await GatherRunner.runPass(passContext); Object.assign(gathererResults, passResults); - // TODO(bckenny): move into collect base or whatever? if (isFirstPass) { - // Copy redirected URL to artifact in the first pass only. - baseArtifacts.URL.finalUrl = passContext.url; - - // Fetch the manifest, if it exists. Currently must be fetched before `start-url` gatherer. - baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext); - - baseArtifacts.Stacks = await stacksGatherer(driver); - - const devtoolsLog = baseArtifacts.devtoolsLogs[passConfig.passName]; - const userAgentEntry = devtoolsLog.find(entry => - entry.method === 'Network.requestWillBeSent' && - !!entry.params.request.headers['User-Agent'] - ); - if (userAgentEntry) { - // @ts-ignore - guaranteed to exist by the find above - baseArtifacts.NetworkUserAgent = userAgentEntry.params.request.headers['User-Agent']; - } - + await GatherRunner.collectBaseArtifacts(passContext); isFirstPass = false; } } @@ -558,7 +527,8 @@ class GatherRunner { const gathererResults = {}; const {driver, passConfig} = passContext; - // On about:blank. + // Setup on about:blank. + await GatherRunner.loadBlank(driver, passConfig.blankPage); await GatherRunner.setupPassNetwork(passContext); const isPerfPass = GatherRunner.isPerfPass(passContext); if (isPerfPass) await driver.cleanBrowserCaches(); // Clear disk & memory cache if it's a perf run From 0f6aa29ca7d157964f615debe4c130b41418a8ff Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Tue, 14 May 2019 12:09:35 -0700 Subject: [PATCH 5/9] revert some stuff --- lighthouse-core/gather/driver.js | 10 +++++----- lighthouse-core/gather/gather-runner.js | 7 ++++--- lighthouse-core/lib/stack-collector.js | 8 +++----- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index bdbfb2b3f709..e3128933ba91 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -1479,12 +1479,12 @@ class Driver { * Clear the network cache on disk and in memory. * @return {Promise} */ - async cleanBrowserCaches() { + cleanBrowserCaches() { // Wipe entire disk cache - await this.sendCommand('Network.clearBrowserCache'); - // Toggle 'Disable Cache' to evict the memory cache - await this.sendCommand('Network.setCacheDisabled', {cacheDisabled: true}); - await this.sendCommand('Network.setCacheDisabled', {cacheDisabled: false}); + return this.sendCommand('Network.clearBrowserCache') + // Toggle 'Disable Cache' to evict the memory cache + .then(_ => this.sendCommand('Network.setCacheDisabled', {cacheDisabled: true})) + .then(_ => this.sendCommand('Network.setCacheDisabled', {cacheDisabled: false})); } /** diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 628a33066a9a..a57669e2e2dd 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -429,6 +429,8 @@ class GatherRunner { /** * Populates the important base artifacts from a fully loaded test page. + * Currently must be run before `start-url` gatherer so that `WebAppManifest` + * is populated for it. * @param {LH.Gatherer.PassContext} passContext */ static async collectBaseArtifacts(passContext) { @@ -437,11 +439,10 @@ class GatherRunner { // Copy redirected URL to artifact in the first pass only. baseArtifacts.URL.finalUrl = passContext.url; - // Fetch the manifest, if it exists. Currently must be fetched before `start-url` gatherer. + // Fetch the manifest, if it exists. baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext); - // TODO(bckenny): move back to passContext - baseArtifacts.Stacks = await stacksGatherer(passContext.driver); + baseArtifacts.Stacks = await stacksGatherer(passContext); const devtoolsLog = baseArtifacts.devtoolsLogs[passContext.passConfig.passName]; const userAgentEntry = devtoolsLog.find(entry => diff --git a/lighthouse-core/lib/stack-collector.js b/lighthouse-core/lib/stack-collector.js index fe45a027889e..984d8b1dca60 100644 --- a/lighthouse-core/lib/stack-collector.js +++ b/lighthouse-core/lib/stack-collector.js @@ -17,8 +17,6 @@ const fs = require('fs'); const libDetectorSource = fs.readFileSync( require.resolve('js-library-detector/library/libraries.js'), 'utf8'); -/** @typedef {import('./../gather/driver.js')} Driver */ - /** @typedef {false | {version: string|null}} JSLibraryDetectorTestResult */ /** * @typedef JSLibraryDetectorTest @@ -68,17 +66,17 @@ async function detectLibraries() { } /** - * @param {Driver} driver + * @param {LH.Gatherer.PassContext} passContext * @return {Promise} */ -async function collectStacks(driver) { +async function collectStacks(passContext) { const expression = `(function () { ${libDetectorSource}; return (${detectLibraries.toString()}()); })()`; /** @type {JSLibrary[]} */ - const jsLibraries = await driver.evaluateAsync(expression); + const jsLibraries = await passContext.driver.evaluateAsync(expression); return jsLibraries.map(lib => ({ detector: /** @type {'js'} */ ('js'), From 3a4a686575ecf423a43a546f6504a69af6e94bc8 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Tue, 14 May 2019 13:41:32 -0700 Subject: [PATCH 6/9] more --- lighthouse-core/gather/gather-runner.js | 172 ++++++++++++------------ types/gatherer.d.ts | 3 +- 2 files changed, 89 insertions(+), 86 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index a57669e2e2dd..afd665569c42 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -93,14 +93,18 @@ class GatherRunner { } /** + * Reset browser state where needed and release the connection. * @param {Driver} driver + * @param {{requestedUrl: string, settings: LH.Config.Settings}} options * @return {Promise} */ - static async disposeDriver(driver) { + static async disposeDriver(driver, options) { const status = {msg: 'Disconnecting from browser...', id: 'lh:gather:disconnect'}; log.time(status); try { + const resetStorage = !options.settings.disableStorageReset; + if (resetStorage) await driver.clearDataForOrigin(options.requestedUrl); await driver.disconnect(); } catch (err) { // Ignore disconnecting error if browser was already closed. @@ -187,7 +191,8 @@ class GatherRunner { } /** - * End recording devtoolsLog and trace (if requested). + * End recording devtoolsLog and trace (if requested), returning an + * `LH.Gatherer.LoadData` with the recorded data. * @param {LH.Gatherer.PassContext} passContext * @return {Promise} */ @@ -219,8 +224,7 @@ class GatherRunner { } /** - * Calls beforePass() on gatherers before tracing - * has started and before navigation to the target page. + * Run beforePass() on gatherers. * @param {LH.Gatherer.PassContext} passContext * @param {Partial} gathererResults * @return {Promise} @@ -280,9 +284,7 @@ class GatherRunner { } /** - * Ends tracing and collects trace data (if requested for this pass), and runs - * afterPass() on gatherers with trace data passed in. Promise resolves with - * object containing trace and network data. + * Run afterPass() on gatherers. * @param {LH.Gatherer.PassContext} passContext * @param {LH.Gatherer.LoadData} loadData * @param {Partial} gathererResults @@ -318,21 +320,22 @@ class GatherRunner { } /** + * Generate a set of artfiacts for the given pass as if all the gatherers + * failed with the given pageLoadError. * @param {LH.Gatherer.PassContext} passContext * @param {LHError} pageLoadError - * @param {Partial} gathererResults - * @return {Promise} + * @return {Partial} */ - static async fillWithPageLoadErrors(passContext, pageLoadError, gathererResults) { - // Fail every gatherer's afterPass with the pageLoadError. + static generatePageLoadErrorArtifacts(passContext, pageLoadError) { + /** @type {Partial>} */ + const errorArtifacts = {}; for (const gathererDefn of passContext.passConfig.gatherers) { const gatherer = gathererDefn.instance; - const artifactPromise = Promise.reject(pageLoadError); - const gathererResult = gathererResults[gatherer.name] || []; - gathererResult.push(artifactPromise); - gathererResults[gatherer.name] = gathererResult; - await artifactPromise.catch(() => {}); + errorArtifacts[gatherer.name] = pageLoadError; } + + // @ts-ignore - TODO(bckenny): figure out how to usefully type errored artifacts. + return errorArtifacts; } /** @@ -341,17 +344,14 @@ class GatherRunner { * gatherer. If an error was rejected from a gatherer phase, * uses that error object as the artifact instead. * @param {Partial} gathererResults - * @param {LH.BaseArtifacts} baseArtifacts - * @return {Promise} + * @return {Promise>} */ - static async collectArtifacts(gathererResults, baseArtifacts) { + static async collectArtifacts(gathererResults) { /** @type {Partial} */ const gathererArtifacts = {}; const resultsEntries = /** @type {GathererResultsEntries} */ (Object.entries(gathererResults)); for (const [gathererName, phaseResultsPromises] of resultsEntries) { - if (gathererArtifacts[gathererName] !== undefined) continue; - try { const phaseResults = await Promise.all(phaseResultsPromises); // Take last defined pass result as artifact. @@ -369,17 +369,12 @@ class GatherRunner { } } - // Take only unique LighthouseRunWarnings. - baseArtifacts.LighthouseRunWarnings = Array.from(new Set(baseArtifacts.LighthouseRunWarnings)); - - // Take the timing entries we've gathered so far. - baseArtifacts.Timing = log.getTimeEntries(); - - // TODO(bckenny): correct Partial at this point to drop cast. - return /** @type {LH.Artifacts} */ ({...baseArtifacts, ...gathererArtifacts}); + return gathererArtifacts; } /** + * Return an initialized but mostly empty set of base artifacts, to be + * populated as the run continues. * @param {{driver: Driver, requestedUrl: string, settings: LH.Config.Settings}} options * @return {Promise} */ @@ -409,34 +404,16 @@ 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 getWebAppManifest(passContext) { - const response = await passContext.driver.getAppManifest(); - if (!response) return null; - return manifestParser(response.data, response.url, passContext.url); - } - /** * Populates the important base artifacts from a fully loaded test page. * Currently must be run before `start-url` gatherer so that `WebAppManifest` - * is populated for it. + * will be available to it. * @param {LH.Gatherer.PassContext} passContext */ - static async collectBaseArtifacts(passContext) { + static async populateBaseArtifacts(passContext) { const baseArtifacts = passContext.baseArtifacts; - // Copy redirected URL to artifact in the first pass only. + // Copy redirected URL to artifact. baseArtifacts.URL.finalUrl = passContext.url; // Fetch the manifest, if it exists. @@ -444,6 +421,7 @@ class GatherRunner { baseArtifacts.Stacks = await stacksGatherer(passContext); + // Find the NetworkUserAgent actually used in the devtoolsLogs. const devtoolsLog = baseArtifacts.devtoolsLogs[passContext.passConfig.passName]; const userAgentEntry = devtoolsLog.find(entry => entry.method === 'Network.requestWillBeSent' && @@ -456,54 +434,83 @@ class GatherRunner { } /** - * @param {Array} passes + * Finalize baseArtifacts after gathering is fully complete. + * @param {LH.BaseArtifacts} baseArtifacts + */ + static finalizeBaseArtifacts(baseArtifacts) { + // Take only unique LighthouseRunWarnings. + baseArtifacts.LighthouseRunWarnings = Array.from(new Set(baseArtifacts.LighthouseRunWarnings)); + + // Take the timing entries we've gathered so far. + baseArtifacts.Timing = log.getTimeEntries(); + } + + /** + * 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 getWebAppManifest(passContext) { + const response = await passContext.driver.getAppManifest(); + if (!response) return null; + return manifestParser(response.data, response.url, passContext.url); + } + + /** + * @param {Array} passConfigs * @param {{driver: Driver, requestedUrl: string, settings: LH.Config.Settings}} options * @return {Promise} */ - static async run(passes, options) { + static async run(passConfigs, options) { const driver = options.driver; - /** @type {Partial} */ - const gathererResults = {}; + /** @type {Partial} */ + const artifacts = {}; try { await driver.connect(); - const baseArtifacts = await GatherRunner.getBaseArtifacts(options); // In the devtools/extension case, we can't still be on the site while trying to clear state // So we first navigate to about:blank, then apply our emulation & setup await GatherRunner.loadBlank(driver); + + const baseArtifacts = await GatherRunner.getBaseArtifacts(options); baseArtifacts.BenchmarkIndex = await options.driver.getBenchmarkIndex(); + await GatherRunner.setupDriver(driver, options); let isFirstPass = true; - for (const passConfig of passes) { + for (const passConfig of passConfigs) { + /** @type {LH.Gatherer.PassContext} */ const passContext = { driver, - // If the main document redirects, we'll update this to keep track url: options.requestedUrl, settings: options.settings, passConfig, baseArtifacts, - // *pass() functions and gatherers can push to this warnings array. LighthouseRunWarnings: baseArtifacts.LighthouseRunWarnings, }; - - const passResults = await GatherRunner.runPass(passContext); - Object.assign(gathererResults, passResults); + const passArtifacts = await GatherRunner.runPass(passContext); + Object.assign(artifacts, passArtifacts); if (isFirstPass) { - await GatherRunner.collectBaseArtifacts(passContext); + await GatherRunner.populateBaseArtifacts(passContext); isFirstPass = false; } } - const resetStorage = !options.settings.disableStorageReset; - if (resetStorage) await driver.clearDataForOrigin(options.requestedUrl); - await GatherRunner.disposeDriver(driver); - return GatherRunner.collectArtifacts(gathererResults, baseArtifacts); + await GatherRunner.disposeDriver(driver, options); + GatherRunner.finalizeBaseArtifacts(baseArtifacts); + return /** @type {LH.Artifacts} */ ({...baseArtifacts, ...artifacts}); // Cast to drop Partial<>. } catch (err) { // cleanup on error - GatherRunner.disposeDriver(driver); + GatherRunner.disposeDriver(driver, options); throw err; } } @@ -521,21 +528,21 @@ class GatherRunner { /** * Starting from about:blank, load the page and run gatherers for this pass. * @param {LH.Gatherer.PassContext} passContext - * @return {Promise>} + * @return {Promise>} */ static async runPass(passContext) { /** @type {Partial} */ const gathererResults = {}; const {driver, passConfig} = passContext; - // Setup on about:blank. + // Go to about:blank, set up, and run `beforePass()` on gatherers. await GatherRunner.loadBlank(driver, passConfig.blankPage); await GatherRunner.setupPassNetwork(passContext); const isPerfPass = GatherRunner.isPerfPass(passContext); if (isPerfPass) await driver.cleanBrowserCaches(); // Clear disk & memory cache if it's a perf run await GatherRunner.beforePass(passContext, gathererResults); - // Navigate. + // Navigate, start recording, and run `pass()` on gatherers. await GatherRunner.beginRecording(passContext); await GatherRunner.loadPage(driver, passContext); await GatherRunner.pass(passContext, gathererResults); @@ -544,28 +551,23 @@ class GatherRunner { // Disable throttling so the afterPass analysis isn't throttled await driver.setThrottling(passContext.settings, {useThrottling: false}); - // Save devtoolsLog and trace (if requested by passConfig). + // Save devtoolsLog and trace. const baseArtifacts = passContext.baseArtifacts; baseArtifacts.devtoolsLogs[passConfig.passName] = loadData.devtoolsLog; if (loadData.trace) baseArtifacts.traces[passConfig.passName] = loadData.trace; - // Check for any load errors (but if the driver was offline, ignore the expected error). + // If there were any load errors, treat all gatherers as if they errored. let pageLoadError = GatherRunner.getPageLoadError(passContext.url, loadData.networkRecords); - if (!driver.online) pageLoadError = undefined; - - // If no error, finish the afterPass and return. - if (!pageLoadError) { - await GatherRunner.afterPass(passContext, loadData, gathererResults); - // TODO(bckenny): actually collect here - return gathererResults; + if (!driver.online) pageLoadError = undefined; // If the driver was offline, ignore the expected load error. + if (pageLoadError) { + log.error('GatherRunner', pageLoadError.message, passContext.url); + passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage); + return GatherRunner.generatePageLoadErrorArtifacts(passContext, pageLoadError); } - // Move out of here? - log.error('GatherRunner', pageLoadError.message, passContext.url); - passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage); - await GatherRunner.fillWithPageLoadErrors(passContext, pageLoadError, gathererResults); - // TODO(bckenny): actually collect here - return gathererResults; + // If no error, run `afterPass()` on gatherers and return collected artifacts. + await GatherRunner.afterPass(passContext, loadData, gathererResults); + return GatherRunner.collectArtifacts(gathererResults); } } diff --git a/types/gatherer.d.ts b/types/gatherer.d.ts index 78e6c711ce77..21237d270712 100644 --- a/types/gatherer.d.ts +++ b/types/gatherer.d.ts @@ -12,13 +12,14 @@ import Driver = require('../lighthouse-core/gather/driver'); declare global { module LH.Gatherer { export interface PassContext { + /** The url of the currently loaded page. If the main document redirects, this will be updated to keep track. */ url: string; driver: Driver; disableJavaScript?: boolean; passConfig: Config.Pass settings: Config.Settings; options?: object; - /** Push to this array to add top-level warnings to the LHR. */ + /** Gatherers can push to this array to add top-level warnings to the LHR. */ LighthouseRunWarnings: Array; baseArtifacts: BaseArtifacts; } From 255ea57ab48a1a3d6a5bff17739bb43bc5162fbf Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Tue, 14 May 2019 15:39:29 -0700 Subject: [PATCH 7/9] fix tests --- .../test/gather/gather-runner-test.js | 48 ++++++++++++++----- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 38b30e3cdba2..169cb227c438 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -354,7 +354,7 @@ describe('GatherRunner', function() { }); }); - it('clears the disk & memory cache on a perf run', () => { + it('clears the disk & memory cache on a perf run', async () => { const asyncFunc = () => Promise.resolve(); const tests = { calledCleanBrowserCaches: false, @@ -368,8 +368,15 @@ describe('GatherRunner', function() { beginTrace: asyncFunc, gotoURL: asyncFunc, cleanBrowserCaches: createCheck('calledCleanBrowserCaches'), + setThrottling: asyncFunc, + blockUrlPatterns: asyncFunc, + setExtraHTTPHeaders: asyncFunc, + endTrace: asyncFunc, + endDevtoolsLog: () => [], + getBrowserVersion: async () => ({userAgent: ''}), }; const passConfig = { + passName: 'default', recordTrace: true, useThrottling: true, gatherers: [], @@ -377,9 +384,16 @@ describe('GatherRunner', function() { const settings = { disableStorageReset: false, }; - return GatherRunner.pass({driver, passConfig, settings}, {TestGatherer: []}).then(_ => { - assert.equal(tests.calledCleanBrowserCaches, true); - }); + const requestedUrl = 'https://example.com'; + const passContext = { + driver, + passConfig, + settings, + baseArtifacts: await GatherRunner.getBaseArtifacts({driver, settings, requestedUrl}), + }; + + await GatherRunner.runPass(passContext, {TestGatherer: []}); + assert.equal(tests.calledCleanBrowserCaches, true); }); it('does not clear origin storage with flag --disable-storage-reset', () => { @@ -937,20 +951,32 @@ describe('GatherRunner', function() { }); }); - it('produces a LighthouseRunWarnings artifact from array of warnings', () => { - const LighthouseRunWarnings = [ + it('produces a deduped LighthouseRunWarnings artifact from array of warnings', async () => { + const runWarnings = [ 'warning0', 'warning1', 'warning2', ]; - const baseArtifacts = { - LighthouseRunWarnings, - }; + class WarningGatherer extends Gatherer { + afterPass(passContext) { + passContext.LighthouseRunWarnings.push(...runWarnings, ...runWarnings); + assert.strictEqual(passContext.LighthouseRunWarnings.length, runWarnings.length * 2); - return GatherRunner.collectArtifacts({}, baseArtifacts).then(artifacts => { - assert.deepStrictEqual(artifacts.LighthouseRunWarnings, LighthouseRunWarnings); + return ''; + } + } + + const passes = [{ + gatherers: [{instance: new WarningGatherer()}], + }]; + const artifacts = await GatherRunner.run(passes, { + driver: fakeDriver, + requestedUrl: 'https://example.com', + settings: {}, + config: new Config({}), }); + assert.deepStrictEqual(artifacts.LighthouseRunWarnings, runWarnings); }); it('supports sync and async throwing of errors from gatherers', () => { From 2a588d1c66d330b8e246c9a241cf3affced83bd1 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Tue, 14 May 2019 16:15:03 -0700 Subject: [PATCH 8/9] more --- lighthouse-core/gather/gather-runner.js | 45 ++++++++++++++----- .../test/gather/gather-runner-test.js | 18 ++++---- 2 files changed, 42 insertions(+), 21 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index afd665569c42..fb5c0842207b 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -103,8 +103,10 @@ class GatherRunner { log.time(status); try { + // If storage was cleared for the run, clear at the end so Lighthouse specifics aren't cached. const resetStorage = !options.settings.disableStorageReset; if (resetStorage) await driver.clearDataForOrigin(options.requestedUrl); + await driver.disconnect(); } catch (err) { // Ignore disconnecting error if browser was already closed. @@ -122,7 +124,7 @@ class GatherRunner { * @param {Array} networkRecords * @return {LHError|undefined} */ - static getPageLoadError(url, networkRecords) { + static getNetworkError(url, networkRecords) { const mainRecord = networkRecords.find(record => { // record.url is actual request url, so needs to be compared without any URL fragment. return URL.equalWithExcludedFragments(record.url, url); @@ -154,6 +156,21 @@ class GatherRunner { } } + /** + * Returns an error if the page load should be considered failed, e.g. from a + * main document request failure, a security issue, etc. + * @param {LH.Gatherer.PassContext} passContext + * @param {LH.Gatherer.LoadData} loadData + */ + static getPageLoadError(passContext, loadData) { + const networkError = GatherRunner.getNetworkError(passContext.url, loadData.networkRecords); + + // If the driver was offline, the load will fail without offline support. Ignore this case. + if (!passContext.driver.online) return; + + return networkError; + } + /** * Initialize network settings for the pass, e.g. throttling, blocked URLs, * and manual request headers. @@ -324,7 +341,7 @@ class GatherRunner { * failed with the given pageLoadError. * @param {LH.Gatherer.PassContext} passContext * @param {LHError} pageLoadError - * @return {Partial} + * @return {{pageLoadError: LHError, artifacts: Partial}} */ static generatePageLoadErrorArtifacts(passContext, pageLoadError) { /** @type {Partial>} */ @@ -334,8 +351,11 @@ class GatherRunner { errorArtifacts[gatherer.name] = pageLoadError; } - // @ts-ignore - TODO(bckenny): figure out how to usefully type errored artifacts. - return errorArtifacts; + return { + pageLoadError, + // @ts-ignore - TODO(bckenny): figure out how to usefully type errored artifacts. + artifacts: errorArtifacts, + }; } /** @@ -344,7 +364,7 @@ class GatherRunner { * gatherer. If an error was rejected from a gatherer phase, * uses that error object as the artifact instead. * @param {Partial} gathererResults - * @return {Promise>} + * @return {Promise<{artifacts: Partial}>} */ static async collectArtifacts(gathererResults) { /** @type {Partial} */ @@ -369,7 +389,9 @@ class GatherRunner { } } - return gathererArtifacts; + return { + artifacts: gathererArtifacts, + }; } /** @@ -496,8 +518,8 @@ class GatherRunner { baseArtifacts, LighthouseRunWarnings: baseArtifacts.LighthouseRunWarnings, }; - const passArtifacts = await GatherRunner.runPass(passContext); - Object.assign(artifacts, passArtifacts); + const passResults = await GatherRunner.runPass(passContext); + Object.assign(artifacts, passResults.artifacts); if (isFirstPass) { await GatherRunner.populateBaseArtifacts(passContext); @@ -528,7 +550,7 @@ class GatherRunner { /** * Starting from about:blank, load the page and run gatherers for this pass. * @param {LH.Gatherer.PassContext} passContext - * @return {Promise>} + * @return {Promise<{artifacts: Partial, pageLoadError?: LHError}>} */ static async runPass(passContext) { /** @type {Partial} */ @@ -557,10 +579,9 @@ class GatherRunner { if (loadData.trace) baseArtifacts.traces[passConfig.passName] = loadData.trace; // If there were any load errors, treat all gatherers as if they errored. - let pageLoadError = GatherRunner.getPageLoadError(passContext.url, loadData.networkRecords); - if (!driver.online) pageLoadError = undefined; // If the driver was offline, ignore the expected load error. + const pageLoadError = GatherRunner.getPageLoadError(passContext, loadData); if (pageLoadError) { - log.error('GatherRunner', pageLoadError.message, passContext.url); + log.error('GatherRunner', pageLoadError.friendlyMessage, passContext.url); passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage); return GatherRunner.generatePageLoadErrorArtifacts(passContext, pageLoadError); } diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 169cb227c438..c95cf735caa8 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -657,19 +657,19 @@ describe('GatherRunner', function() { }); }); - describe('#getPageLoadError', () => { + describe('#getNetworkError', () => { it('passes when the page is loaded', () => { const url = 'http://the-page.com'; const mainRecord = new NetworkRequest(); mainRecord.url = url; - assert.ok(!GatherRunner.getPageLoadError(url, [mainRecord])); + assert.ok(!GatherRunner.getNetworkError(url, [mainRecord])); }); it('passes when the page is loaded, ignoring any fragment', () => { const url = 'http://example.com/#/page/list'; const mainRecord = new NetworkRequest(); mainRecord.url = 'http://example.com'; - assert.ok(!GatherRunner.getPageLoadError(url, [mainRecord])); + assert.ok(!GatherRunner.getNetworkError(url, [mainRecord])); }); it('fails when page fails to load', () => { @@ -678,7 +678,7 @@ describe('GatherRunner', function() { mainRecord.url = url; mainRecord.failed = true; mainRecord.localizedFailDescription = 'foobar'; - const error = GatherRunner.getPageLoadError(url, [mainRecord]); + const error = GatherRunner.getNetworkError(url, [mainRecord]); assert.equal(error.message, 'FAILED_DOCUMENT_REQUEST'); assert.equal(error.code, 'FAILED_DOCUMENT_REQUEST'); expect(error.friendlyMessage) @@ -688,7 +688,7 @@ describe('GatherRunner', function() { it('fails when page times out', () => { const url = 'http://the-page.com'; const records = []; - const error = GatherRunner.getPageLoadError(url, records); + const error = GatherRunner.getNetworkError(url, records); assert.equal(error.message, 'NO_DOCUMENT_REQUEST'); assert.equal(error.code, 'NO_DOCUMENT_REQUEST'); expect(error.friendlyMessage).toBeDisplayString(/^Lighthouse was unable to reliably load/); @@ -699,7 +699,7 @@ describe('GatherRunner', function() { const mainRecord = new NetworkRequest(); mainRecord.url = url; mainRecord.statusCode = 404; - const error = GatherRunner.getPageLoadError(url, [mainRecord]); + const error = GatherRunner.getNetworkError(url, [mainRecord]); assert.equal(error.message, 'ERRORED_DOCUMENT_REQUEST'); assert.equal(error.code, 'ERRORED_DOCUMENT_REQUEST'); expect(error.friendlyMessage) @@ -711,7 +711,7 @@ describe('GatherRunner', function() { const mainRecord = new NetworkRequest(); mainRecord.url = url; mainRecord.statusCode = 500; - const error = GatherRunner.getPageLoadError(url, [mainRecord]); + const error = GatherRunner.getNetworkError(url, [mainRecord]); assert.equal(error.message, 'ERRORED_DOCUMENT_REQUEST'); assert.equal(error.code, 'ERRORED_DOCUMENT_REQUEST'); expect(error.friendlyMessage) @@ -724,7 +724,7 @@ describe('GatherRunner', function() { mainRecord.url = url; mainRecord.failed = true; mainRecord.localizedFailDescription = 'net::ERR_NAME_NOT_RESOLVED'; - const error = GatherRunner.getPageLoadError(url, [mainRecord]); + const error = GatherRunner.getNetworkError(url, [mainRecord]); assert.equal(error.message, 'DNS_FAILURE'); assert.equal(error.code, 'DNS_FAILURE'); expect(error.friendlyMessage).toBeDisplayString(/^DNS servers could not resolve/); @@ -943,7 +943,7 @@ describe('GatherRunner', function() { ], }; - return GatherRunner.collectArtifacts(gathererResults, {}).then(artifacts => { + return GatherRunner.collectArtifacts(gathererResults, {}).then(({artifacts}) => { assert.strictEqual(artifacts.AfterGatherer, 97); assert.strictEqual(artifacts.PassGatherer, 284); assert.strictEqual(artifacts.SingleErrorGatherer, recoverableError); From 99f1e1549f8f24a7fd6f7d12e3101133a5db0121 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Mon, 20 May 2019 17:57:38 -0700 Subject: [PATCH 9/9] back by popular demand --- lighthouse-core/gather/driver.js | 15 ++++++++++----- lighthouse-core/gather/gather-runner.js | 14 ++++++++++++-- lighthouse-core/test/gather/gather-runner-test.js | 2 +- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index e3128933ba91..f238a1227e3e 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -1479,12 +1479,17 @@ class Driver { * Clear the network cache on disk and in memory. * @return {Promise} */ - cleanBrowserCaches() { + async cleanBrowserCaches() { + const status = {msg: 'Cleaning browser cache', id: 'lh:driver:cleanBrowserCaches'}; + log.time(status); + // Wipe entire disk cache - return this.sendCommand('Network.clearBrowserCache') - // Toggle 'Disable Cache' to evict the memory cache - .then(_ => this.sendCommand('Network.setCacheDisabled', {cacheDisabled: true})) - .then(_ => this.sendCommand('Network.setCacheDisabled', {cacheDisabled: false})); + await this.sendCommand('Network.clearBrowserCache'); + // Toggle 'Disable Cache' to evict the memory cache + await this.sendCommand('Network.setCacheDisabled', {cacheDisabled: true}); + await this.sendCommand('Network.setCacheDisabled', {cacheDisabled: false}); + + log.timeEnd(status); } /** diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index fb5c0842207b..9c637cfcb642 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -178,6 +178,9 @@ class GatherRunner { * @return {Promise} */ static async setupPassNetwork(passContext) { + const status = {msg: 'Setting up network for the pass trace', id: `lh:gather:setupPassNetwork`}; + log.time(status); + const passConfig = passContext.passConfig; await passContext.driver.setThrottling(passContext.settings, passConfig); @@ -189,6 +192,8 @@ class GatherRunner { // neccessary at the beginning of the next pass. await passContext.driver.blockUrlPatterns(blockedUrls); await passContext.driver.setExtraHTTPHeaders(passContext.settings.extraHeaders); + + log.timeEnd(status); } /** @@ -197,6 +202,9 @@ class GatherRunner { * @return {Promise} */ static async beginRecording(passContext) { + const status = {msg: 'Beginning devtoolsLog and trace', id: 'lh:gather:beginRecording'}; + log.time(status); + const {driver, passConfig, settings} = passContext; // Always record devtoolsLog @@ -205,6 +213,8 @@ class GatherRunner { if (passConfig.recordTrace) { await driver.beginTrace(settings); } + + log.timeEnd(status); } /** @@ -400,7 +410,7 @@ class GatherRunner { * @param {{driver: Driver, requestedUrl: string, settings: LH.Config.Settings}} options * @return {Promise} */ - static async getBaseArtifacts(options) { + static async initializeBaseArtifacts(options) { const hostUserAgent = (await options.driver.getBrowserVersion()).userAgent; const {emulatedFormFactor} = options.settings; @@ -502,7 +512,7 @@ class GatherRunner { // So we first navigate to about:blank, then apply our emulation & setup await GatherRunner.loadBlank(driver); - const baseArtifacts = await GatherRunner.getBaseArtifacts(options); + const baseArtifacts = await GatherRunner.initializeBaseArtifacts(options); baseArtifacts.BenchmarkIndex = await options.driver.getBenchmarkIndex(); await GatherRunner.setupDriver(driver, options); diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index c95cf735caa8..90b09daa4d75 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -389,7 +389,7 @@ describe('GatherRunner', function() { driver, passConfig, settings, - baseArtifacts: await GatherRunner.getBaseArtifacts({driver, settings, requestedUrl}), + baseArtifacts: await GatherRunner.initializeBaseArtifacts({driver, settings, requestedUrl}), }; await GatherRunner.runPass(passContext, {TestGatherer: []});