-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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(driver): extract waitFor methods #11685
Conversation
130c60f
to
f72b2aa
Compare
* Returns a promise that resolve when a frame has been navigated. | ||
* Used for detecting that our about:blank reset has been completed. | ||
* @param {LH.Gatherer.FRProtocolSession} session | ||
* @return {CancellableWait} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the only return type change, being more consistent felt like it belonged here even if we don't use cancel
yet
* @param {number} networkQuietThresholdMs | ||
* @return {CancellableWait} | ||
*/ | ||
function waitForNetworkIdle(session, networkMonitor, networkQuietThresholdMs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extracting session
and networkMonitor
to parameters are the only substantive changes in this PR
|
||
const checkForQuietExpression = `(${pageFunctions.checkTimeSinceLastLongTaskString})()`; | ||
/** | ||
* @param {ExecutionContext} executionContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
directly using executionContext
is an FR-compat change and functionally equivalent to using driver when isolation is disabled
async function waitForFullyLoaded(session, networkMonitor, options) { | ||
const {pauseAfterFcpMs, pauseAfterLoadMs, networkQuietThresholdMs, | ||
cpuQuietThresholdMs, maxWaitForLoadedMs, maxWaitForFcpMs} = options; | ||
const {waitForFcp, waitForLoadEvent, waitForNetworkIdle, waitForCPUIdle} = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the last remaining change that exposes the underlying waits for easier testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Connor Clark <cjamcl@google.com>
Summary
Paves the way for adding the extra waitFor methods needed for #11180 in a way that still helps out with fraggle rock and makes future testing easier.
This is a move and slight restructuring to convert
this
references to parameters only. No functionality changes, and no substantive test additions (though the gaps are much more obvious and should be much easier to test now).Related Issues/PRs
ref #11313 , #11180 , #10810