core: create flag to prevent fatal error on bad status code #15494
Conversation
|
It should definitely be possible to test 404 pages, but otoh 404 is definitely an error state. You won't get CrUX data for URLs served with a 404 status, for instance. I personally favor the flag solution from #10493, but I also don't know the balance between folks wanting to test 404 pages explicitly vs other folks inadvertently testing the wrong page or a broken page without noticing (#10493 (comment), #10493 (comment), etc) to inform what the default behavior should be. So I don't have a strong opinion on warning vs error with flag to disable for 404s. It does seem like a breaking change, though? e.g. I'm going to have to change my BigQuery queries to weed out 404 pages based on the warning after this. People running this in CI (or the suggestion that LHCI should handle the check) will have to go make that change as well. |
It seemed that the consensus was to do a warning instead of the flag, though I also don't have a strong opinion.
This is a good point. Can't think of one on the spot but is there currently another instance where we would get a Lighthouse result, but not CrUX data? My thinking here is that if we do this already, it won't be so bad here? Maybe some more discussion is needed on #10493? |
|
I don't want to re-litigate if there was consensus, but if this is going to happen, it would definitely be ideal if it were rolled into a major release. |
|
IMO there should be a command line switch to invoke error page testing. If an error is encountered when the switch is not present, the error message should highlight that the page errored and to try again, or that you can intentionally ignore the error HTTP status to test the page. |
|
Or: keep current behavior, but have a flag for |
adrianaixba
changed the title
| @@ -46,6 +54,8 @@ function getNetworkError(mainRecord) { | |||
| return new LighthouseError( | |||
| LighthouseError.errors.FAILED_DOCUMENT_REQUEST, {errorDetails: netErr}); | |||
| } | |||
| } else if (mainRecord.statusCode === 404 && context.ignoreStatusCode) { | |||
| context.warnings.push(str_(UIStrings.warning404)); | |||
| } else if (mainRecord.hasErrorStatusCode()) { | |||
There was a problem hiding this comment.
We should generalize this to all error status codes (4xx/5xx). So I think it's better to use this mainRecord.hasErrorStatusCode() block instead of doing a === 404 check.
There was a problem hiding this comment.
does generalizing this to include all error status codes change our initial thoughts on the defaults? or do they feel good still?
- devtools - ignore status code
- node - fail on status code
- cli - fail on status code
- etc
There was a problem hiding this comment.
I think the defaults should be the same as we discussed
adrianaixba
requested review from
connorjclark
and removed request for
a team
adrianaixba
changed the title
| expect(error.message).toEqual('ERRORED_DOCUMENT_REQUEST'); | ||
| expect(error.code).toEqual('ERRORED_DOCUMENT_REQUEST'); | ||
| expect(error.friendlyMessage).toBeDisplayString(/^Lighthouse was unable to reliably load.*404/); | ||
| expect(error.friendlyMessage).toBeDisplayString(/^Lighthouse was unable to reliably load/); |
There was a problem hiding this comment.
The 404 is important IMO, otherwise this could pass for one of the non-status code related page load errors.
There was a problem hiding this comment.
It would also likely be helpful to add a documentStatusCode (or something similar) to track the result easily pragmatically.
There was a problem hiding this comment.
runWarnings only accepts string type currently. I think it would be a larger change to allow extra data fields on warnings. Seems out of scope for this PR.
FWIW the bad status code is also tracked by our http-status-code audit which is easier to track programatically.
adamraine
changed the title
sample report
fixes #10493