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

patrickhulce · 2017-06-20T19:25:34Z

split out from #2420 to reduce the noise there

brendankenny

review!

brendankenny · 2017-06-21T04:42:19Z

lighthouse-core/gather/driver.js

@@ -276,7 +276,7 @@ class Driver {
            }

            // If both the data and the url are empty strings, the page had no manifest.
-            return reject('No web app manifest found.');
+            return resolve(null);


do you want to flatten the promise setup here while you're in here? Getting rid of the surrounding promise and then the resolve and rejects should be able to just be return/throw

brendankenny · 2017-06-21T04:57:51Z

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

      })
      .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.start_url || !manifest.value.start_url.raw) {


I may have been the cause of this original conditional, but I don't think it's quite right.

!manifest makes sense now (in case getAppManifest returned null, the real no-manifest case)

if there's a manifest, then manifest.value can be undefined if the manifest was malformed JSON

if there's a manifest and a manifest.value there will always be a manifest.value.start_url, as even if it's missing or the completely wrong type in the raw manifest, start_url will then default to the document URL

(!manifest || !manifest.value) should be sufficient, and up to you if you want to make the error strings different for the two cases

brendankenny · 2017-06-21T05:00:42Z

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

        }

        if (manifest.value.start_url.debugString) {
-          return Promise.reject(new Error(manifest.value.start_url.debugString));
+          throw new Error(manifest.value.start_url.debugString);


do we want this to be an error? It'll be things like invalid start_url relative to ${manifestUrl} or start_url must be same-origin as document, which is nice to put in front of the developer, but the parsed manifest will still have a valid start_url in those cases.

Not sure if we want to fall on the "this is broken" side or the side of how the browser actually treats it since we don't have a great logging spot for that type of message if no error here

it wasn't it was being caught below

brendankenny · 2017-06-21T05:09:28Z

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

+      })
+      .then(_ => this.executeFetchRequest(options.driver, this.startUrl))
+      .catch(err => {
+        this.debugString = err.message;


probably better to set this.debugString above and return rather than throwing errors and catching everything here. Doing that would also let errors from getAppManifest, the manifest parser, and executeFetchRequest get through without all being turned into debugStrings

that was kinda the point though since all of this could reasonably fail not because of a bug but just because of the page not having a service worker, fine with switching for now though and going back to this if it's too noisy

brendankenny · 2017-06-21T05:15:02Z

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

-      return Promise.reject(new Error('No start_url found inside the manifest'));
+    if (this.debugString || !this.startUrl) {
+      const debugString = this.debugString || 'No start_url found inside the manifest';
+      return Promise.resolve({debugString});


statusCode of -1 in this case?

brendankenny · 2017-06-21T05:15:13Z

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

@@ -76,7 +81,9 @@ class StartUrl extends Gatherer {
    }).pop(); // Take the last record that matches.

    return options.driver.goOnline(options)
-      .then(_ => navigationRecord ? navigationRecord.statusCode : -1);
+      .then(_ => {
+        return {statusCode: navigationRecord ? navigationRecord.statusCode : -1};


debugString in the -1 case?

codecov · 2017-06-21T17:06:31Z

Codecov Report

Merging #2549 into master will increase coverage by 0.02%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #2549      +/-   ##
==========================================
+ Coverage   84.52%   84.54%   +0.02%     
==========================================
  Files         169      169              
  Lines        5614     5615       +1     
  Branches      774      777       +3     
==========================================
+ Hits         4745     4747       +2     
+ Misses        855      854       -1     
  Partials       14       14

Impacted Files	Coverage Δ
lighthouse-core/gather/driver.js	`61.9% <0%> (ø)`	⬆️
lighthouse-core/gather/gatherers/manifest.js	`100% <100%> (ø)`	⬆️
lighthouse-core/audits/webapp-install-banner.js	`100% <100%> (ø)`	⬆️
lighthouse-core/gather/gatherers/start-url.js	`100% <100%> (+2.32%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cea443e...d3c8dfd. Read the comment docs.

brendankenny · 2017-06-22T01:35:29Z

lighthouse-core/gather/driver.js

+        // 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) {


why drop the response.url check? If the manifest 404s, for instance, response.data will be undefined but that should probably be considered an error state, not that there is no manifest

IMO, anything that's not a bug should not say 'GathererError: blah', if the manifest 404s that's not a bug so it shouldn't be an error, at best we can give them the slightly different debug strings, is that what you're pushing back against?

Saying "No web app manifest found on page" is definitely wrong in that case, so a better error string is needed. But I'm not sure I agree about Gatherer errors. I have no problem with a method fetching a manifest throwing on a manifest URL that doesn't exist.

But I assume the issue is us conflating Lighthouse bugs with exceptional states? We may need to get back into that conversation to get the functionality we need. We don't want to slide back into -1 land where every audit has to handle all possible gatherer error states, but agreed we probably need a better separation between the classes of errors.

I have no problem with a method fetching a manifest throwing on a manifest URL that doesn't exist.

I've got no problem with this throwing either just the overall pass throwing instead of returning debugString. There should be some distinguishing from a user's perspective between a bug they have no control over and something they need to take action on.

Also re:

Saying "No web app manifest found on page" is definitely wrong in that case, so a better error string is needed.

I'd don't actually agree that "Unable to retrieve manifest" is better than "No web app manifest found on page", as a user receiving the former I may believe that it's LH problem not being able to retrieve it and there's nothing for me to do whereas the latter gets me to check my manifest. There's probably room for improvement on both, but calling it "definitely wrong" seems overly critical. It's not wrong, there was no usable manifest found.

maybe we need to start laying out the failure states for an audit ahead of time so we can decide on them once (in theory) and then it's obvious when they're implemented or not :)

If there's no <link rel="manifest"> in the page, it's obviously not an error, the audits simply just won't pass. However, to me, if there is a <link rel="manifest"> and the url 404s, that is an error worth reporting to the user, as much as a manifest made up of invalid JSON would be. They tried to include one and it's failing for some reason.

Agreed "Unable to retrieve manifest" is a bad error string for that and we should be as specific as possible to help with those situations. Also totally on board with exploring non-exception error returns, expected errors, etc after this so we can separate the two (or more?) clear classes of errors we're dealing with here.

"Unable to retrieve manifest" is a bad error string for that and we should be as specific as possible to help with those situations

One downside of the current Page.getAppManifest protocol call is that it gives no network status information (as far as I can tell). So e.g. if the server returns a 404 with a Cannot GET /manifest.json response string, the protocol will just return a JSON.parse error from trying to parse that string (with no other indication of what happened) and we just have to infer that there was a 404. Maybe we could see about getting more detail there.

(the devtools Application panel suffers from the same problem, only showing "Errors and warnings: Line: 1, column: 1, Unexpected token." and you have to notice the 404 in the console/network panel to see what happened)

One downside of the current Page.getAppManifest protocol call is that it gives no network status information (as far as I can tell).

Yeah feels like it should return something similar to an artifact {value: null, debugString: ''} or whatever

brendankenny · 2017-06-22T01:42:55Z

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

        }

        if (manifest.value.start_url.debugString) {
-          return Promise.reject(new Error(manifest.value.start_url.debugString));
+          this.debugString = manifest.value.start_url.debugString;


should this be a state with a debugString and try to still fetch the start_url?

hm, why? there's nothing to fetch if the start_url is bad

hm, why? there's nothing to fetch if the start_url is bad

if there's a parseable JSON file there's always a good start_url, even if it has to fall back to just the page's URL.

oh I see now, faulty start_urls are always ignored

debugString seems rather useless then there will always be a usable start_url...

debugString seems rather useless then there will always be a usable start_url

it is useful for debugging to see if your manifest isn't acting like you think it should...e.g. you have one start_url in mind and it's defaulting to another. That might be really hard to track down (or even notice that it's happening) otherwise.

I guess that makes me fall on the side of the current code and just show the error :) Lighthouse can be stricter than the spec in that case

I guess this gets into the division of labor between DevTools and LH. LH shouldn't be your debugger IMO outside of general messages to get you on the right track/where to start looking. If everything is working from the PWA perspective (and document url was the right start url), do you really care about being flagged that you don't have a valid start url?

Either way, I added back the debug string to still do the check so if it passes it should pass with the debugString

Either way, I added back the debug string to still do the check so if it passes it should pass with the debugString

I think that's a good solution for now and the devtools/LH discussion is a good one to revisit now that Audits 2.0 is a real thing. Pavel and Paul also love drawing diagrams about that sort of division so I'm sure they'd be happy to get in on it :)

Pavel and Paul also love drawing diagrams about that sort of division so I'm sure they'd be happy to get in on it :)

haha :)

brendankenny · 2017-06-22T01:45:14Z

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

  }

  afterPass(options, tracingData) {
-    if (!this.startUrl) {
-      return Promise.reject(new Error('No start_url found inside the manifest'));
+    if (this.debugString || !this.startUrl) {


seems like either there's a debugString or a startUrl for all possible states?

not if manifestParser/getAppManifest fails, but I guess that's a different can of worms altogether

not if manifestParser/getAppManifest fails, but I guess that's a different can of worms altogether

yeah, in that case (if pass rejects) afterPass should be ignored, but it will still run, which is weird. We should probably fix that...

paulirish · 2017-07-07T19:21:46Z

@brendankenny will review and we'll get this sorted today!

brendankenny · 2017-07-11T01:06:13Z

lighthouse-core/gather/gatherers/manifest.js

@@ -27,19 +27,16 @@ class Manifest extends Gatherer {
  afterPass(options) {
    return options.driver.getAppManifest()
      .then(response => {
+        if (!response || !response.data) {


!response.data not needed here?

brendankenny · 2017-07-11T01:31:18Z

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

-        failures,
-        manifestValues
-      };
+      const result = {failures, manifestValues};


if result already has failures and manifestValues on it but all three are being passed into these methods, should they instead just have a single result parameter?

brendankenny · 2017-07-11T01:33:13Z

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

+      const result = {failures, manifestValues};
+      WebappInstallBanner.assessManifest(manifestValues, failures, result);
+      WebappInstallBanner.assessServiceWorker(artifacts, failures, result);
+      WebappInstallBanner.assessOfflineStartUrl(artifacts, failures, result);


MultiCheckAudit is already somewhat confusing and this doesn't exactly simplify things :)

Short of a MultiCheckAudit refactor, what about something like a warnings array, which is used for debugString, just like failures but they won't fail the audit?

Just an idea, maybe we can come up with something simpler/more self-explanatory

brendankenny · 2017-07-11T01:45:08Z

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;


what new path needs this?

this was a drive-by fix of something I encountered on another branch, can't remember what site now though it was over a month ago :P

but to answer your question the equalWithExcludedFragments can now be run on a non-existent startUrl which causes this to fail for every site that doesn't have the manifest

but to answer your question the equalWithExcludedFragments can now be run on a non-existent startUrl which causes this to fail for every site that doesn't have the manifest

ha, I had forgotten we had done that. I guess that's fine vs moving the .map(rewriteChromeInternalUrl); into the try/catch

brendankenny · 2017-07-11T01:54:15Z

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

-        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) {
+          this.debugString = `No usable web app manifest found on page ${options.url}`;


if there's a manifest but no manifest.value, there will be a manifest.debugString probably worth keeping

brendankenny · 2017-07-11T01:55:18Z

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

        }

        if (manifest.value.start_url.debugString) {
-          return Promise.reject(new Error(manifest.value.start_url.debugString));
+          this.debugString = manifest.value.start_url.debugString;


maybe a comment here as a reminder that even with a debugString there will be a fallback start_url

brendankenny · 2017-07-11T02:02:04Z

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

    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(_ => {
+        let debugString = this.debugString;


can we handle these cases separately for clarity? I think there's:

debugString and no startUrl

debugString and no matching navigationRecord

matching navigationRecord (and maybe a debugString from start_url parsing and other future messages)

we could probably give more information (e.g. start_url was fetched but had some error, wasn't covered by the SW, etc), but that can wait for another day

patrickhulce · 2017-07-11T18:20:23Z

PTAL

brendankenny · 2017-07-11T22:46:19Z

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

+        result.failures.push(artifacts.StartUrl.debugString);
+      }
+    } else if (artifacts.StartUrl.debugString) {
+      result.debugString = artifacts.StartUrl.debugString;


weird, I left this comment before (and it was in the email), but it was eaten in the web interface:

MultiCheckAudit is already somewhat confusing and this doesn't exactly simplify things :)

Short of a MultiCheckAudit refactor, what about something like a warnings array, which is used for debugString, just like failures but they won't fail the audit?

Just an idea, maybe we can come up with something simpler/more self-explanatory

so here (for this idea) it would result.warnings.push(artifacts.StartUrl.debugString); whether or not it hasOfflineStartUrl, and then MultiCheckAudit would handle appending result.warnings.join(', ') to the debugString

brendankenny · 2017-07-11T22:55:16Z

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

@@ -157,7 +157,7 @@ describe('PWA: webapp install banner audit', () => {

    return WebappInstallBannerAudit.audit(artifacts).then(result => {
      assert.strictEqual(result.rawValue, true);
-      assert.equal(result.debugString, 'Warning!');
+      assert.equal(result.debugString, 'Warnings: Warning!');


brendankenny

LGTM

I'd like us to give more clues on how to fix things when we know (e.g. the manifest 404ed), but we can build that up in the future, and the real purpose of the PR (moving away from exceptions for non-errors) looks good.

refactor(StartUrl): switch from error to debugString object

fbd724e

patrickhulce mentioned this pull request Jun 20, 2017

core(lib): add error reporting #2420

Merged

brendankenny requested changes Jun 21, 2017

View reviewed changes

feedback

d3c8dfd

brendankenny requested changes Jun 22, 2017

View reviewed changes

patrickhulce added 5 commits June 22, 2017 15:38

Merge branch 'master' into start_url_debug_string

f98d4b9

allow debugString during passing

6589cdd

fix test

1cf9b18

pre-existing bug fix

369058d

Merge branch 'master' into start_url_debug_string

6cefcd6

paulirish assigned brendankenny Jul 7, 2017

brendankenny reviewed Jul 11, 2017

View reviewed changes

patrickhulce added 2 commits July 11, 2017 10:35

Merge branch 'master' into start_url_debug_string

5c9b84b

more feedback

4a0e7d4

brendankenny reviewed Jul 11, 2017

View reviewed changes

more more feedback

c552cab

brendankenny reviewed Jul 11, 2017

View reviewed changes

brendankenny approved these changes Jul 11, 2017

View reviewed changes

patrickhulce merged commit 64b015e into master Jul 11, 2017

patrickhulce deleted the start_url_debug_string branch July 11, 2017 23:10

paulirish mentioned this pull request Jul 20, 2017

Report app manifest errors #2349

Closed

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

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

Conversation

patrickhulce commented Jun 20, 2017

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 21, 2017

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Jun 28, 2017 •

edited

Loading

patrickhulce Jun 22, 2017 •

edited

Loading