Skip to content

Commit

Permalink
fix: always ensure tracing is off before starting (#2279)
Browse files Browse the repository at this point in the history
* fix: always ensure tracing is off before starting

* add test

* change bool to object of options

* add default options

* feedback
  • Loading branch information
patrickhulce committed May 25, 2017
1 parent e332691 commit 48b72a8
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 5 deletions.
8 changes: 5 additions & 3 deletions lighthouse-core/gather/connections/connection.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,16 @@ class Connection {
* Call protocol methods
* @param {!string} method
* @param {!Object} params
* @param {{silent: boolean}=} cmdOpts
* @return {!Promise}
*/
sendCommand(method, params = {}) {
sendCommand(method, params = {}, cmdOpts = {}) {
log.formatProtocol('method => browser', {method, params}, 'verbose');
const id = ++this._lastCommandId;
const message = JSON.stringify({id, method, params});
this.sendRawMessage(message);
return new Promise((resolve, reject) => {
this._callbacks.set(id, {resolve, reject, method});
this._callbacks.set(id, {resolve, reject, method, options: cmdOpts});
});
}

Expand Down Expand Up @@ -98,7 +99,8 @@ class Connection {

return callback.resolve(Promise.resolve().then(_ => {
if (object.error) {
log.formatProtocol('method <= browser ERR', {method: callback.method}, 'error');
const logLevel = callback.options && callback.options.silent ? 'verbose' : 'error';
log.formatProtocol('method <= browser ERR', {method: callback.method}, logLevel);
throw new Error(`Protocol error (${callback.method}): ${object.error.message}`);
}

Expand Down
19 changes: 17 additions & 2 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -187,9 +187,10 @@ class Driver {
* Call protocol methods
* @param {!string} method
* @param {!Object} params
* @param {{silent: boolean}=} cmdOpts
* @return {!Promise}
*/
sendCommand(method, params) {
sendCommand(method, params, cmdOpts) {
const domainCommand = /^(\w+)\.(enable|disable)$/.exec(method);
if (domainCommand) {
const enable = domainCommand[2] === 'enable';
Expand All @@ -198,7 +199,7 @@ class Driver {
}
}

return this._connection.sendCommand(method, params);
return this._connection.sendCommand(method, params, cmdOpts);
}

/**
Expand Down Expand Up @@ -713,9 +714,23 @@ class Driver {

// Enable Page domain to wait for Page.loadEventFired
return this.sendCommand('Page.enable')
// ensure tracing is stopped before we can start
// see https://github.com/GoogleChrome/lighthouse/issues/1091
.then(_ => this.endTraceIfStarted())
.then(_ => this.sendCommand('Tracing.start', tracingOpts));
}

endTraceIfStarted() {
return new Promise((resolve) => {
const traceCallback = () => resolve();
this.once('Tracing.tracingComplete', traceCallback);
return this.sendCommand('Tracing.end', undefined, {silent: true}).catch(() => {
this.off('Tracing.tracingComplete', traceCallback);
traceCallback();
});
});
}

endTrace() {
return new Promise((resolve, reject) => {
// When the tracing has ended this will fire with a stream handle.
Expand Down
17 changes: 17 additions & 0 deletions lighthouse-core/test/gather/driver-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ connection.sendCommand = function(command, params) {
case 'ServiceWorker.enable':
case 'ServiceWorker.disable':
return Promise.resolve();
case 'Tracing.end':
return Promise.reject(new Error('tracing not started'));
default:
throw Error(`Stub not implemented: ${command}`);
}
Expand Down Expand Up @@ -186,6 +188,21 @@ describe('Browser Driver', () => {
});
});

it('waits for tracingComplete when tracing already started', () => {
const fakeConnection = new Connection();
const fakeDriver = new Driver(fakeConnection);
const commands = [];
fakeConnection.sendCommand = evt => {
commands.push(evt);
return Promise.resolve();
};

fakeDriver.once = createOnceStub({'Tracing.tracingComplete': {}});
return fakeDriver.beginTrace().then(() => {
assert.deepEqual(commands, ['Page.enable', 'Tracing.end', 'Tracing.start']);
});
});

it('will request default traceCategories', () => {
return driverStub.beginTrace().then(() => {
const traceCmd = sendCommandParams.find(obj => obj.command === 'Tracing.start');
Expand Down

0 comments on commit 48b72a8

Please sign in to comment.