From 6d9280c67c96dbb0826d8aab09b5811655b6bcc3 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Thu, 20 Sep 2018 15:03:58 -0700 Subject: [PATCH 1/2] remove 50% pageLoadError -> throw error --- lighthouse-core/gather/gather-runner.js | 8 -------- lighthouse-core/lib/lh-error.js | 9 --------- 2 files changed, 17 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 07b988094ed3..d3df26229c97 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -333,7 +333,6 @@ class GatherRunner { /** @type {Partial} */ const gathererArtifacts = {}; - const pageLoadFailures = []; const resultsEntries = /** @type {GathererResultsEntries} */ (Object.entries(gathererResults)); for (const [gathererName, phaseResultsPromises] of resultsEntries) { if (gathererArtifacts[gathererName] !== undefined) continue; @@ -349,18 +348,11 @@ class GatherRunner { // 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. gathererArtifacts[gathererName] = err; - // Track page load errors separately, so we can fail loudly if needed. - if (LHError.isPageLoadError(err)) pageLoadFailures.push(err); } if (gathererArtifacts[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(gathererArtifacts).length * 0.5) { - throw LHError.fromLighthouseError(pageLoadFailures[0]); - } } // Take only unique LighthouseRunWarnings. diff --git a/lighthouse-core/lib/lh-error.js b/lighthouse-core/lib/lh-error.js index 4adeee996013..557fcc746d53 100644 --- a/lighthouse-core/lib/lh-error.js +++ b/lighthouse-core/lib/lh-error.js @@ -31,15 +31,6 @@ class LighthouseError extends Error { Error.captureStackTrace(this, LighthouseError); } - /** - * @param {{code?: string}} err - * @return {err is LighthouseError} - */ - static isPageLoadError(err) { - return err.code === ERRORS.NO_DOCUMENT_REQUEST.code || - err.code === ERRORS.FAILED_DOCUMENT_REQUEST.code; - } - /** * @param {LighthouseError} err * @return {LighthouseError} From 6f6bde8f9b89245b0f7835af985d79f5aaf25deb Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Thu, 20 Sep 2018 15:54:28 -0700 Subject: [PATCH 2/2] more errors --- lighthouse-core/gather/gather-runner.js | 24 ++++++++----------- lighthouse-core/lib/strings.js | 2 +- lighthouse-core/runner.js | 4 +--- .../test/gather/gather-runner-test.js | 8 +++---- lighthouse-core/test/runner-test.js | 2 +- 5 files changed, 17 insertions(+), 23 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index d3df26229c97..918aa0e13553 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -156,22 +156,19 @@ class GatherRunner { return URL.equalWithExcludedFragments(record.url, url); }); - let errorCode; - let errorReason; + let errorDef; if (!mainRecord) { - errorCode = LHError.errors.NO_DOCUMENT_REQUEST; + errorDef = LHError.errors.NO_DOCUMENT_REQUEST; } else if (mainRecord.failed) { - errorCode = LHError.errors.FAILED_DOCUMENT_REQUEST; - errorReason = mainRecord.localizedFailDescription; + errorDef = {...LHError.errors.FAILED_DOCUMENT_REQUEST}; + errorDef.message += ` ${mainRecord.localizedFailDescription}.`; } else if (mainRecord.hasErrorStatusCode()) { - errorCode = LHError.errors.ERRORED_DOCUMENT_REQUEST; - errorReason = `Status code: ${mainRecord.statusCode}`; + errorDef = {...LHError.errors.ERRORED_DOCUMENT_REQUEST}; + errorDef.message += ` Status code: ${mainRecord.statusCode}.`; } - if (errorCode) { - const error = new LHError(errorCode, {reason: errorReason}); - log.error('GatherRunner', error.message, url); - return error; + if (errorDef) { + return new LHError(errorDef); } } @@ -279,9 +276,8 @@ class GatherRunner { if (!driver.online) pageLoadError = undefined; if (pageLoadError) { - passContext.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.'); + log.error('GatherRunner', pageLoadError.message, passContext.url); + passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage); } // Expose devtoolsLog, networkRecords, and trace (if present) to gatherers diff --git a/lighthouse-core/lib/strings.js b/lighthouse-core/lib/strings.js index d6f56f5a1362..24bf4d91cd71 100644 --- a/lighthouse-core/lib/strings.js +++ b/lighthouse-core/lib/strings.js @@ -10,7 +10,7 @@ module.exports = { didntCollectScreenshots: `Chrome didn't collect any screenshots during the page load. Please make sure there is content visible on the page, and then try re-running Lighthouse.`, badTraceRecording: `Something went wrong with recording the trace over your page load. Please run Lighthouse again.`, pageLoadTookTooLong: `Your page took too long to load. Please follow the opportunities in the report to reduce your page load time, and then try re-running Lighthouse.`, - pageLoadFailed: `Your page failed to load. Verify that the URL is valid and re-run Lighthouse.`, + pageLoadFailed: `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.`, internalChromeError: `An internal Chrome error occurred. Please restart Chrome and try re-running Lighthouse.`, requestContentTimeout: 'Fetching resource content has exceeded the allotted time', urlInvalid: `The URL you have provided appears to be invalid.`, diff --git a/lighthouse-core/runner.js b/lighthouse-core/runner.js index 77605732f1c3..12c84e57f2ad 100644 --- a/lighthouse-core/runner.js +++ b/lighthouse-core/runner.js @@ -301,9 +301,7 @@ class Runner { static getArtifactRuntimeError(artifacts) { for (const possibleErrorArtifact of Object.values(artifacts)) { if (possibleErrorArtifact instanceof LHError && possibleErrorArtifact.lhrRuntimeError) { - const errorMessage = possibleErrorArtifact.friendlyMessage ? - `${possibleErrorArtifact.friendlyMessage} (${possibleErrorArtifact.message})` : - possibleErrorArtifact.message; + const errorMessage = possibleErrorArtifact.friendlyMessage || possibleErrorArtifact.message; return { code: possibleErrorArtifact.code, diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 6e8ae8efeba0..8afb23feedcd 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -627,7 +627,7 @@ describe('GatherRunner', function() { mainRecord.localizedFailDescription = 'foobar'; const error = GatherRunner.getPageLoadError(url, [mainRecord]); assert.equal(error.message, 'FAILED_DOCUMENT_REQUEST'); - assert.ok(/Your page failed to load/.test(error.friendlyMessage)); + assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); it('fails when page times out', () => { @@ -635,7 +635,7 @@ describe('GatherRunner', function() { const records = []; const error = GatherRunner.getPageLoadError(url, records); assert.equal(error.message, 'NO_DOCUMENT_REQUEST'); - assert.ok(/Your page failed to load/.test(error.friendlyMessage)); + assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); it('fails when page returns with a 404', () => { @@ -645,7 +645,7 @@ describe('GatherRunner', function() { mainRecord.statusCode = 404; const error = GatherRunner.getPageLoadError(url, [mainRecord]); assert.equal(error.message, 'ERRORED_DOCUMENT_REQUEST'); - assert.ok(/Your page failed to load/.test(error.friendlyMessage)); + assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); it('fails when page returns with a 500', () => { @@ -655,7 +655,7 @@ describe('GatherRunner', function() { mainRecord.statusCode = 500; const error = GatherRunner.getPageLoadError(url, [mainRecord]); assert.equal(error.message, 'ERRORED_DOCUMENT_REQUEST'); - assert.ok(/Your page failed to load/.test(error.friendlyMessage)); + assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); }); diff --git a/lighthouse-core/test/runner-test.js b/lighthouse-core/test/runner-test.js index 3bc17cea57b4..068b2990aad0 100644 --- a/lighthouse-core/test/runner-test.js +++ b/lighthouse-core/test/runner-test.js @@ -639,7 +639,7 @@ describe('Runner', () => { assert.ok(lhr.audits['test-audit'].errorMessage.includes(NO_FCP.code)); // And it bubbled up to the runtimeError. assert.strictEqual(lhr.runtimeError.code, NO_FCP.code); - assert.ok(lhr.runtimeError.message.includes(NO_FCP.code)); + assert.ok(lhr.runtimeError.message.includes(NO_FCP.message)); }); it('can handle array of outputs', async () => {