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): treat NO_FCP as a pageLoadError #8340

Merged
merged 16 commits into from
May 28, 2019

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Apr 16, 2019

Summary
As discussed in chats/meetings/ #7534, we're moving NO_FCP to be a pageLoadError like NO_DOCUMENT_REQUEST/DNS_FAILURE/etc. This also does the same to PAGE_HUNG and INSECURE_DOCUMENT_REQUEST which are in identical positions of fatally throwing in loadPage.

To clarify what this means since we've been through several iterations (see #6336 for discussion):

  1. Node/CLI will write the LHR/artifacts to disk but exit with exit code 1, the LHR will have a runtimeError property set with the NO_FCP message and the HTML will have a toplevel warning with the message.
  2. DevTools/Extension will complete without error, the HTML will have a toplevel warning with the message.
  3. PSI will not return an LHR but instead a 500 status code with a payload that contains an error object with the error message.

Outstanding

We should do the same for INSECURE_DOCUMENT_REQUEST and PAGE_HUNG since they are in identical boat to NO_FCP, but it complicated the PR somewhat significantly to handle the edge cases there so punting for now.

Related Issues/PRs
fixes #7534

@patrickhulce patrickhulce changed the title core(gather-runner): set errors from loadPage as the pageLoadError core(gather-runner): treat NO_FCP as a pageLoadError Apr 16, 2019
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for NO_FCP in particular, do we need to bubble it up like this? We could shove it in runWarnings to get it at the top of the report and then the individual audits will take care of themselves.

runtimeError is also currently only populated by artifact errors. I wonder if we should be graduating some audit errors into there?

lighthouse-cli/run.js Show resolved Hide resolved
lighthouse-cli/run.js Outdated Show resolved Hide resolved
lighthouse-cli/test/smokehouse/smokehouse.js Outdated Show resolved Hide resolved
@@ -18,6 +18,7 @@ declare global {
passConfig: Config.Pass
settings: Config.Settings;
options?: object;
pageLoadError?: LighthouseError;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would give us LighthouseRunWarnings and pageLoadError passed around on the context, with runtimeError on the LHR...is there anything we can do to join up these similar but slightly different things?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename pageLoadError to futureRuntimeError? :)

all pageLoadErrors will eventually become runtimeErrors and most runtimeErrors are pageLoadErrors but it is a subset relationship

there's a big difference between LighthouseRunWarnings and the pageLoadError/runtimeError on context, I don't think those should be consolidated

lighthouse-core/gather/gather-runner.js Outdated Show resolved Hide resolved
@brendankenny
Copy link
Member

really excited to get our error handling per client finally sorted out!

@patrickhulce
Copy link
Collaborator Author

for NO_FCP in particular, do we need to bubble it up like this? We could shove it in runWarnings to get it at the top of the report and then the individual audits will take care of themselves.

IMO, handling all of these page load errors consistently is a big benefit of this direction. Also preventing the trap of PROTOCOL_TIMEOUT by not running gatherers we know in advance will just be returning useless results. What's the primary concern with bubbling into pageLoadError?

runtimeError is also currently only populated by artifact errors. I wonder if we should be graduating some audit errors into there?

Seems like a good candidate for future PR 👍

@patrickhulce
Copy link
Collaborator Author

This is currently waiting on #8964

@patrickhulce
Copy link
Collaborator Author

OK it's been rebased on top of #8964! Bring it on!

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any way I can make it sound fun to separate the NO_FCP changes from the cli changes? :D

The cli stuff wouldn't be a huge deal, but it does seem inline with your question in #8865 (comment), in that we still don't seem to have what we want completely nailed down across clients (and now there's talk of emergencyArtifacts or whatever again...). I mentioned this in the meeting on Tuesday, but I'd like to get us all in one place with a whiteboard and decide #6336 once and...maybe not for all, but at least for a long while :)

@patrickhulce
Copy link
Collaborator Author

any way I can make it sound fun to separate the NO_FCP changes from the cli changes? :D

If you say you're OK with commented out tests in the interim :)

I'd like to avoid doing a bunch of breaking changes to rework the tests only to break them right back in an immediate followup PR, i.e. we exit with an error code for NO_FCP today, we test against it, and the CLI changes ensure we still do that.

I'd like to get us all in one place with a whiteboard and decide #6336 once and...maybe not for all, but at least for a long while :)

Plus one! I'll check some flights....errrr I mean my calendar for a video call 😆

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any way I can make it sound fun to separate the NO_FCP changes from the cli changes?

after remembering that we currently have exit code 0 for a DNS_FAILURE, I now disagree with myself and think we should land this and revise after we do some planning (if we need to revise :)

Some last nits, but otherwise LGTM!

lighthouse-core/gather/gather-runner.js Show resolved Hide resolved
@@ -576,7 +596,7 @@ class GatherRunner {

// Navigate, start recording, and run `pass()` on gatherers.
await GatherRunner.beginRecording(passContext);
await GatherRunner.loadPage(driver, passContext);
const {navigationError} = await GatherRunner.loadPage(driver, passContext);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about something like const possibleNavigationError = await GatherRunner.loadPage(driver, passContext).navigationError;. Clunkier on this line but it makes it slightly more obvious that we don't always have one

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can I sell you on possibleNavError to prevent a cascade of line breaks? 😏

const {runtimeError} = runnerResult.lhr;
return printErrorAndExit({
name: 'LHError',
friendlyMessage: runtimeError.message,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to include that we've saved the results, as well? I guess we have the normal save log entries, but it could be helpful?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, where are you thinking of adding it?

I'm ideally thinking it might be like blah blah usual error message, and your assets have still be saved to disk or something, but mucking with the error message seems like more trouble than its worth

name: 'LHError',
friendlyMessage: runtimeError.message,
lhrRuntimeError: true,
...runtimeError,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the use of these two lines since the lhr doesn't give this type anything else and we're just printing the friendlyMessage and exiting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typechecking and a theoretical future of using .code to be more discerning, it's just code and message though so I'll make it explicit

@connorjclark
Copy link
Collaborator

connorjclark commented May 25, 2019

oh no I'm jumping in soo late, sry

PSI will not return an LHR but instead a 500 status code with a payload that contains an error object with the error message.

Seems fine for PSI to show a 500 for NO_FCP (there's not gonna be a useful report anyways), but I think LR should not behave this way - it should still return a LHR and, just like we're doing here, defer how to handle errors to clients / consumers.

Can we do something like:

  • PSI html viewer: don't show report, show 500 error (or even, do the DevTools treatment of report + top level warning --- why did we decided DevTools would do that but PSI wouldn't?)
  • PSI api / LR: still return LHR but with 500 as appropriate

Mainly, I'm concerned with debugging. Even if a site has NO_FCP, I'd still want to gather the artifacts and such from LR.

again sry for jumping in so late after not joining the discussion earlier :(

@patrickhulce
Copy link
Collaborator Author

Can we do something like:
PSI html viewer: don't show report, show 500 error (or even, do the DevTools treatment of report + top level warning --- why did we decided DevTools would do that but PSI wouldn't?)
PSI api / LR: still return LHR but with 500 as appropriate

That all sounds fine to me! My primary concern would be with being consistent to return an error status code like how we would exit with an error code from CLI. Your second bullet addresses this nicely :)

Just to clarify this PR shouldn't actually change the status quo from LR's perspective. NO_FCP is currently fatal and now it will be not fatal but still runtimeError. We can move runtimeErrors in LR to also return an LHR when available separately 👍

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.

Running with headless Chrome crashes when provided with non-existent url.
3 participants