Skip to content

Commit

Permalink
fix: evaluateAsync behavior
Browse files Browse the repository at this point in the history
addresses #1000 and #976

1. Ensures all `Runtime.evaluate` calls result in the native Promise
2. Transforms all errors produced during the evaluation to a standard object that can be rejected by our driver wrapper
  • Loading branch information
patrickhulce committed Nov 24, 2016
1 parent dcdabb7 commit 8732398
Show file tree
Hide file tree
Showing 7 changed files with 57 additions and 11 deletions.
1 change: 0 additions & 1 deletion lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,5 @@ <h2>Do better web tester page</h2>
}
}
</script>

</body>
</html>
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ class LinkBlockingFirstPaintAudit extends Audit {
if (typeof artifact === 'undefined' || artifact.value === -1) {
return {
rawValue: -1,
debugString: 'TagsBlockingFirstPaint gatherer did not run'
debugString: (artifact && artifact.debugString) ||
'TagsBlockingFirstPaint gatherer did not run'
};
}

Expand Down
41 changes: 38 additions & 3 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -143,17 +143,33 @@ class Driver {
(_ => reject(new Error('The asynchronous expression exceeded the allotted time of 60s'))),
60000
);

this.sendCommand('Runtime.evaluate', {
expression: asyncExpression,
expression: `(function wrapInNativePromise() {
const __nativePromise = window.__nativePromise || Promise;
return new __nativePromise(function(resolve) {
const wrapError = ${wrapRuntimeEvalErrorInBrowser.toString()};
try {
(${asyncExpression}).then(resolve, wrapError);
} catch (e) {
wrapError(e);
}
});
}())`,
includeCommandLineAPI: true,
awaitPromise: true,
returnByValue: true
}).then(result => {
clearTimeout(asyncTimeout);
const value = result.result.value;

if (result.exceptionDetails) {
reject(result.exceptionDetails.exception.value);
// An error occurred before we could even enter our try block, should be *very* rare
reject(new Error('an unknown driver error occurred'));
} if (value.__failedInBrowser) {
reject(Object.assign(new Error(), value));
} else {
resolve(result.result.value);
resolve(value);
}
}).catch(err => {
clearTimeout(asyncTimeout);
Expand Down Expand Up @@ -713,4 +729,23 @@ function captureJSCallUsage(funcRef, set) {
};
}

/**
* The `exceptionDetails` provided by the debugger protocol does not contain the useful
* information such as name, message, and stack trace of the error when it's wrapped in a
* promise. Instead, map to a successful object that contains this information.
* @param {string|Error} err The error to convert
* istanbul ignore next
*/
function wrapRuntimeEvalErrorInBrowser(err) {
err = err || new Error();
const fallbackMessage = typeof err === 'string' ? err : 'unknown error';

resolve({
__failedInBrowser: true,
name: err.name || 'Error',
message: err.message || fallbackMessage,
stack: err.stack || (new Error()).stack,
});
};

module.exports = Driver;
7 changes: 5 additions & 2 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ const path = require('path');
* C. GatherRunner.setupDriver()
* i. assertNoSameOriginServiceWorkerClients
* ii. beginEmulation
* iii. cleanAndDisableBrowserCaches
* iiii. clearDataForOrigin
* iii. enableRuntimeEvents
* iv. evaluateScriptOnLoad rescue native Promise from potential polyfill
* v. cleanAndDisableBrowserCaches
* vi. clearDataForOrigin
*
* 2. For each pass in the config:
* A. GatherRunner.beforePass()
Expand Down Expand Up @@ -90,6 +92,7 @@ class GatherRunner {
return driver.assertNoSameOriginServiceWorkerClients(options.url)
.then(_ => driver.beginEmulation(options.flags))
.then(_ => driver.enableRuntimeEvents())
.then(_ => driver.evaluateScriptOnLoad('window.__nativePromise = Promise;'))
.then(_ => driver.cleanAndDisableBrowserCaches())
.then(_ => driver.clearDataForOrigin(options.url));
}
Expand Down
6 changes: 5 additions & 1 deletion lighthouse-core/gather/gatherers/accessibility.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,13 @@ class Accessibility extends Gatherer {

afterPass(options) {
const driver = options.driver;
const expression = `(function () {
${axe};
return (${runA11yChecks.toString()}());
})()`;

return driver
.evaluateAsync(`${axe};(${runA11yChecks.toString()}())`)
.evaluateAsync(expression)
.then(returnedValue => {
if (!returnedValue) {
this.artifact = Accessibility._errorAccessibility('Unable to parse axe results');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ function collectTagsThatBlockFirstPaint() {
});
resolve(tagList);
} catch (e) {
reject(`Unable to gather Scripts/Stylesheets/HTML Imports on the page: ${e.message}`);
const friendly = 'Unable to gather Scripts/Stylesheets/HTML Imports on the page';
reject(new Error(`${friendly}: ${e.message}`));
}
});
}
Expand Down Expand Up @@ -117,10 +118,10 @@ class TagsBlockingFirstPaint extends Gatherer {
.then(artifact => {
this.artifact = artifact;
})
.catch(debugString => {
.catch(err => {
this.artifact = {
value: -1,
debugString
debugString: err.toString()
};
});
}
Expand Down
3 changes: 3 additions & 0 deletions lighthouse-core/test/gather/fake-driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ module.exports = {
enableRuntimeEvents() {
return Promise.resolve();
},
evaluateScriptOnLoad() {
return Promise.resolve();
},
cleanAndDisableBrowserCaches() {},
clearDataForOrigin() {},
beginTrace() {
Expand Down

0 comments on commit 8732398

Please sign in to comment.