core(driver): pause after FCP event before resolving load #10505
Conversation
Co-Authored-By: Connor Clark <cjamcl@google.com>
|
Alright @mattzeunert I was 2/2 on reproing that URL before this change and 0/10 with this change. Any more complicated URL that prompted your investigation you want to try before we merge? :) |
|
Looks good to me. Thanks for the quick fix @patrickhulce! |
| @@ -26,6 +26,22 @@ module.exports = [ | |||
| }, | |||
| }, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
Is it suspicious that the above expectations didn't change after dropping any throttling? Were the bounds just really really loose? Should we switch to a ± assertion for them?
| }, | ||
| 'interactive': { | ||
| // FCP at ~5 seconds out | ||
| numericValue: '>4900', |
There was a problem hiding this comment.
same for these bounds...doesn't really differentiate between ~5 seconds and ~30 seconds, but will flakiness prevent us from putting a useful upper bound?
| @@ -65,7 +65,7 @@ const smokeTests = [{ | |||
| }, { | |||
| id: 'metrics', | |||
| expectations: require('./tricky-metrics/expectations.js'), | |||
| config: require('../../../../lighthouse-core/config/perf-config.js'), | |||
| config: require('../../../../lighthouse-core/config/no-throttling-config.js'), | |||
There was a problem hiding this comment.
maybe it would be good to document why this config is being used here for future reference? e.g. I'm pretty sure we only used perf-config here because it was a convenient, preexisting config to check out the perf category with devtools throttling, but if there were more specific tricky reasons as well, I would have long forgotten them by now :)
(I assume from the comment in tricky-tti-late-fcp.html the test would fail if switching to a different throttling, but that might not always be the case)
There was a problem hiding this comment.
maybe it would be good to document why this config is being used here for future reference?
Or would that comment make more sense in the expectations file instead? Since it would allow a comment per test url, but authors are still likely to look there if changing things (whereas they might not look at comments in the html fixture until a test actually starts failing).
Don't throw things at me if this is yet another thing your smokehouse refactor would have made easy 😬
| @@ -83,6 +83,7 @@ const defaultPassConfig = { | |||
| loadFailureMode: 'fatal', | |||
| recordTrace: false, | |||
| useThrottling: false, | |||
| pauseAfterFcpMs: 0, | |||
There was a problem hiding this comment.
we should probably document sometime why these are defaulted to 0 in here but then all have non-0 defaults in default-config.js :)
| @@ -0,0 +1,17 @@ | |||
| /** | |||
There was a problem hiding this comment.
is the intention for this to be for testing only? Consider moving to lighthouse-cli/test/smokehouse/test-definitions/?
| 'use strict'; | ||
|
|
||
| /** @type {LH.Config.Json} */ | ||
| const perfConfig = { |
There was a problem hiding this comment.
rename? (we already ended up with two perf-config.jss already, and I think I'm responsible for that terrible decision, but we can at least avoid a third perfConfig local name :)
| @@ -649,10 +651,11 @@ class Driver { | |||
|
|
|||
| /** | |||
| * Returns a promise that resolve when a frame has a FCP. | |||
| * @param {number} pauseAfterFcpMs | |||
| * @param {number} maxWaitForFCPMs | |||
There was a problem hiding this comment.
* @param {number} pauseAfterFcpMs
* @param {number} maxWaitForFCPMs🥴capitalization
| let maxWaitMs = passContext.settings && passContext.settings.maxWaitForLoad; | ||
| let maxFCPMs = passContext.settings && passContext.settings.maxWaitForFcp; | ||
|
|
||
| /* eslint-disable max-len */ | ||
| if (typeof pauseAfterFcpMs !== 'number') pauseAfterFcpMs = DEFAULT_PAUSE_AFTER_FCP; |
There was a problem hiding this comment.
we should really clean this up as well, after all the effort we go through to get our settings and constants in order before loading a page :)
It looks like these will not be set only if a passContext wasn't passed in, and we don't pass in a passContext only when we're loading blankPage, which ends up mostly doing _waitForNothing, so it doesn't need defaults for these anyways :)
| @@ -92,6 +93,7 @@ const defaultPassConfig = { | |||
| }; | |||
|
|
|||
| const nonSimulatedPassConfigOverrides = { | |||
| pauseAfterFcpMs: 5250, | |||
| pauseAfterLoadMs: 5250, | |||
| networkQuietThresholdMs: 5250, | |||
| cpuQuietThresholdMs: 5250, | |||
There was a problem hiding this comment.
maybe we could have gotten away with a single pauseAfterLoadingLandmark that just waits that much after the last of these, but there's probably unavoidable trickery in there, and that ship sailed a long time ago regardless
Summary
TTI needs at least 5s of quiet after FCP in order to be computed for observed metrics. Currently we wait for 5s after load, network quiet, and CPU quiet and in a vast majority of cases at least one of these occurs at or after FCP so it doesn't matter, but in some cases it does. This PR updates our logic to wait 5s after FCP as well before resolving the page as "loaded"
Related Issues/PRs
fixes #10503