Repository: GoogleChrome/lighthouse
Pull request #9290 (2019-06-27 01:36:33)

misc(runner): add assertion for devtoolsLog as requiredArtifact
when we're checking required artifacts before running an audit, we assert that the default trace exists if the audit needs a trace, but we only assert that the top-level `devtoolsLogs` artifact exists (it always does since it's a base artifact), not if any logs on it exist (they might not if there was a pageLoadError).

As a result, while nearly everyone in an error report will have `"Required {Artifact} gatherer did not run."` error string, a few audits that rely only on the devtoolsLog will have the lovely `"Cannot read property 'forEach' of undefined"` error string instead.

This adds a check for devtoolsLog just like the trace check.


Patch:
diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js
index 2a857035e..f506b5f5c 100644
--- a/lighthouse-core/config/config.js
+++ b/lighthouse-core/config/config.js
@@ -22,6 +22,28 @@ const {requireAudits, mergeOptionsOfItems, resolveModule} = require('./config-he
 /** @typedef {typeof import('../gather/gatherers/gatherer.js')} GathererConstructor */
 /** @typedef {InstanceType<GathererConstructor>} Gatherer */
 
+/**
+ * Define with object literal so that tsc will require it to stay updated.
+ * @type {Record<keyof LH.BaseArtifacts, ''>}
+ */
+const BASE_ARTIFACT_BLANKS = {
+  fetchTime: '',
+  LighthouseRunWarnings: '',
+  TestedAsMobileDevice: '',
+  HostUserAgent: '',
+  NetworkUserAgent: '',
+  BenchmarkIndex: '',
+  WebAppManifest: '',
+  Stacks: '',
+  traces: '',
+  devtoolsLogs: '',
+  settings: '',
+  URL: '',
+  Timing: '',
+  PageLoadError: '',
+};
+const BASE_ARTIFACT_NAMES = Object.keys(BASE_ARTIFACT_BLANKS);
+
 /**
  * @param {Config['passes']} passes
  * @param {Config['audits']} audits
@@ -32,11 +54,14 @@ function assertValidPasses(passes, audits) {
   }
 
   const requiredGatherers = Config.getGatherersNeededByAudits(audits);
+  // Base artifacts are provided by GatherRunner, so start foundGatherers with them.
+  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 => {
     pass.gatherers.forEach(gathererDefn => {
       const gatherer = gathererDefn.instance;
+      foundGatherers.add(gatherer.name);
       const isGatherRequiredByAudits = requiredGatherers.has(gatherer.name);
       if (!isGatherRequiredByAudits) {
         const msg = `${gatherer.name} gatherer requested, however no audit requires it.`;
@@ -45,6 +70,17 @@ function assertValidPasses(passes, audits) {
     });
   });
 
+  // All required gatherers must be found in the config. Throw otherwise.
+  for (const auditDefn of audits || []) {
+    const auditMeta = auditDefn.implementation.meta;
+    for (const requiredArtifact of auditMeta.requiredArtifacts) {
+      if (!foundGatherers.has(requiredArtifact)) {
+        throw new Error(`${requiredArtifact} gatherer, required by audit ${auditMeta.id}, ` +
+            'was not found in config.');
+      }
+    }
+  }
+
   // Passes must have unique `passName`s. Throw otherwise.
   const usedNames = new Set();
   passes.forEach(pass => {
diff --git a/lighthouse-core/lib/i18n/en-US.json b/lighthouse-core/lib/i18n/en-US.json
index 95ed40853..b0dd8ba42 100644
--- a/lighthouse-core/lib/i18n/en-US.json
+++ b/lighthouse-core/lib/i18n/en-US.json
@@ -1671,10 +1671,18 @@
     "message": "DNS servers could not resolve the provided domain.",
     "description": "Error message explaining that the requested page could not be resolved by the DNS server."
   },
+  "lighthouse-core/lib/lh-error.js | erroredRequiredArtifact": {
+    "message": "Required {artifactName} gatherer encountered an error: {errorMessage}",
+    "description": "Error message explaning that there was an error while trying to collect a resource that was required for testing. \"artifactName\" will be replaced with the name of the resource that wasn't collected; \"errorMessage\" will be replaced with a string description of the error that occurred."
+  },
   "lighthouse-core/lib/lh-error.js | internalChromeError": {
     "message": "An internal Chrome error occurred. Please restart Chrome and try re-running Lighthouse.",
     "description": "Error message explaining that Chrome has encountered an error during the Lighthouse run, and that Chrome should be restarted."
   },
+  "lighthouse-core/lib/lh-error.js | missingRequiredArtifact": {
+    "message": "Required {artifactName} gatherer did not run.",
+    "description": "Error message explaning that a resource that was required for testing was never collected. \"artifactName\" will be replaced with the name of the resource that wasn't collected."
+  },
   "lighthouse-core/lib/lh-error.js | pageLoadFailed": {
     "message": "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.",
     "description": "Error message explaining that Lighthouse could not load the requested URL and the steps that might be taken to fix the unreliability."
diff --git a/lighthouse-core/lib/lh-error.js b/lighthouse-core/lib/lh-error.js
index cf2c5be0d..63804c1ac 100644
--- a/lighthouse-core/lib/lh-error.js
+++ b/lighthouse-core/lib/lh-error.js
@@ -39,6 +39,10 @@ const UIStrings = {
   pageLoadFailedHung: 'Lighthouse was unable to reliably load the URL you requested because the page stopped responding.',
   /** Error message explaining that Lighthouse timed out while waiting for the initial connection to the Chrome Devtools protocol. */
   criTimeout: 'Timeout waiting for initial Debugger Protocol connection.',
+  /** Error message explaning that a resource that was required for testing was never collected. "artifactName" will be replaced with the name of the resource that wasn't collected. */
+  missingRequiredArtifact: 'Required {artifactName} gatherer did not run.',
+  /** Error message explaning that there was an error while trying to collect a resource that was required for testing. "artifactName" will be replaced with the name of the resource that wasn't collected; "errorMessage" will be replaced with a string description of the error that occurred. */
+  erroredRequiredArtifact: 'Required {artifactName} gatherer encountered an error: {errorMessage}',
 };
 
 const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);
