Skip to content

Commit

Permalink
feat(SELENIUM_PROMISE_MANAGER=0): Improve support for SELENIUM_PROMIS…
Browse files Browse the repository at this point in the history
…E_MANAGER=0

There are three major ways this was done in this change:
* In `callWhenIdle`, if `flow.isIdle` is not defined, we assume we are working
  with a `SimpleScheduler` instance, and so the flow is effectively idle.
* In `initJasmineWd`, if `flow.reset` is not defined, we assume we are working
  with a `SimpleScheduler` instance, and so don't bother resetting the flow.
* In `wrapInControlFlow`, we use `flow.promise` to create a new promise if
  possible.  Since `new webdriver.promise.Promise()` would have always made a
  `ManagedPromise`, but `flow.promise` will do the right thing.
* In `wrapCompare`, we avoid the webdriver library entirely, and never instance
  any extra promises. Using `webdriver.promise.when` and `webdriver.promise.all`
  could have been a problem if our instance of `webdriver` had the control flow
  turned on, but another instance somewhere did not (or even the same instance,
  but just at a different point in time).  Instead we use the new `maybePromise`
  tool, which is a mess but is also exactly what we want.
* In `specs/*`, we replace `webdriver.promise.fulfilled` with
  `webdriver.promise.when`.
* In `specs/*`, a new version of `adapterSpec.js` and `errorSpec.js` are
  created: `asyncAwaitAdapterSpec.ts` and `asyncAwaitErrorSpec.ts`.

I also also fixed a minor bug where we weren't correctly checking for promises
inside an array of expected results.  Before we had

```js
expected = Array.prototype.slice.call(arguments, 0)

...

webdriver.promise.isPromise(expected)
```

I thought about it for a little while, and there's no way that's correct.
`expected` is an `Array<any>`, there's no way it has a `.then` function.

Closes #69
  • Loading branch information
