add explicit result error property for erroneous audit results (#1591)

GoogleChrome · Feb 3, 2017 · a067d30 · a067d30
1 parent 82858de
commit a067d30
Show file tree

Hide file tree

Showing 9 changed files with 157 additions and 16 deletions.
diff --git a/lighthouse-cli/printer.ts b/lighthouse-cli/printer.ts
@@ -110,7 +110,9 @@ function createOutput(results: Results, outputMode: OutputMode): string {
         if (auditResult.comingSoon === true)
           return;
 
-        let lineItem = ` ${log.doubleLightHorizontal} ${formatAggregationResultItem(auditResult.score)} ${auditResult.description}`;
+        const formattedScore = auditResult.error ? `${log.redify('‽')}` :
+            `${formatAggregationResultItem(auditResult.score)}`;
+        let lineItem = ` ${log.doubleLightHorizontal} ${formattedScore} ${auditResult.description}`;
         if (auditResult.displayValue) {
           lineItem += ` (${log.bold}${auditResult.displayValue}${log.reset})`;
         }

diff --git a/lighthouse-cli/types/types.ts b/lighthouse-cli/types/types.ts
@@ -3,6 +3,7 @@ interface AuditResult {
   debugString: string;
   comingSoon?: boolean;
   score: number;
+  error?: boolean;
   description: string;
   name: string;
   category: string;

diff --git a/lighthouse-core/aggregator/aggregate.js b/lighthouse-core/aggregator/aggregate.js
@@ -77,6 +77,12 @@ class Aggregate {
       throw new Error(msg);
     }
 
+    // Audit resulted in an error, so doesn't contribute to score.
+    // TODO: could do NaN instead, as score becomes somewhat meaningless.
+    if (result.error) {
+      return 0;
+    }
+
     if (typeof result === 'undefined' ||
         typeof result.score === 'undefined') {
       let msg =

diff --git a/lighthouse-core/audits/audit.js b/lighthouse-core/audits/audit.js
@@ -33,6 +33,18 @@ class Audit {
     throw new Error('Audit meta information must be overridden.');
   }
 
+  /**
+   * @param {string} debugString
+   * @return {!AuditResult}
+   */
+  static generateErrorAuditResult(debugString) {
+    return this.generateAuditResult({
+      rawValue: null,
+      error: true,
+      debugString
+    });
+  }
+
   /**
    * @param {!AuditResultInput} result
    * @return {!AuditResult}
@@ -57,6 +69,7 @@ class Audit {
       score,
       displayValue: `${displayValue}`,
       rawValue: result.rawValue,
+      error: result.error,
       debugString: result.debugString,
       optimalValue: result.optimalValue,
       extendedInfo: result.extendedInfo,

diff --git a/lighthouse-core/report/styles/report.css b/lighthouse-core/report/styles/report.css
@@ -33,6 +33,7 @@ span, div, p, section, header, h1, h2, li, ul {
   --good-color: #1ac123;
   --average-color: #ffae00;
   --unknown-color: #b3b3b3;
+  --warning-color: #f6be00;
   --gutter-gap: 12px;
   --gutter-width: 40px;
   --body-font-size: 14px;
@@ -896,6 +897,9 @@ body {
 .score-poor-bg {
   background-color: var(--poor-color);
 }
+.score-warning-bg {
+  background-color: var(--warning-color);
+}
 .score-unknown-bg {
   background-color: var(--unknown-color);
 }

diff --git a/lighthouse-core/report/templates/report-template.html b/lighthouse-core/report/templates/report-template.html
@@ -189,6 +189,8 @@ <h2>{{ aggregation.name }}</h2>
                   <div class="subitem-result">
                     {{#if subItem.comingSoon}}
                           <span class="subitem-result__unknown score-unknown-bg">N/A</span>
+                    {{else if subItem.error}}
+                      <span class="subitem-result__unknown score-warning-bg">N/A</span>
                     {{else}}
                       {{#if (is-bool subItem.score)}}
                         {{#if subItem.score}}

diff --git a/lighthouse-core/runner.js b/lighthouse-core/runner.js
@@ -174,10 +174,7 @@ class Runner {
         if (noArtifact || noTrace) {
           log.warn('Runner',
               `${artifactName} gatherer, required by audit ${audit.meta.name}, did not run.`);
-          return audit.generateAuditResult({
-            rawValue: -1,
-            debugString: `Required ${artifactName} gatherer did not run.`
-          });
+          throw new Error(`Required ${artifactName} gatherer did not run.`);
         }
 
         // If artifact was an error, it must be non-fatal (or gatherRunner would
@@ -186,14 +183,19 @@ class Runner {
           const artifactError = artifacts[artifactName];
           log.warn('Runner', `${artifactName} gatherer, required by audit ${audit.meta.name},` +
             ` encountered an error: ${artifactError.message}`);
-          return audit.generateAuditResult({
-            rawValue: -1,
-            debugString: artifactError.message
-          });
+          throw new Error(
+              `Required ${artifactName} gatherer encountered an error: ${artifactError.message}`);
         }
       }
 
       return audit.audit(artifacts);
+    }).catch(err => {
+      if (err.fatal) {
+        throw err;
+      }
+
+      // Non-fatal error become error audit result.
+      return audit.generateErrorAuditResult('Audit error: ' + err.message);
     }).then(result => {
       log.verbose('statusEnd', status);
       return result;

diff --git a/lighthouse-core/test/aggregator/aggregate-test.js b/lighthouse-core/test/aggregator/aggregate-test.js
@@ -18,7 +18,7 @@
 const Aggregate = require('../../aggregator/aggregate');
 const assert = require('assert');
 
-/* global describe, it*/
+/* eslint-env mocha */
 
 describe('Aggregate', () => {
   it('filters empty results', () => {
@@ -211,6 +211,20 @@ describe('Aggregate', () => {
     return assert.throws(_ => Aggregate._convertToWeight(result, expected));
   });
 
+  it('returns 0 if audit result was an error', () => {
+    const expected = {
+      expectedValue: true,
+      weight: 1
+    };
+
+    const result = {
+      rawValue: null,
+      error: true,
+    };
+
+    assert.strictEqual(Aggregate._convertToWeight(result, expected), 0);
+  });
+
   it('scores a set correctly (contributesToScore: true)', () => {
     const items = [{
       audits: {
@@ -405,6 +419,37 @@ describe('Aggregate', () => {
     return assert.equal(Aggregate.compare(results, items, scored)[0].overall, 1);
   });
 
+  it('if audit result is an error it does not contribute to score', () => {
+    const items = [{
+      audits: {
+        'test': {
+          expectedValue: true,
+          weight: 1
+        },
+        'alternate-test': {
+          expectedValue: 100,
+          weight: 1
+        }
+      }
+    }];
+
+    const errorResult = new Error('error message');
+    errorResult.name = 'alternate-test';
+    const results = [{
+      name: 'test',
+      rawValue: true,
+      score: true,
+      displayValue: ''
+    }, {
+      name: 'alternate-test',
+      rawValue: null,
+      error: true
+    }];
+
+    const aggregate = Aggregate.compare(results, items, true)[0];
+    assert.strictEqual(aggregate.overall, 0.5);
+  });
+
   it('outputs subitems', () => {
     const items = [{
       audits: {

diff --git a/lighthouse-core/test/runner-test.js b/lighthouse-core/test/runner-test.js
@@ -141,8 +141,10 @@ describe('Runner', () => {
       });
 
       return Runner.run({}, {url, config}).then(results => {
-        assert.equal(results.audits['user-timings'].rawValue, -1);
-        assert.ok(results.audits['user-timings'].debugString);
+        const auditResult = results.audits['user-timings'];
+        assert.strictEqual(auditResult.rawValue, null);
+        assert.strictEqual(auditResult.error, true);
+        assert.ok(auditResult.debugString.includes('traces'));
       });
     });
 
@@ -158,8 +160,10 @@ describe('Runner', () => {
       });
 
       return Runner.run({}, {url, config}).then(results => {
-        assert.equal(results.audits['is-on-https'].rawValue, -1);
-        assert.ok(results.audits['is-on-https'].debugString);
+        const auditResult = results.audits['is-on-https'];
+        assert.strictEqual(auditResult.rawValue, null);
+        assert.strictEqual(auditResult.error, true);
+        assert.ok(auditResult.debugString.includes('HTTPS'));
       });
     });
 
@@ -182,12 +186,74 @@ describe('Runner', () => {
       config.artifacts.HTTPS = artifactError;
 
       return Runner.run({}, {url, config}).then(results => {
-        assert.equal(results.audits['is-on-https'].rawValue, -1);
-        assert.equal(results.audits['is-on-https'].debugString, errorMessage);
+        const auditResult = results.audits['is-on-https'];
+        assert.strictEqual(auditResult.rawValue, null);
+        assert.strictEqual(auditResult.error, true);
+        assert.ok(auditResult.debugString.includes(errorMessage));
       });
     });
   });
 
+  describe('Bad audit behavior handling', () => {
+    const testAuditMeta = {
+      category: 'ThrowThrow',
+      name: 'throwy-audit',
+      description: 'Always throws',
+      requiredArtifacts: []
+    };
+
+    it('produces an error audit result when an audit throws a non-fatal Error', () => {
+      const errorMessage = 'Audit yourself';
+      const url = 'https://example.com';
+      const config = new Config({
+        audits: [
+          class ThrowyAudit extends Audit {
+            static get meta() {
+              return testAuditMeta;
+            }
+            static audit() {
+              throw new Error(errorMessage);
+            }
+          }
+        ],
+
+        artifacts: {}
+      });
+
+      return Runner.run({}, {url, config}).then(results => {
+        const auditResult = results.audits['throwy-audit'];
+        assert.strictEqual(auditResult.rawValue, null);
+        assert.strictEqual(auditResult.error, true);
+        assert.ok(auditResult.debugString.includes(errorMessage));
+      });
+    });
+
+    it('rejects if an audit throws a fatal error', () => {
+      const errorMessage = 'Uh oh';
+      const url = 'https://example.com';
+      const config = new Config({
+        audits: [
+          class FatalThrowyAudit extends Audit {
+            static get meta() {
+              return testAuditMeta;
+            }
+            static audit() {
+              const fatalError = new Error(errorMessage);
+              fatalError.fatal = true;
+              throw fatalError;
+            }
+          }
+        ],
+
+        artifacts: {}
+      });
+
+      return Runner.run({}, {url, config}).then(
+        _ => assert.ok(false),
+        err => assert.strictEqual(err.message, errorMessage));
+    });
+  });
+
   it('accepts performance logs as an artifact', () => {
     const url = 'https://example.com';
     const config = new Config({