Merge pull request #564 from GoogleChrome/alwaysusemanifestparser

Improve the robustness of the manifest unittests

Author: paulirish

lighthouse-core/audits/manifest-background-color.js

@@ -18,6 +18,7 @@
 'use strict';
 
 const Audit = require('./audit');
+const Formatter = require('../formatters/formatter');
 
 class ManifestBackgroundColor extends Audit {
   /**
@@ -36,22 +37,24 @@ class ManifestBackgroundColor extends Audit {
    * @param {!Manifest=} manifest
    * @return {boolean}
    */
-  static hasBackgroundColorValue(manifest) {
+  static getBackgroundColorValue(manifest) {
     return manifest !== undefined &&
-      manifest.background_color !== undefined &&
-      manifest.background_color.value !== undefined;
+      manifest.background_color.value;
   }
 
   /**
    * @param {!Artifacts} artifacts
    * @return {!AuditResult}
    */
   static audit(artifacts) {
-    const hasBackgroundColor = ManifestBackgroundColor
-        .hasBackgroundColorValue(artifacts.Manifest.value);
+    const bgColor = ManifestBackgroundColor.getBackgroundColorValue(artifacts.Manifest.value);
 
     return ManifestBackgroundColor.generateAuditResult({
-      rawValue: hasBackgroundColor
+      rawValue: !!bgColor,
+      extendedInfo: {
+        value: bgColor,
+        formatter: Formatter.SUPPORTED_FORMATS.NULL
+      }
     });
   }
 }

lighthouse-core/test/audits/background-color.js

@@ -15,40 +15,63 @@
  */
 const Audit = require('../../audits/manifest-background-color.js');
 const assert = require('assert');
+const manifestSrc = JSON.stringify(require('../fixtures/manifest.json'));
+const manifestParser = require('../../lib/manifest-parser');
+const exampleManifest = manifestParser(manifestSrc);
 
 /* global describe, it*/
 
 // Need to disable camelcase check for dealing with background_color.
 /* eslint-disable camelcase */
 describe('Manifest: background color audit', () => {
-  it('fails when no manifest present', () => {
+  it('fails when no manifest artifact present', () => {
     return assert.equal(Audit.audit({Manifest: {
       value: undefined
     }}).rawValue, false);
   });
 
-  it('fails when no background color present', () => {
-    return assert.equal(Audit.audit({Manifest: {
-      value: {
-        foo: 1
-      }
-    }}).rawValue, false);
+  it('fails when an empty manifest is present', () => {
+    const artifacts = {
+      Manifest: manifestParser('{}')
+    };
+    return assert.equal(Audit.audit(artifacts).rawValue, false);
   });
 
-  it('fails when no background color value present', () => {
-    return assert.equal(Audit.audit({Manifest: {
-      value: {
+  it('fails when a minimal manifest contains no background_color', () => {
+    const artifacts = {
+      Manifest: manifestParser(JSON.stringify({
+        start_url: '/'
+      }))
+    };
+    const output = Audit.audit(artifacts);
+    assert.equal(output.rawValue, false);
+    assert.equal(output.debugString, undefined);
+  });
+
+  it('fails when a minimal manifest contains an invalid background_color', () => {
+    const artifacts = {
+      Manifest: manifestParser(JSON.stringify({
         background_color: 'no'
-      }
-    }}).rawValue, false);
+      }))
+    };
+    const output = Audit.audit(artifacts);
+    assert.equal(output.rawValue, false);
+    assert.equal(output.debugString, undefined);
   });
 
-  it('passes when color is present', () => {
-    return assert.equal(Audit.audit({Manifest: {
-      value: {
-        background_color: {value: 'black'}
-      }
-    }}).rawValue, true);
+  it('succeeds when a minimal manifest contains a valid background_color', () => {
+    const artifacts = {
+      Manifest: manifestParser(JSON.stringify({
+        background_color: '#FAFAFA'
+      }))
+    };
+    const output = Audit.audit(artifacts);
+    assert.equal(output.rawValue, true);
+    assert.equal(output.extendedInfo.value, '#FAFAFA');
+  });
+
+  it('succeeds when a complete manifest contains a background_color', () => {
+    return assert.equal(Audit.audit({Manifest: exampleManifest}).rawValue, true);
   });
 });
 /* eslint-enable */

lighthouse-core/test/audits/cache-start-url.js

@@ -17,7 +17,7 @@ const Audit = require('../../audits/cache-start-url.js');
 const assert = require('assert');
 const manifestSrc = JSON.stringify(require('../fixtures/manifest.json'));
 const manifestParser = require('../../lib/manifest-parser');
-const Manifest = manifestParser(manifestSrc);
+const exampleManifest = manifestParser(manifestSrc);
 const CacheContents = ['https://another.example.com/', 'https://example.com/'];
 const URL = 'https://example.com';
 const AltURL = 'https://example.com/?utm_source=http203';
@@ -30,50 +30,42 @@ describe('Cache: start_url audit', () => {
   });
 
   it('fails when no cache contents given', () => {
-    return assert.equal(Audit.audit({Manifest, URL}).rawValue, false);
+    const artifacts = {Manifest: exampleManifest, URL};
+    const output = Audit.audit(artifacts);
+    assert.equal(output.rawValue, false);
+    assert.equal(output.debugString, 'No cache or URL detected');
   });
 
   it('fails when no URL given', () => {
-    return assert.equal(Audit.audit({Manifest, CacheContents}).rawValue, false);
+    const artifacts = {Manifest: exampleManifest, CacheContents};
+    const output = Audit.audit(artifacts);
+    assert.equal(output.rawValue, false);
+    assert.equal(output.debugString, 'No cache or URL detected');
   });
 
   // Need to disable camelcase check for dealing with start_url.
   /* eslint-disable camelcase */
-  it('fails when a manifest artifact contains no start_url', () => {
-    const inputs = {
-      Manifest: { }
-    };
-    return assert.equal(Audit.audit(inputs).rawValue, false);
-  });
-
-  it('fails when a manifest artifact contains a null start_url', () => {
-    const inputs = {
-      Manifest: {
-        start_url: null
-      }
-    };
-    return assert.equal(Audit.audit(inputs).rawValue, false);
-  });
-
   it('fails when a manifest contains no start_url', () => {
-    const inputs = {
-      Manifest: manifestParser({})
+    const artifacts = {
+      Manifest: manifestParser('{}')
     };
-    return assert.equal(Audit.audit(inputs).rawValue, false);
+    const output = Audit.audit(artifacts);
+    assert.equal(output.rawValue, false);
+    assert.equal(output.debugString, 'start_url not present in Manifest');
   });
   /* eslint-enable camelcase */
 
   it('succeeds when given a manifest with a start_url, cache contents, and a URL', () => {
     return assert.equal(Audit.audit({
-      Manifest,
+      Manifest: exampleManifest,
       CacheContents,
       URL
     }).rawValue, true);
   });
 
   it('handles URLs with utm params', () => {
     return assert.equal(Audit.audit({
-      Manifest,
+      Manifest: exampleManifest,
       CacheContents,
       URL: AltURL
     }).rawValue, true);

lighthouse-core/test/audits/display.js

@@ -14,8 +14,12 @@
  * limitations under the License.
  */
 const Audit = require('../../audits/manifest-display.js');
+const manifestParser = require('../../lib/manifest-parser');
 const assert = require('assert');
 
+const manifestSrc = JSON.stringify(require('../fixtures/manifest.json'));
+const exampleManifest = manifestParser(manifestSrc);
+
 /* global describe, it*/
 
 describe('Mobile-friendly: display audit', () => {
@@ -26,28 +30,36 @@ describe('Mobile-friendly: display audit', () => {
     assert.equal(Audit.hasRecommendedValue(undefined), false);
   });
 
-  it('handles the case where there is no display property', () => {
-    const output = Audit.audit({Manifest: {}});
+  it('fails when no manifest artifact present', () => {
+    return assert.equal(Audit.audit({Manifest: {
+      value: undefined
+    }}).rawValue, false);
+  });
+
+  it('handles the case where there is no manifest display property', () => {
+    const artifacts = {
+      Manifest: manifestParser('{}')
+    };
+    const output = Audit.audit(artifacts);
 
     assert.equal(output.score, false);
     assert.equal(output.displayValue, '');
     assert.equal(output.rawValue, false);
   });
 
-  it('audits a manifest\'s display property', () => {
-    const expected = 'standalone';
-    const output = Audit.audit({
-      Manifest: {
-        value: {
-          display: {
-            value: expected
-          }
-        }
-      }
-    });
-
+  it('succeeds when a manifest has a display property', () => {
+    const artifacts = {
+      Manifest: manifestParser(JSON.stringify({
+        display: 'standalone'
+      }))
+    };
+    const output = Audit.audit(artifacts);
     assert.equal(output.score, true);
-    assert.equal(output.displayValue, expected);
+    assert.equal(output.displayValue, 'standalone');
     assert.equal(output.rawValue, true);
   });
+
+  it('succeeds when a complete manifest contains a display property', () => {
+    return assert.equal(Audit.audit({Manifest: exampleManifest}).rawValue, true);
+  });
 });

lighthouse-core/test/audits/exists.js

@@ -16,19 +16,37 @@
 const Audit = require('../../audits/manifest-exists.js');
 const assert = require('assert');
 
+const manifestSrc = JSON.stringify(require('../fixtures/manifest.json'));
+const manifestParser = require('../../lib/manifest-parser');
+const exampleManifest = manifestParser(manifestSrc);
+
 /* global describe, it*/
 
 describe('Manifest: exists audit', () => {
-  it('fails when no manifest present', () => {
+  it('fails when no manifest artifact present', () => {
     return assert.equal(Audit.audit({Manifest: {
       value: undefined
     }}).rawValue, false);
   });
 
-  it('succeeds when a manifest is present', () => {
-    return assert.equal(Audit.audit({Manifest: {
-      value: {}
-    }}).rawValue, true);
+  it('succeeds with a valid minimal manifest', () => {
+    const artifacts = {
+      Manifest: manifestParser('{}')
+    };
+    const output = Audit.audit(artifacts);
+    assert.equal(output.rawValue, true);
+    assert.equal(output.debugString, undefined);
+  });
+
+  it('succeeds with a valid minimal manifest', () => {
+    const artifacts = {
+      Manifest: manifestParser(JSON.stringify({
+        name: 'Lighthouse PWA'
+      }))
+    };
+    const output = Audit.audit(artifacts);
+    assert.equal(output.rawValue, true);
+    assert.equal(output.debugString, undefined);
   });
 
   it('correctly passes through debug strings', () => {
@@ -41,4 +59,17 @@ describe('Manifest: exists audit', () => {
       }
     }).debugString, debugString);
   });
+
+  it('correctly passes through a JSON parsing failure', () => {
+    const artifacts = {
+      Manifest: manifestParser('{ \name: Definitely not valid JSON }')
+    };
+    const output = Audit.audit(artifacts);
+    assert.equal(output.rawValue, false);
+    assert.ok(output.debugString.includes('Unexpected token'), 'No JSON error message');
+  });
+
+  it('succeeds with a complete manifest', () => {
+    return assert.equal(Audit.audit({Manifest: exampleManifest}).rawValue, true);
+  });
 });

lighthouse-core/test/audits/icons.js

@@ -19,6 +19,7 @@ const Audit144 = require('../../audits/manifest-icons-min-144.js');
 const Audit192 = require('../../audits/manifest-icons-min-192.js');
 const assert = require('assert');
 const manifestParser = require('../../lib/manifest-parser');
+const exampleManifest = JSON.stringify(require('../fixtures/manifest.json'));
 
 /* global describe, it*/
 
@@ -58,8 +59,7 @@ describe('Manifest: icons audits', () => {
 
     it('succeeds when a manifest contains icons that are large enough', () => {
       // stub manifest contains a 192 icon
-      const manifestSrc = JSON.stringify(require('../fixtures/manifest.json'));
-      const Manifest = manifestParser(manifestSrc);
+      const Manifest = manifestParser(exampleManifest);
       assert.equal(Audit144.audit({Manifest}).rawValue, true);
       assert.equal(Audit192.audit({Manifest}).rawValue, true);
     });

lighthouse-core/test/audits/name.js

@@ -17,32 +17,43 @@ const Audit = require('../../audits/manifest-name.js');
 const assert = require('assert');
 const manifestSrc = JSON.stringify(require('../fixtures/manifest.json'));
 const manifestParser = require('../../lib/manifest-parser');
-const manifest = manifestParser(manifestSrc);
+const exampleManifest = manifestParser(manifestSrc);
 
 /* global describe, it*/
 
 describe('Manifest: name audit', () => {
-  it('fails when no manifest present', () => {
+  it('fails when no manifest artifact present', () => {
     return assert.equal(Audit.audit({Manifest: {
       value: undefined
     }}).rawValue, false);
   });
 
   it('fails when an empty manifest is present', () => {
-    return assert.equal(Audit.audit({Manifest: {}}).rawValue, false);
+    const artifacts = {
+      Manifest: manifestParser('{}')
+    };
+    return assert.equal(Audit.audit(artifacts).rawValue, false);
   });
 
   it('fails when a manifest contains no name', () => {
-    const inputs = {
-      Manifest: {
-        name: null
-      }
+    const artifacts = {
+      Manifest: manifestParser(JSON.stringify({
+        display: '/'
+      }))
     };
+    return assert.equal(Audit.audit(artifacts).rawValue, false);
+  });
 
-    return assert.equal(Audit.audit(inputs).rawValue, false);
+  it('succeeds when a minimal manifest contains a name', () => {
+    const artifacts = {
+      Manifest: manifestParser(JSON.stringify({
+        name: 'Lighthouse PWA'
+      }))
+    };
+    return assert.equal(Audit.audit(artifacts).rawValue, true);
   });
 
-  it('succeeds when a manifest contains a name', () => {
-    return assert.equal(Audit.audit({Manifest: manifest}).rawValue, true);
+  it('succeeds when a complete manifest contains a name', () => {
+    return assert.equal(Audit.audit({Manifest: exampleManifest}).rawValue, true);
   });
 });