refactor(StartUrl): switch from error to debugString object (#2549)

* refactor(StartUrl): switch from error to debugString object

* feedback

* allow debugString during passing

* fix test

* pre-existing bug fix

* more feedback

* more more feedback

Author: patrickhulce

lighthouse-core/audits/multi-check-audit.js

@@ -38,10 +38,16 @@ class MultiCheckAudit extends Audit {
       };
     }
 
+    let debugString;
+    if (result.warnings && result.warnings.length > 0) {
+      debugString = `Warnings: ${result.warnings.join(', ')}`;
+    }
+
     // Otherwise, we pass
     return {
       rawValue: true,
-      extendedInfo
+      extendedInfo,
+      debugString,
     };
   }
 

lighthouse-core/audits/webapp-install-banner.js

@@ -46,7 +46,8 @@ class WebappInstallBanner extends MultiCheckAudit {
     };
   }
 
-  static assessManifest(manifestValues, failures) {
+  static assessManifest(artifacts, result) {
+    const {manifestValues, failures} = result;
     if (manifestValues.isParseFailure) {
       failures.push(manifestValues.parseFailureReason);
       return;
@@ -69,32 +70,36 @@ class WebappInstallBanner extends MultiCheckAudit {
   }
 
 
-  static assessServiceWorker(artifacts, failures) {
+  static assessServiceWorker(artifacts, result) {
     const hasServiceWorker = SWAudit.audit(artifacts).rawValue;
     if (!hasServiceWorker) {
-      failures.push('Site does not register a Service Worker');
+      result.failures.push('Site does not register a Service Worker');
     }
   }
 
-  static assessOfflineStartUrl(artifacts, failures) {
-    const hasOfflineStartUrl = artifacts.StartUrl === 200;
+  static assessOfflineStartUrl(artifacts, result) {
+    const hasOfflineStartUrl = artifacts.StartUrl.statusCode === 200;
+
     if (!hasOfflineStartUrl) {
-      failures.push('Manifest start_url is not cached by a Service Worker');
+      result.failures.push('Manifest start_url is not cached by a Service Worker');
+    }
+
+    if (artifacts.StartUrl.debugString) {
+      result.warnings.push(artifacts.StartUrl.debugString);
     }
   }
 
   static audit_(artifacts) {
     const failures = [];
+    const warnings = [];
 
     return artifacts.requestManifestValues(artifacts.Manifest).then(manifestValues => {
-      WebappInstallBanner.assessManifest(manifestValues, failures);
-      WebappInstallBanner.assessServiceWorker(artifacts, failures);
-      WebappInstallBanner.assessOfflineStartUrl(artifacts, failures);
-
-      return {
-        failures,
-        manifestValues
-      };
+      const result = {warnings, failures, manifestValues};
+      WebappInstallBanner.assessManifest(artifacts, result);
+      WebappInstallBanner.assessServiceWorker(artifacts, result);
+      WebappInstallBanner.assessOfflineStartUrl(artifacts, result);
+
+      return result;
     });
   }
 }

lighthouse-core/gather/driver.js

@@ -265,25 +265,18 @@ class Driver {
   }
 
   getAppManifest() {
-    return new Promise((resolve, reject) => {
-      this.sendCommand('Page.getAppManifest')
-        .then(response => {
-          // We're not reading `response.errors` however it may contain critical and noncritical
-          // errors from Blink's manifest parser:
-          //   https://chromedevtools.github.io/debugger-protocol-viewer/tot/Page/#type-AppManifestError
-          if (!response.data) {
-            if (response.url) {
-              return reject(new Error(`Unable to retrieve manifest at ${response.url}.`));
-            }
-
-            // If both the data and the url are empty strings, the page had no manifest.
-            return reject('No web app manifest found.');
-          }
+    return this.sendCommand('Page.getAppManifest')
+      .then(response => {
+        // We're not reading `response.errors` however it may contain critical and noncritical
+        // errors from Blink's manifest parser:
+        //   https://chromedevtools.github.io/debugger-protocol-viewer/tot/Page/#type-AppManifestError
+        if (!response.data) {
+          // If the data is empty, the page had no manifest.
+          return null;
+        }
 
-          resolve(response);
-        })
-        .catch(err => reject(err));
-    });
+        return response;
+      });
   }
 
   getSecurityState() {

lighthouse-core/gather/gatherers/manifest.js

@@ -27,19 +27,16 @@ class Manifest extends Gatherer {
   afterPass(options) {
     return options.driver.getAppManifest()
       .then(response => {
+        if (!response) {
+          return null;
+        }
+
         const isBomEncoded = response.data.charCodeAt(0) === BOM_FIRSTCHAR;
         if (isBomEncoded) {
           response.data = Buffer.from(response.data).slice(BOM_LENGTH).toString();
         }
 
         return manifestParser(response.data, response.url, options.url);
-      })
-      .catch(err => {
-        if (err === 'No web app manifest found.') {
-          return null;
-        }
-
-        return Promise.reject(err);
       });
   }
 }

lighthouse-core/gather/gatherers/start-url.js

@@ -49,34 +49,54 @@ class StartUrl extends Gatherer {
   pass(options) {
     return options.driver.getAppManifest()
       .then(response => {
-        return manifestParser(response.data, response.url, options.url);
+        return response && manifestParser(response.data, response.url, options.url);
       })
       .then(manifest => {
-        if (!manifest.value.start_url || !manifest.value.start_url.raw) {
-          return Promise.reject(new Error(`No web app manifest found on page ${options.url}`));
+        if (!manifest || !manifest.value) {
+          const detailedMsg = manifest && manifest.debugString;
+          this.debugString = detailedMsg ?
+              `Error fetching web app manifest: ${detailedMsg}` :
+              `No usable web app manifest found on page ${options.url}`;
+          return;
         }
 
         if (manifest.value.start_url.debugString) {
-          return Promise.reject(new Error(manifest.value.start_url.debugString));
+          // Even if the start URL had an error, the browser will still supply a fallback URL.
+          // Therefore, we only set the debugString here and continue with the fetch.
+          this.debugString = manifest.value.start_url.debugString;
         }
 
         this.startUrl = manifest.value.start_url.value;
-      }).then(_ => this.executeFetchRequest(options.driver, this.startUrl));
+        return this.executeFetchRequest(options.driver, this.startUrl);
+      });
   }
 
   afterPass(options, tracingData) {
-    if (!this.startUrl) {
-      return Promise.reject(new Error('No start_url found inside the manifest'));
-    }
-
     const networkRecords = tracingData.networkRecords;
     const navigationRecord = networkRecords.filter(record => {
       return URL.equalWithExcludedFragments(record._url, this.startUrl) &&
         record._fetchedViaServiceWorker;
     }).pop(); // Take the last record that matches.
 
     return options.driver.goOnline(options)
-      .then(_ => navigationRecord ? navigationRecord.statusCode : -1);
+      .then(_ => {
+        if (!this.startUrl) {
+          return {
+            statusCode: -1,
+            debugString: this.debugString || 'No start URL to fetch',
+          };
+        } else if (!navigationRecord) {
+          return {
+            statusCode: -1,
+            debugString: this.debugString || 'Did not fetch start URL from service worker',
+          };
+        } else {
+          return {
+            statusCode: navigationRecord.statusCode,
+            debugString: this.debugString,
+          };
+        }
+      });
   }
 }
 

lighthouse-core/lib/url-shim.js

@@ -103,7 +103,7 @@ URL.elideDataURI = function elideDataURI(url) {
 // Why? Special handling was added by Chrome team to allow a pushState transition between chrome:// pages.
 // As a result, the network URL (chrome://chrome/settings/) doesn't match the final document URL (chrome://settings/).
 function rewriteChromeInternalUrl(url) {
-  if (!url.startsWith('chrome://')) return url;
+  if (!url || !url.startsWith('chrome://')) return url;
   return url.replace(/^chrome:\/\/chrome\//, 'chrome://');
 }
 

lighthouse-core/test/audits/webapp-install-banner-test.js

@@ -26,7 +26,7 @@ function generateMockArtifacts() {
         scriptURL: 'https://example.com/sw.js'
       }]
     },
-    StartUrl: 200,
+    StartUrl: {statusCode: 200},
     URL: {finalUrl: 'https://example.com'}
   }));
   const mockArtifacts = Object.assign({}, computedArtifacts, clonedArtifacts);
@@ -128,7 +128,7 @@ describe('PWA: webapp install banner audit', () => {
   it('fails if page had no SW', () => {
     const artifacts = generateMockArtifacts();
     artifacts.ServiceWorker.versions = [];
-    artifacts.StartUrl = -1;
+    artifacts.StartUrl = {statusCode: -1};
 
     return WebappInstallBannerAudit.audit(artifacts).then(result => {
       assert.strictEqual(result.rawValue, false);
@@ -141,7 +141,7 @@ describe('PWA: webapp install banner audit', () => {
 
   it('fails if start_url is not cached', () => {
     const artifacts = generateMockArtifacts();
-    artifacts.StartUrl = -1;
+    artifacts.StartUrl = {statusCode: -1};
 
     return WebappInstallBannerAudit.audit(artifacts).then(result => {
       assert.strictEqual(result.rawValue, false);
@@ -150,4 +150,14 @@ describe('PWA: webapp install banner audit', () => {
       assert.strictEqual(failures.length, 1, failures);
     });
   });
+
+  it('includes debugString from start_url', () => {
+    const artifacts = generateMockArtifacts();
+    artifacts.StartUrl = {statusCode: 200, debugString: 'Warning!'};
+
+    return WebappInstallBannerAudit.audit(artifacts).then(result => {
+      assert.strictEqual(result.rawValue, true);
+      assert.equal(result.debugString, 'Warnings: Warning!');
+    });
+  });
 });

lighthouse-core/test/gather/gatherers/manifest-test.js

@@ -97,7 +97,7 @@ describe('Manifest gatherer', () => {
     return manifestGather.afterPass({
       driver: {
         getAppManifest() {
-          return Promise.reject('No web app manifest found.');
+          return Promise.resolve(null);
         }
       }
     }).then(artifact => {

lighthouse-core/test/gather/gatherers/start-url-test.js

@@ -90,8 +90,10 @@ describe('Start-url gatherer', () => {
       startUrlGathererWithQueryString.pass(optionsWithQueryString)
         .then(_ => startUrlGathererWithQueryString.afterPass(optionsWithQueryString, tracingData))
     ]).then(([artifact, artifactWithQueryString]) => {
-      assert.strictEqual(artifact, -1);
-      assert.strictEqual(artifactWithQueryString, -1);
+      assert.equal(artifact.statusCode, -1);
+      assert.ok(artifact.debugString, 'did not set debug string');
+      assert.equal(artifactWithQueryString.statusCode, -1);
+      assert.ok(artifactWithQueryString.debugString, 'did not set debug string');
     });
   });
 
@@ -113,25 +115,22 @@ describe('Start-url gatherer', () => {
       startUrlGathererWithFragment.pass(optionsWithQueryString)
         .then(_ => startUrlGathererWithFragment.afterPass(optionsWithQueryString, tracingData))
     ]).then(([artifact, artifactWithFragment]) => {
-      assert.strictEqual(artifact, 200);
-      assert.strictEqual(artifactWithFragment, 200);
+      assert.equal(artifact.statusCode, 200);
+      assert.equal(artifactWithFragment.statusCode, 200);
     });
   });
 
-  it('returns an error when manifest cannot be found', () => {
+  it('returns a debugString when manifest cannot be found', () => {
     const startUrlGatherer = new StartUrlGatherer();
     const options = {
       url: 'https://ifixit-pwa.appspot.com/',
       driver: wrapSendCommand(mockDriver, '')
     };
 
-    startUrlGatherer.pass(options)
+    return startUrlGatherer.pass(options)
       .then(_ => startUrlGatherer.afterPass(options, tracingData))
-      .then(_ => {
-        assert.ok(false, 'should fail because manifest is empty');
-      })
-      .catch(err => {
-        assert.strictEqual(err.message, `No web app manifest found on page ${options.url}`);
+      .then(artifact => {
+        assert.equal(artifact.debugString, 'ERROR: start_url string empty');
       });
   });
 
@@ -142,13 +141,10 @@ describe('Start-url gatherer', () => {
       driver: wrapSendCommand(mockDriver, 'https://not-same-origin.com/')
     };
 
-    startUrlGatherer.pass(options)
+    return startUrlGatherer.pass(options)
       .then(_ => startUrlGatherer.afterPass(options, tracingData))
-      .then(_ => {
-        assert.ok(false, 'should fail because origin is not the same');
-      })
-      .catch(err => {
-        assert.strictEqual(err.message, 'ERROR: start_url must be same-origin as document');
+      .then(artifact => {
+        assert.equal(artifact.debugString, 'ERROR: start_url must be same-origin as document');
       });
   });
 });