Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(gather-runner): include error status codes in pageLoadError #6051

Merged
merged 4 commits into from
Sep 18, 2018

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Sep 18, 2018

Adds a pageLoadError for 400/500 http status codes. Fixes #4082; depends on #6014 (see dcb468a for actual diff).

Example Report
screen shot 2018-09-18 at 12 32 48

Good:

  • catches the error
  • warns user at the top of the report that it happened and they should fix their URL
  • issues runtimeError

Less good:

  • redundant with the http-status-code SEO audit (new SEO audit: HTTP status code #3311). That audit successfully marks the page as 404ing (or whatever), but all the other audits around it are marked as Error! (since no gatherers in that pass return an artifact), so it's pretty much lost in the noise

screen shot 2018-09-18 at 12 31 02

ref #5881

@brendankenny
Copy link
Member Author

The redundancy might be fine, and we can just update the smoke test expectations, but it is another example of the tension between automatically failing gatherers (or not) in pageLoadError cases.

On the one hand, why is Lighthouse wasting time auditing stuff that was plainly in error (nothing like getting results for the "There was no service found for the uri requested." page)? On the other, why should Lighthouse care? Just warn me prominently but in the meantime give me the results for the page I asked for.

@paulirish
Copy link
Member

HUGE. 👏
This also removes the need for the only remaining patch that LR applies on top of LH.

@brendankenny
Copy link
Member Author

After discussion, we're going to maintain the change mostly as is.

I've split the SEO expectations into separate runs for HTTP 403 and other failures. That way we still get the end-to-end request failure test (including that it spawns error artifacts -> audit errors) while not losing all the other useful failure results to Error!.

@brendankenny
Copy link
Member Author

appveyor failure is a transient chromestatus issue (no manifest found). Other error is bundlesize, which all the PRs are facing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add top level warning if page returned an error status code
2 participants