-
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
Stop the trace when multiple tabs are found. #639
Changes from all commits
2a9d09b
eb0d9a9
e21fd75
065d327
5d571f4
cc4b7b4
40919bb
5f7f0a6
acc3c4f
fcf2b0b
3b4379e
809c565
3d2d822
a6238f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,9 +27,10 @@ const path = require('path'); | |
* 1. Setup | ||
* A. driver.connect() | ||
* B. GatherRunner.setupDriver() | ||
* i. beginEmulation | ||
* ii. cleanAndDisableBrowserCaches | ||
* iii. clearDataForOrigin | ||
* i. checkForMultipleTabsAttached | ||
* ii. beginEmulation | ||
* iii. cleanAndDisableBrowserCaches | ||
* iiii. clearDataForOrigin | ||
* | ||
* 2. For each pass in the config: | ||
* A. GatherRunner.beforePass() | ||
|
@@ -85,12 +86,20 @@ class GatherRunner { | |
static setupDriver(driver, options) { | ||
log.log('status', 'Initializing…'); | ||
// Enable emulation based on flags | ||
return driver.beginEmulation(options.flags) | ||
return driver.checkForMultipleTabsAttached(options.url) | ||
.then(_ => driver.beginEmulation(options.flags)) | ||
.then(_ => driver.enableRuntimeEvents()) | ||
.then(_ => driver.cleanAndDisableBrowserCaches()) | ||
.then(_ => driver.clearDataForOrigin(options.url)); | ||
} | ||
|
||
static disposeDriver(driver) { | ||
// We dont need to hold up the reporting for the reload/disconnect, | ||
// so we will not return a promise in here. | ||
log.log('status', 'Disconnecting from browser...'); | ||
driver.disconnect(); | ||
} | ||
|
||
/** | ||
* Navigates to about:blank and calls beforePass() on gatherers before tracing | ||
* has started and before navigation to the target page. | ||
|
@@ -240,10 +249,8 @@ class GatherRunner { | |
options.url = urlAfterRedirects; | ||
}); | ||
}) | ||
.then(_ => GatherRunner.disposeDriver(driver)) | ||
.then(_ => { | ||
log.log('status', 'Disconnecting from browser...'); | ||
return driver.disconnect(); | ||
}).then(_ => { | ||
// Collate all the gatherer results. | ||
const computedArtifacts = this.instantiateComputedArtifacts(); | ||
const artifacts = Object.assign({}, computedArtifacts, tracingData); | ||
|
@@ -258,6 +265,12 @@ class GatherRunner { | |
}); | ||
}); | ||
return artifacts; | ||
}) | ||
// cleanup on error | ||
.catch(err => { | ||
GatherRunner.disposeDriver(driver); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that way you can have an error specific log here. Even as simple as `log.log('status', 'Disconnecting from browser after error...'); or something There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice job adding this here. We should have been doing this for other errors already |
||
|
||
throw err; | ||
}); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,32 @@ const assert = require('assert'); | |
const connection = new Connection(); | ||
const driverStub = new Driver(connection); | ||
|
||
function createOnceStub(events) { | ||
return (eventName, cb) => { | ||
if (events[eventName]) { | ||
return cb(events[eventName]); | ||
} | ||
|
||
throw Error(`Stub not implemented: ${eventName}`); | ||
}; | ||
} | ||
|
||
function createSWRegistration(id, url, isDeleted) { | ||
return { | ||
isDeleted: !!isDeleted, | ||
registrationId: id, | ||
scopeURL: url, | ||
}; | ||
} | ||
|
||
function createActiveWorker(id, url, controlledClients) { | ||
return { | ||
registrationId: id, | ||
scriptURL: url, | ||
controlledClients, | ||
}; | ||
} | ||
|
||
connection.sendCommand = function(command, params) { | ||
switch (command) { | ||
case 'DOM.getDocument': | ||
|
@@ -34,6 +60,9 @@ connection.sendCommand = function(command, params) { | |
return Promise.resolve({ | ||
nodeId: params.selector === 'invalid' ? 0 : 231 | ||
}); | ||
case 'ServiceWorker.enable': | ||
case 'ServiceWorker.disable': | ||
return Promise.resolve(); | ||
default: | ||
throw Error(`Stub not implemented: ${command}`); | ||
} | ||
|
@@ -83,3 +112,73 @@ describe('Browser Driver', () => { | |
assert.equal(opts.url, req3.url, 'opts.url matches the last redirect'); | ||
}); | ||
}); | ||
|
||
describe('Multiple tab check', () => { | ||
it('will fail when multiple tabs are found with the same active serviceworker', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be helpful to surround these tests with a nested |
||
const pageUrl = 'https://example.com/'; | ||
const swUrl = `${pageUrl}sw.js`; | ||
const registrations = [ | ||
createSWRegistration(1, pageUrl), | ||
]; | ||
const versions = [ | ||
createActiveWorker(1, swUrl, ['unique']) | ||
]; | ||
|
||
driverStub.getCurrentTabId = () => Promise.resolve('unique2'); | ||
driverStub.> | ||
'ServiceWorker.workerRegistrationUpdated': { | ||
registrations | ||
}, | ||
'ServiceWorker.workerVersionUpdated': { | ||
versions | ||
} | ||
}); | ||
|
||
return driverStub.checkForMultipleTabsAttached(pageUrl) | ||
.then(_ => assert.ok(false), _ => assert.ok(true)); | ||
}); | ||
|
||
it('will succeed when service worker is already registered on current tab', () => { | ||
const pageUrl = 'https://example.com/'; | ||
const swUrl = `${pageUrl}sw.js`; | ||
const registrations = [ | ||
createSWRegistration(1, pageUrl), | ||
]; | ||
const versions = [ | ||
createActiveWorker(1, swUrl, ['unique']) | ||
]; | ||
|
||
driverStub.getCurrentTabId = () => Promise.resolve('unique'); | ||
driverStub.> | ||
'ServiceWorker.workerRegistrationUpdated': { | ||
registrations | ||
}, | ||
'ServiceWorker.workerVersionUpdated': { | ||
versions | ||
} | ||
}); | ||
|
||
return driverStub.checkForMultipleTabsAttached(pageUrl) | ||
.then(_ => assert.ok(true), _ => assert.ok(false)); | ||
}); | ||
|
||
it('will succeed when only one service worker loaded', () => { | ||
const pageUrl = 'https://example.com/'; | ||
const swUrl = `${pageUrl}sw.js`; | ||
const registrations = [createSWRegistration(1, pageUrl)]; | ||
const versions = [createActiveWorker(1, swUrl, [])]; | ||
|
||
driverStub.getCurrentTabId = () => Promise.resolve('unique'); | ||
driverStub.> | ||
'ServiceWorker.workerRegistrationUpdated': { | ||
registrations | ||
}, | ||
'ServiceWorker.workerVersionUpdated': { | ||
versions | ||
} | ||
}); | ||
|
||
return driverStub.checkForMultipleTabsAttached(pageUrl) | ||
.then(_ => assert.ok(true), _ => assert.ok(false)); | ||
}); | ||
}); |
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.
should copy the
comment up here so it's clear why
driver.disconnect();
isn't being returned