Repository: GoogleChrome/lighthouse
Pull request #9987 (2019-11-19 17:38:29)

core(config): add loadFailureMode warn
**Summary**
Instead of using the `driver.offline` as a signal if we should ignore load failures for a pass, this introduces `loadFailureMode` as a pass configuration that explicitly controls how failures to load should be handled.

BREAKING CHANGE: A pass that goes offline and wants to have load failures ignored will need to set `loadFailureMode: 'ignore'` in its pass config.

**Related Issues/PRs**
fixes https://github.com/GoogleChrome/lighthouse/issues/1217
ref https://github.com/GoogleChrome/lighthouse-ci/issues/105

paulirish: sg. two snapshot failures now btw

![image](https://user-images.githubusercontent.com/39191/70675144-3110f880-1c3d-11ea-92ee-06b85bff8998.png)


Patch:
diff --git a/docs/readme.md b/docs/readme.md
index 7df910663..94808894e 100644
--- a/docs/readme.md
+++ b/docs/readme.md
@@ -15,7 +15,7 @@ function launchChromeAndRunLighthouse(url, opts, config = null) {
   return chromeLauncher.launch({chromeFlags: opts.chromeFlags}).then(chrome => {
     opts.port = chrome.port;
     return lighthouse(url, opts, config).then(results => {
-      // use results.lhr for the JS-consumeable output
+      // use results.lhr for the JS-consumable output
       // https://github.com/GoogleChrome/lighthouse/blob/master/types/lhr.d.ts
       // use results.report for the HTML/JSON/CSV output as a string
       // use results.artifacts for the trace/screenshots/other specific case you need (rarer)
diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js
index c90743fee..6786ea331 100644
--- a/lighthouse-core/config/config.js
+++ b/lighthouse-core/config/config.js
@@ -58,7 +58,13 @@ function assertValidPasses(passes, audits) {
   const foundGatherers = new Set(BASE_ARTIFACT_NAMES);
 
   // Log if we are running gathers that are not needed by the audits listed in the config
-  passes.forEach(pass => {
+  passes.forEach((pass, passIndex) => {
+    if (passIndex === 0 && pass.loadFailureMode !== 'fatal') {
+      log.warn(`"${pass.passName}" is the first pass but was marked as non-fatal. ` +
+        `The first pass will always be treated as loadFailureMode=fatal.`);
+      pass.loadFailureMode = 'fatal';
+    }
+
     pass.gatherers.forEach(gathererDefn => {
       const gatherer = gathererDefn.instance;
       foundGatherers.add(gatherer.name);
diff --git a/lighthouse-core/config/constants.js b/lighthouse-core/config/constants.js
index 578342747..95e41a9fc 100644
--- a/lighthouse-core/config/constants.js
+++ b/lighthouse-core/config/constants.js
@@ -70,6 +70,7 @@ const defaultSettings = {
 /** @type {LH.Config.Pass} */
 const defaultPassConfig = {
   passName: 'defaultPass',
+  loadFailureMode: 'fatal',
   recordTrace: false,
   useThrottling: false,
   pauseAfterLoadMs: 0,
diff --git a/lighthouse-core/config/default-config.js b/lighthouse-core/config/default-config.js
index cc909cad4..c479d2bff 100644
--- a/lighthouse-core/config/default-config.js
+++ b/lighthouse-core/config/default-config.js
@@ -154,6 +154,7 @@ const defaultConfig = {
   },
   {
     passName: 'offlinePass',
+    loadFailureMode: 'ignore',
     gatherers: [
       'service-worker',
       'offline',
@@ -162,6 +163,7 @@ const defaultConfig = {
   },
   {
     passName: 'redirectPass',
+    loadFailureMode: 'warn',
     // Speed up the redirect pass by blocking stylesheets, fonts, and images
     blockedUrlPatterns: ['*.css', '*.jpg', '*.jpeg', '*.png', '*.gif', '*.svg', '*.ttf', '*.woff', '*.woff2'],
     gatherers: [
diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js
index 8e094289a..f33140281 100644
--- a/lighthouse-core/gather/gather-runner.js
+++ b/lighthouse-core/gather/gather-runner.js
@@ -211,8 +211,9 @@ class GatherRunner {
     const networkError = GatherRunner.getNetworkError(mainRecord);
     const interstitialError = GatherRunner.getInterstitialError(mainRecord, networkRecords);
 
-    // If the driver was offline, the load will fail without offline support. Ignore this case.
-    if (!passContext.driver.online) return;
+    // Check to see if we need to ignore the page load failure.
+    // e.g. When the driver is offline, the load will fail without page offline support.
+    if (passContext.passConfig.loadFailureMode === 'ignore') return;
 
     // We want to special-case the interstitial beyond FAILED_DOCUMENT_REQUEST. See https://github.com/GoogleChrome/lighthouse/pull/8865#issuecomment-497507618
     if (interstitialError) return interstitialError;
@@ -576,7 +577,7 @@ 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) {
+        if (passResults.pageLoadError && passConfig.loadFailureMode === 'fatal') {
           baseArtifacts.PageLoadError = passResults.pageLoadError;
           break;
         }
diff --git a/types/config.d.ts b/types/config.d.ts
index 1d142142c..a5e4d9b98 100644
--- a/types/config.d.ts
+++ b/types/config.d.ts
@@ -25,6 +25,7 @@ declare global {
 
       export interface PassJson {
         passName: string;
+        loadFailureMode?: 'fatal'|'warn'|'ignore';
         recordTrace?: boolean;
         useThrottling?: boolean;
         pauseAfterLoadMs?: number;


Test patch:
diff --git a/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap b/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
index 087a030dc..41d7d171c 100644
--- a/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
+++ b/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
@@ -1255,6 +1255,7 @@ Object {
           "path": "accessibility",
         },
       ],
+      "loadFailureMode": "fatal",
       "networkQuietThresholdMs": 1000,
       "passName": "defaultPass",
       "pauseAfterLoadMs": 1000,
@@ -1276,6 +1277,7 @@ Object {
           "path": "start-url",
         },
       ],
+      "loadFailureMode": "ignore",
       "networkQuietThresholdMs": 0,
       "passName": "offlinePass",
       "pauseAfterLoadMs": 0,
@@ -1304,6 +1306,7 @@ Object {
           "path": "html-without-javascript",
         },
       ],
+      "loadFailureMode": "warn",
       "networkQuietThresholdMs": 0,
       "passName": "redirectPass",
       "pauseAfterLoadMs": 0,
@@ -1439,6 +1442,7 @@ Object {
       "blockedUrlPatterns": Array [],
       "cpuQuietThresholdMs": 1000,
       "gatherers": Array [],
+      "loadFailureMode": "fatal",
       "networkQuietThresholdMs": 1000,
       "passName": "defaultPass",
       "pauseAfterLoadMs": 1000,
diff --git a/lighthouse-core/test/config/config-test.js b/lighthouse-core/test/config/config-test.js
index 3cd86c552..ca9c7a352 100644
--- a/lighthouse-core/test/config/config-test.js
+++ b/lighthouse-core/test/config/config-test.js
@@ -502,6 +502,26 @@ describe('Config', () => {
     assert.equal(config.passes[0].recordTrace, false, 'turns off tracing if not needed');
   });
 
+  it('forces the first pass to have a fatal loadFailureMode', () => {
+    const warnings = [];
+    const saveWarning = evt => warnings.push(evt);
+    log.events.addListener('warning', saveWarning);
+    const config = new Config({
+      extends: true,
+      settings: {
+        onlyCategories: ['performance', 'pwa'],
+      },
+      passes: [
+        {passName: 'defaultPass', loadFailureMode: 'warn'},
+      ],
+    });
+
+    log.events.removeListener('warning', saveWarning);
+    expect(warnings).toHaveLength(1);
+    expect(warnings[0][0]).toMatch(/loadFailureMode.*fatal/);
+    expect(config.passes[0]).toHaveProperty('loadFailureMode', 'fatal');
+  });
+
   it('filters works with extension', () => {
     const config = new Config({
       extends: true,
diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js
index 66095f8b1..74e0ea855 100644
--- a/lighthouse-core/test/gather/gather-runner-test.js
+++ b/lighthouse-core/test/gather/gather-runner-test.js
@@ -447,6 +447,7 @@ describe('GatherRunner', function() {
     };
     const passConfig = {
       passName: 'default',
+      loadFailureMode: 'ignore',
       recordTrace: true,
       useThrottling: true,
       gatherers: [],
@@ -531,6 +532,44 @@ describe('GatherRunner', function() {
     expect(artifacts.TestGatherer).toBeUndefined();
   });
 
+  it('succeeds when there is a navigation error but loadFailureMode was warn', async () => {
+    const requestedUrl = 'https://example.com';
+    // NO_FCP should be ignored because it's a warn pass.
+    const navigationError = new LHError(LHError.errors.NO_FCP);
+
+    const gotoUrlForAboutBlank = jest.fn().mockResolvedValue(null);
+    const gotoUrlForRealUrl = jest.fn()
+      .mockResolvedValueOnce(null)
+      .mockRejectedValueOnce(navigationError);
+    const driver = Object.assign({}, fakeDriver, {
+      online: true,
+      gotoURL: url => url.includes('blank') ? gotoUrlForAboutBlank() : gotoUrlForRealUrl(),
+      endDevtoolsLog() {
+        return networkRecordsToDevtoolsLog([{url: requestedUrl}]);
+      },
+    });
+
+    const config = new Config({
+      passes: [{passName: 'defaultPass', recordTrace: true}, {
+        loadFailureMode: 'warn',
+        recordTrace: true,
+        passName: 'nextPass',
+        gatherers: [{instance: new TestGatherer()}],
+      }],
+    });
+    const options = {
+      driver,
+      requestedUrl,
+      settings: config.settings,
+    };
+
+    const artifacts = await GatherRunner.run(config.passes, options);
+    expect(artifacts.LighthouseRunWarnings).toHaveLength(1);
+    expect(artifacts.PageLoadError).toEqual(null);
+    expect(artifacts.TestGatherer).toBeUndefined();
+    expect(artifacts.devtoolsLogs).toHaveProperty('pageLoadError-nextPass');
+  });
+
   it('does not clear origin storage with flag --disable-storage-reset', () => {
     const asyncFunc = () => Promise.resolve();
     const tests = {
@@ -1069,7 +1108,7 @@ describe('GatherRunner', function() {
     });
 
     it('passes when the page is loaded', () => {
-      const passContext = {url: 'http://the-page.com', driver: {online: true}};
+      const passContext = {url: 'http://the-page.com', passConfig: {loadFailureMode: 'fatal'}};
       const mainRecord = new NetworkRequest();
       const loadData = {networkRecords: [mainRecord]};
       mainRecord.url = passContext.url;
@@ -1080,15 +1119,15 @@ describe('GatherRunner', function() {
     it('passes when the page is loaded, ignoring any fragment', () => {
       const url = 'http://example.com/#/page/list';
       const mainRecord = new NetworkRequest();
-      const passContext = {url, driver: {online: true}};
+      const passContext = {url, passConfig: {loadFailureMode: 'fatal'}};
       const loadData = {networkRecords: [mainRecord]};
       mainRecord.url = 'http://example.com';
       const error = GatherRunner.getPageLoadError(passContext, loadData, undefined);
       expect(error).toBeUndefined();
     });
 
-    it('passes when the page is offline', () => {
-      const passContext = {url: 'http://the-page.com', driver: {online: false}};
+    it('passes when the page is expected to fail', () => {
+      const passContext = {url: 'http://the-page.com', passConfig: {loadFailureMode: 'ignore'}};
       const mainRecord = new NetworkRequest();
       const loadData = {networkRecords: [mainRecord]};
       mainRecord.url = passContext.url;
@@ -1099,7 +1138,7 @@ describe('GatherRunner', function() {
     });
 
     it('fails with interstitial error first', () => {
-      const passContext = {url: 'http://the-page.com', driver: {online: true}};
+      const passContext = {url: 'http://the-page.com', passConfig: {loadFailureMode: 'fatal'}};
       const mainRecord = new NetworkRequest();
       const interstitialRecord = new NetworkRequest();
       const loadData = {networkRecords: [mainRecord, interstitialRecord]};
@@ -1114,7 +1153,7 @@ describe('GatherRunner', function() {
     });
 
     it('fails with network error next', () => {
-      const passContext = {url: 'http://the-page.com', driver: {online: true}};
+      const passContext = {url: 'http://the-page.com', passConfig: {loadFailureMode: 'fatal'}};
       const mainRecord = new NetworkRequest();
       const loadData = {networkRecords: [mainRecord]};
 
@@ -1126,7 +1165,18 @@ describe('GatherRunner', function() {
     });
 
     it('fails with nav error last', () => {
-      const passContext = {url: 'http://the-page.com', driver: {online: true}};
+      const passContext = {url: 'http://the-page.com', passConfig: {loadFailureMode: 'fatal'}};
+      const mainRecord = new NetworkRequest();
+      const loadData = {networkRecords: [mainRecord]};
+
+      mainRecord.url = passContext.url;
+
+      const error = GatherRunner.getPageLoadError(passContext, loadData, navigationError);
+      expect(error.message).toEqual('NAVIGATION_ERROR');
+    });
+
+    it('fails when loadFailureMode is warn', () => {
+      const passContext = {url: 'http://the-page.com', passConfig: {loadFailureMode: 'warn'}};
       const mainRecord = new NetworkRequest();
       const loadData = {networkRecords: [mainRecord]};