From 6e10f67ad61c86ced741faa8a664836f196b672b Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Thu, 13 Jun 2019 13:26:32 -0700 Subject: [PATCH 1/4] core(gather-runner): don't save trace on pass with pageLoadError --- lighthouse-core/gather/gather-runner.js | 12 +-- .../test/gather/gather-runner-test.js | 75 +++++++++++++++++++ 2 files changed, 81 insertions(+), 6 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index deb8d4611fe6..5b5777f85329 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -648,11 +648,6 @@ class GatherRunner { // Disable throttling so the afterPass analysis isn't throttled await driver.setThrottling(passContext.settings, {useThrottling: false}); - // Save devtoolsLog and trace. - const baseArtifacts = passContext.baseArtifacts; - baseArtifacts.devtoolsLogs[passConfig.passName] = loadData.devtoolsLog; - 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, possibleNavError); if (pageLoadError) { @@ -661,7 +656,12 @@ class GatherRunner { return GatherRunner.generatePageLoadErrorArtifacts(passContext, pageLoadError); } - // If no error, run `afterPass()` on gatherers and return collected artifacts. + // If no error, save devtoolsLog and trace. + const baseArtifacts = passContext.baseArtifacts; + baseArtifacts.devtoolsLogs[passConfig.passName] = loadData.devtoolsLog; + if (loadData.trace) baseArtifacts.traces[passConfig.passName] = loadData.trace; + + // Run `afterPass()` on gatherers and return collected artifacts. await GatherRunner.afterPass(passContext, loadData, gathererResults); return GatherRunner.collectArtifacts(gathererResults); } diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 234bd7fd776a..55807faa6c3e 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -776,6 +776,81 @@ describe('GatherRunner', function() { }); }); + it('does not return a trace or devtoolsLog when there was a runtime error', async () => { + const requestedUrl = 'https://example.com'; + const passes = [{ + passName: 'firstPass', + recordTrace: true, + gatherers: [{instance: new TestGatherer()}], + }]; + const driver = Object.assign({}, fakeDriver, { + // resolved URL here does not match any request in the network records, causing a runtime error. + gotoURL: async _ => requestedUrl, + online: true, + }); + + const config = new Config({}); + const options = {driver, requestedUrl, settings: config.settings, config}; + const artifacts = await GatherRunner.run(passes, options); + + expect(artifacts.TestGatherer.code).toEqual('NO_DOCUMENT_REQUEST'); + expect(Object.keys(artifacts.traces)).toHaveLength(0); + expect(Object.keys(artifacts.devtoolsLogs)).toHaveLength(0); + }); + + it('does not run additional passes after a runtime error', async () => { + const t1 = new (class Test1 extends TestGatherer {})(); + const t2 = new (class Test2 extends TestGatherer {})(); + const t3 = new (class Test3 extends TestGatherer {})(); + const passes = [{ + passName: 'firstPass', + recordTrace: true, + gatherers: [{instance: t1}], + }, { + passName: 'secondPass', + recordTrace: true, + gatherers: [{instance: t2}], + }, { + passName: 'thirdPass', + recordTrace: true, + gatherers: [{instance: t3}], + }]; + + const requestedUrl = 'https://www.reddit.com/r/nba'; + let firstLoad = true; + const driver = Object.assign({}, fakeDriver, { + // Loads the page successfully in the first pass, fails with NO_FCP in the second. + async gotoURL(url) { + if (url.includes('blank')) return null; + if (firstLoad) { + firstLoad = false; + return requestedUrl; + } else { + throw new LHError(LHError.errors.NO_FCP); + } + }, + online: true, + }); + const config = new Config({}); + const options = {driver, requestedUrl, settings: config.settings, config}; + const artifacts = await GatherRunner.run(passes, options); + + // t1.pass() and t2.pass() called, t3.pass(), after the error, was not. + expect(t1.called).toBe(true); + 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. + expect(artifacts.Test1).toBe('MyArtifact'); + expect(artifacts.Test2).toBeInstanceOf(LHError); + expect(artifacts.Test2.code).toEqual('NO_FCP'); + expect(artifacts.Test3).toBeUndefined(); + + // Only the firstPass has a saved trace and devtoolsLog. + expect(Object.keys(artifacts.traces)).toEqual(['firstPass']); + expect(Object.keys(artifacts.devtoolsLogs)).toEqual(['firstPass']); + }); + describe('#getNetworkError', () => { it('passes when the page is loaded', () => { const url = 'http://the-page.com'; From 50b7511a04d4e6c1b2619c38ef853eabf77ad7ab Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Thu, 13 Jun 2019 14:42:12 -0700 Subject: [PATCH 2/4] update smoke test --- lighthouse-cli/test/smokehouse/seo/expectations.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lighthouse-cli/test/smokehouse/seo/expectations.js b/lighthouse-cli/test/smokehouse/seo/expectations.js index 098d0f0358e6..0d370cd37a1e 100644 --- a/lighthouse-cli/test/smokehouse/seo/expectations.js +++ b/lighthouse-cli/test/smokehouse/seo/expectations.js @@ -216,8 +216,7 @@ module.exports = [ }, audits: { 'http-status-code': { - score: 0, - displayValue: '403', + score: null, }, 'viewport': { score: null, From d25ad04bd7a1c91bceabbd9528b437b7d4368e79 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Mon, 17 Jun 2019 18:07:46 -0700 Subject: [PATCH 3/4] pageLoadError- --- lighthouse-core/gather/gather-runner.js | 17 ++++- .../test/gather/gather-runner-test.js | 66 ++++++++++--------- 2 files changed, 49 insertions(+), 34 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 5b5777f85329..9f3b2b5c5c82 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -622,6 +622,18 @@ class GatherRunner { return !settings.disableStorageReset && passConfig.recordTrace && passConfig.useThrottling; } + /** + * Save the devtoolsLog and trace (if applicable) to baseArtifacts. + * @param {LH.Gatherer.PassContext} passContext + * @param {LH.Gatherer.LoadData} loadData + * @param {string} passName + */ + static _saveLoadData(passContext, loadData, passName) { + const baseArtifacts = passContext.baseArtifacts; + baseArtifacts.devtoolsLogs[passName] = loadData.devtoolsLog; + if (loadData.trace) baseArtifacts.traces[passName] = loadData.trace; + } + /** * Starting from about:blank, load the page and run gatherers for this pass. * @param {LH.Gatherer.PassContext} passContext @@ -653,13 +665,12 @@ class GatherRunner { if (pageLoadError) { log.error('GatherRunner', pageLoadError.friendlyMessage, passContext.url); passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage); + GatherRunner._saveLoadData(passContext, loadData, `pageLoadError-${passConfig.passName}`); return GatherRunner.generatePageLoadErrorArtifacts(passContext, pageLoadError); } // If no error, save devtoolsLog and trace. - const baseArtifacts = passContext.baseArtifacts; - baseArtifacts.devtoolsLogs[passConfig.passName] = loadData.devtoolsLog; - if (loadData.trace) baseArtifacts.traces[passConfig.passName] = loadData.trace; + GatherRunner._saveLoadData(passContext, loadData, passConfig.passName); // Run `afterPass()` on gatherers and return collected artifacts. await GatherRunner.afterPass(passContext, loadData, gathererResults); diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 55807faa6c3e..e78eeb5afa9e 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -776,45 +776,50 @@ describe('GatherRunner', function() { }); }); - it('does not return a trace or devtoolsLog when there was a runtime error', async () => { + it('saves trace and devtoolsLog with error prefix when there was a runtime error', async () => { const requestedUrl = 'https://example.com'; - const passes = [{ - passName: 'firstPass', - recordTrace: true, - gatherers: [{instance: new TestGatherer()}], - }]; const driver = Object.assign({}, fakeDriver, { // resolved URL here does not match any request in the network records, causing a runtime error. gotoURL: async _ => requestedUrl, online: true, }); - const config = new Config({}); - const options = {driver, requestedUrl, settings: config.settings, config}; - const artifacts = await GatherRunner.run(passes, options); + const config = new Config({ + passes: [{ + passName: 'firstPass', + recordTrace: true, + gatherers: [{instance: new TestGatherer()}], + }], + }); + const options = {driver, requestedUrl, settings: config.settings}; + const artifacts = await GatherRunner.run(config.passes, options); expect(artifacts.TestGatherer.code).toEqual('NO_DOCUMENT_REQUEST'); - expect(Object.keys(artifacts.traces)).toHaveLength(0); - expect(Object.keys(artifacts.devtoolsLogs)).toHaveLength(0); + + // The only loadData available should be prefixed with `pageLoadError-`. + expect(Object.keys(artifacts.traces)).toEqual(['pageLoadError-firstPass']); + expect(Object.keys(artifacts.devtoolsLogs)).toEqual(['pageLoadError-firstPass']); }); it('does not run additional passes after a runtime error', async () => { const t1 = new (class Test1 extends TestGatherer {})(); const t2 = new (class Test2 extends TestGatherer {})(); const t3 = new (class Test3 extends TestGatherer {})(); - const passes = [{ - passName: 'firstPass', - recordTrace: true, - gatherers: [{instance: t1}], - }, { - passName: 'secondPass', - recordTrace: true, - gatherers: [{instance: t2}], - }, { - passName: 'thirdPass', - recordTrace: true, - gatherers: [{instance: t3}], - }]; + const config = new Config({ + passes: [{ + passName: 'firstPass', + recordTrace: true, + gatherers: [{instance: t1}], + }, { + passName: 'secondPass', + recordTrace: true, + gatherers: [{instance: t2}], + }, { + passName: 'thirdPass', + recordTrace: true, + gatherers: [{instance: t3}], + }], + }); const requestedUrl = 'https://www.reddit.com/r/nba'; let firstLoad = true; @@ -831,11 +836,10 @@ describe('GatherRunner', function() { }, online: true, }); - const config = new Config({}); - const options = {driver, requestedUrl, settings: config.settings, config}; - const artifacts = await GatherRunner.run(passes, options); + const options = {driver, requestedUrl, settings: config.settings}; + const artifacts = await GatherRunner.run(config.passes, options); - // t1.pass() and t2.pass() called, t3.pass(), after the error, was not. + // t1.pass() and t2.pass() called; t3.pass(), after the error, was not. expect(t1.called).toBe(true); expect(t2.called).toBe(true); expect(t3.called).toBe(false); @@ -846,9 +850,9 @@ describe('GatherRunner', function() { expect(artifacts.Test2.code).toEqual('NO_FCP'); expect(artifacts.Test3).toBeUndefined(); - // Only the firstPass has a saved trace and devtoolsLog. - expect(Object.keys(artifacts.traces)).toEqual(['firstPass']); - expect(Object.keys(artifacts.devtoolsLogs)).toEqual(['firstPass']); + // 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']); }); describe('#getNetworkError', () => { From 645f42733406a88e44e7d00452aaa97469a96225 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Tue, 18 Jun 2019 16:11:51 -0700 Subject: [PATCH 4/4] feedback --- lighthouse-core/gather/gather-runner.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 9f3b2b5c5c82..36216a0740b9 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -628,7 +628,7 @@ class GatherRunner { * @param {LH.Gatherer.LoadData} loadData * @param {string} passName */ - static _saveLoadData(passContext, loadData, passName) { + static _addLoadDataToBaseArtifacts(passContext, loadData, passName) { const baseArtifacts = passContext.baseArtifacts; baseArtifacts.devtoolsLogs[passName] = loadData.devtoolsLog; if (loadData.trace) baseArtifacts.traces[passName] = loadData.trace; @@ -665,12 +665,13 @@ class GatherRunner { if (pageLoadError) { log.error('GatherRunner', pageLoadError.friendlyMessage, passContext.url); passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage); - GatherRunner._saveLoadData(passContext, loadData, `pageLoadError-${passConfig.passName}`); + GatherRunner._addLoadDataToBaseArtifacts(passContext, loadData, + `pageLoadError-${passConfig.passName}`); return GatherRunner.generatePageLoadErrorArtifacts(passContext, pageLoadError); } // If no error, save devtoolsLog and trace. - GatherRunner._saveLoadData(passContext, loadData, passConfig.passName); + GatherRunner._addLoadDataToBaseArtifacts(passContext, loadData, passConfig.passName); // Run `afterPass()` on gatherers and return collected artifacts. await GatherRunner.afterPass(passContext, loadData, gathererResults);