From cf43c2bcd8aed3c4eff088a1a16d4f121e97557e Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Wed, 12 Jun 2019 13:42:35 -0700 Subject: [PATCH 1/4] core(gather-runner): introduce PageLoadError base artifact --- .../test/smokehouse/error-config.js | 2 - .../test/smokehouse/error-expectations.js | 8 +-- .../test/smokehouse/smokehouse-report.js | 6 +- lighthouse-core/gather/gather-runner.js | 39 +++-------- lighthouse-core/runner.js | 10 ++- .../test/gather/gather-runner-test.js | 33 ++++++---- lighthouse-core/test/runner-test.js | 65 +++++++++++++++---- types/artifacts.d.ts | 2 + 8 files changed, 101 insertions(+), 64 deletions(-) diff --git a/lighthouse-cli/test/smokehouse/error-config.js b/lighthouse-cli/test/smokehouse/error-config.js index 6be23c61645d..771bdf60461e 100644 --- a/lighthouse-cli/test/smokehouse/error-config.js +++ b/lighthouse-cli/test/smokehouse/error-config.js @@ -14,8 +14,6 @@ module.exports = { maxWaitForLoad: 5000, onlyAudits: [ 'first-contentful-paint', - // TODO: this audit collects a non-base artifact, allowing the runtimeError to be collected. - 'content-width', ], }, }; diff --git a/lighthouse-cli/test/smokehouse/error-expectations.js b/lighthouse-cli/test/smokehouse/error-expectations.js index 7f0f9bc80401..c29472aacadd 100644 --- a/lighthouse-cli/test/smokehouse/error-expectations.js +++ b/lighthouse-cli/test/smokehouse/error-expectations.js @@ -14,11 +14,11 @@ module.exports = [ requestedUrl: 'http://localhost:10200/infinite-loop.html', finalUrl: 'http://localhost:10200/infinite-loop.html', audits: {}, - // TODO: runtimeError only exists because of selection of audits. runtimeError: {code: 'PAGE_HUNG'}, }, artifacts: { - ViewportDimensions: {code: 'PAGE_HUNG'}, + // Assert undefined because pageLoadError prevented gatherer from running. + ViewportDimensions: undefined, }, }, { @@ -26,11 +26,11 @@ module.exports = [ requestedUrl: 'https://expired.badssl.com', finalUrl: 'https://expired.badssl.com/', audits: {}, - // TODO: runtimeError only exists because of selection of audits. runtimeError: {code: 'INSECURE_DOCUMENT_REQUEST'}, }, artifacts: { - ViewportDimensions: {code: 'INSECURE_DOCUMENT_REQUEST'}, + // Assert undefined because pageLoadError prevented gatherer from running. + ViewportDimensions: undefined, }, }, ]; diff --git a/lighthouse-cli/test/smokehouse/smokehouse-report.js b/lighthouse-cli/test/smokehouse/smokehouse-report.js index 5f670a38a394..dcf7409be62d 100644 --- a/lighthouse-cli/test/smokehouse/smokehouse-report.js +++ b/lighthouse-cli/test/smokehouse/smokehouse-report.js @@ -138,11 +138,13 @@ function collateResults(actual, expected) { const artifactNames = /** @type {(keyof LH.Artifacts)[]} */ (Object.keys(expectedArtifacts)); artifactAssertions = artifactNames.map(artifactName => { const actualResult = (actual.artifacts || {})[artifactName]; - if (!actualResult) { + const expectedResult = expectedArtifacts[artifactName]; + + // Actual artifact *must* exist unless expected is explicitly set to `undefined`. + if (!actualResult && expectedResult !== undefined) { throw new Error(`Config run did not generate artifact ${artifactName}`); } - const expectedResult = expectedArtifacts[artifactName]; return makeComparison(artifactName + ' artifact', actualResult, expectedResult); }); } diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 36216a0740b9..ad9b29bee722 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -408,28 +408,6 @@ class GatherRunner { log.timeEnd(apStatus); } - /** - * 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 - * @return {{pageLoadError: LHError, artifacts: Partial}} - */ - static generatePageLoadErrorArtifacts(passContext, pageLoadError) { - /** @type {Partial>} */ - const errorArtifacts = {}; - for (const gathererDefn of passContext.passConfig.gatherers) { - const gatherer = gathererDefn.instance; - errorArtifacts[gatherer.name] = pageLoadError; - } - - return { - pageLoadError, - // @ts-ignore - TODO(bckenny): figure out how to usefully type errored artifacts. - artifacts: errorArtifacts, - }; - } - /** * Takes the results of each gatherer phase for each gatherer and uses the * last produced value (that's not undefined) as the artifact for that @@ -495,6 +473,7 @@ class GatherRunner { settings: options.settings, URL: {requestedUrl: options.requestedUrl, finalUrl: options.requestedUrl}, Timing: [], + PageLoadError: null, }; } @@ -594,7 +573,10 @@ class GatherRunner { Object.assign(artifacts, passResults.artifacts); // If we encountered a pageLoadError, don't try to keep loading the page in future passes. - if (passResults.pageLoadError) break; + if (passResults.pageLoadError) { + baseArtifacts.PageLoadError = passResults.pageLoadError; + break; + } if (isFirstPass) { await GatherRunner.populateBaseArtifacts(passContext); @@ -602,13 +584,11 @@ class GatherRunner { } } - await GatherRunner.disposeDriver(driver, options); GatherRunner.finalizeBaseArtifacts(baseArtifacts); return /** @type {LH.Artifacts} */ ({...baseArtifacts, ...artifacts}); // Cast to drop Partial<>. - } catch (err) { - // cleanup on error + } finally { + // Clean up regardless of error. GatherRunner.disposeDriver(driver, options); - throw err; } } @@ -660,14 +640,15 @@ class GatherRunner { // Disable throttling so the afterPass analysis isn't throttled await driver.setThrottling(passContext.settings, {useThrottling: false}); - // If there were any load errors, treat all gatherers as if they errored. + // In case of load error, save log and trace with an error prefix, return no artifacts for this pass. const pageLoadError = GatherRunner.getPageLoadError(passContext, loadData, possibleNavError); if (pageLoadError) { log.error('GatherRunner', pageLoadError.friendlyMessage, passContext.url); passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage); GatherRunner._addLoadDataToBaseArtifacts(passContext, loadData, `pageLoadError-${passConfig.passName}`); - return GatherRunner.generatePageLoadErrorArtifacts(passContext, pageLoadError); + + return {artifacts: {}, pageLoadError}; } // If no error, save devtoolsLog and trace. diff --git a/lighthouse-core/runner.js b/lighthouse-core/runner.js index 153b26075f12..c96a9e6a55a9 100644 --- a/lighthouse-core/runner.js +++ b/lighthouse-core/runner.js @@ -352,12 +352,18 @@ class Runner { } /** - * Returns first runtimeError found in artifacts. + * Searches a pass's artifacts for any `lhrRuntimeError` error artifacts. + * Returns the first one found or `null` if none found. * @param {LH.Artifacts} artifacts * @return {LH.Result['runtimeError']|undefined} */ static getArtifactRuntimeError(artifacts) { - for (const possibleErrorArtifact of Object.values(artifacts)) { + const possibleErrorArtifacts = [ + artifacts.PageLoadError, // Preferentially use `PageLoadError`, if it exists. + ...Object.values(artifacts), // Otherwise check amongst all artifacts. + ]; + + for (const possibleErrorArtifact of possibleErrorArtifacts) { if (possibleErrorArtifact instanceof LHError && possibleErrorArtifact.lhrRuntimeError) { const errorMessage = possibleErrorArtifact.friendlyMessage || possibleErrorArtifact.message; diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 533e77646dfd..1161af861cdd 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -424,9 +424,10 @@ describe('GatherRunner', function() { assert.equal(tests.calledCleanBrowserCaches, true); }); - it('fails artifacts with network errors', async () => { + it('returns a pageLoadError and no 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 + // This page load error should be overriden by NO_DOCUMENT_REQUEST (for being + // more specific) since requestedUrl does not match any network request in fixture. const navigationError = new LHError(LHError.errors.NO_FCP); const driver = Object.assign({}, fakeDriver, { online: true, @@ -450,13 +451,14 @@ describe('GatherRunner', function() { baseArtifacts: await GatherRunner.initializeBaseArtifacts({driver, settings, requestedUrl}), }; - const {artifacts} = await GatherRunner.runPass(passContext); + const {artifacts, pageLoadError} = await GatherRunner.runPass(passContext); expect(passContext.LighthouseRunWarnings).toHaveLength(1); - expect(artifacts.TestGatherer).toBeInstanceOf(Error); - expect(artifacts.TestGatherer.code).toEqual('NO_DOCUMENT_REQUEST'); + expect(pageLoadError).toBeInstanceOf(Error); + expect(pageLoadError.code).toEqual('NO_DOCUMENT_REQUEST'); + expect(artifacts).toEqual({}); }); - it('fails artifacts with navigation errors', async () => { + it('returns a pageLoadError and no 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); @@ -485,10 +487,11 @@ describe('GatherRunner', function() { baseArtifacts: await GatherRunner.initializeBaseArtifacts({driver, settings, requestedUrl}), }; - const {artifacts} = await GatherRunner.runPass(passContext); + const {artifacts, pageLoadError} = await GatherRunner.runPass(passContext); expect(passContext.LighthouseRunWarnings).toHaveLength(1); - expect(artifacts.TestGatherer).toBeInstanceOf(Error); - expect(artifacts.TestGatherer.code).toEqual('NO_FCP'); + expect(pageLoadError).toBeInstanceOf(Error); + expect(pageLoadError.code).toEqual('NO_FCP'); + expect(artifacts).toEqual({}); }); it('does not clear origin storage with flag --disable-storage-reset', () => { @@ -806,7 +809,8 @@ describe('GatherRunner', function() { const options = {driver, requestedUrl, settings: config.settings}; const artifacts = await GatherRunner.run(config.passes, options); - expect(artifacts.TestGatherer.code).toEqual('NO_DOCUMENT_REQUEST'); + expect(artifacts.PageLoadError.code).toEqual('NO_DOCUMENT_REQUEST'); + expect(artifacts.TestGatherer).toBeUndefined(); // The only loadData available should be prefixed with `pageLoadError-`. expect(Object.keys(artifacts.traces)).toEqual(['pageLoadError-firstPass']); @@ -856,12 +860,15 @@ describe('GatherRunner', function() { expect(t2.called).toBe(true); expect(t3.called).toBe(false); - // But only t1 has a valid artifact, t2 has an error artifact, and t3 never ran. + // But only t1 has a valid artifact; t2 and t3 aren't defined. expect(artifacts.Test1).toBe('MyArtifact'); - expect(artifacts.Test2).toBeInstanceOf(LHError); - expect(artifacts.Test2.code).toEqual('NO_FCP'); + expect(artifacts.Test2).toBeUndefined(); expect(artifacts.Test3).toBeUndefined(); + // PageLoadError artifact has the error. + expect(artifacts.PageLoadError).toBeInstanceOf(LHError); + expect(artifacts.PageLoadError.code).toEqual('NO_FCP'); + // firstPass has a saved trace and devtoolsLog, secondPass has an error trace and log. expect(Object.keys(artifacts.traces)).toEqual(['firstPass', 'pageLoadError-secondPass']); expect(Object.keys(artifacts.devtoolsLogs)).toEqual(['firstPass', 'pageLoadError-secondPass']); diff --git a/lighthouse-core/test/runner-test.js b/lighthouse-core/test/runner-test.js index 5f894a4e87a0..e94e8e6ee2cf 100644 --- a/lighthouse-core/test/runner-test.js +++ b/lighthouse-core/test/runner-test.js @@ -631,13 +631,18 @@ describe('Runner', () => { }); }); - it('includes a top-level runtimeError when a gatherer throws one', async () => { + describe('lhr.runtimeError', () => { const NO_FCP = LHError.errors.NO_FCP; class RuntimeErrorGatherer extends Gatherer { afterPass() { throw new LHError(NO_FCP); } } + class RuntimeError2Gatherer extends Gatherer { + afterPass() { + throw new LHError(LHError.errors.NO_SCREENSHOTS); + } + } class WarningAudit extends Audit { static get meta() { return { @@ -652,19 +657,55 @@ describe('Runner', () => { } } - const config = new Config({ - passes: [{gatherers: [RuntimeErrorGatherer]}], + const configJson = { + passes: [ + {gatherers: [RuntimeErrorGatherer]}, + {gatherers: [RuntimeError2Gatherer], passName: 'second'}, + ], audits: [WarningAudit], - }); - const {lhr} = await Runner.run(null, {url: 'https://example.com/', config, driverMock}); + }; - // Audit error included the runtimeError - assert.strictEqual(lhr.audits['test-audit'].scoreDisplayMode, 'error'); - 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); - expect(lhr.runtimeError.message) - .toBeDisplayString(/Something .*\(NO_FCP\)/); + it('includes a top-level runtimeError when a gatherer throws one', async () => { + const config = new Config(configJson); + const {lhr} = await Runner.run(null, {url: 'https://example.com/', config, driverMock}); + + // Audit error included the runtimeError + expect(lhr.audits['test-audit'].scoreDisplayMode).toEqual('error'); + expect(lhr.audits['test-audit'].errorMessage).toEqual(expect.stringContaining(NO_FCP.code)); + + // And it bubbled up to the runtimeError. + expect(lhr.runtimeError.code).toEqual(NO_FCP.code); + expect(lhr.runtimeError.message).toBeDisplayString(/Something .*\(NO_FCP\)/); + }); + + it('includes a pageLoadError runtimeError over any gatherer runtimeErrors', async () => { + const url = 'https://www.reddit.com/r/nba'; + let firstLoad = true; + const errorDriverMock = Object.assign({}, driverMock, { + online: true, + // Loads the page successfully in the first pass, fails with PAGE_HUNG in the second. + async gotoURL(url) { + if (url.includes('blank')) return null; + if (firstLoad) { + firstLoad = false; + return url; + } else { + throw new LHError(LHError.errors.PAGE_HUNG); + } + }, + }); + + const config = new Config(configJson); + const {lhr} = await Runner.run(null, {url, config, driverMock: errorDriverMock}); + + // Audit error still includes the gatherer runtimeError. + expect(lhr.audits['test-audit'].scoreDisplayMode).toEqual('error'); + expect(lhr.audits['test-audit'].errorMessage).toEqual(expect.stringContaining(NO_FCP.code)); + + // But top-level runtimeError is the pageLoadError. + expect(lhr.runtimeError.code).toEqual(LHError.errors.PAGE_HUNG.code); + expect(lhr.runtimeError.message).toBeDisplayString(/because the page stopped responding/); + }); }); it('localized errors thrown from driver', async () => { diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 3129406a9b90..a3d4cb8c36c9 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -49,6 +49,8 @@ declare global { URL: {requestedUrl: string, finalUrl: string}; /** The timing instrumentation of the gather portion of a run. */ Timing: Artifacts.MeasureEntry[]; + /** If loading the page failed, value is the error that caused it. Otherwise null. */ + PageLoadError: LighthouseError | null; } /** From 4a2001851556289e830fd5fcaa977441e3dc7d47 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Wed, 19 Jun 2019 17:42:03 -0700 Subject: [PATCH 2/4] better error expectations --- .../test/smokehouse/error-expectations.js | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/lighthouse-cli/test/smokehouse/error-expectations.js b/lighthouse-cli/test/smokehouse/error-expectations.js index c29472aacadd..40ea63c07282 100644 --- a/lighthouse-cli/test/smokehouse/error-expectations.js +++ b/lighthouse-cli/test/smokehouse/error-expectations.js @@ -17,8 +17,13 @@ module.exports = [ runtimeError: {code: 'PAGE_HUNG'}, }, artifacts: { - // Assert undefined because pageLoadError prevented gatherer from running. - ViewportDimensions: undefined, + PageLoadError: {code: 'PAGE_HUNG'}, + devtoolsLogs: { + 'pageLoadError-defaultPass': [/* ... */], + }, + traces: { + 'pageLoadError-defaultPass': {traceEvents: [/* ... */]}, + }, }, }, { @@ -29,8 +34,13 @@ module.exports = [ runtimeError: {code: 'INSECURE_DOCUMENT_REQUEST'}, }, artifacts: { - // Assert undefined because pageLoadError prevented gatherer from running. - ViewportDimensions: undefined, + PageLoadError: {code: 'INSECURE_DOCUMENT_REQUEST'}, + devtoolsLogs: { + 'pageLoadError-defaultPass': [/* ... */], + }, + traces: { + 'pageLoadError-defaultPass': {traceEvents: [/* ... */]}, + }, }, }, ]; From 964c30a27ba6fdc2ab6d2801c0270dfb1728466c Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Wed, 19 Jun 2019 17:49:04 -0700 Subject: [PATCH 3/4] betterer error expectations --- .../test/smokehouse/error-expectations.js | 14 ++++++++++++-- .../test/smokehouse/smokehouse-report.js | 6 ++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/lighthouse-cli/test/smokehouse/error-expectations.js b/lighthouse-cli/test/smokehouse/error-expectations.js index 40ea63c07282..57c3e60cb073 100644 --- a/lighthouse-cli/test/smokehouse/error-expectations.js +++ b/lighthouse-cli/test/smokehouse/error-expectations.js @@ -13,8 +13,13 @@ module.exports = [ lhr: { requestedUrl: 'http://localhost:10200/infinite-loop.html', finalUrl: 'http://localhost:10200/infinite-loop.html', - audits: {}, runtimeError: {code: 'PAGE_HUNG'}, + audits: { + 'first-contentful-paint': { + scoreDisplayMode: 'error', + errorMessage: 'Required traces gatherer did not run.', + }, + }, }, artifacts: { PageLoadError: {code: 'PAGE_HUNG'}, @@ -30,8 +35,13 @@ module.exports = [ lhr: { requestedUrl: 'https://expired.badssl.com', finalUrl: 'https://expired.badssl.com/', - audits: {}, runtimeError: {code: 'INSECURE_DOCUMENT_REQUEST'}, + audits: { + 'first-contentful-paint': { + scoreDisplayMode: 'error', + errorMessage: 'Required traces gatherer did not run.', + }, + }, }, artifacts: { PageLoadError: {code: 'INSECURE_DOCUMENT_REQUEST'}, diff --git a/lighthouse-cli/test/smokehouse/smokehouse-report.js b/lighthouse-cli/test/smokehouse/smokehouse-report.js index dcf7409be62d..5f670a38a394 100644 --- a/lighthouse-cli/test/smokehouse/smokehouse-report.js +++ b/lighthouse-cli/test/smokehouse/smokehouse-report.js @@ -138,13 +138,11 @@ function collateResults(actual, expected) { const artifactNames = /** @type {(keyof LH.Artifacts)[]} */ (Object.keys(expectedArtifacts)); artifactAssertions = artifactNames.map(artifactName => { const actualResult = (actual.artifacts || {})[artifactName]; - const expectedResult = expectedArtifacts[artifactName]; - - // Actual artifact *must* exist unless expected is explicitly set to `undefined`. - if (!actualResult && expectedResult !== undefined) { + if (!actualResult) { throw new Error(`Config run did not generate artifact ${artifactName}`); } + const expectedResult = expectedArtifacts[artifactName]; return makeComparison(artifactName + ' artifact', actualResult, expectedResult); }); } From 72f501d01a3384c8805fa96eca21672cf430e6e8 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Thu, 20 Jun 2019 13:37:20 -0700 Subject: [PATCH 4/4] same as it ever was --- lighthouse-core/gather/gather-runner.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index ad9b29bee722..3e98ca23bb1d 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -584,11 +584,14 @@ class GatherRunner { } } + await GatherRunner.disposeDriver(driver, options); GatherRunner.finalizeBaseArtifacts(baseArtifacts); return /** @type {LH.Artifacts} */ ({...baseArtifacts, ...artifacts}); // Cast to drop Partial<>. - } finally { - // Clean up regardless of error. + } catch (err) { + // Clean up on error. Don't await so that the root error, not a disposal error, is shown. GatherRunner.disposeDriver(driver, options); + + throw err; } }