Fix NaN% in CLI report (#763)

* fix(cli): Don't report scores on nonscored items * add test for nonscored items on CLI printer
GoogleChrome · Oct 11, 2016 · a21ec65 · a21ec65
1 parent 5476243
commit a21ec65
Show file tree

Hide file tree

Showing 5 changed files with 50 additions and 20 deletions.
diff --git a/lighthouse-cli/printer.js b/lighthouse-cli/printer.js
@@ -80,6 +80,7 @@ function formatScore(score, suffix) {
   if (typeof score !== 'number') {
     return `${purple}${score}${reset}`;
   }
+
   let colorChoice = red;
   if (score > 45) {
     colorChoice = yellow;
@@ -123,7 +124,7 @@ function createOutput(results, outputMode) {
       let score = (item.overall * 100).toFixed(0);
 
       if (item.name) {
-        output += `${bold}${item.name}${reset}: ${formatScore(Number(score), '%')}\n`;
+        output += `${bold}${item.name}${reset}: ${item.scored ? formatScore(score, '%') : ''}\n`;
       }
 
       item.subItems.forEach(subitem => {

diff --git a/lighthouse-cli/test/cli/printer-test.js b/lighthouse-cli/test/cli/printer-test.js
@@ -57,6 +57,11 @@ describe('Printer', () => {
     // Just check there's no HTML / JSON there.
     assert.throws(_ => JSON.parse(prettyOutput));
     assert.equal(/<!doctype/gim.test(prettyOutput), false);
+
+    const hasScoreOnNonScoredItem = /Using modern offline features.*?(0%|NaN)/.test(prettyOutput);
+    const hasAggregationPresent = prettyOutput.includes('Using modern offline features');
+    assert.equal(hasScoreOnNonScoredItem, false, 'non-scored item was scored');
+    assert.equal(hasAggregationPresent, true, 'non-scored aggregation item is not there');
   });
 
   it('creates HTML for results', () => {

diff --git a/lighthouse-cli/test/fixtures/sample.json b/lighthouse-cli/test/fixtures/sample.json
@@ -60,6 +60,24 @@
       "category": "JavaScript",
       "description": "Page contains some content when its scripts are not available"
     },
+    "appcache-manifest": {
+      "score": true,
+      "displayValue": "",
+      "rawValue": true,
+      "name": "appcache-manifest",
+      "category": "Offline",
+      "description": "Site is not using Application Cache",
+      "helpText": "Application Cache has been <a href=\"https://html.spec.whatwg.org/multipage/browsers.html#offline\" target=\"_blank\">deprecated</a> by <a href=\"https://developer.mozilla.org/en-US/docs/Web/API/Service_Worker_API/Using_Service_Workers\" target=\"_blank\">Service Workers</a>. Consider implementing an offline solution using the <a href=\"https://developer.mozilla.org/en-US/docs/Web/API/Cache\" target=\"_blank\">Cache Storage API</a>."
+    },
+    "no-websql": {
+      "score": true,
+      "displayValue": "",
+      "rawValue": true,
+      "name": "no-websql",
+      "category": "Offline",
+      "description": "Site is not using WebSQL DB.",
+      "helpText": "Web SQL Database is <a href=\"https://dev.w3.org/html5/webdatabase/\" target=\"_blank\">deprecated</a>. Consider implementing an offline solution using <a href=\"https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/Using_IndexedDB\" target=\"_blank\">IndexedDB</a>."
+    },
     "first-meaningful-paint": {
       "score": 100,
       "displayValue": "615.8ms",
@@ -610,6 +628,22 @@
           ]
         }
       ]
+    },
+     {
+      "name": "Do Better Web",
+      "description": "We've compiled some recommendations for modernizing your web app and avoiding performance pitfalls.",
+      "scored": false,
+      "categorizable": true,
+      "score": [
+        {
+          "overall": null,
+          "name": "Using modern offline features",
+          "subItems": [
+            "appcache-manifest",
+            "no-websql"
+          ]
+        }
+      ]
     }
   ]
 }
diff --git a/lighthouse-core/aggregator/aggregate.js b/lighthouse-core/aggregator/aggregate.js
@@ -37,12 +37,8 @@ class Aggregate {
    */
   static _getTotalWeight(expected) {
     const expectedNames = Object.keys(expected);
-    let weight = expectedNames.reduce((last, e) => last + expected[e].weight, 0);
-    if (weight === 0) {
-      weight = 1;
-    }
-
-    return weight;
+    let totalWeight = expectedNames.reduce((last, e) => last + (expected[e].weight || 0), 0);
+    return totalWeight;
   }
 
   /**
@@ -158,9 +154,10 @@ class Aggregate {
           Aggregate._remapResultsByName(
             Aggregate._filterResultsByAuditNames(results, item.audits)
           );
-      const maxScore = Aggregate._getTotalWeight(item.audits);
+
       const subItems = [];
       let overallScore = 0;
+      let maxScore = 1;
 
       // Step through each item in the expected results, and add them
       // to the overall score and add each to the subItems list.
@@ -197,6 +194,10 @@ class Aggregate {
             e);
       });
 
+      if (aggregationIsScored) {
+        maxScore = Aggregate._getTotalWeight(item.audits);
+      }
+
       return {
         overall: (overallScore / maxScore),
         name: item.name,

diff --git a/lighthouse-core/test/aggregator/aggregate-test.js b/lighthouse-core/test/aggregator/aggregate-test.js
@@ -54,18 +54,7 @@ describe('Aggregate', () => {
     const a = {};
 
     const weight = Aggregate._getTotalWeight(a);
-    return assert.equal(weight, 1);
-  });
-
-  it('returns a weight of at least 1', () => {
-    const a = {
-      x: {
-        weight: 0
-      }
-    };
-
-    const weight = Aggregate._getTotalWeight(a);
-    return assert.equal(weight, 1);
+    return assert.equal(weight, 0);
   });
 
   it('generates the correct total weight', () => {