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.

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

feat: configurable page timeout #1672

Merged
merged 6 commits into from
Feb 16, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
13 changes: 11 additions & 2 deletions lighthouse-cli/bin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {ChromeLauncher} from './chrome-launcher';
import * as Commands from './commands/commands';
const lighthouse = require('../lighthouse-core');
const log = require('../lighthouse-core/lib/log');
const Driver = require('../lighthouse-core/gather/driver.js');
import * as path from 'path';
const perfOnlyConfig = require('../lighthouse-core/config/perf.json');
const performanceXServer = require('./performance-experiment/server');
Expand Down Expand Up @@ -62,7 +63,8 @@ const cliFlags = yargs
'list-trace-categories',
'config-path',
'perf',
'port'
'port',
'max-wait-for-load'
], 'Configuration:')
.describe({
'disable-device-emulation': 'Disable Nexus 5X emulation',
Expand All @@ -75,6 +77,7 @@ const cliFlags = yargs
'config-path': 'The path to the config JSON.',
'perf': 'Use a performance-test-only configuration',
'port': 'The port to use for the debugging protocol. Use 0 for a random port',
'max-wait-for-load': 'The timeout (in milliseconds) to wait before the page is considered done loading and the run should continue. WARNING: Very high values can lead to large traces and instability',
'skip-autolaunch': 'Skip autolaunch of Chrome when already running instance is not found',
'select-chrome': 'Interactively choose version of Chrome to use when multiple installations are found',
'interactive': 'Open Lighthouse in interactive mode'
Expand Down Expand Up @@ -114,6 +117,7 @@ Example: --output-path=./lighthouse-results.html`
.default('output', Printer.GetValidOutputOptions()[Printer.OutputMode.pretty])
.default('output-path', 'stdout')
.default('port', 9222)
.default('max-wait-for-load', Driver.MAX_WAIT_FOR_FULLY_LOADED)
.check((argv: {listAllAudits?: boolean, listTraceCategories?: boolean, _: Array<any>}) => {
// Make sure lighthouse has been passed a url, or at least one of --list-all-audits
// or --list-trace-categories. If not, stop the program and ask for a url
Expand Down Expand Up @@ -143,6 +147,10 @@ if (!cliFlags.outputPath && cliFlags['output-path']) {
cliFlags.outputPath = cliFlags['output-path'];
}

if (!cliFlags.maxWaitForLoad && cliFlags['max-wait-for-load']) {
Copy link
Member

Choose a reason for hiding this comment

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

how would this be possible? AFAIK, yargs creates the former and makes sure they match at this point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The bug as mentioned in the above identical check makes this happen when the default is used.

Copy link
Contributor

Choose a reason for hiding this comment

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

How do we feel about updating yargs? Current is 6.6.0 :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What was the google3 dependency on yargs that kept us here, are they game to update?

Copy link
Contributor

Choose a reason for hiding this comment

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

suspect that's it. Just complaining out loud that that restriction continues to hold us back.

Copy link
Member

Choose a reason for hiding this comment

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

Let's bump yargs from our 3.30.0 to 3.32.0.
It has the fix for these: yargs/yargs@51c0063

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, from the looks of it google3 is still on lighthouse ~1.1 so we likely will have quite a few other issues to iron out when they want to upgrade anyway...

cliFlags.maxWaitForLoad = cliFlags['max-wait-for-load'];
}

let config: Object | null = null;
if (cliFlags.configPath) {
// Resolve the config file path relative to where cli was called.
Expand Down Expand Up @@ -278,7 +286,8 @@ function saveResults(results: Results,

function runLighthouse(url: string,
flags: {port: number, skipAutolaunch: boolean, selectChrome: boolean, output: any,
outputPath: string, interactive: boolean, saveArtifacts: boolean, saveAssets: boolean},
outputPath: string, interactive: boolean, saveArtifacts: boolean, saveAssets: boolean
maxWaitForLoad: number},
config: Object): Promise<undefined> {

let chromeLauncher: ChromeLauncher;
Expand Down
26 changes: 15 additions & 11 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@ const URL = require('../lib/url-shim');
const log = require('../lib/log.js');
const DevtoolsLog = require('./devtools-log');

const MAX_WAIT_FOR_FULLY_LOADED = 25 * 1000;
const PAUSE_AFTER_LOAD = 500;

class Driver {
static get MAX_WAIT_FOR_FULLY_LOADED() {
return 25 * 1000;
}

/**
* @param {!Connection} connection
Expand Down Expand Up @@ -417,13 +419,14 @@ class Driver {
* Returns a promise that resolves when:
* - it's been pauseAfterLoadMs milliseconds after both onload and the network
* has gone idle, or
* - MAX_WAIT_FOR_FULLY_LOADED milliseconds have passed.
* - maxWaitForLoadedMs milliseconds have passed.
* See https://github.com/GoogleChrome/lighthouse/issues/627 for more.
* @param {number} pauseAfterLoadMs
* @param {number} maxWaitForLoadedMs
* @return {!Promise}
* @private
*/
_waitForFullyLoaded(pauseAfterLoadMs) {
_waitForFullyLoaded(pauseAfterLoadMs, maxWaitForLoadedMs) {
let maxTimeoutHandle;

// Listener for onload. Resolves pauseAfterLoadMs ms after load.
Expand All @@ -443,10 +446,10 @@ class Driver {
};
});

// Last resort timeout. Resolves MAX_WAIT_FOR_FULLY_LOADED ms from now on
// Last resort timeout. Resolves maxWaitForLoadedMs ms from now on
// cleanup function that removes loadEvent and network idle listeners.
const maxTimeoutPromise = new Promise((resolve, reject) => {
maxTimeoutHandle = setTimeout(resolve, MAX_WAIT_FOR_FULLY_LOADED);
maxTimeoutHandle = setTimeout(resolve, maxWaitForLoadedMs);
}).then(_ => {
return function() {
log.warn('Driver', 'Timed out waiting for page load. Moving on...');
Expand All @@ -472,16 +475,17 @@ class Driver {
* @param {!Object} options
* @return {!Promise}
*/
gotoURL(url, options) {
const _options = options || {};
const waitForLoad = _options.waitForLoad || false;
const disableJS = _options.disableJavaScript || false;
const pauseAfterLoadMs = (_options.flags && _options.flags.pauseAfterLoad) || PAUSE_AFTER_LOAD;
gotoURL(url, options = {}) {
const waitForLoad = options.waitForLoad || false;
const disableJS = options.disableJavaScript || false;
const pauseAfterLoadMs = (options.flags && options.flags.pauseAfterLoad) || PAUSE_AFTER_LOAD;
const maxWaitMs = (options.flags && options.flags.maxWaitForLoad) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

should we support --max-wait-for-load = 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would say that would lead to a useless lighthouse run that wouldn't return any results so filling in default is fine with me :)

Driver.MAX_WAIT_FOR_FULLY_LOADED;

return this.sendCommand('Page.enable')
.then(_ => this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS}))
.then(_ => this.sendCommand('Page.navigate', {url}))
.then(_ => waitForLoad && this._waitForFullyLoaded(pauseAfterLoadMs));
.then(_ => waitForLoad && this._waitForFullyLoaded(pauseAfterLoadMs, maxWaitMs));
}

/**
Expand Down
8 changes: 7 additions & 1 deletion readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,12 @@ Configuration:
--list-trace-categories Prints a list of all required trace categories and exits [boolean]
--config-path The path to the config JSON.
--perf Use a performance-test-only configuration [boolean]
--port The port to use for the debugging protocol. Use 0 for a
random port. [default: 9222]
--max-wait-for-load The timeout (in milliseconds) to wait before the page is
considered done loading and the run should continue.
WARNING: Very high values can lead to large traces and
instability. [default: 25000]

Output:
--output Reporter for the results
Expand Down Expand Up @@ -176,7 +182,7 @@ Options:
* `./report.json`
* `./report.artifacts.log`

`--save-artifacts` prints a pretty report to `stdout` **and** generates
`--save-artifacts` prints a pretty report to `stdout` **and** generates
* `./<HOST>_<DATE>.report.html`
* `./<HOST>_<DATE>.artifacts.log`

Expand Down