diff --git a/lighthouse-cli/run.js b/lighthouse-cli/run.js index 357b19046a89..88c6a4ba848d 100644 --- a/lighthouse-cli/run.js +++ b/lighthouse-cli/run.js @@ -65,25 +65,25 @@ function getDebuggableChrome(flags) { } /** @return {never} */ -function showConnectionError() { +function printConnectionErrorAndExit() { console.error('Unable to connect to Chrome'); return process.exit(_RUNTIME_ERROR_CODE); } /** @return {never} */ -function showProtocolTimeoutError() { +function printProtocolTimeoutErrorAndExit() { console.error('Debugger protocol timed out while connecting to Chrome.'); return process.exit(_PROTOCOL_TIMEOUT_EXIT_CODE); } /** @param {LighthouseError} err @return {never} */ -function showPageHungError(err) { +function printPageHungErrorAndExit(err) { console.error('Page hung:', err.friendlyMessage); return process.exit(_PAGE_HUNG_EXIT_CODE); } /** @param {LighthouseError} err @return {never} */ -function showInsecureDocumentRequestError(err) { +function printInsecureDocumentRequestErrorAndExit(err) { console.error('Insecure document request:', err.friendlyMessage); return process.exit(_INSECURE_DOCUMENT_REQUEST_EXIT_CODE); } @@ -92,7 +92,7 @@ function showInsecureDocumentRequestError(err) { * @param {LighthouseError} err * @return {never} */ -function showRuntimeError(err) { +function printRuntimeErrorAndExit(err) { console.error('Runtime error encountered:', err.friendlyMessage || err.message); if (err.stack) { console.error(err.stack); @@ -104,17 +104,17 @@ function showRuntimeError(err) { * @param {LighthouseError} err * @return {never} */ -function handleError(err) { +function printErrorAndExit(err) { if (err.code === 'ECONNREFUSED') { - return showConnectionError(); + return printConnectionErrorAndExit(); } else if (err.code === 'CRI_TIMEOUT') { - return showProtocolTimeoutError(); + return printProtocolTimeoutErrorAndExit(); } else if (err.code === 'PAGE_HUNG') { - return showPageHungError(err); + return printPageHungErrorAndExit(err); } else if (err.code === 'INSECURE_DOCUMENT_REQUEST') { - return showInsecureDocumentRequestError(err); + return printInsecureDocumentRequestErrorAndExit(err); } else { - return showRuntimeError(err); + return printRuntimeErrorAndExit(err); } } @@ -218,10 +218,24 @@ async function runLighthouse(url, flags, config) { await potentiallyKillChrome(launchedChrome); process.removeListener('unhandledRejection', handleTheUnhandled); + // Runtime errors indicate something was *very* wrong with the page result. + // We don't want the user to have to parse the report to figure it out, so we'll still exit + // with an error code after we saved the results. + if (runnerResult && runnerResult.lhr.runtimeError) { + const {runtimeError} = runnerResult.lhr; + return printErrorAndExit({ + name: 'LHError', + friendlyMessage: runtimeError.message, + lhrRuntimeError: true, + code: runtimeError.code, + message: runtimeError.message, + }); + } + return runnerResult; } catch (err) { await potentiallyKillChrome(launchedChrome).catch(() => {}); - handleError(err); + return printErrorAndExit(err); } } diff --git a/lighthouse-cli/test/smokehouse/smokehouse.js b/lighthouse-cli/test/smokehouse/smokehouse.js index 603af8bc3b55..4f40cad43363 100755 --- a/lighthouse-cli/test/smokehouse/smokehouse.js +++ b/lighthouse-cli/test/smokehouse/smokehouse.js @@ -38,6 +38,21 @@ function resolveLocalOrCwd(payloadPath) { return resolved; } +/** + * Determines if the Lighthouse run ended in an unexpected fatal result. + * @param {number} exitCode + * @param {string} outputPath + */ +function isUnexpectedFatalResult(exitCode, outputPath) { + return exitCode !== 0 + // These runtime errors are currently fatal but "expected" runtime errors we are asserting against. + && exitCode !== PAGE_HUNG_EXIT_CODE + && exitCode !== INSECURE_DOCUMENT_REQUEST_EXIT_CODE + // On runtime errors we exit with a error status code, but still output a report. + // If the report exists, it wasn't a fatal LH error we need to abort on, it's one we're asserting :) + && !fs.existsSync(outputPath); +} + /** * Launch Chrome and do a full Lighthouse run. * @param {string} url @@ -85,22 +100,20 @@ function runLighthouse(url, configPath, isDebug) { runResults = spawnSync(command, args, {encoding: 'utf8', stdio: ['pipe', 'pipe', 'inherit']}); } while (runResults.status === PROTOCOL_TIMEOUT_EXIT_CODE && runCount <= RETRIES); + if (isDebug) { + console.log(`STDOUT: ${runResults.stdout}`); + console.error(`STDERR: ${runResults.stderr}`); + } + if (runResults.status === PROTOCOL_TIMEOUT_EXIT_CODE) { console.error(`Lighthouse debugger connection timed out ${RETRIES} times. Giving up.`); process.exit(1); - } else if (runResults.status !== 0 - && runResults.status !== PAGE_HUNG_EXIT_CODE - && runResults.status !== INSECURE_DOCUMENT_REQUEST_EXIT_CODE) { + } else if (isUnexpectedFatalResult(runResults.status, outputPath)) { console.error(`Lighthouse run failed with exit code ${runResults.status}. stderr to follow:`); console.error(runResults.stderr); process.exit(runResults.status); } - if (isDebug) { - console.log(`STDOUT: ${runResults.stdout}`); - console.error(`STDERR: ${runResults.stderr}`); - } - let errorCode; let lhr = {requestedUrl: url, finalUrl: url, audits: {}}; if (runResults.status === PAGE_HUNG_EXIT_CODE) { diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 9c637cfcb642..e23eff7333dd 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -48,10 +48,11 @@ class GatherRunner { * Loads options.url with specified options. If the main document URL * redirects, options.url will be updated accordingly. As such, options.url * will always represent the post-redirected URL. options.requestedUrl is the - * pre-redirect starting URL. + * pre-redirect starting URL. If the navigation errors with "expected" errors such as + * NO_FCP, a `navigationError` is returned. * @param {Driver} driver * @param {LH.Gatherer.PassContext} passContext - * @return {Promise} + * @return {Promise<{navigationError?: LH.LighthouseError}>} */ static async loadPage(driver, passContext) { const gatherers = passContext.passConfig.gatherers; @@ -61,15 +62,25 @@ class GatherRunner { args: [gatherers.map(g => g.instance.name).join(', ')], }; log.time(status); + try { + const finalUrl = await driver.gotoURL(passContext.url, { + waitForFCP: passContext.passConfig.recordTrace, + waitForLoad: true, + passContext, + }); + passContext.url = finalUrl; + } catch (err) { + // If it's one of our loading-based LHErrors, we'll treat it as a page load error. + if (err.code === 'NO_FCP') { + return {navigationError: err}; + } - const finalUrl = await driver.gotoURL(passContext.url, { - waitForFCP: passContext.passConfig.recordTrace, - waitForLoad: true, - passContext, - }); - passContext.url = finalUrl; + throw err; + } finally { + log.timeEnd(status); + } - log.timeEnd(status); + return {}; } /** @@ -122,7 +133,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 {LHError|undefined} + * @return {LH.LighthouseError|undefined} */ static getNetworkError(url, networkRecords) { const mainRecord = networkRecords.find(record => { @@ -161,14 +172,24 @@ class GatherRunner { * main document request failure, a security issue, etc. * @param {LH.Gatherer.PassContext} passContext * @param {LH.Gatherer.LoadData} loadData + * @param {LighthouseError|undefined} navigationError + * @return {LighthouseError|undefined} */ - static getPageLoadError(passContext, loadData) { + static getPageLoadError(passContext, loadData, navigationError) { 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; + // Network errors are usually the most specific and provide the best reason for why the page failed to load. + // Prefer networkError over navigationError. + // Example: `DNS_FAILURE` is better than `NO_FCP`. + if (networkError) return networkError; + + // Navigation errors are rather generic and express some failure of the page to render properly. + // Use `navigationError` as the last resort. + // Example: `NO_FCP`, the page never painted content for some unknown reason. + return navigationError; } /** @@ -576,7 +597,7 @@ class GatherRunner { // Navigate, start recording, and run `pass()` on gatherers. await GatherRunner.beginRecording(passContext); - await GatherRunner.loadPage(driver, passContext); + const {navigationError: possibleNavError} = await GatherRunner.loadPage(driver, passContext); await GatherRunner.pass(passContext, gathererResults); const loadData = await GatherRunner.endRecording(passContext); @@ -589,7 +610,7 @@ class GatherRunner { if (loadData.trace) baseArtifacts.traces[passConfig.passName] = loadData.trace; // If there were any load errors, treat all gatherers as if they errored. - const pageLoadError = GatherRunner.getPageLoadError(passContext, loadData); + const pageLoadError = GatherRunner.getPageLoadError(passContext, loadData, possibleNavError); if (pageLoadError) { log.error('GatherRunner', pageLoadError.friendlyMessage, passContext.url); passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage); diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 90b09daa4d75..065f2967fdfe 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -13,6 +13,8 @@ const assert = require('assert'); const Config = require('../../config/config'); const unresolvedPerfLog = require('./../fixtures/unresolved-perflog.json'); const NetworkRequest = require('../../lib/network-request.js'); +const LHError = require('../../lib/lh-error.js'); +const networkRecordsToDevtoolsLog = require('../network-records-to-devtools-log.js'); jest.mock('../../lib/stack-collector.js', () => () => Promise.resolve([])); @@ -100,7 +102,7 @@ describe('GatherRunner', function() { }; const passContext = { - requestedUrl: url1, + url: url1, settings: {}, passConfig: { gatherers: [], @@ -112,6 +114,26 @@ describe('GatherRunner', function() { }); }); + it('loads a page and returns a pageLoadError', async () => { + const url = 'https://example.com'; + const error = new LHError(LHError.errors.NO_FCP); + const driver = { + gotoURL() { + return Promise.reject(error); + }, + }; + + const passContext = { + url, + settings: {}, + passConfig: {gatherers: []}, + }; + + const {navigationError} = await GatherRunner.loadPage(driver, passContext); + expect(navigationError).toEqual(error); + expect(passContext.url).toEqual(url); + }); + it('collects benchmark as an artifact', async () => { const url = 'https://example.com'; const driver = fakeDriver; @@ -396,6 +418,73 @@ describe('GatherRunner', function() { assert.equal(tests.calledCleanBrowserCaches, true); }); + it('fails artifacts with network errors', async () => { + const requestedUrl = 'https://example.com'; + // This page load error should be overriden by NO_DOCUMENT_REQUEST for being more specific + const navigationError = new LHError(LHError.errors.NO_FCP); + const driver = Object.assign({}, fakeDriver, { + online: true, + gotoURL: url => url.includes('blank') ? null : Promise.reject(navigationError), + }); + + const passConfig = { + gatherers: [ + {instance: new TestGatherer()}, + ], + }; + + const settings = {}; + + const passContext = { + url: requestedUrl, + driver, + passConfig, + settings, + LighthouseRunWarnings: [], + baseArtifacts: await GatherRunner.initializeBaseArtifacts({driver, settings, requestedUrl}), + }; + + const {artifacts} = await GatherRunner.runPass(passContext); + expect(passContext.LighthouseRunWarnings).toHaveLength(1); + expect(artifacts.TestGatherer).toBeInstanceOf(Error); + expect(artifacts.TestGatherer.code).toEqual('NO_DOCUMENT_REQUEST'); + }); + + it('fails artifacts with navigation errors', async () => { + const requestedUrl = 'https://example.com'; + // This time, NO_FCP should win because it's the only error left. + const navigationError = new LHError(LHError.errors.NO_FCP); + const driver = Object.assign({}, fakeDriver, { + online: true, + gotoURL: url => url.includes('blank') ? null : Promise.reject(navigationError), + endDevtoolsLog() { + return networkRecordsToDevtoolsLog([{url: requestedUrl}]); + }, + }); + + const passConfig = { + gatherers: [ + {instance: new TestGatherer()}, + ], + }; + + const settings = {}; + + const passContext = { + url: requestedUrl, + driver, + passConfig, + settings, + LighthouseRunWarnings: [], + baseArtifacts: await GatherRunner.initializeBaseArtifacts({driver, settings, requestedUrl}), + }; + + const {artifacts} = await GatherRunner.runPass(passContext); + expect(passContext.LighthouseRunWarnings).toHaveLength(1); + expect(artifacts.TestGatherer).toBeInstanceOf(Error); + expect(artifacts.TestGatherer.code).toEqual('NO_FCP'); + }); + it('does not clear origin storage with flag --disable-storage-reset', () => { const asyncFunc = () => Promise.resolve(); const tests = { diff --git a/types/lhr.d.ts b/types/lhr.d.ts index 28c60b9ed894..3bee01cf1057 100644 --- a/types/lhr.d.ts +++ b/types/lhr.d.ts @@ -8,6 +8,8 @@ import LHError = require('../lighthouse-core/lib/lh-error.js'); declare global { module LH { + export type LighthouseError = LHError; + export type I18NMessageEntry = string | {path: string, values: any}; export interface I18NMessages {