core(gather-runner): treat NO_FCP as a pageLoadError #8340

patrickhulce · 2019-04-16T21:03:04Z

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):

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.
DevTools/Extension will complete without error, the HTML will have a toplevel warning with the message.
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

brendankenny

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

lighthouse-cli/test/smokehouse/smokehouse.js

brendankenny · 2019-04-19T23:51:32Z

types/gatherer.d.ts

@@ -18,6 +18,7 @@ declare global {
      passConfig: Config.Pass
      settings: Config.Settings;
      options?: object;
+      pageLoadError?: LighthouseError;


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?

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

brendankenny · 2019-04-20T00:07:42Z

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

patrickhulce · 2019-04-20T03:36:47Z

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 · 2019-05-20T23:19:14Z

This is currently waiting on #8964

patrickhulce · 2019-05-21T20:03:23Z

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

brendankenny

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 · 2019-05-23T14:40:35Z

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 😆

brendankenny

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

brendankenny · 2019-05-24T23:58:38Z

lighthouse-core/gather/gather-runner.js

@@ -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);


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

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

brendankenny · 2019-05-25T00:12:52Z

lighthouse-cli/run.js

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


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?

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

brendankenny · 2019-05-25T00:13:12Z

lighthouse-cli/run.js

+        name: 'LHError',
+        friendlyMessage: runtimeError.message,
+        lhrRuntimeError: true,
+        ...runtimeError,


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?

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 · 2019-05-25T03:22:14Z

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 · 2019-05-25T04:06:20Z

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 👍

patrickhulce added 2 commits April 16, 2019 14:41

core(gather-runner): turn NO_FCP into pageLoadError

e8b2044

use lhr runtime error instead

0c377e6

patrickhulce requested a review from paulirish as a code owner April 16, 2019 21:03

smoke fixes

499ae5c

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

Merge branch 'master' into no_fcp_page_load_error

a86e53e

vercel bot deployed to staging April 16, 2019 21:32 View deployment

patrickhulce mentioned this pull request Apr 16, 2019

Running with headless Chrome crashes when provided with non-existent url. #7534

Closed

patrickhulce added 3 commits April 17, 2019 10:11

lint

9644598

Merge branch 'master' into no_fcp_page_load_error

5a57689

add back LighthouseError type

492360d

vercel bot deployed to staging April 17, 2019 15:19 View deployment

bring back smokes

8ca051b

patrickhulce requested a review from brendankenny as a code owner April 17, 2019 16:00

vercel bot deployed to staging April 17, 2019 16:00 View deployment

patrickhulce mentioned this pull request Apr 19, 2019

Don't fail localhost pages with INSECURE #6655

Closed

brendankenny reviewed Apr 20, 2019

View reviewed changes

patrickhulce added 3 commits April 19, 2019 22:37

feedback

6348130

Merge branch 'master' into no_fcp_page_load_error

c815f80

lint

c0e2da9

vercel bot deployed to staging April 20, 2019 03:39 View deployment

This was referenced May 5, 2019

core(driver): security errors are no longer a fatal or pageload error #8865

Merged

core: bail on gathering if we have a failure in the first pass #8866

Closed

brendankenny mentioned this pull request May 15, 2019

core: refactor gather-runner #8964

Merged

patrickhulce added 3 commits May 21, 2019 14:23

Merge branch 'master' into no_fcp_page_load_error

4e9a2e2

more merge fixes

16f82fc

update tests

8e3d091

lint

771e8f2

vercel bot deployed to staging May 21, 2019 20:01 View deployment

brendankenny reviewed May 22, 2019

View reviewed changes

brendankenny approved these changes May 25, 2019

View reviewed changes

feedback

6310a18

vercel bot deployed to staging May 25, 2019 04:07 View deployment

brendankenny merged commit ff4f486 into master May 28, 2019

brendankenny deleted the no_fcp_page_load_error branch May 28, 2019 23:55

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(gather-runner): treat NO_FCP as a pageLoadError #8340

core(gather-runner): treat NO_FCP as a pageLoadError #8340

core(gather-runner): treat NO_FCP as a pageLoadError #8340

core(gather-runner): treat NO_FCP as a pageLoadError #8340

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment