core(gather-runner): covert assertPageLoaded into soft failure #4048
Conversation
| log.verbose('statusEnd', status); | ||
|
|
||
| pageLoadError = GatherRunner.getPageLoadError(options.url, networkRecords); | ||
| // If the driver was offline, a page load error is expected, so do not save it. | ||
| if (!driver.online) pageLoadError = null; |
There was a problem hiding this comment.
can we do this check within getPageLoadError? seems reasonable.
There was a problem hiding this comment.
heh, I explicitly moved it out of there since it felt like it was in the wrong place over there :)
In fact I'd eventually want to remove this special check entirely to make the passes on similar footing. Is it primarily the extra line here that you don't like?
| @@ -319,8 +329,10 @@ class GatherRunner { | |||
| const artifacts = {}; | |||
|
|
|||
| // Nest LighthouseRunWarnings, if any, so they will be collected into artifact. | |||
| gathererResults.LighthouseRunWarnings = [gathererResults.LighthouseRunWarnings]; | |||
| const uniqueWarnings = Array.from(new Set(gathererResults.LighthouseRunWarnings)); | |||
There was a problem hiding this comment.
would it be worthwhile to change LighthouseRunWarnings to be a set instead?
There was a problem hiding this comment.
eh, it's not clear when we would do the flip back to array needed for JSON.stringify, so I'm not necessarily sold. since we were already manipulating it here and aggregating across multiple passes, seems reasonable to de-dupe but let audits keep track of it on their own
| }); | ||
| }); | ||
| }, Promise.resolve()).then(_ => { | ||
| // Fail the run if more than 50% of all audits failed due to page load failure. |
There was a problem hiding this comment.
"50% of all audits" => "50% of all artifacts"
There was a problem hiding this comment.
oh oops, yeah done :)
closes #1978 according to feedback in #2461
converts the assertPageLoaded into a soft failure that just fails every gatherer afterPass
if more than half of all artifacts failed due to page load failure then the error becomes fatal (like the case where you try to audit http://total-junk-hostname).