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.

Sign up for GitHub

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

Jump to bottom

core(driver): add waitForFCP timeout #7356

Merged
merged 8 commits into from
Mar 5, 2019
Merged

core(driver): add waitForFCP timeout #7356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ac5f9f4
core(driver): add waitForFCP timeout
patrickhulce Mar 1, 2019
7905fe5
extra test for fix
patrickhulce Mar 1, 2019
a571bf0
snapshots
patrickhulce Mar 1, 2019
e69e3f1
Update types/externs.d.ts
brendankenny Mar 2, 2019
83c043e
lower to 15s
patrickhulce Mar 2, 2019
9c708e4
gzip fix
patrickhulce Mar 2, 2019
5030864
Merge branch 'master' into max_wait_for_fcp
patrickhulce Mar 5, 2019
cbce736
Merge branch 'master' into max_wait_for_fcp
patrickhulce Mar 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1208,6 +1208,7 @@ Object {
"extraHeaders": null,
"gatherMode": false,
"locale": "en-US",
"maxWaitForFcp": 15000,
"maxWaitForLoad": 45000,
"onlyAudits": null,
"onlyCategories": null,
Expand Down Expand Up @@ -1336,6 +1337,7 @@ Object {
"extraHeaders": null,
"gatherMode": false,
"locale": "en-US",
"maxWaitForFcp": 15000,
"maxWaitForLoad": 45000,
"onlyAudits": Array [
"metrics",
Expand Down
1 change: 1 addition & 0 deletions lighthouse-cli/test/fixtures/byte-efficiency/gzip.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,6 @@
<script src="script.js"></script>
</head>
<body>
GZIP FTW!
</body>
</html>
1 change: 1 addition & 0 deletions lighthouse-core/config/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ const throttling = {
/** @type {LH.Config.Settings} */
const defaultSettings = {
output: 'json',
maxWaitForFcp: 15 * 1000,
maxWaitForLoad: 45 * 1000,
throttlingMethod: 'simulate',
throttling: throttling.mobileSlow4G,
Expand Down
33 changes: 24 additions & 9 deletions lighthouse-core/gather/driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,20 +562,25 @@ class Driver {

/**
* Returns a promise that resolve when a frame has a FCP.
* @param {number} maxWaitForFCPMs
* @return {{promise: Promise<void>, cancel: function(): void}}
*/
_waitForFCP() {
_waitForFCP(maxWaitForFCPMs) {
/** @type {(() => void)} */
let cancel = () => {
throw new Error('_waitForFCP.cancel() called before it was defined');
};

const promise = new Promise(resolve => {
const promise = new Promise((resolve, reject) => {
const maxWaitTimeout = setTimeout(() => {
reject(new LHError(LHError.errors.NO_FCP));
}, maxWaitForFCPMs);

/** @param {LH.Crdp.Page.LifecycleEventEvent} e */
const lifecycleListener = e => {
if (e.name === 'firstContentfulPaint') {
cancel();
resolve();
cancel();
}
};

Expand All @@ -586,6 +591,8 @@ class Driver {
if (canceled) return;
canceled = true;
this.off('Page.lifecycleEvent', lifecycleListener);
maxWaitTimeout && clearTimeout(maxWaitTimeout);
reject(new Error('Wait for FCP canceled'));
};
});

Expand Down Expand Up @@ -857,17 +864,17 @@ class Driver {
* @param {number} networkQuietThresholdMs
* @param {number} cpuQuietThresholdMs
* @param {number} maxWaitForLoadedMs
* @param {boolean} shouldWaitForFCP
* @param {number=} maxWaitForFCPMs
* @return {Promise<void>}
* @private
*/
async _waitForFullyLoaded(pauseAfterLoadMs, networkQuietThresholdMs, cpuQuietThresholdMs,
maxWaitForLoadedMs, shouldWaitForFCP) {
maxWaitForLoadedMs, maxWaitForFCPMs) {
/** @type {NodeJS.Timer|undefined} */
let maxTimeoutHandle;

// Listener for onload. Resolves on first FCP event.
const waitForFCP = shouldWaitForFCP ? this._waitForFCP() : this._waitForNothing();
const waitForFCP = maxWaitForFCPMs ? this._waitForFCP(maxWaitForFCPMs) : this._waitForNothing();
// Listener for onload. Resolves pauseAfterLoadMs ms after load.
const waitForLoadEvent = this._waitForLoadEvent(pauseAfterLoadMs);
// Network listener. Resolves when the network has been idle for networkQuietThresholdMs.
Expand Down Expand Up @@ -895,6 +902,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.
Copy link
Member

Choose a reason for hiding this comment

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

this isn't getting easier to follow... :P

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, it's not, but it's very well tested now and ready for refactoring :)

return function() {
throw err;
};
});

// Last resort timeout. Resolves maxWaitForLoadedMs ms from now on
Expand Down Expand Up @@ -1038,7 +1050,7 @@ class Driver {
await this.sendCommand('Page.enable');
await this.sendCommand('Page.setLifecycleEventsEnabled', {enabled: true});
await this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS});
// No timeout needed for Page.navigate. See #6413.
// No timeout needed for Page.navigate. See https://github.com/GoogleChrome/lighthouse/pull/6413.
const waitforPageNavigateCmd = this._innerSendCommand('Page.navigate', {url});

if (waitForNavigated) {
Expand All @@ -1047,19 +1059,22 @@ class Driver {
const passConfig = /** @type {Partial<LH.Config.Pass>} */ (passContext.passConfig || {});
let {pauseAfterLoadMs, networkQuietThresholdMs, cpuQuietThresholdMs} = passConfig;
let maxWaitMs = passContext.settings && passContext.settings.maxWaitForLoad;
let maxFCPMs = passContext.settings && passContext.settings.maxWaitForFcp;

/* eslint-disable max-len */
if (typeof pauseAfterLoadMs !== 'number') pauseAfterLoadMs = DEFAULT_PAUSE_AFTER_LOAD;
if (typeof networkQuietThresholdMs !== 'number') networkQuietThresholdMs = DEFAULT_NETWORK_QUIET_THRESHOLD;
if (typeof cpuQuietThresholdMs !== 'number') cpuQuietThresholdMs = DEFAULT_CPU_QUIET_THRESHOLD;
if (typeof maxWaitMs !== 'number') maxWaitMs = constants.defaultSettings.maxWaitForLoad;
if (typeof maxFCPMs !== 'number') maxFCPMs = constants.defaultSettings.maxWaitForFcp;
/* eslint-enable max-len */

if (!waitForFCP) maxFCPMs = undefined;
Copy link
Member

Choose a reason for hiding this comment

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

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

await this._waitForFullyLoaded(pauseAfterLoadMs, networkQuietThresholdMs, cpuQuietThresholdMs,
maxWaitMs, waitForFCP);
maxWaitMs, maxFCPMs);
}

// Bring `Page.navigate` errors back into the promise chain. See #6739.
// Bring `Page.navigate` errors back into the promise chain. See https://github.com/GoogleChrome/lighthouse/pull/6739.
await waitforPageNavigateCmd;

return this._endNetworkStatusMonitoring();
Expand Down
65 changes: 65 additions & 0 deletions lighthouse-core/test/gather/driver-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,19 @@ describe('.gotoURL', () => {
expect(driver._waitForCPUIdle.getMockCancelFn()).toHaveBeenCalled();
});

it('should cleanup listeners even when waits reject', async () => {
driver._waitForLoadEvent = createMockWaitForFn();

const loadPromise = makePromiseInspectable(driver.gotoURL(url, {waitForLoad: true}));

driver._waitForLoadEvent.mockReject();
await flushAllTimersAndMicrotasks();
expect(loadPromise).toBeDone('Did not reject load promise when load rejected');
await expect(loadPromise).rejects.toBeTruthy();
// Make sure we still cleaned up our listeners
expect(driver._waitForLoadEvent.getMockCancelFn()).toHaveBeenCalled();
});

it('does not reject when page is secure', async () => {
const secureSecurityState = {
explanations: [],
Expand Down Expand Up @@ -740,6 +753,58 @@ describe('.gotoURL', () => {
});
});

describe('._waitForFCP', () => {
it('should not resolve until FCP fires', async () => {
driver.on = driver.once = createMockOnceFn();

const waitPromise = makePromiseInspectable(driver._waitForFCP(60 * 1000).promise);
const listener = driver.on.findListener('Page.lifecycleEvent');

await flushAllTimersAndMicrotasks();
expect(waitPromise).not.toBeDone('Resolved without FCP');

listener({name: 'domContentLoaded'});
await flushAllTimersAndMicrotasks();
expect(waitPromise).not.toBeDone('Resolved on wrong event');

listener({name: 'firstContentfulPaint'});
await flushAllTimersAndMicrotasks();
expect(waitPromise).toBeDone('Did not resolve with FCP');
await waitPromise;
});

it('should timeout', async () => {
driver.on = driver.once = createMockOnceFn();

const waitPromise = makePromiseInspectable(driver._waitForFCP(5000).promise);

await flushAllTimersAndMicrotasks();
expect(waitPromise).not.toBeDone('Resolved before timeout');

jest.advanceTimersByTime(5001);
await flushAllTimersAndMicrotasks();
expect(waitPromise).toBeDone('Did not resolve after timeout');
await expect(waitPromise).rejects.toMatchObject({code: 'NO_FCP'});
});

it('should be cancellable', async () => {
driver.on = driver.once = createMockOnceFn();
driver.off = jest.fn();

const {promise: rawPromise, cancel} = driver._waitForFCP(5000);
const waitPromise = makePromiseInspectable(rawPromise);

await flushAllTimersAndMicrotasks();
expect(waitPromise).not.toBeDone('Resolved before timeout');

cancel();
await flushAllTimersAndMicrotasks();
expect(waitPromise).toBeDone('Did not cancel promise');
expect(driver.off).toHaveBeenCalled();
await expect(waitPromise).rejects.toMatchObject({message: 'Wait for FCP canceled'});
});
});

describe('.assertNoSameOriginServiceWorkerClients', () => {
beforeEach(() => {
connectionStub.sendCommand = createMockSendCommandFn()
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/test/results/sample_v2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3164,6 +3164,7 @@
"output": [
"json"
],
"maxWaitForFcp": 15000,
"maxWaitForLoad": 45000,
"throttlingMethod": "devtools",
"throttling": {
Expand Down
2 changes: 2 additions & 0 deletions types/externs.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ declare global {
output?: OutputMode|OutputMode[];
/** The locale to use for the output. */
locale?: Locale;
/** The maximum amount of time to wait for a page content render, in ms. If no content is rendered within this limit, the run is aborted with an error. */
maxWaitForFcp?: number;
/** The maximum amount of time to wait for a page to load, in ms. */
maxWaitForLoad?: number;
/** List of URL patterns to block. */
Expand Down