core: suppress protocol timeout for app manifest bug in LR #7184
Conversation
connorjclark
changed the title
| const response = await passContext.driver.getAppManifest(); | ||
| if (response) return manifestParser(response.data, response.url, passContext.url); | ||
| } catch (err) { | ||
| // LR will timeout fetching the app manifest in some cases |
There was a problem hiding this comment.
LR will timeout fetching the app manifest in some cases
maybe just add ", move on without one." to this line, delete the lines after this, and link to the full https://github.com/GoogleChrome/lighthouse/issues/7147 (even better if there's a definitive summary comment to link to)
| // see #7147 or b/124008171 | ||
| // instead of failing completely by throwing a protocol timeout error, | ||
| // just pretend there is no app manifest. | ||
| log.error('Failed fetching manifest', err); |
There was a problem hiding this comment.
need a log.error('GatherRunner', 'Failed fetching...', err);
| return manifestParser(response.data, response.url, passContext.url); | ||
| try { | ||
| const response = await passContext.driver.getAppManifest(); | ||
| if (response) return manifestParser(response.data, response.url, passContext.url); |
There was a problem hiding this comment.
this should move below the catch so we aren't accidentally hiding errors in the parser
| // instead of failing completely by throwing a protocol timeout error, | ||
| // just pretend there is no app manifest. | ||
| log.error('Failed fetching manifest', err); | ||
| } |
There was a problem hiding this comment.
should the catch only apply for a err.code === 'PROTOCOL_TIMEOUT', otherwise rethrow?
|
FWIW I feel like the real fix (in a follow up) is a way to express manifest fetching errors in the There's already a way to say "this was invalid JSON"; adding information about the network fetch doesn't seem like it would be too bad. |
There was a problem hiding this comment.
WDYT about just catching this in driver's getAppManifest instead? I think several of @brendankenny's comments would be non-factors if we did that
I guess that's a little silly to worry about because we don't expose the PWA section yet in PSI, so we have a bit of time there :) |
what should it return, though? Ideally you'd get a "timed out waiting for the manifest" explanation in the report, and not just "you didn't have a manifest". |
Yeah that would be better, but I think we want to deploy something soon. Some big sites like toyota.com are failing completely (and ~1% of urls). Are we okay with this hotfix for the LR fix, and following up with a more thorough workaround later? The root cause of this bug likely won't be fixed until late next week. |
sorry, yes, I definitely agree with that. I guess just anticipating the follow up discussion :) |
lighthouse-core/gather/driver.js
Outdated
| if (err.code === 'PROTOCOL_TIMEOUT') { | ||
| // LR will timeout fetching the app manifest in some cases, move on without one. | ||
| // https://github.com/GoogleChrome/lighthouse/issues/7147#issuecomment-461210921 | ||
| log.error('Driver', 'Failed fetching...', err); |
There was a problem hiding this comment.
ha, sorry, I meant "..." for the rest of the message you had before, I just didn't want to retype it :)
|
ha, I was just going to add that codecov brings up a good point about needing a test or two. We have good existing coverage and the |
Fixes #7147
See b/124008171.