From 53741e5eacf9d0e4c478672a126b4ab4e608fb1c Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Fri, 6 Apr 2018 15:27:36 -0700 Subject: [PATCH 1/4] core(tsc): add type checking to gather-runner --- lighthouse-cli/bin.js | 12 +- lighthouse-cli/run.js | 4 +- .../lantern-consistently-interactive.js | 2 +- .../metrics/lantern-first-contentful-paint.js | 4 +- .../metrics/lantern-first-meaningful-paint.js | 6 +- .../gather/computed/metrics/lantern-metric.js | 8 +- lighthouse-core/gather/connections/cri.js | 2 - lighthouse-core/gather/driver.js | 25 +- lighthouse-core/gather/gather-runner.js | 273 +++++++++--------- lighthouse-core/index.js | 6 +- lighthouse-core/lib/emulation.js | 2 +- lighthouse-core/lib/errors.js | 2 +- lighthouse-core/runner.js | 4 +- .../test/gather/gather-runner-test.js | 126 +++++--- typings/artifacts.d.ts | 57 ++++ typings/audit.d.ts | 2 +- typings/config.d.ts | 71 +++++ typings/externs.d.ts | 34 +-- typings/gatherer.d.ts | 43 +-- typings/web-inspector.d.ts | 2 + 20 files changed, 413 insertions(+), 272 deletions(-) create mode 100644 typings/artifacts.d.ts create mode 100644 typings/config.d.ts diff --git a/lighthouse-cli/bin.js b/lighthouse-cli/bin.js index c699e55eb748..b14dbe0ba1f8 100644 --- a/lighthouse-cli/bin.js +++ b/lighthouse-cli/bin.js @@ -33,7 +33,7 @@ function isDev() { // Tell user if there's a newer version of LH. updateNotifier({pkg}).notify(); -const /** @type {!LH.Flags} */ cliFlags = getFlags(); +const /** @type {LH.Flags} */ cliFlags = getFlags(); // Process terminating command if (cliFlags.listAllAudits) { @@ -48,16 +48,16 @@ if (cliFlags.listTraceCategories) { /** @type {string} */ const url = cliFlags._[0]; -/** @type {!LH.Config|undefined} */ +/** @type {LH.Config.Json|undefined} */ let config; if (cliFlags.configPath) { // Resolve the config file path relative to where cli was called. cliFlags.configPath = path.resolve(process.cwd(), cliFlags.configPath); - config = /** @type {!LH.Config} */ (require(cliFlags.configPath)); + config = /** @type {LH.Config.Json} */ (require(cliFlags.configPath)); } else if (cliFlags.perf) { - config = /** @type {!LH.Config} */ (perfOnlyConfig); + config = /** @type {LH.Config.Json} */ (perfOnlyConfig); } else if (cliFlags.mixedContent) { - config = /** @type {!LH.Config} */ (mixedContentConfig); + config = /** @type {LH.Config.Json} */ (mixedContentConfig); // The mixed-content audits require headless Chrome (https://crbug.com/764505). cliFlags.chromeFlags = `${cliFlags.chromeFlags} --headless`; } @@ -84,7 +84,7 @@ if (cliFlags.extraHeaders) { } /** - * @return {!Promise<(void|!LH.Results)>} + * @return {Promise} */ function run() { return Promise.resolve() diff --git a/lighthouse-cli/run.js b/lighthouse-cli/run.js index 55212daeed06..95b276a372e1 100644 --- a/lighthouse-cli/run.js +++ b/lighthouse-cli/run.js @@ -147,8 +147,8 @@ function saveResults(results, artifacts, flags) { /** * @param {string} url - * @param {!LH.Flags} flags - * @param {!LH.Config|undefined} config + * @param {LH.Flags} flags + * @param {LH.Config.Json|undefined} config * @return {Promise} */ function runLighthouse(url, flags, config) { diff --git a/lighthouse-core/gather/computed/metrics/lantern-consistently-interactive.js b/lighthouse-core/gather/computed/metrics/lantern-consistently-interactive.js index e837a580d712..d61e9a77f8db 100644 --- a/lighthouse-core/gather/computed/metrics/lantern-consistently-interactive.js +++ b/lighthouse-core/gather/computed/metrics/lantern-consistently-interactive.js @@ -84,7 +84,7 @@ class ConsistentlyInteractive extends MetricArtifact { /** * @param {{trace: Object, devtoolsLog: Object}} data * @param {Object} artifacts - * @return {Promise} + * @return {Promise} */ async compute_(data, artifacts) { const fmpResult = await artifacts.requestLanternFirstMeaningfulPaint(data, artifacts); diff --git a/lighthouse-core/gather/computed/metrics/lantern-first-contentful-paint.js b/lighthouse-core/gather/computed/metrics/lantern-first-contentful-paint.js index 3a40b61952fb..5e52888df4f7 100644 --- a/lighthouse-core/gather/computed/metrics/lantern-first-contentful-paint.js +++ b/lighthouse-core/gather/computed/metrics/lantern-first-contentful-paint.js @@ -28,7 +28,7 @@ class FirstContentfulPaint extends MetricArtifact { /** * @param {!Node} dependencyGraph - * @param {LH.Gatherer.Artifact.TraceOfTab} traceOfTab + * @param {LH.Artifacts.TraceOfTab} traceOfTab * @return {!Node} */ getOptimisticGraph(dependencyGraph, traceOfTab) { @@ -54,7 +54,7 @@ class FirstContentfulPaint extends MetricArtifact { /** * @param {!Node} dependencyGraph - * @param {LH.Gatherer.Artifact.TraceOfTab} traceOfTab + * @param {LH.Artifacts.TraceOfTab} traceOfTab * @return {!Node} */ getPessimisticGraph(dependencyGraph, traceOfTab) { diff --git a/lighthouse-core/gather/computed/metrics/lantern-first-meaningful-paint.js b/lighthouse-core/gather/computed/metrics/lantern-first-meaningful-paint.js index b6c3192fe860..aff454c455a1 100644 --- a/lighthouse-core/gather/computed/metrics/lantern-first-meaningful-paint.js +++ b/lighthouse-core/gather/computed/metrics/lantern-first-meaningful-paint.js @@ -28,7 +28,7 @@ class FirstMeaningfulPaint extends MetricArtifact { /** * @param {!Node} dependencyGraph - * @param {LH.Gatherer.Artifact.TraceOfTab} traceOfTab + * @param {LH.Artifacts.TraceOfTab} traceOfTab * @return {!Node} */ getOptimisticGraph(dependencyGraph, traceOfTab) { @@ -54,7 +54,7 @@ class FirstMeaningfulPaint extends MetricArtifact { /** * @param {!Node} dependencyGraph - * @param {LH.Gatherer.Artifact.TraceOfTab} traceOfTab + * @param {LH.Artifacts.TraceOfTab} traceOfTab * @return {!Node} */ getPessimisticGraph(dependencyGraph, traceOfTab) { @@ -80,7 +80,7 @@ class FirstMeaningfulPaint extends MetricArtifact { /** * @param {{trace: Object, devtoolsLog: Object}} data * @param {Object} artifacts - * @return {Promise} + * @return {Promise} */ async compute_(data, artifacts) { const fcpResult = await artifacts.requestLanternFirstContentfulPaint(data, artifacts); diff --git a/lighthouse-core/gather/computed/metrics/lantern-metric.js b/lighthouse-core/gather/computed/metrics/lantern-metric.js index da998e1ed5de..5595b3903036 100644 --- a/lighthouse-core/gather/computed/metrics/lantern-metric.js +++ b/lighthouse-core/gather/computed/metrics/lantern-metric.js @@ -40,7 +40,7 @@ class LanternMetricArtifact extends ComputedArtifact { /** * @param {!Node} dependencyGraph - * @param {LH.Gatherer.Artifact.TraceOfTab} traceOfTab + * @param {LH.Artifacts.TraceOfTab} traceOfTab * @return {!Node} */ getOptimisticGraph(dependencyGraph, traceOfTab) { // eslint-disable-line no-unused-vars @@ -49,7 +49,7 @@ class LanternMetricArtifact extends ComputedArtifact { /** * @param {!Node} dependencyGraph - * @param {LH.Gatherer.Artifact.TraceOfTab} traceOfTab + * @param {LH.Artifacts.TraceOfTab} traceOfTab * @return {!Node} */ getPessimisticGraph(dependencyGraph, traceOfTab) { // eslint-disable-line no-unused-vars @@ -69,7 +69,7 @@ class LanternMetricArtifact extends ComputedArtifact { * @param {{trace: Object, devtoolsLog: Object}} data * @param {Object} artifacts * @param {any=} extras - * @return {Promise} + * @return {Promise} */ async computeMetricWithGraphs(data, artifacts, extras) { const {trace, devtoolsLog} = data; @@ -112,7 +112,7 @@ class LanternMetricArtifact extends ComputedArtifact { /** * @param {{trace: Object, devtoolsLog: Object}} data * @param {Object} artifacts - * @return {Promise} + * @return {Promise} */ compute_(data, artifacts) { return this.computeMetricWithGraphs(data, artifacts); diff --git a/lighthouse-core/gather/connections/cri.js b/lighthouse-core/gather/connections/cri.js index adf0b232696a..f0b2713d1e36 100644 --- a/lighthouse-core/gather/connections/cri.js +++ b/lighthouse-core/gather/connections/cri.js @@ -113,7 +113,6 @@ class CriConnection extends Connection { // After aborting, we expect an ECONNRESET error. Ignore. request.on('error', err => { - // @ts-ignore `code` property extension to Error by Node. if (err.code !== 'ECONNRESET') { throw err; } @@ -122,7 +121,6 @@ class CriConnection extends Connection { // TODO: Replace this with an LHError on next major version bump // Reject on error with code specifically indicating timeout in connection setup. const err = new Error('Timeout waiting for initial Debugger Protocol connection.'); - // @ts-ignore fixed by above TODO err.code = 'CRI_TIMEOUT'; log.error('CriConnection', err.message); reject(err); diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index d981b11dd24e..89965dbc49e3 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -664,7 +664,7 @@ class Driver { */ async gotoURL(url, options = {}) { const waitForLoad = options.waitForLoad || false; - const passContext = options.passContext || {}; + const passContext = /** @type {Partial} */ (options.passContext || {}); const disableJS = passContext.disableJavaScript || false; await this._beginNetworkStatusMonitoring(url); @@ -678,7 +678,7 @@ class Driver { this.sendCommand('Page.navigate', {url}); if (waitForLoad) { - const passConfig = passContext.passConfig || {}; + const passConfig = /** @type {Partial} */ (passContext.passConfig || {}); let {pauseAfterLoadMs, networkQuietThresholdMs, cpuQuietThresholdMs} = passConfig; let maxWaitMs = passContext.settings && passContext.settings.maxWaitForLoad; @@ -876,7 +876,7 @@ class Driver { } /** - * @return {Promise} + * @return {Promise} */ endTrace() { return new Promise((resolve, reject) => { @@ -893,6 +893,7 @@ class Driver { /** * @param {LH.Crdp.Tracing.TracingCompleteEvent} traceCompleteEvent + * @return {Promise} */ _readTraceFromStream(traceCompleteEvent) { return new Promise((resolve, reject) => { @@ -955,7 +956,7 @@ class Driver { } /** - * @param {LH.ConfigSettings} settings + * @param {LH.Config.Settings} settings * @return {Promise} */ async beginEmulation(settings) { @@ -967,7 +968,7 @@ class Driver { } /** - * @param {LH.ConfigSettings} settings + * @param {LH.Config.Settings} settings * @param {{useThrottling?: boolean}} passConfig * @return {Promise} */ @@ -998,7 +999,7 @@ class Driver { /** * Enable internet connection, using emulated mobile settings if applicable. - * @param {{settings: LH.ConfigSettings, passConfig: LH.ConfigPass}} options + * @param {{settings: LH.Config.Settings, passConfig: LH.Config.Pass}} options * @return {Promise} */ async goOnline(options) { @@ -1019,17 +1020,15 @@ class Driver { } /** - * @param {LH.Crdp.Network.Headers} headers key/value pairs of HTTP Headers. + * @param {LH.Crdp.Network.Headers=} headers key/value pairs of HTTP Headers. * @return {Promise} */ - setExtraHTTPHeaders(headers) { - if (headers) { - return this.sendCommand('Network.setExtraHTTPHeaders', { - headers, - }); + async setExtraHTTPHeaders(headers) { + if (!headers) { + return; } - return Promise.resolve(); + return this.sendCommand('Network.setExtraHTTPHeaders', {headers}); } /** diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index b6ddcfbfc8e2..02ea0e0644be 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -3,19 +3,23 @@ * 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. */ -// @ts-nocheck 'use strict'; const log = require('lighthouse-logger'); -const Audit = require('../audits/audit'); const LHError = require('../lib/errors'); const URL = require('../lib/url-shim'); const NetworkRecorder = require('../lib/network-recorder.js'); const constants = require('../config/constants'); +const Driver = require('../gather/driver.js'); // eslint-disable-line no-unused-vars + /** - * @typedef {!Object>>} GathererResults + * Each entry in each gatherer result array is the output of a gatherer phase: + * `beforePass`, `pass`, and `afterPass`. Flattened into an `LH.Artifacts` in + * `collectArtifacts`. + * @typedef {{LighthouseRunWarnings: Array>, [artifactName: string]: Array<*>}} GathererResults */ +/** @typedef {{traces: Object, devtoolsLogs: Object>}} TracingData */ /** * Class that drives browser to load the page and runs gatherer lifecycle hooks. @@ -61,10 +65,10 @@ class GatherRunner { * not let a service worker take over, we navigate away and then come back to * reload. We do not `waitForLoad` on about:blank since a page load event is * never fired on it. - * @param {!Driver} driver - * @param {url=} url + * @param {Driver} driver + * @param {string=} url * @param {number=} duration - * @return {!Promise} + * @return {Promise} */ static loadBlank( driver, @@ -79,9 +83,9 @@ class GatherRunner { * redirects, options.url will be updated accordingly. As such, options.url * will always represent the post-redirected URL. options.initialUrl is the * pre-redirect starting URL. - * @param {!Driver} driver - * @param {!Object} passContext - * @return {!Promise} + * @param {Driver} driver + * @param {LH.Gatherer.PassContext} passContext + * @return {Promise} */ static loadPage(driver, passContext) { return driver.gotoURL(passContext.url, { @@ -93,10 +97,10 @@ class GatherRunner { } /** - * @param {!Driver} driver - * @param {!GathererResults} gathererResults - * @param {!Object} options - * @return {!Promise} + * @param {Driver} driver + * @param {GathererResults} gathererResults + * @param {{url: string, settings: LH.Config.Settings}} options + * @return {Promise} */ static setupDriver(driver, gathererResults, options) { log.log('status', 'Initializing…'); @@ -107,16 +111,21 @@ class GatherRunner { .then(userAgent => { gathererResults.UserAgent = [userAgent]; GatherRunner.warnOnHeadless(userAgent, gathererResults); - gathererResults.fetchedAt = [(new Date()).toJSON()]; }) .then(_ => driver.beginEmulation(options.settings)) .then(_ => driver.enableRuntimeEvents()) .then(_ => driver.cacheNatives()) .then(_ => driver.registerPerformanceObserver()) .then(_ => driver.dismissJavaScriptDialogs()) - .then(_ => resetStorage && driver.clearDataForOrigin(options.url)); + .then(_ => { + if (resetStorage) return driver.clearDataForOrigin(options.url); + }); } + /** + * @param {Driver} driver + * @return {Promise} + */ static disposeDriver(driver) { log.log('status', 'Disconnecting from browser...'); return driver.disconnect().catch(err => { @@ -131,8 +140,8 @@ class GatherRunner { /** * Test any error output from the promise, absorbing non-fatal errors and * throwing on fatal ones so that run is stopped. - * @param {!Promise<*>} promise - * @return {!Promise<*>} + * @param {Promise<*>} promise + * @return {Promise} */ static recoverOrThrow(promise) { return promise.catch(err => { @@ -146,7 +155,7 @@ class GatherRunner { * Returns an error if the original network request failed or wasn't found. * @param {string} url The URL of the original requested page. * @param {Array} networkRecords - * @return {?Error} + * @return {LHError|undefined} */ static getPageLoadError(url, networkRecords) { const mainRecord = networkRecords.find(record => { @@ -164,6 +173,7 @@ class GatherRunner { } if (errorCode) { + // @ts-ignore TODO(bckenny): fix LHError constructor/errors mismatch const error = new LHError(errorCode, {reason: errorReason}); log.error('GatherRunner', error.message, url); return error; @@ -173,7 +183,7 @@ class GatherRunner { /** * Add run warning if running in Headless Chrome. * @param {string} userAgent - * @param {!GathererResults} gathererResults + * @param {GathererResults} gathererResults */ static warnOnHeadless(userAgent, gathererResults) { const chromeVersion = userAgent.split(/HeadlessChrome\/(.*) /)[1]; @@ -181,7 +191,7 @@ class GatherRunner { // https://chromium.googlesource.com/chromium/src/+/8931a104b145ccf92390f6f48fba6553a1af92e4 const minVersion = '63.0.3239.0'; if (chromeVersion && chromeVersion < minVersion) { - gathererResults.LighthouseRunWarnings.push('Your site\'s mobile performance may be ' + + gathererResults.LighthouseRunWarnings[0].push('Your site\'s mobile performance may be ' + 'worse than the numbers presented in this report. Lighthouse could not test on a ' + 'mobile connection because Headless Chrome does not support network throttling ' + 'prior to version ' + minVersion + '. The version used was ' + chromeVersion); @@ -191,9 +201,9 @@ class GatherRunner { /** * Navigates to about:blank and calls beforePass() on gatherers before tracing * has started and before navigation to the target page. - * @param {!Object} passContext - * @param {!GathererResults} gathererResults - * @return {!Promise} + * @param {LH.Gatherer.PassContext} passContext + * @param {GathererResults} gathererResults + * @return {Promise} */ static beforePass(passContext, gathererResults) { const blockedUrls = (passContext.passConfig.blockedUrlPatterns || []) @@ -222,9 +232,9 @@ class GatherRunner { /** * Navigates to requested URL and then runs pass() on gatherers while trace * (if requested) is still being recorded. - * @param {!Object} passContext - * @param {!GathererResults} gathererResults - * @return {!Promise} + * @param {LH.Gatherer.PassContext} passContext + * @param {GathererResults} gathererResults + * @return {Promise} */ static pass(passContext, gathererResults) { const driver = passContext.driver; @@ -241,11 +251,15 @@ class GatherRunner { const pass = Promise.resolve() // Clear disk & memory cache if it's a perf run - .then(_ => isPerfRun && driver.cleanBrowserCaches()) + .then(_ => { + if (isPerfRun) driver.cleanBrowserCaches(); + }) // Always record devtoolsLog .then(_ => driver.beginDevtoolsLog()) // Begin tracing if requested by config. - .then(_ => recordTrace && driver.beginTrace(settings)) + .then(_ => { + if (recordTrace) driver.beginTrace(settings); + }) // Navigate. .then(_ => GatherRunner.loadPage(driver, passContext)) .then(_ => log.log('statusEnd', status)); @@ -266,77 +280,69 @@ 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. - * @param {!Object} passContext - * @param {!GathererResults} gathererResults - * @return {!Promise} + * @param {LH.Gatherer.PassContext} passContext + * @param {GathererResults} gathererResults + * @return {Promise} */ - static afterPass(passContext, gathererResults) { + static async afterPass(passContext, gathererResults) { const driver = passContext.driver; const config = passContext.passConfig; const gatherers = config.gatherers; - const passData = {}; - - let pass = Promise.resolve(); - let pageLoadError; + let trace; if (config.recordTrace) { - pass = pass.then(_ => { - log.log('status', 'Retrieving trace'); - return driver.endTrace(); - }).then(traceContents => { - // Before Chrome 54.0.2816 (codereview.chromium.org/2161583004), - // traceContents was an array of trace events; after, traceContents is - // an object with a traceEvents property. Normalize to object form. - passData.trace = Array.isArray(traceContents) ? - {traceEvents: traceContents} : traceContents; - log.verbose('statusEnd', 'Retrieving trace'); - }); + log.log('status', 'Retrieving trace'); + trace = await driver.endTrace(); + log.verbose('statusEnd', 'Retrieving trace'); } - pass = pass.then(_ => { - const status = 'Retrieving devtoolsLog and network records'; - log.log('status', status); - const devtoolsLog = driver.endDevtoolsLog(); - const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog); - log.verbose('statusEnd', status); + const status = 'Retrieving devtoolsLog and network records'; + log.log('status', status); + const devtoolsLog = driver.endDevtoolsLog(); + const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog); + log.verbose('statusEnd', status); - 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 = null; + 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) { - gathererResults.LighthouseRunWarnings.push('Lighthouse was unable to reliably load the ' + - 'page you requested. Make sure you are testing the correct URL and that the server is ' + - 'properly responding to all requests.'); - } + if (pageLoadError) { + gathererResults.LighthouseRunWarnings[0].push('Lighthouse was unable to reliably load the ' + + 'page you requested. Make sure you are testing the correct URL and that the server is ' + + 'properly responding to all requests.'); + } - // Expose devtoolsLog and networkRecords to gatherers - passData.devtoolsLog = devtoolsLog; - passData.networkRecords = networkRecords; - }); + // Expose devtoolsLog, networkRecords, and trace (if present) to gatherers + /** @type {LH.Gatherer.LoadData} */ + const passData = { + networkRecords, + devtoolsLog, + trace, + }; // Disable throttling so the afterPass analysis isn't throttled - pass = pass.then(_ => driver.setThrottling(passContext.settings, {useThrottling: false})); + await driver.setThrottling(passContext.settings, {useThrottling: false}); - pass = gatherers.reduce((chain, gathererDefn) => { + for (const gathererDefn of gatherers) { const gatherer = gathererDefn.instance; const status = `Retrieving: ${gatherer.name}`; - return chain.then(_ => { - log.log('status', status); - // Abuse the passContext to pass through gatherer options - passContext.options = gathererDefn.options || {}; - const artifactPromise = pageLoadError ? - Promise.reject(pageLoadError) : - Promise.resolve().then(_ => gatherer.afterPass(passContext, passData)); - gathererResults[gatherer.name].push(artifactPromise); - return GatherRunner.recoverOrThrow(artifactPromise); - }).then(_ => { - log.verbose('statusEnd', status); - }); - }, pass); + log.log('status', status); + + // 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)); + gathererResults[gatherer.name].push(artifactPromise); + await GatherRunner.recoverOrThrow(artifactPromise); + log.verbose('statusEnd', status); + } // Resolve on tracing data using passName from config. - return pass.then(_ => passData); + return passData; } /** @@ -344,67 +350,70 @@ class GatherRunner { * last produced value (that's not undefined) as the artifact for that * gatherer. If a non-fatal error was rejected from a gatherer phase, * uses that error object as the artifact instead. - * @param {!GathererResults} gathererResults - * @return {!Promise} + * @param {GathererResults} gathererResults + * @param {TracingData} tracingData + * @param {LH.Config.Settings} settings + * @return {Promise} */ - static collectArtifacts(gathererResults) { - const artifacts = {}; + static async collectArtifacts(gathererResults, tracingData, settings) { + // Can't handle dynamic GathererResults -> Artifacts, so explicitly make *. + /** @type {Object} */ + const artifacts = { + traces: tracingData.traces, + devtoolsLogs: tracingData.devtoolsLogs, + settings, + }; - // Nest LighthouseRunWarnings, if any, so they will be collected into artifact. - const uniqueWarnings = Array.from(new Set(gathererResults.LighthouseRunWarnings)); + // Take only unique LighthouseRunWarnings, if any. + const uniqueWarnings = Array.from(new Set(gathererResults.LighthouseRunWarnings[0])); gathererResults.LighthouseRunWarnings = [uniqueWarnings]; const pageLoadFailures = []; - return Object.keys(gathererResults).reduce((chain, gathererName) => { - return chain.then(_ => { - const phaseResultsPromises = gathererResults[gathererName]; - return Promise.all(phaseResultsPromises).then(phaseResults => { - // Take last defined pass result as artifact. - const definedResults = phaseResults.filter(element => element !== undefined); - const artifact = definedResults[definedResults.length - 1]; - if (artifact === undefined) { - throw new Error(`${gathererName} failed to provide an artifact.`); - } - artifacts[gathererName] = artifact; - }, err => { - // To reach this point, all errors are non-fatal, so return err to - // runner to handle turning it into an error audit. - artifacts[gathererName] = err; - // Track page load errors separately, so we can fail loudly if needed. - if (LHError.isPageLoadError(err)) pageLoadFailures.push(err); - }); - }); - }, Promise.resolve()).then(_ => { + for (const [gathererName, phaseResultsPromises] of Object.entries(gathererResults)) { + try { + const phaseResults = await Promise.all(phaseResultsPromises); + // Take last defined pass result as artifact. + const definedResults = phaseResults.filter(element => element !== undefined); + const artifact = definedResults[definedResults.length - 1]; + artifacts[gathererName] = artifact; + } catch (err) { + // An error result must be non-fatal to not have caused an exit by now, + // so return it to runner to handle turning it into an error audit. + artifacts[gathererName] = err; + // Track page load errors separately, so we can fail loudly if needed. + if (LHError.isPageLoadError(err)) pageLoadFailures.push(err); + } + + if (artifacts[gathererName] === undefined) { + throw new Error(`${gathererName} failed to provide an artifact.`); + } + // Fail the run if more than 50% of all artifacts failed due to page load failure. if (pageLoadFailures.length > Object.keys(artifacts).length * 0.5) { throw pageLoadFailures[0]; } + } - return artifacts; - }); + return /** @type {LH.Artifacts } */ (artifacts); } + /** + * @param {Array} passes + * @param {{driver: Driver, url: string, settings: LH.Config.Settings}} options + * @return {Promise} + */ static run(passes, options) { const driver = options.driver; + /** @type {TracingData} */ const tracingData = { traces: {}, devtoolsLogs: {}, }; - if (typeof options.url !== 'string' || options.url.length === 0) { - return Promise.reject(new Error('You must provide a url to the gather-runner')); - } - - if (typeof options.config === 'undefined') { - return Promise.reject(new Error('You must provide a config')); - } - - if (typeof options.settings === 'undefined') { - options.settings = {}; - } - + /** @type {GathererResults} */ const gathererResults = { - LighthouseRunWarnings: [], + LighthouseRunWarnings: [[]], + fetchedAt: [(new Date()).toJSON()], }; return driver.connect() @@ -414,7 +423,7 @@ class GatherRunner { // Run each pass .then(_ => { // If the main document redirects, we'll update this to keep track - let urlAfterRedirects; + let urlAfterRedirects = options.url; return passes.reduce((chain, passConfig, passIndex) => { const passContext = Object.assign({}, options, {passConfig}); return chain @@ -423,14 +432,12 @@ class GatherRunner { .then(_ => GatherRunner.pass(passContext, gathererResults)) .then(_ => GatherRunner.afterPass(passContext, gathererResults)) .then(passData => { - const passName = passConfig.passName || Audit.DEFAULT_PASS; - - // networkRecords are discarded and not added onto artifacts. - tracingData.devtoolsLogs[passName] = passData.devtoolsLog; + // Save devtoolsLog, but networkRecords are discarded and not added onto artifacts. + tracingData.devtoolsLogs[passConfig.passName] = passData.devtoolsLog; - // If requested by config, add trace to pass's tracingData - if (passConfig.recordTrace) { - tracingData.traces[passName] = passData.trace; + // If requested by config, save pass's trace. + if (passData.trace) { + tracingData.traces[passConfig.passName] = passData.trace; } if (passIndex === 0) { @@ -442,11 +449,7 @@ class GatherRunner { }); }) .then(_ => GatherRunner.disposeDriver(driver)) - .then(_ => GatherRunner.collectArtifacts(gathererResults)) - .then(artifacts => { - // Add tracing data and settings used to the artifacts object. - return Object.assign(artifacts, tracingData, {settings: options.settings}); - }) + .then(_ => GatherRunner.collectArtifacts(gathererResults, tracingData, options.settings)) // cleanup on error .catch(err => { GatherRunner.disposeDriver(driver); diff --git a/lighthouse-core/index.js b/lighthouse-core/index.js index f2f81f2192dd..b5128da9f3de 100644 --- a/lighthouse-core/index.js +++ b/lighthouse-core/index.js @@ -28,9 +28,9 @@ const Config = require('./config/config'); /** * @param {string} url - * @param {!LH.Flags} flags - * @param {!LH.Config|undefined} configJSON - * @return {!Promise} + * @param {LH.Flags} flags + * @param {LH.Config.Json|undefined} configJSON + * @return {Promise} */ function lighthouse(url, flags = {}, configJSON) { const startTime = Date.now(); diff --git a/lighthouse-core/lib/emulation.js b/lighthouse-core/lib/emulation.js index a12f23400daa..0b2c939e886f 100644 --- a/lighthouse-core/lib/emulation.js +++ b/lighthouse-core/lib/emulation.js @@ -131,7 +131,7 @@ function disableCPUThrottling(driver) { } /** - * @param {LH.ConfigSettings} settings + * @param {LH.Config.Settings} settings * @return {{deviceEmulation: string, cpuThrottling: string, networkThrottling: string}} */ function getEmulationDesc(settings) { diff --git a/lighthouse-core/lib/errors.js b/lighthouse-core/lib/errors.js index 12f90bd5d1bf..b3b4fb20fbaf 100644 --- a/lighthouse-core/lib/errors.js +++ b/lighthouse-core/lib/errors.js @@ -30,7 +30,7 @@ class LighthouseError extends Error { } /** - * @param {LH.LighthouseError} err + * @param {Error} err */ static isPageLoadError(err) { return err.code === ERRORS.NO_DOCUMENT_REQUEST.code || diff --git a/lighthouse-core/runner.js b/lighthouse-core/runner.js index e3eedab5a093..f1c458396075 100644 --- a/lighthouse-core/runner.js +++ b/lighthouse-core/runner.js @@ -142,11 +142,11 @@ class Runner { * Establish connection, load page and collect all required artifacts * @param {*} opts * @param {*} connection - * @return {!Promise} + * @return {Promise} */ static async _gatherArtifactsFromBrowser(opts, connection) { if (!opts.config.passes) { - return Promise.reject(new Error('No browser artifacts are either provided or requested.')); + throw new Error('No browser artifacts are either provided or requested.'); } opts.driver = opts.driverMock || new Driver(connection); diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index ec631c5df095..ddd540189152 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -106,22 +106,12 @@ describe('GatherRunner', function() { }); }); - it('creates settings if needed', () => { - const url = 'https://example.com'; - const driver = fakeDriver; - const config = new Config({}); - const options = {url, driver, config}; - - return GatherRunner.run([], options).then(_ => { - assert.equal(typeof options.settings, 'object'); - }); - }); - it('collects user agent as an artifact', () => { const url = 'https://example.com'; const driver = fakeDriver; const config = new Config({}); - const options = {url, driver, config}; + const settings = {}; + const options = {url, driver, config, settings}; return GatherRunner.run([], options).then(results => { assert.equal(results.UserAgent, 'Fake user agent', 'did not find expected user agent string'); @@ -501,21 +491,6 @@ describe('GatherRunner', function() { }); }); - it('rejects when not given a URL', () => { - return GatherRunner.run({}, {}).then(_ => assert.ok(false), _ => assert.ok(true)); - }); - - it('rejects when given a URL of zero length', () => { - return GatherRunner.run({}, {url: ''}).then(_ => assert.ok(false), _ => assert.ok(true)); - }); - - it('rejects when not given a config', () => { - return GatherRunner.run({}, {url: 'http://example.com'}) - .then(_ => assert.ok(false), err => { - assert.ok(/config/i.test(err)); - }); - }); - it('does as many passes as are required', () => { const t1 = new TestGatherer(); const t2 = new TestGatherer(); @@ -622,6 +597,85 @@ describe('GatherRunner', function() { }); describe('artifact collection', () => { + it('runs gatherer lifecycle methods strictly in sequence', async () => { + const counter = { + beforePass: 0, + pass: 0, + afterPass: 0 + }; + const shortPause = () => new Promise(resolve => setTimeout(resolve, 50)); + async function fastish(counterName, value) { + assert.strictEqual(counter[counterName], value - 1); + counter[counterName] = value; + await shortPause(); + assert.strictEqual(counter[counterName], value); + } + async function medium(counterName, value) { + await Promise.resolve(); + await Promise.resolve(); + await fastish(counterName, value); + } + async function slowwwww(counterName, value) { + await shortPause(); + await shortPause(); + await medium(counterName, value); + } + + const gatherers = [ + class First extends Gatherer { + async beforePass() { + await slowwwww('beforePass', 1); + } + async pass() { + await slowwwww('pass', 1); + } + async afterPass() { + await slowwwww('afterPass', 1); + return this.name; + } + }, + class Second extends Gatherer { + async beforePass() { + await medium('beforePass', 2); + } + async pass() { + await medium('pass', 2); + } + async afterPass() { + await medium('afterPass', 2); + return this.name; + } + }, + class Third extends Gatherer { + beforePass() { + return fastish('beforePass', 3); + } + pass() { + return fastish('pass', 3); + } + async afterPass() { + await fastish('afterPass', 3); + return this.name; + } + } + ]; + const passes = [{ + blankDuration: 0, + gatherers: gatherers.map(G => ({instance: new G()})), + }]; + + const artifacts = await GatherRunner.run(passes, { + driver: fakeDriver, + url: 'https://example.com', + settings: {}, + }); + + // Ensure artifacts returned and not errors. + gatherers.forEach(gatherer => { + assert.strictEqual(artifacts[gatherer.name], gatherer.name); + }); + }); + it('supports sync and async return of artifacts from gatherers', () => { const gatherers = [ // sync @@ -755,7 +809,7 @@ describe('GatherRunner', function() { LighthouseRunWarnings: [], }; - 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); @@ -764,18 +818,18 @@ describe('GatherRunner', function() { }); it('produces a LighthouseRunWarnings artifact from array of warnings', () => { - const LighthouseRunWarnings = [ + const LighthouseRunWarnings = [[ 'warning0', 'warning1', 'warning2', - ]; + ]]; const gathererResults = { LighthouseRunWarnings, }; - return GatherRunner.collectArtifacts(gathererResults).then(artifacts => { - assert.deepStrictEqual(artifacts.LighthouseRunWarnings, LighthouseRunWarnings); + return GatherRunner.collectArtifacts(gathererResults, {}).then(artifacts => { + assert.deepStrictEqual(artifacts.LighthouseRunWarnings, LighthouseRunWarnings[0]); }); }); @@ -953,17 +1007,17 @@ describe('GatherRunner', function() { it('issues a lighthouseRunWarnings if running an old version of Headless', () => { const gathererResults = { - LighthouseRunWarnings: [], + LighthouseRunWarnings: [[]], }; const userAgent = 'Mozilla/5.0 AppleWebKit/537.36 HeadlessChrome/63.0.3239.0 Safari/537.36'; GatherRunner.warnOnHeadless(userAgent, gathererResults); - assert.strictEqual(gathererResults.LighthouseRunWarnings.length, 0); + assert.strictEqual(gathererResults.LighthouseRunWarnings[0].length, 0); const oldUserAgent = 'Mozilla/5.0 AppleWebKit/537.36 HeadlessChrome/62.0.3239.0 Safari/537.36'; GatherRunner.warnOnHeadless(oldUserAgent, gathererResults); - assert.strictEqual(gathererResults.LighthouseRunWarnings.length, 1); - const warning = gathererResults.LighthouseRunWarnings[0]; + assert.strictEqual(gathererResults.LighthouseRunWarnings[0].length, 1); + const warning = gathererResults.LighthouseRunWarnings[0][0]; assert.ok(/Headless Chrome/.test(warning)); }); }); diff --git a/typings/artifacts.d.ts b/typings/artifacts.d.ts new file mode 100644 index 000000000000..fba19393866a --- /dev/null +++ b/typings/artifacts.d.ts @@ -0,0 +1,57 @@ +/** + * @license Copyright 2018 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. + */ + +declare global { + module LH { + export interface Artifacts { + fetchedAt: string; + LanternMetric: Artifacts.LanternMetric; + LighthouseRunWarnings: string[]; + TraceOfTab: Artifacts.TraceOfTab; + TraceTimes: Artifacts.TraceTimes; + UserAgent: string; + traces: {[passName: string]: Trace}; + devtoolsLogs: {[passName: string]: Protocol.RawEventMessage}; + } + + module Artifacts { + export interface LanternMetric { + timing: number; + optimisticEstimate: Gatherer.Simulation.Result + pessimisticEstimate: Gatherer.Simulation.Result; + optimisticGraph: Gatherer.Simulation.GraphNode; + pessimisticGraph: Gatherer.Simulation.GraphNode; + } + + export interface TraceTimes { + navigationStart: number; + firstPaint: number; + firstContentfulPaint: number; + firstMeaningfulPaint: number; + traceEnd: number; + onLoad: number; + domContentLoaded: number; + } + + export interface TraceOfTab { + timings: TraceTimes; + timestamps: TraceTimes; + processEvents: Array; + mainThreadEvents: Array; + startedInPageEvt: TraceEvent; + navigationStartEvt: TraceEvent; + firstPaintEvt: TraceEvent; + firstContentfulPaintEvt: TraceEvent; + firstMeaningfulPaintEvt: TraceEvent; + onLoadEvt: TraceEvent; + fmpFellBack: boolean; + } + } + } +} + +// empty export to keep file a module +export {} diff --git a/typings/audit.d.ts b/typings/audit.d.ts index 1e5612eeb9b4..65499c6ac5ee 100644 --- a/typings/audit.d.ts +++ b/typings/audit.d.ts @@ -8,7 +8,7 @@ declare global { module LH.Audit { export interface Context { options: Object; // audit options - settings: ConfigSettings; + settings: Config.Settings; } export interface ScoreOptions { diff --git a/typings/config.d.ts b/typings/config.d.ts new file mode 100644 index 000000000000..f9e00ab867d1 --- /dev/null +++ b/typings/config.d.ts @@ -0,0 +1,71 @@ +/** + * @license Copyright 2018 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. + */ + +import * as Gatherer from '../lighthouse-core/gather/gatherers/gatherer.js'; + +declare global { + module LH { + /** + * The full, normalized Lighthouse Config. Also a namespace for Config- + * related types (see below). + */ + export interface Config { + settings: Config.Settings; + passes: Config.Pass[]; + } + + module Config { + export interface Json { + settings?: SettingsJson; + passes?: PassJson[]; + } + + export interface SettingsJson extends SharedFlagsSettings { + extraHeaders?: Crdp.Network.Headers; + } + + export interface PassJson { + passName: string; + recordTrace?: boolean; + useThrottling?: boolean; + pauseAfterLoadMs?: number; + networkQuietThresholdMs?: number; + cpuQuietThresholdMs?: number; + blockedUrlPatterns?: string[]; + blankPage?: string; + blankDuration?: number; + gatherers: GathererJson[]; + } + + export type GathererJson = { + path: string; + options?: {}; + } | { + implementation: typeof Gatherer; + options?: {}; + } | { + instance: InstanceType; + options?: {}; + } | string; + + // TODO(bckenny): we likely don't want to require all these + export type Settings = Required; + + export interface Pass extends Required { + gatherers: GathererDefn[]; + } + + export interface GathererDefn { + implementation: typeof Gatherer; + instance: InstanceType; + options: {}; + } + } + } +} + +// empty export to keep file a module +export {} \ No newline at end of file diff --git a/typings/externs.d.ts b/typings/externs.d.ts index 0f234d0448b0..dbff1c351bc1 100644 --- a/typings/externs.d.ts +++ b/typings/externs.d.ts @@ -9,6 +9,12 @@ import _StrictEventEmitter from '../third-party/strict-event-emitter-types/index import { EventEmitter } from 'events'; declare global { + // Augment global Error type to include node's optional `code` property + // see https://nodejs.org/api/errors.html#errors_error_code + interface Error { + code?: string; + } + module LH { // re-export useful type modules under global LH module. export import Crdp = _Crdp; @@ -30,7 +36,7 @@ declare global { interface SharedFlagsSettings { maxWaitForLoad?: number; blockedUrlPatterns?: string[]; - additionalTraceCategories?: string[]; + additionalTraceCategories?: string; auditMode?: boolean | string; gatherMode?: boolean | string; disableStorageReset?: boolean; @@ -64,27 +70,6 @@ declare global { extraHeaders?: string; } - // TODO: type checking for Config - export interface Config { - passes?: ConfigPass[]; - settings?: ConfigSettings; - } - - export interface ConfigSettings extends SharedFlagsSettings { - extraHeaders?: Crdp.Network.Headers; - } - - export interface ConfigPass { - recordTrace?: boolean; - useThrottling?: boolean; - pauseAfterLoadMs?: number; - networkQuietThresholdMs?: number; - cpuQuietThresholdMs?: number; - blockedUrlPatterns?: string[]; - blankPage?: string; - blankDuration?: string; - } - export interface Results { url: string; audits: Audit.Results; @@ -101,10 +86,13 @@ declare global { } export interface LighthouseError extends Error { - code?: string; friendlyMessage?: string; } + export interface Trace { + traceEvents: TraceEvent[]; + } + export interface TraceEvent { name: string; args: { diff --git a/typings/gatherer.d.ts b/typings/gatherer.d.ts index f3f8e7435348..3f9d0ac195e6 100644 --- a/typings/gatherer.d.ts +++ b/typings/gatherer.d.ts @@ -7,54 +7,23 @@ import * as _Node from '../lighthouse-core/lib/dependency-graph/node'; import * as _NetworkNode from '../lighthouse-core/lib/dependency-graph/network-node'; import * as _CPUNode from '../lighthouse-core/lib/dependency-graph/cpu-node'; +import * as Driver from '../lighthouse-core/gather/driver'; declare global { module LH.Gatherer { export interface PassContext { + url: string; + driver: InstanceType; disableJavaScript?: boolean; - passConfig?: ConfigPass; - settings?: ConfigSettings; + passConfig: Config.Pass + settings: Config.Settings; options?: object; } export interface LoadData { networkRecords: Array; devtoolsLog: Array; - trace: {traceEvents: Array}; - } - - namespace Artifact { - export interface LanternMetric { - timing: number; - optimisticEstimate: Simulation.Result; - pessimisticEstimate: Simulation.Result; - optimisticGraph: Simulation.GraphNode; - pessimisticGraph: Simulation.GraphNode; - } - - export interface TraceTimes { - navigationStart: number; - firstPaint: number; - firstContentfulPaint: number; - firstMeaningfulPaint: number; - traceEnd: number; - onLoad: number; - domContentLoaded: number; - } - - export interface TraceOfTab { - timings: TraceTimes; - timestamps: TraceTimes; - processEvents: Array; - mainThreadEvents: Array; - startedInPageEvt: TraceEvent; - navigationStartEvt: TraceEvent; - firstPaintEvt: TraceEvent; - firstContentfulPaintEvt: TraceEvent; - firstMeaningfulPaintEvt: TraceEvent; - onLoadEvt: TraceEvent; - fmpFellBack: boolean; - } + trace?: Trace; } namespace Simulation { diff --git a/typings/web-inspector.d.ts b/typings/web-inspector.d.ts index 67b8782bf88d..96dcc0ffee96 100644 --- a/typings/web-inspector.d.ts +++ b/typings/web-inspector.d.ts @@ -26,6 +26,8 @@ declare global { redirectSource?: { url: string; } + failed?: boolean; + localizedFailDescription?: string; _initiator: NetworkRequestInitiator; _timing: NetworkRequestTiming; From fcf96a2468f9a1c50a116be9f7157700a0ef1719 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Mon, 9 Apr 2018 15:01:08 -0700 Subject: [PATCH 2/4] tweaks --- lighthouse-cli/bin.js | 4 ++-- lighthouse-core/gather/gather-runner.js | 3 ++- typings/config.d.ts | 8 +++++--- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/lighthouse-cli/bin.js b/lighthouse-cli/bin.js index b14dbe0ba1f8..93703cc674cf 100644 --- a/lighthouse-cli/bin.js +++ b/lighthouse-cli/bin.js @@ -55,9 +55,9 @@ if (cliFlags.configPath) { cliFlags.configPath = path.resolve(process.cwd(), cliFlags.configPath); config = /** @type {LH.Config.Json} */ (require(cliFlags.configPath)); } else if (cliFlags.perf) { - config = /** @type {LH.Config.Json} */ (perfOnlyConfig); + config = perfOnlyConfig; } else if (cliFlags.mixedContent) { - config = /** @type {LH.Config.Json} */ (mixedContentConfig); + config = mixedContentConfig; // The mixed-content audits require headless Chrome (https://crbug.com/764505). cliFlags.chromeFlags = `${cliFlags.chromeFlags} --headless`; } diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 02ea0e0644be..9440da4d9b4a 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -356,7 +356,8 @@ class GatherRunner { * @return {Promise} */ static async collectArtifacts(gathererResults, tracingData, settings) { - // Can't handle dynamic GathererResults -> Artifacts, so explicitly make *. + // Can't handle dynamic GathererResults -> Artifacts, so explicitly make * + // and cast to Artifacts before returning. /** @type {Object} */ const artifacts = { traces: tracingData.traces, diff --git a/typings/config.d.ts b/typings/config.d.ts index f9e00ab867d1..1400befb747c 100644 --- a/typings/config.d.ts +++ b/typings/config.d.ts @@ -9,8 +9,7 @@ import * as Gatherer from '../lighthouse-core/gather/gatherers/gatherer.js'; declare global { module LH { /** - * The full, normalized Lighthouse Config. Also a namespace for Config- - * related types (see below). + * The full, normalized Lighthouse Config. */ export interface Config { settings: Config.Settings; @@ -18,6 +17,9 @@ declare global { } module Config { + /** + * The pre-normalization Lighthouse Config format. + */ export interface Json { settings?: SettingsJson; passes?: PassJson[]; @@ -68,4 +70,4 @@ declare global { } // empty export to keep file a module -export {} \ No newline at end of file +export {} From a4f55aa583ffabd6e7855f479896d477213bba10 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Mon, 9 Apr 2018 16:29:06 -0700 Subject: [PATCH 3/4] feedback --- lighthouse-core/test/gather/gather-runner-test.js | 1 + typings/artifacts.d.ts | 3 --- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index ddd540189152..a4f019a2d0b6 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -597,6 +597,7 @@ describe('GatherRunner', function() { }); describe('artifact collection', () => { + // Make sure our gatherers never execute in parallel it('runs gatherer lifecycle methods strictly in sequence', async () => { const counter = { beforePass: 0, diff --git a/typings/artifacts.d.ts b/typings/artifacts.d.ts index fba19393866a..add45cc2367f 100644 --- a/typings/artifacts.d.ts +++ b/typings/artifacts.d.ts @@ -8,10 +8,7 @@ declare global { module LH { export interface Artifacts { fetchedAt: string; - LanternMetric: Artifacts.LanternMetric; LighthouseRunWarnings: string[]; - TraceOfTab: Artifacts.TraceOfTab; - TraceTimes: Artifacts.TraceTimes; UserAgent: string; traces: {[passName: string]: Trace}; devtoolsLogs: {[passName: string]: Protocol.RawEventMessage}; From e290bc88bb211dbc46530da4961439be6efd6436 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Mon, 9 Apr 2018 16:43:56 -0700 Subject: [PATCH 4/4] make LighthouseRunWarnings not terrible --- lighthouse-core/gather/gather-runner.js | 16 +++++++-------- .../test/gather/gather-runner-test.js | 20 +++++++++---------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 9440da4d9b4a..7b32ad6f784e 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -17,7 +17,7 @@ const Driver = require('../gather/driver.js'); // eslint-disable-line no-unused- * Each entry in each gatherer result array is the output of a gatherer phase: * `beforePass`, `pass`, and `afterPass`. Flattened into an `LH.Artifacts` in * `collectArtifacts`. - * @typedef {{LighthouseRunWarnings: Array>, [artifactName: string]: Array<*>}} GathererResults + * @typedef {{LighthouseRunWarnings: Array, [artifactName: string]: Array<*>}} GathererResults */ /** @typedef {{traces: Object, devtoolsLogs: Object>}} TracingData */ @@ -191,7 +191,7 @@ class GatherRunner { // https://chromium.googlesource.com/chromium/src/+/8931a104b145ccf92390f6f48fba6553a1af92e4 const minVersion = '63.0.3239.0'; if (chromeVersion && chromeVersion < minVersion) { - gathererResults.LighthouseRunWarnings[0].push('Your site\'s mobile performance may be ' + + gathererResults.LighthouseRunWarnings.push('Your site\'s mobile performance may be ' + 'worse than the numbers presented in this report. Lighthouse could not test on a ' + 'mobile connection because Headless Chrome does not support network throttling ' + 'prior to version ' + minVersion + '. The version used was ' + chromeVersion); @@ -307,7 +307,7 @@ class GatherRunner { if (!driver.online) pageLoadError = undefined; if (pageLoadError) { - gathererResults.LighthouseRunWarnings[0].push('Lighthouse was unable to reliably load the ' + + gathererResults.LighthouseRunWarnings.push('Lighthouse was unable to reliably load the ' + 'page you requested. Make sure you are testing the correct URL and that the server is ' + 'properly responding to all requests.'); } @@ -363,14 +363,14 @@ class GatherRunner { traces: tracingData.traces, devtoolsLogs: tracingData.devtoolsLogs, settings, + // Take only unique LighthouseRunWarnings, if any. + LighthouseRunWarnings: Array.from(new Set(gathererResults.LighthouseRunWarnings)), }; - // Take only unique LighthouseRunWarnings, if any. - const uniqueWarnings = Array.from(new Set(gathererResults.LighthouseRunWarnings[0])); - gathererResults.LighthouseRunWarnings = [uniqueWarnings]; - const pageLoadFailures = []; for (const [gathererName, phaseResultsPromises] of Object.entries(gathererResults)) { + if (artifacts[gathererName] !== undefined) continue; + try { const phaseResults = await Promise.all(phaseResultsPromises); // Take last defined pass result as artifact. @@ -413,7 +413,7 @@ class GatherRunner { /** @type {GathererResults} */ const gathererResults = { - LighthouseRunWarnings: [[]], + LighthouseRunWarnings: [], fetchedAt: [(new Date()).toJSON()], }; diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index a4f019a2d0b6..6e9bab457ff2 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -602,7 +602,7 @@ describe('GatherRunner', function() { const counter = { beforePass: 0, pass: 0, - afterPass: 0 + afterPass: 0, }; const shortPause = () => new Promise(resolve => setTimeout(resolve, 50)); async function fastish(counterName, value) { @@ -621,7 +621,7 @@ describe('GatherRunner', function() { await shortPause(); await medium(counterName, value); } - + const gatherers = [ class First extends Gatherer { async beforePass() { @@ -658,7 +658,7 @@ describe('GatherRunner', function() { await fastish('afterPass', 3); return this.name; } - } + }, ]; const passes = [{ blankDuration: 0, @@ -819,18 +819,18 @@ describe('GatherRunner', function() { }); it('produces a LighthouseRunWarnings artifact from array of warnings', () => { - const LighthouseRunWarnings = [[ + const LighthouseRunWarnings = [ 'warning0', 'warning1', 'warning2', - ]]; + ]; const gathererResults = { LighthouseRunWarnings, }; return GatherRunner.collectArtifacts(gathererResults, {}).then(artifacts => { - assert.deepStrictEqual(artifacts.LighthouseRunWarnings, LighthouseRunWarnings[0]); + assert.deepStrictEqual(artifacts.LighthouseRunWarnings, LighthouseRunWarnings); }); }); @@ -1008,17 +1008,17 @@ describe('GatherRunner', function() { it('issues a lighthouseRunWarnings if running an old version of Headless', () => { const gathererResults = { - LighthouseRunWarnings: [[]], + LighthouseRunWarnings: [], }; const userAgent = 'Mozilla/5.0 AppleWebKit/537.36 HeadlessChrome/63.0.3239.0 Safari/537.36'; GatherRunner.warnOnHeadless(userAgent, gathererResults); - assert.strictEqual(gathererResults.LighthouseRunWarnings[0].length, 0); + assert.strictEqual(gathererResults.LighthouseRunWarnings.length, 0); const oldUserAgent = 'Mozilla/5.0 AppleWebKit/537.36 HeadlessChrome/62.0.3239.0 Safari/537.36'; GatherRunner.warnOnHeadless(oldUserAgent, gathererResults); - assert.strictEqual(gathererResults.LighthouseRunWarnings[0].length, 1); - const warning = gathererResults.LighthouseRunWarnings[0][0]; + assert.strictEqual(gathererResults.LighthouseRunWarnings.length, 1); + const warning = gathererResults.LighthouseRunWarnings[0]; assert.ok(/Headless Chrome/.test(warning)); }); });