core(driver): add waitForFCP timeout #7356
Conversation
lighthouse-core/config/constants.js
Outdated
| @@ -42,6 +42,7 @@ const throttling = { | |||
| /** @type {LH.Config.Settings} */ | |||
| const defaultSettings = { | |||
| output: 'json', | |||
| maxWaitForFcp: 30 * 1000, | |||
There was a problem hiding this comment.
open to other defaults, 20s maybe? not sure what's reasonable
There was a problem hiding this comment.
That'll probably be too low when using real throttling though, no? :/
how about 15?
|
sure
|
| /* eslint-enable max-len */ | ||
|
|
||
| if (!waitForFCP) maxFCPMs = undefined; |
There was a problem hiding this comment.
it's a little confusing that there are two similar-but-different parameters for this, but this way does seem to make the most sense from both gather-runner and driver's perspectives, so I guess this is where we live now :)
| @@ -881,6 +888,11 @@ class Driver { | |||
| return function() { | |||
| log.verbose('Driver', 'loadEventFired and network considered idle'); | |||
| }; | |||
| }).catch(err => { | |||
| // Throw the error in the cleanupFn so we still cleanup all our handlers. | |||
There was a problem hiding this comment.
this isn't getting easier to follow... :P
There was a problem hiding this comment.
no, it's not, but it's very well tested now and ready for refactoring :)
|
NO_FCP in the smoke tests, though, and that's for the 30s timeout? 😟 |
Co-Authored-By: patrickhulce <patrick.hulce@gmail.com>
yeah this makes me pretty nervous :/ |
|
oh @brendankenny the FCP timeout was a legit issue with one of the smoketests having no content, now this fails early instead of letting the audits fail |
|
I think this is likely to cause issues when "real" throttling is used… (but of course I have no idea). |
Yeah this was my concern with 10s. I'm not sure I've seen beyond more than 15s, but maybe this is more of a breaking change from that perspective... |
Summary
Adds a
maxWaitForFcpoption that can optionally timeout the wait for FCP trigger earlier than overall load.Related Issues/PRs
closes #7174