core(gather-runner): don't save trace on pass with pageLoadError (#9198)

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,

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 _addLoadDataToBaseArtifacts(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
@@ -648,20 +660,20 @@ 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) {
       log.error('GatherRunner', pageLoadError.friendlyMessage, passContext.url);
       passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage);
+      GatherRunner._addLoadDataToBaseArtifacts(passContext, loadData,
+          `pageLoadError-${passConfig.passName}`);
       return GatherRunner.generatePageLoadErrorArtifacts(passContext, pageLoadError);
     }
 
-    // If no error, run `afterPass()` on gatherers and return collected artifacts.
+    // If no error, save devtoolsLog and trace.
+    GatherRunner._addLoadDataToBaseArtifacts(passContext, loadData, passConfig.passName);
+
+    // Run `afterPass()` on gatherers and return collected artifacts.
     await GatherRunner.afterPass(passContext, loadData, gathererResults);
     return GatherRunner.collectArtifacts(gathererResults);
   }

lighthouse-core/test/gather/gather-runner-test.js

@@ -788,6 +788,85 @@ describe('GatherRunner', function() {
       });
   });
 
+  it('saves trace and devtoolsLog with error prefix when there was a runtime error', async () => {
+    const requestedUrl = 'https://example.com';
+    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({
+      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');
+
+    // 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 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;
+    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 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.
+    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();
+
+    // 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', () => {
     it('passes when the page is loaded', () => {
       const url = 'http://the-page.com';