core(gather-runner): add PageLoadError base artifact #9236
Conversation
|
|
||
| // And it bubbled up to the runtimeError. | ||
| expect(lhr.runtimeError.code).toEqual(NO_FCP.code); | ||
| expect(lhr.runtimeError.message).toBeDisplayString(/Something .*\(NO_FCP\)/); |
There was a problem hiding this comment.
these are unchanged, just nested now to share the test gatherer/audit/config with the following test (and assert -> expect)
| @@ -408,28 +408,6 @@ class GatherRunner { | |||
| log.timeEnd(apStatus); | |||
| } | |||
|
|
|||
| /** | |||
| * Generate a set of artfiacts for the given pass as if all the gatherers | |||
There was a problem hiding this comment.
aw, no more artfiacts 😢
| } catch (err) { | ||
| // cleanup on error | ||
| } finally { | ||
| // Clean up regardless of error. | ||
| GatherRunner.disposeDriver(driver, options); |
There was a problem hiding this comment.
I like the simplification, but now we can't await it in the success case :/
maybe it doesn't matter and we can add a comment calling out why it doesn't matter?
worth noting that it does clearDataForOrigin which given the recent changes in Chrome has taken multiple seconds on my machine lately :(
There was a problem hiding this comment.
but now we can't await it in the success case
Good catch, that wasn't intentional. I wonder how bad it would be to await the error case... Presumably it was so errors in disposing didn't overwrite the originating error, which probably often go hand in hand?
Kind of ridiculous we can't express what we mean here.
I could also just change it back, it's not a huge win.
There was a problem hiding this comment.
yeah maybe just change it back with a comment so we don't forget this again next time :)
There was a problem hiding this comment.
This pattern originally popped up way back in #639 (in the course of @wardpeet adding the multiple tabs check!), though originally we didn't wait for disconnect in the failure or success cases.
Looking at #2359, where the successful path was integrated into the promise chain, I'm half convinced we should be doing await GatherRunner.disposeDriver(driver, options).catch(() => {}); in the error case to avoid ever exiting before Chrome is able to exit, but I'm not able to induce a problem, so leaving it as it was :)
There was a problem hiding this comment.
I think I'm with you on .catch even if we don't await it, but w/e :)
|
|
||
| it('includes a pageLoadError runtimeError over any gatherer runtimeErrors', async () => { | ||
| const url = 'https://www.reddit.com/r/nba'; | ||
| let firstLoad = true; |
There was a problem hiding this comment.
firstLoad strikes again 😢
There was a problem hiding this comment.
firstLoad strikes again 😢
yeah, I'm sorry!
need an actual LGTM :) |
| } catch (err) { | ||
| // cleanup on error | ||
| } finally { | ||
| // Clean up regardless of error. | ||
| GatherRunner.disposeDriver(driver, options); |
There was a problem hiding this comment.
I think I'm with you on .catch even if we don't await it, but w/e :)
for some background, see conversation (and all the links from it) starting in #8865 (comment)
In this PR
This proposal is to disconnect
pageLoadErrorfrom artifact errors:PageLoadErroras a base artifact, populated if there was an error for that gathering run, otherwise nullundefinedin the pass that had the pageLoadError (just like all the artifacts from passes after that one, since we bail on gathering after a pageLoadError after core: bail on gathering if we have a failure in the first pass #8866/core(gather-runner): convert PAGE_HUNG to non-fatal runtimeError #9121)."Required x gatherer encountered an error"errors to"Required x gatherer did not run"errors, which is arguably more correct (for artifacts collected inafterPass) and doesn't suggest to the user that the issue lies in the gatherer when there was really a runtimeError they should look intoThis gives us a clear and unambiguous place to mark that there was an error in the page load, which makes artifacts easier to understand as well (no more combing all artifacts to be sure there wasn't a pageLoadError in some pass).
Current behavior
When a
pageLoadErroroccurs, it's communicated back toRunneras a bunch of artifacts from that bad pass set to that page load error. This shows up in the LHR at the audit level ("Required x gatherer encountered an error") and then runner loops over all the artifacts, and if it finds one marked as a runtime error it puts it in thelhr.runtimeErrorslot.In the HTML report, the errors are displayed in similar positions (in each audit and at the top).
So,
pageLoadError-> error artifact(s) ->lhr.runtimeError/errors audit result(s)As a result, whether or not a runtime error is visible in the end LHR--even fundamental page load issues--is a function of which gatherers are run. Even more weirdly, if filtering using
--only-audits, runtime error visibility is a function of which audits are run, since the gatherers that might have passed on the pageLoadError could get filtered out. This is why we had to have an audit that uses a non-trace artifact in theerrorssmoke test so that the runtimeError is actually seen and can be tested against.