From 38286b86a08bd9b7055b7b450cbc8d8a4a3d13cf Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Fri, 6 Oct 2017 11:26:39 -0700 Subject: [PATCH 1/5] fix(driver): use execution context isolation when necessary --- .../a11y_tester.html} | 10 +- .../test/smokehouse/a11y/a11y-config.js | 18 ++ .../test/smokehouse/a11y/expectations.js | 242 ++++++++++++++++++ .../test/smokehouse/a11y/run-tests.sh | 16 ++ .../test/smokehouse/run-all-tests.sh | 12 + lighthouse-core/gather/driver.js | 39 ++- .../gatherers/dobetterweb/js-libraries.js | 2 +- package.json | 2 +- 8 files changed, 333 insertions(+), 8 deletions(-) rename lighthouse-cli/test/fixtures/{accessibility/accessibility_tester.html => a11y/a11y_tester.html} (92%) create mode 100644 lighthouse-cli/test/smokehouse/a11y/a11y-config.js create mode 100644 lighthouse-cli/test/smokehouse/a11y/expectations.js create mode 100755 lighthouse-cli/test/smokehouse/a11y/run-tests.sh create mode 100644 lighthouse-cli/test/smokehouse/run-all-tests.sh diff --git a/lighthouse-cli/test/fixtures/accessibility/accessibility_tester.html b/lighthouse-cli/test/fixtures/a11y/a11y_tester.html similarity index 92% rename from lighthouse-cli/test/fixtures/accessibility/accessibility_tester.html rename to lighthouse-cli/test/fixtures/a11y/a11y_tester.html index 8dea189a14a5..41320950a14a 100644 --- a/lighthouse-cli/test/fixtures/accessibility/accessibility_tester.html +++ b/lighthouse-cli/test/fixtures/a11y/a11y_tester.html @@ -120,7 +120,7 @@
@@ -197,5 +197,13 @@
+ diff --git a/lighthouse-cli/test/smokehouse/a11y/a11y-config.js b/lighthouse-cli/test/smokehouse/a11y/a11y-config.js new file mode 100644 index 000000000000..363d13f5cdfc --- /dev/null +++ b/lighthouse-cli/test/smokehouse/a11y/a11y-config.js @@ -0,0 +1,18 @@ +/** + * @license Copyright 2017 Google Inc. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +/** + * Config file for running PWA smokehouse audits for axe. + */ +module.exports = { + extends: 'lighthouse:default', + settings: { + onlyCategories: [ + 'accessibility', + ], + }, +}; diff --git a/lighthouse-cli/test/smokehouse/a11y/expectations.js b/lighthouse-cli/test/smokehouse/a11y/expectations.js new file mode 100644 index 000000000000..f6d161cdff80 --- /dev/null +++ b/lighthouse-cli/test/smokehouse/a11y/expectations.js @@ -0,0 +1,242 @@ +/** + * @license Copyright 2017 Google Inc. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +/** + * Expected Lighthouse audit values for byte efficiency tests + */ +module.exports = [ + { + initialUrl: 'http://localhost:10200/a11y/a11y_tester.html', + url: 'http://localhost:10200/a11y/a11y_tester.html', + audits: { + accesskeys: { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'aria-allowed-attr': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'aria-required-children': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'aria-required-parent': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'aria-roles': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'aria-valid-attr-value': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'aria-valid-attr': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'button-name': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + bypass: { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'color-contrast': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'definition-list': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + dlitem: { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'document-title': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'duplicate-id': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'frame-title': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'html-has-lang': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'image-alt': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'input-image-alt': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + label: { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'layout-table': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'link-name': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + list: { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + listitem: { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'meta-viewport': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'object-alt': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + tabindex: { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'td-headers-attr': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + 'valid-lang': { + score: false, + details: { + items: { + length: 1, + }, + }, + }, + }, + }, +]; diff --git a/lighthouse-cli/test/smokehouse/a11y/run-tests.sh b/lighthouse-cli/test/smokehouse/a11y/run-tests.sh new file mode 100755 index 000000000000..f51c4c704949 --- /dev/null +++ b/lighthouse-cli/test/smokehouse/a11y/run-tests.sh @@ -0,0 +1,16 @@ +#!/usr/bin/env bash + +node lighthouse-cli/test/fixtures/static-server.js & + +sleep 0.5s + +config="lighthouse-cli/test/smokehouse/a11y/a11y-config.js" +expectations="lighthouse-cli/test/smokehouse/a11y/expectations.js" + +yarn smokehouse -- --config-path=$config --expectations-path=$expectations +exit_code=$? + +# kill test servers +kill $(jobs -p) + +exit "$exit_code" diff --git a/lighthouse-cli/test/smokehouse/run-all-tests.sh b/lighthouse-cli/test/smokehouse/run-all-tests.sh new file mode 100644 index 000000000000..c4ed77028f5a --- /dev/null +++ b/lighthouse-cli/test/smokehouse/run-all-tests.sh @@ -0,0 +1,12 @@ +#!/usr/bin/env bash + +DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" + +for d in "$DIR"/*/ ; do + bash "${d}run-tests.sh" + if [ $? -ne 0 ] + then + echo "Error: $d smoke test failed" + exit 1 + fi +done diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 1c599a146523..716012f16411 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -41,6 +41,7 @@ class Driver { this._devtoolsLog = new DevtoolsLog(/^(Page|Network)\./); this.online = true; this._domainEnabledCounts = new Map(); + this._isolatedExecutionContextId = undefined; /** * Used for monitoring network status events during gotoURL. @@ -219,9 +220,11 @@ class Driver { * Evaluate an expression in the context of the current page. * Returns a promise that resolves on the expression's value. * @param {string} expression + * @param {{useIsolation: boolean}=} options * @return {!Promise<*>} */ - evaluateAsync(expression) { + evaluateAsync(expression, options) { + const {useIsolation = true} = options || {}; return new Promise((resolve, reject) => { // If this gets to 60s and it hasn't been resolved, reject the Promise. const asyncTimeout = setTimeout( @@ -229,7 +232,7 @@ class Driver { 60000 ); - this.sendCommand('Runtime.evaluate', { + const evaluationParams = { // We need to explicitly wrap the raw expression for several purposes: // 1. Ensure that the expression will be a native Promise and not a polyfill/non-Promise. // 2. Ensure that errors in the expression are captured by the Promise. @@ -247,7 +250,13 @@ class Driver { includeCommandLineAPI: true, awaitPromise: true, returnByValue: true, - }).then(result => { + }; + + if (useIsolation && typeof this._isolatedExecutionContextId === 'number') { + evaluationParams.contextId = this._isolatedExecutionContextId; + } + + this.sendCommand('Runtime.evaluate', evaluationParams).then(result => { clearTimeout(asyncTimeout); const value = result.result.value; @@ -431,10 +440,12 @@ class Driver { let lastTimeout; let cancelled = false; + + const checkForQuietExpression = `(${checkTimeSinceLastLongTask.toString()})()`; function checkForQuiet(driver, resolve) { if (cancelled) return; - return driver.evaluateAsync(`(${checkTimeSinceLastLongTask.toString()})()`) + return driver.evaluateAsync(checkForQuietExpression, {useIsolation: false}) .then(timeSinceLongTask => { if (cancelled) return; @@ -595,6 +606,22 @@ class Driver { return finalUrl; } + /** + * @return {!Promise} + */ + _createIsolatedWorld() { + return this.sendCommand('Page.getResourceTree') + .then(data => { + const frameId = data.frameTree.frame.id; + return this.sendCommand('Page.createIsolatedWorld', {frameId}); + }) + .then(data => this._isolatedExecutionContextId = data.executionContextId); + } + + _clearIsolatedWorld() { + this._isolatedExecutionContextId = undefined; + } + /** * Navigate to the given URL. Direct use of this method isn't advised: if * the current page is already at the given URL, navigation will not occur and @@ -623,6 +650,7 @@ class Driver { /* eslint-enable max-len */ return this._beginNetworkStatusMonitoring(url) + .then(_ => this._clearIsolatedWorld()) .then(_ => { // These can 'race' and that's OK. // We don't want to wait for Page.navigate's resolution, as it can now @@ -633,6 +661,7 @@ class Driver { }) .then(_ => waitForLoad && this._waitForFullyLoaded(pauseAfterLoadMs, networkQuietThresholdMs, cpuQuietThresholdMs, maxWaitMs)) + .then(_ => this._createIsolatedWorld()) .then(_ => this._endNetworkStatusMonitoring()); } @@ -949,7 +978,7 @@ class Driver { const globalVarToPopulate = `window['__${funcName}StackTraces']`; const collectUsage = () => { return this.evaluateAsync( - `Promise.resolve(Array.from(${globalVarToPopulate}).map(item => JSON.parse(item)))`) + `Array.from(${globalVarToPopulate}).map(item => JSON.parse(item))`, {useIsolation: false}) .then(result => { if (!Array.isArray(result)) { throw new Error( diff --git a/lighthouse-core/gather/gatherers/dobetterweb/js-libraries.js b/lighthouse-core/gather/gatherers/dobetterweb/js-libraries.js index 4031099b53ce..2357ceefa611 100644 --- a/lighthouse-core/gather/gatherers/dobetterweb/js-libraries.js +++ b/lighthouse-core/gather/gatherers/dobetterweb/js-libraries.js @@ -57,7 +57,7 @@ class JSLibraries extends Gatherer { return (${detectLibraries.toString()}()); })()`; - return options.driver.evaluateAsync(expression); + return options.driver.evaluateAsync(expression, {useIsolation: false}); } } diff --git a/package.json b/package.json index c7ac2d6245d8..411429432870 100644 --- a/package.json +++ b/package.json @@ -25,7 +25,7 @@ "build-viewer": "cd ./lighthouse-viewer && yarn build", "clean": "rimraf *.report.html *.report.dom.html *.report.json *.screenshots.html *.devtoolslog.json *.trace.json || true", "lint": "[ \"$CI\" = true ] && eslint --quiet -f codeframe . || eslint .", - "smoke": "bash lighthouse-cli/test/smokehouse/offline-local/run-tests.sh && bash lighthouse-cli/test/smokehouse/perf/run-tests.sh && bash lighthouse-cli/test/smokehouse/dobetterweb/run-tests.sh && bash lighthouse-cli/test/smokehouse/byte-efficiency/run-tests.sh && bash lighthouse-cli/test/smokehouse/tricky-ttci/run-tests.sh && bash lighthouse-cli/test/smokehouse/seo/run-tests.sh && bash lighthouse-cli/test/smokehouse/redirects/run-tests.sh", + "smoke": "bash lighthouse-cli/test/smokehouse/run-all-tests.sh", "coverage": "istanbul cover -x \"**/third_party/**\" _mocha -- $(find */test -name '*-test.js') --timeout 10000 --reporter progress --report lcovonly", "start": "node ./lighthouse-cli/index.js", "test": "yarn lint --quiet && yarn unit && yarn closure && yarn test-cli-formatting", From a860423d48aedf8015d3a9df91922a43f3c378e6 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 10 Oct 2017 17:12:36 -0700 Subject: [PATCH 2/5] feedback --- .../test/smokehouse/a11y/expectations.js | 14 +++--- lighthouse-core/gather/driver.js | 43 +++++++++++++------ .../gather/gatherers/accessibility.js | 2 +- .../gatherers/dobetterweb/js-libraries.js | 2 +- 4 files changed, 38 insertions(+), 23 deletions(-) diff --git a/lighthouse-cli/test/smokehouse/a11y/expectations.js b/lighthouse-cli/test/smokehouse/a11y/expectations.js index f6d161cdff80..81ecde776b57 100644 --- a/lighthouse-cli/test/smokehouse/a11y/expectations.js +++ b/lighthouse-cli/test/smokehouse/a11y/expectations.js @@ -13,7 +13,7 @@ module.exports = [ initialUrl: 'http://localhost:10200/a11y/a11y_tester.html', url: 'http://localhost:10200/a11y/a11y_tester.html', audits: { - accesskeys: { + 'accesskeys': { score: false, details: { items: { @@ -77,7 +77,7 @@ module.exports = [ }, }, }, - bypass: { + 'bypass': { score: false, details: { items: { @@ -101,7 +101,7 @@ module.exports = [ }, }, }, - dlitem: { + 'dlitem': { score: false, details: { items: { @@ -157,7 +157,7 @@ module.exports = [ }, }, }, - label: { + 'label': { score: false, details: { items: { @@ -181,7 +181,7 @@ module.exports = [ }, }, }, - list: { + 'list': { score: false, details: { items: { @@ -189,7 +189,7 @@ module.exports = [ }, }, }, - listitem: { + 'listitem': { score: false, details: { items: { @@ -213,7 +213,7 @@ module.exports = [ }, }, }, - tabindex: { + 'tabindex': { score: false, details: { items: { diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 716012f16411..bb70e656bb6c 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -217,14 +217,29 @@ class Driver { } /** - * Evaluate an expression in the context of the current page. + * Evaluate an expression in the context of the current page. If useIsolation is true, the expression + * will be evaluated in a content script that has access to the page's DOM but whose JavaScript state + * is completely separate. * Returns a promise that resolves on the expression's value. * @param {string} expression * @param {{useIsolation: boolean}=} options * @return {!Promise<*>} */ - evaluateAsync(expression, options) { - const {useIsolation = true} = options || {}; + evaluateAsync(expression, options = {}) { + const contextIdPromise = options.useIsolation ? + this._getOrCreateIsolatedContextId() : + Promise.resolve(undefined); + return contextIdPromise.then(contextId => this._evaluateInContext(expression, contextId)); + } + + /** + * Evaluate an expression in the given execution context; an undefined contextId implies the main + * page without isolation. + * @param {string} expression + * @param {number|undefined} contextId + * @return {!Promise<*>} + */ + _evaluateInContext(expression, contextId) { return new Promise((resolve, reject) => { // If this gets to 60s and it hasn't been resolved, reject the Promise. const asyncTimeout = setTimeout( @@ -250,12 +265,9 @@ class Driver { includeCommandLineAPI: true, awaitPromise: true, returnByValue: true, + contextId, }; - if (useIsolation && typeof this._isolatedExecutionContextId === 'number') { - evaluationParams.contextId = this._isolatedExecutionContextId; - } - this.sendCommand('Runtime.evaluate', evaluationParams).then(result => { clearTimeout(asyncTimeout); const value = result.result.value; @@ -445,7 +457,7 @@ class Driver { function checkForQuiet(driver, resolve) { if (cancelled) return; - return driver.evaluateAsync(checkForQuietExpression, {useIsolation: false}) + return driver.evaluateAsync(checkForQuietExpression) .then(timeSinceLongTask => { if (cancelled) return; @@ -607,9 +619,13 @@ class Driver { } /** - * @return {!Promise} + * @return {!Promise} */ - _createIsolatedWorld() { + _getOrCreateIsolatedContextId() { + if (typeof this._isolatedExecutionContextId === 'number') { + return Promise.resolve(this._isolatedExecutionContextId); + } + return this.sendCommand('Page.getResourceTree') .then(data => { const frameId = data.frameTree.frame.id; @@ -618,7 +634,7 @@ class Driver { .then(data => this._isolatedExecutionContextId = data.executionContextId); } - _clearIsolatedWorld() { + _clearIsolatedContextId() { this._isolatedExecutionContextId = undefined; } @@ -650,7 +666,7 @@ class Driver { /* eslint-enable max-len */ return this._beginNetworkStatusMonitoring(url) - .then(_ => this._clearIsolatedWorld()) + .then(_ => this._clearIsolatedContextId()) .then(_ => { // These can 'race' and that's OK. // We don't want to wait for Page.navigate's resolution, as it can now @@ -661,7 +677,6 @@ class Driver { }) .then(_ => waitForLoad && this._waitForFullyLoaded(pauseAfterLoadMs, networkQuietThresholdMs, cpuQuietThresholdMs, maxWaitMs)) - .then(_ => this._createIsolatedWorld()) .then(_ => this._endNetworkStatusMonitoring()); } @@ -978,7 +993,7 @@ class Driver { const globalVarToPopulate = `window['__${funcName}StackTraces']`; const collectUsage = () => { return this.evaluateAsync( - `Array.from(${globalVarToPopulate}).map(item => JSON.parse(item))`, {useIsolation: false}) + `Array.from(${globalVarToPopulate}).map(item => JSON.parse(item))`) .then(result => { if (!Array.isArray(result)) { throw new Error( diff --git a/lighthouse-core/gather/gatherers/accessibility.js b/lighthouse-core/gather/gatherers/accessibility.js index 84a11d42bfb9..935824ab52b6 100644 --- a/lighthouse-core/gather/gatherers/accessibility.js +++ b/lighthouse-core/gather/gatherers/accessibility.js @@ -96,7 +96,7 @@ class Accessibility extends Gatherer { return (${runA11yChecks.toString()}()); })()`; - return driver.evaluateAsync(expression).then(returnedValue => { + return driver.evaluateAsync(expression, {useIsolation: true}).then(returnedValue => { if (!returnedValue) { throw new Error('No axe-core results returned'); } diff --git a/lighthouse-core/gather/gatherers/dobetterweb/js-libraries.js b/lighthouse-core/gather/gatherers/dobetterweb/js-libraries.js index 2357ceefa611..4031099b53ce 100644 --- a/lighthouse-core/gather/gatherers/dobetterweb/js-libraries.js +++ b/lighthouse-core/gather/gatherers/dobetterweb/js-libraries.js @@ -57,7 +57,7 @@ class JSLibraries extends Gatherer { return (${detectLibraries.toString()}()); })()`; - return options.driver.evaluateAsync(expression, {useIsolation: false}); + return options.driver.evaluateAsync(expression); } } From 0a423dd4e5b7ebf90780c57f5ea095ca166b7d2f Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Thu, 12 Oct 2017 10:22:32 -0700 Subject: [PATCH 3/5] add unit test --- lighthouse-core/test/gather/driver-test.js | 30 ++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index a5424f54390f..08a047b18d1c 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -58,6 +58,8 @@ connection.sendCommand = function(command, params) { return Promise.resolve({ nodeIds: params.selector === 'invalid' ? [] : [231], }); + case 'Runtime.evaluate': + return Promise.resolve({result: {value: 123}}); case 'Runtime.getProperties': return Promise.resolve({ result: params.objectId === 'invalid' ? [] : [{ @@ -69,6 +71,10 @@ connection.sendCommand = function(command, params) { name: 'novalue', }], }); + case 'Page.getResourceTree': + return Promise.resolve({frameTree: {frame: {id: 1}}}); + case 'Page.createIsolatedWorld': + return Promise.resolve({executionContextId: 1}); case 'Page.enable': case 'Tracing.start': case 'ServiceWorker.enable': @@ -131,6 +137,30 @@ describe('Browser Driver', () => { }); }); + it('evaluates an expression', () => { + return driverStub.evaluateAsync('120 + 3').then(value => { + assert.deepEqual(value, 123); + assert.equal(sendCommandParams[0].command, 'Runtime.evaluate'); + }); + }); + + it('evaluates an expression in isolation', () => { + return driverStub.evaluateAsync('120 + 3', {useIsolation: true}).then(value => { + assert.deepEqual(value, 123); + + assert.ok(sendCommandParams.length > 1, 'did not create execution context'); + const evaluateCommand = sendCommandParams.find(data => data.command === 'Runtime.evaluate'); + assert.equal(evaluateCommand.params.contextId, 1); + + // test repeat isolation evaluations + sendCommandParams = []; + return driverStub.evaluateAsync('120 + 3', {useIsolation: true}); + }).then(value => { + assert.deepEqual(value, 123); + assert.ok(sendCommandParams.length === 1, 'created unnecessary 2nd execution context'); + }); + }); + it('will track redirects through gotoURL load', () => { const delay = _ => new Promise(resolve => setTimeout(resolve)); From 807fc31b9aac363ec25d043ea55af483453ec902 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Fri, 13 Oct 2017 13:47:57 -0700 Subject: [PATCH 4/5] feedback --- lighthouse-cli/test/fixtures/a11y/a11y_tester.html | 4 ++-- lighthouse-core/gather/driver.js | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lighthouse-cli/test/fixtures/a11y/a11y_tester.html b/lighthouse-cli/test/fixtures/a11y/a11y_tester.html index 41320950a14a..afa2429441cb 100644 --- a/lighthouse-cli/test/fixtures/a11y/a11y_tester.html +++ b/lighthouse-cli/test/fixtures/a11y/a11y_tester.html @@ -198,8 +198,8 @@