sjelin committed Jan 11, 2017
1 parent f0d0f06 commit 9ced31d
Show file tree
Hide file tree
Showing 21 changed files with 841 additions and 106 deletions.
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
*.log
node_modules
spec/asyncAwaitSpec.js
spec/asyncAwait*Spec.js
spec/common.js
4 changes: 3 additions & 1 deletion .jshintignore
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
./spec/asyncAwaitSpec.js
./spec/asyncAwaitAdapterSpec.js
./spec/asyncAwaitErrorSpec.js
./spec/common.js
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,5 @@ available via several compilers. At the moment, they often break the WebDriver
control flow.
([GitHub issue](https://github.com/SeleniumHQ/selenium/issues/3037)). You can
still use them, but if you do then you will have to use `await`/Promises for
almost all your synchronization. See `spec/asyncAwaitSpec.ts` for details.
almost all your synchronization. See `spec/asyncAwaitAdapterSpec.ts` and
`spec/asyncAwaitErrorSpec.ts` for examples.
69 changes: 36 additions & 33 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

var webdriver = require('selenium-webdriver');
var maybePromise = require('./maybePromise');

/**
* Validates that the parameter is a function.
Expand Down Expand Up @@ -54,7 +55,7 @@ function validateString(stringtoValidate) {
* @param {!Function} fn The function to call
*/
function callWhenIdle(flow, fn) {
if (flow.isIdle()) {
if (!flow.isIdle || flow.isIdle()) {
fn();
} else {
flow.once(webdriver.promise.ControlFlow.EventType.IDLE, function() {
Expand Down Expand Up @@ -84,7 +85,14 @@ function wrapInControlFlow(flow, globalFn, fnName) {
var testFn = fn.bind(this);

flow.execute(function controlFlowExecute() {
return new webdriver.promise.Promise(function(fulfill, reject) {
function newPromise(resolver) {
if (typeof flow.promise == 'function') {
return flow.promise(resolver);
} else {
return new webdriver.promise.Promise(resolver, flow);
}
}
return newPromise(function(fulfill, reject) {
function wrappedReject(err) {
var wrappedErr = new Error(err);
reject(wrappedErr);
Expand All @@ -106,7 +114,7 @@ function wrapInControlFlow(flow, globalFn, fnName) {
fulfill(ret);
}
}
}, flow);
});
}, 'Run ' + fnName + description + ' in control flow').then(
callWhenIdle.bind(null, flow, done), function(err) {
if (!err) {
Expand Down Expand Up @@ -173,15 +181,17 @@ function initJasmineWd(flow) {
global.beforeAll = wrapInControlFlow(flow, global.beforeAll, 'beforeAll');
global.afterAll = wrapInControlFlow(flow, global.afterAll, 'afterAll');

// On timeout, the flow should be reset. This will prevent webdriver tasks
// from overflowing into the next test and causing it to fail or timeout
// as well. This is done in the reporter instead of an afterEach block
// to ensure that it runs after any afterEach() blocks with webdriver tasks
// get to complete first.
jasmine.getEnv().addReporter(new OnTimeoutReporter(function() {
console.warn('A Jasmine spec timed out. Resetting the WebDriver Control Flow.');
flow.reset();
}));
if (flow.reset) {
// On timeout, the flow should be reset. This will prevent webdriver tasks
// from overflowing into the next test and causing it to fail or timeout
// as well. This is done in the reporter instead of an afterEach block
// to ensure that it runs after any afterEach() blocks with webdriver tasks
// get to complete first.
jasmine.getEnv().addReporter(new OnTimeoutReporter(function() {
console.warn('A Jasmine spec timed out. Resetting the WebDriver Control Flow.');
flow.reset();
}));
}
}

var originalExpect = global.expect;
Expand All @@ -196,6 +206,10 @@ global.expect = function(actual) {
/**
* Creates a matcher wrapper that resolves any promises given for actual and
* expected values, as well as the `pass` property of the result.
*
* Wrapped matchers will return either `undefined` or a promise which resolves
* when the matcher is complete, depending on if the matcher had to resolve any
* promises.
*/
jasmine.Expectation.prototype.wrapCompare = function(name, matcherFactory) {
return function() {
Expand All @@ -205,16 +219,12 @@ jasmine.Expectation.prototype.wrapCompare = function(name, matcherFactory) {

matchError.stack = matchError.stack.replace(/ +at.+jasminewd.+\n/, '');

if (!webdriver.promise.isPromise(expectation.actual) &&
!webdriver.promise.isPromise(expected)) {
compare(expectation.actual, expected);
} else {
webdriver.promise.when(expectation.actual).then(function(actual) {
return webdriver.promise.all(expected).then(function(expected) {
return compare(actual, expected);
});
// Return either undefined or a promise of undefined
return maybePromise(expectation.actual, function(actual) {
return maybePromise.all(expected, function(expected) {
return compare(actual, expected);
});
}
});

function compare(actual, expected) {
var args = expected.slice(0);
Expand All @@ -229,12 +239,9 @@ jasmine.Expectation.prototype.wrapCompare = function(name, matcherFactory) {

var result = matcherCompare.apply(null, args);

if (webdriver.promise.isPromise(result.pass)) {
return webdriver.promise.when(result.pass).then(compareDone);
} else {
return compareDone(result.pass);
}
return maybePromise(result.pass, compareDone);

// compareDone always returns undefined
function compareDone(pass) {
var message = '';

Expand Down Expand Up @@ -268,13 +275,9 @@ jasmine.Expectation.prototype.wrapCompare = function(name, matcherFactory) {

function defaultNegativeCompare() {
var result = matcher.compare.apply(null, args);
if (webdriver.promise.isPromise(result.pass)) {
result.pass = result.pass.then(function(pass) {
return !pass;
});
} else {
result.pass = !result.pass;
}
result.pass = maybePromise(result.pass, function(pass) {
return !pass;
});
return result;
}
}
Expand Down
58 changes: 58 additions & 0 deletions maybePromise.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/**
* This file implements jasminewd's peculiar alternatives to Promise.resolve()
* and Promise.all(). Do not use the code from this file as pollyfill for
* Promise.resolve() or Promise.all(). There are a number of reasons why this
* implementation will cause unexpected errors in most codebases.
*
* Called "maybePromise" because both the parameters and the return values may
* or may not be promises, and code execution may or may not be synchronous.
*/

/**
* Runs a callback synchronously against non-promise values and asynchronously
* against promises. Similar to ES6's `Promise.resolve` except that it is
* synchronous when possible and won't wrap the return value.
*
* This is not what you normally want. Normally you want the code to be
* consistently asynchronous, and you want the result wrapped into a promise.
* But because of webdriver's control flow, we're better off not introducing any
* extra layers of promises or asynchronous activity.
*
* @param {*} val The value to call the callback with.
* @param {!Function} callback The callback function
* @return {*} If val isn't a promise, the return value of the callback is
* directly returned. If val is a promise, a promise (generated by val.then)
* resolving to the callback's return value is returned.
*/
var maybePromise = module.exports = function maybePromise(val, callback) {
if (val && (typeof val.then == 'function')) {
return val.then(callback);
} else {
return callback(val);
}
}

/**
* Like maybePromise() but for an array of values. Analogous to `Promise.all`.
*
* @param {!Array<*>} vals An array of values to call the callback with
* @param {!Function} callback the callback function
* @return {*} If nothing in vals is a promise, the return value of the callback
* is directly returned. Otherwise, a promise (generated by the .then
* functions in vals) resolving to the callback's return value is returned.
*/
maybePromise.all = function all(vals, callback) {
var resolved = new Array(vals.length);
function resolveAt(i) {
if (i >= vals.length) {
return callback(resolved);
} else {
return maybePromise(vals[i], function(val) {
resolved[i] = val;
return resolveAt(i+1);
});
}
}
return resolveAt(0);
}

16 changes: 13 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,15 @@
"selenium-webdriver": "3.0.1"
},
"devDependencies": {
"@types/node": "^6.0.56",
"@types/selenium-webdriver": "^2.53.38",
"jasmine": "2.4.1",
"jshint": "^2.9.4",
"typescript": "^2.0.10"
"selenium-webdriver": "2.53.3",
"tslint": "^4.2.0",
"tslint-eslint-rules": "^3.2.3",
"typescript": "^2.0.10",
"vrsource-tslint-rules": "^4.0.0"
},
"repository": {
"type": "git",
Expand All @@ -26,8 +33,11 @@
"main": "index.js",
"scripts": {
"jshint": "jshint index.js spec",
"tsc": "tsc -t ES2015 spec/asyncAwaitSpec.ts",
"pretest": "npm run jshint && npm run tsc",
"tslint": "tslint spec/*.ts",
"lint": "npm run jshint && npm run tslint",
"tsc": "tsc",
"clean": "rm spec/asyncAwait*Spec.js spec/common.js",
"pretest": "npm run lint && npm run tsc",
"test": "scripts/test.sh"
},
"license": "MIT",
Expand Down
44 changes: 41 additions & 3 deletions scripts/test.sh
Original file line number Diff line number Diff line change
@@ -1,21 +1,59 @@
LIB_SPECS="spec/support/lib_specs.json"
PASSING_SPECS="spec/support/passing_specs.json"
FAILING_SPECS="spec/support/failing_specs.json"
NO_CF_PASSING_SPECS="spec/support/no_cf_passing_specs.json"
NO_CF_FAILING_SPECS="spec/support/no_cf_failing_specs.json"
CMD_BASE="node node_modules/.bin/jasmine JASMINE_CONFIG_PATH="

echo "### running passing specs"
# Run unit tests

echo "### running all unit tests"
CMD=$CMD_BASE$LIB_SPECS
echo "### $CMD"
$CMD
[ "$?" -eq 0 ] || exit 1
echo


# Run all tests when the control flow is enabled

export SELENIUM_PROMISE_MANAGER=1

echo "### running all passing specs"
CMD=$CMD_BASE$PASSING_SPECS
echo "### $CMD"
$CMD
[ "$?" -eq 0 ] || exit 1
echo

EXPECTED_RESULTS="18 specs, 16 failures"
echo "### running failing specs (expecting $EXPECTED_RESULTS)"
EXPECTED_RESULTS="38 specs, 34 failures"
echo "### running all failing specs (expecting $EXPECTED_RESULTS)"
CMD=$CMD_BASE$FAILING_SPECS
echo "### $CMD"
res=`$CMD 2>/dev/null`
results_line=`echo "$res" | tail -2 | head -1`
echo "result: $results_line"
[ "$results_line" = "$EXPECTED_RESULTS" ] || exit 1

# Run only the async/await tests when the control flow is disabled

export SELENIUM_PROMISE_MANAGER=0

echo "### running async/await passing specs"
CMD=$CMD_BASE$NO_CF_PASSING_SPECS
echo "### $CMD"
$CMD
[ "$?" -eq 0 ] || exit 1
echo

EXPECTED_RESULTS="19 specs, 17 failures"
echo "### running async/await failing specs (expecting $EXPECTED_RESULTS)"
CMD=$CMD_BASE$NO_CF_FAILING_SPECS
echo "### $CMD"
res=`$CMD 2>/dev/null`
results_line=`echo "$res" | tail -2 | head -1`
echo "result: $results_line"
[ "$results_line" = "$EXPECTED_RESULTS" ] || exit 1


echo "all pass"
8 changes: 7 additions & 1 deletion spec/adapterSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ describe('webdriverJS Jasmine adapter', function() {

fakeDriver.getValueList().then(function(list) {
var result = list.map(function(webElem) {
var webElemsPromise = webdriver.promise.fulfilled(webElem).then(function(webElem) {
var webElemsPromise = webdriver.promise.when(webElem).then(function(webElem) {
return [webElem];
});
return webdriver.promise.fullyResolved(checkTexts(webElemsPromise));
Expand Down Expand Up @@ -244,6 +244,12 @@ describe('webdriverJS Jasmine adapter', function() {
});

describe('native promises', function() {
it('should have done argument override return returned promise', function(done) {
var ret = new Promise(function() {});
done();
return ret;
});

var currentTest = null;

it('should wait for webdriver events sent from native promise', function() {
Expand Down
Loading

0 comments on commit 9ced31d

Please sign in to comment.