@@ -245,6 +249,24 @@ const ERRORS = {
     lhrRuntimeError: true,
   },
 
+  /**
+   * Error internal to Runner used when an artifact required for an audit is missing.
+   * Requires an additional `artifactName` field for translation.
+  */
+  MISSING_REQUIRED_ARTIFACT: {
+    code: 'MISSING_REQUIRED_ARTIFACT',
+    message: UIStrings.missingRequiredArtifact,
+  },
+
+  /**
+   * Error internal to Runner used when an artifact required for an audit was an error.
+   * Requires additional `artifactName` and `errorMessage` fields for translation.
+  */
+  ERRORED_REQUIRED_ARTIFACT: {
+    code: 'ERRORED_REQUIRED_ARTIFACT',
+    message: UIStrings.erroredRequiredArtifact,
+  },
+
   // Hey! When adding a new error type, update lighthouse-result.proto too.
 };
 
diff --git a/lighthouse-core/runner.js b/lighthouse-core/runner.js
index a798eda2e..689ed6799 100644
--- a/lighthouse-core/runner.js
+++ b/lighthouse-core/runner.js
@@ -289,14 +289,16 @@ class Runner {
       for (const artifactName of audit.meta.requiredArtifacts) {
         const noArtifact = artifacts[artifactName] === undefined;
 
-        // If trace required, check that DEFAULT_PASS trace exists.
-        // TODO: need pass-specific check of networkRecords and traces.
-        const noTrace = artifactName === 'traces' && !artifacts.traces[Audit.DEFAULT_PASS];
+        // If trace/devtoolsLog required, check that DEFAULT_PASS trace/devtoolsLog exists.
+        // NOTE: for now, not a pass-specific check of traces or devtoolsLogs.
+        const noRequiredTrace = artifactName === 'traces' && !artifacts.traces[Audit.DEFAULT_PASS];
+        const noRequiredDevtoolsLog = artifactName === 'devtoolsLogs' &&
+            !artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
 
-        if (noArtifact || noTrace) {
+        if (noArtifact || noRequiredTrace || noRequiredDevtoolsLog) {
           log.warn('Runner',
               `${artifactName} gatherer, required by audit ${audit.meta.id}, did not run.`);
-          throw new Error(`Required ${artifactName} gatherer did not run.`);
+          throw new LHError(LHError.errors.MISSING_REQUIRED_ARTIFACT, {artifactName});
         }
 
         // If artifact was an error, output error result on behalf of audit.
@@ -314,8 +316,8 @@ class Runner {
             ` encountered an error: ${artifactError.message}`);
 
           // Create a friendlier display error and mark it as expected to avoid duplicates in Sentry
-          const error = new Error(
-              `Required ${artifactName} gatherer encountered an error: ${artifactError.message}`);
+          const error = new LHError(LHError.errors.ERRORED_REQUIRED_ARTIFACT,
+              {artifactName, errorMessage: artifactError.message});
           // @ts-ignore Non-standard property added to Error
           error.expected = true;
           throw error;
@@ -340,7 +342,10 @@ class Runner {
       const product = await audit.audit(requiredArtifacts, auditContext);
       auditResult = Audit.generateAuditResult(audit, product);
     } catch (err) {
-      log.warn(audit.meta.id, `Caught exception: ${err.message}`);
+      // Log error if it hasn't already been logged above.
+      if (err.code !== 'MISSING_REQUIRED_ARTIFACT' && err.code !== 'ERRORED_REQUIRED_ARTIFACT') {
+        log.warn(audit.meta.id, `Caught exception: ${err.message}`);
+      }
 
       Sentry.captureException(err, {tags: {audit: audit.meta.id}, level: 'error'});
       // Errors become error audit result.
diff --git a/package.json b/package.json
index 556bc7f4c..8a7effe65 100644
--- a/package.json
+++ b/package.json
@@ -61,7 +61,8 @@
     "mixed-content": "./lighthouse-cli/index.js --chrome-flags='--headless' --preset=mixed-content",
     "minify-latest-run": "./lighthouse-core/scripts/lantern/minify-trace.js ./latest-run/defaultPass.trace.json ./latest-run/defaultPass.trace.min.json && ./lighthouse-core/scripts/lantern/minify-devtoolslog.js ./latest-run/defaultPass.devtoolslog.json ./latest-run/defaultPass.devtoolslog.min.json",
     "compile-proto": "protoc --python_out=./ ./proto/lighthouse-result.proto && mv ./proto/*_pb2.py ./proto/scripts || (echo \"❌ Install protobuf ≥ 3.7.1 to compile the proto file.\" && false)",
-    "build-proto-roundtrip": "cd proto/scripts && PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=cpp PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION_VERSION=2 python json_roundtrip_via_proto.py"
+    "build-proto-roundtrip": "cd proto/scripts && PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=cpp PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION_VERSION=2 python json_roundtrip_via_proto.py",
+    "static-server": "node lighthouse-cli/test/fixtures/static-server.js"
   },
   "devDependencies": {
     "@firebase/app-types": "0.3.1",


Test patch:
diff --git a/lighthouse-core/test/config/config-test.js b/lighthouse-core/test/config/config-test.js
index ee4772668..c3700008f 100644
--- a/lighthouse-core/test/config/config-test.js
+++ b/lighthouse-core/test/config/config-test.js
@@ -30,7 +30,7 @@ describe('Config', () => {
     assert.notEqual(config, newConfig);
   });
 
-  it('doesn\'t change directly injected plugins', () => {
+  it('doesn\'t change directly injected gatherer implementations', () => {
     class MyGatherer extends Gatherer {}
     class MyAudit extends Audit {
       static get meta() {
@@ -45,7 +45,7 @@ describe('Config', () => {
       static audit() {}
     }
     const config = {
-      // Extend to default to double test our ability to handle plugins
+      // Extend default to double test our ability to handle injection.
       extends: 'lighthouse:default',
       settings: {onlyAudits: ['my-audit']},
       passes: [{
@@ -58,7 +58,7 @@ describe('Config', () => {
     assert.equal(MyAudit, newConfig.audits[0].implementation);
   });
 
-  it('doesn\'t change directly injected plugin instances', () => {
+  it('doesn\'t change directly injected gatherer instances', () => {
     class MyGatherer extends Gatherer {
       constructor(secretVal) {
         super();
@@ -120,6 +120,56 @@ describe('Config', () => {
     );
   });
 
+  it('throws when an audit requires an artifact with no gatherer supplying it', async () => {
+    class NeedsWhatYouCantGive extends Audit {
+      static get meta() {
+        return {
+          id: 'missing-artifact-audit',
+          title: 'none',
+          description: 'none',
+          requiredArtifacts: [
+            // Require fake artifact amidst base artifact and default artifacts.
+            'URL',
+            'ConsoleMessages',
+            'VRMLElements', // not a real gatherer
+            'ViewportDimensions',
+          ],
+        };
+      }
+
+      static audit() {}
+    }
+
+    expect(() => new Config({
+      extends: 'lighthouse:default',
+      audits: [NeedsWhatYouCantGive],
+    // eslint-disable-next-line max-len
+    })).toThrow('VRMLElements gatherer, required by audit missing-artifact-audit, was not found in config');
+  });
+
+  it('does not throw when an audit requires only base artifacts', () => {
+    class BaseArtifactsAudit extends Audit {
+      static get meta() {
+        return {
+          id: 'base-artifacts-audit',
+          title: 'base',
+          description: 'base',
+          requiredArtifacts: ['HostUserAgent', 'URL', 'Stacks', 'WebAppManifest'],
+        };
+      }
+
+      static audit() {}
+    }
+
+    const config = new Config({
+      extends: 'lighthouse:default',
+      audits: [BaseArtifactsAudit],
+    }, {onlyAudits: ['base-artifacts-audit']});
+
+    assert.strictEqual(config.audits.length, 1);
+    assert.strictEqual(config.audits[0].implementation.meta.id, 'base-artifacts-audit');
+  });
+
   it('throws for unknown gatherers', () => {
     const config = {
       passes: [{
diff --git a/lighthouse-core/test/runner-test.js b/lighthouse-core/test/runner-test.js
index e94e8e6ee..bb9b38c6c 100644
--- a/lighthouse-core/test/runner-test.js
+++ b/lighthouse-core/test/runner-test.js
@@ -293,6 +293,24 @@ describe('Runner', () => {
       });
     });
 
+    it('outputs an error audit result when devtoolsLog required but not provided', async () => {
+      const config = new Config({
+        settings: {
+          auditMode: __dirname + '/fixtures/artifacts/empty-artifacts/',
+        },
+        audits: [
+          // requires devtoolsLogs[Audit.DEFAULT_PASS]
+          'is-on-https',
+        ],
+      });
+
+      const results = await Runner.run({}, {config});
+      const auditResult = results.lhr.audits['is-on-https'];
+      assert.strictEqual(auditResult.score, null);
+      assert.strictEqual(auditResult.scoreDisplayMode, 'error');
+      assert.strictEqual(auditResult.errorMessage, 'Required devtoolsLogs gatherer did not run.');
+    });
+
     it('outputs an error audit result when missing a required artifact', () => {
       const config = new Config({
         settings: {