feat(SELENIUM_PROMISE_MANAGER): Support `SELENIUM_PROMISE_MANAGER=0` #72

sjelin · 2016-12-17T07:18:26Z

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

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

sjelin · 2016-12-28T21:54:39Z

This PR is so lonely 😢 @juliemr @cnishina @mgiambalvo

sjelin · 2017-01-03T23:49:12Z

Probably needs tests. I'll probably just do adapterSpec.js again but with async await.

juliemr

Yup, please add some tests :)

juliemr · 2017-01-04T00:44:28Z

index.js

-        return webdriver.promise.all(expected).then(function(expected) {
-          return compare(actual, expected);
-        });
+    expectation.actual = extractPromiseFromDeferred(expectation.actual);


I'm a bit confused here. Before, expectation.actual could potentially be a regular value or a promise. Being a deferred wasn't on the table - why is this a concern now? (Commit message says it's "needed for selenium 3", but why?)

Sorry, I was referencing another commit from the beta branch: 70c9f62

For the record: we looked into this offline, and we can't figure out why that change was originally made. As far as we can tell, there's no reason anyone should do expect(foo)... where foo is a deferred, not a promise.

In selenium-webdriver@2, all deferred objects were promises. That's changed with version 3, and is a breaking change, but we don't expect this to affect very many users at all (and it will be released in a major version).

So we're going to remove the special logic to handle deferreds,

juliemr · 2017-01-04T00:44:46Z

index.js

+ *   is directly returned.  Otherwise, a promise (generated by the .then
+ *   functions in vals) resolving to the callback's return value is returned.
+ */
+function maybePromises(vals, callback) {


We should have a test for this.

Good idea. I'll spin it out into a helper file to make unit testing easy

sjelin · 2017-01-11T01:53:07Z

@juliemr after a solid week of getting side tracked, I finally returned to this PR and did everything you told me to do!

It turns out originally this was only supposed to be a step towards support, which is why there weren't tests. Now this PR is the full thing, which is why it is so much longer than it was before. I've also updated the PR/commit title/description to reflect that change

juliemr · 2017-01-11T22:44:43Z

maybePromise.js

@@ -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


Typo: polyfill

juliemr · 2017-01-11T23:15:33Z

spec/common.ts

@@ -1,110 +1,110 @@
-var webdriver = require('selenium-webdriver');
+import {promise as wdpromise, WebElement} from 'selenium-webdriver';


Is there a compelling reason to move this to TS?

It was important back when I was using noImplicitAny, and will be important again in #79. Removed from this PR and transferred over

juliemr · 2017-01-11T23:16:25Z

spec/errorSpec.js

@@ -104,6 +104,10 @@ describe('things that should fail', function() {
  });

  describe('native promises', function() {
+    it('should have done argument override return returned promise', function(done) {


I think the description string here needs clarification - maybe:

it should time out if done argument is never called, even if promise is returned

?

juliemr · 2017-01-11T23:21:55Z

tsconfig.json

@@ -0,0 +1,14 @@
+{


I think it would be cleaner for ts to output files to a folder, maybe spec/built, so that if we add more ts files later we don't need to special case every one in the .ignore files.

I went with built_spec/ instead because keeping the files in the same location relative to index.js is important.

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 angular#69

sjelin · 2017-01-12T04:03:59Z

@juliemr All comments addressed

juliemr · 2017-01-13T00:51:36Z

LGTM

sjelin added the pr: needs review label Dec 17, 2016

sjelin changed the title ~~feat(SELENIUM_PROMISE_MANAGER=0): Improve support for SELENIUM_PROMIS…~~ feat(SELENIUM_PROMISE_MANAGER=0): Improve support for SELENIUM_PROMISE_MANAGER=0 Dec 17, 2016

juliemr self-assigned this Jan 3, 2017

juliemr reviewed Jan 4, 2017

View reviewed changes

cnishina force-pushed the jasminewd2 branch from ffae218 to 6023066 Compare January 6, 2017 20:23

sjelin force-pushed the cf branch from eaa1ff8 to 9ced31d Compare January 11, 2017 01:46

sjelin changed the title ~~feat(SELENIUM_PROMISE_MANAGER=0): Improve support for SELENIUM_PROMISE_MANAGER=0~~ feat(SELENIUM_PROMISE_MANAGER=0): Support SELENIUM_PROMISE_MANAGER=0 Jan 11, 2017

sjelin changed the title ~~feat(SELENIUM_PROMISE_MANAGER=0): Support SELENIUM_PROMISE_MANAGER=0~~ feat(SELENIUM_PROMISE_MANAGER=0): Support SELENIUM_PROMISE_MANAGER=0 Jan 11, 2017

sjelin changed the title ~~feat(SELENIUM_PROMISE_MANAGER=0): Support SELENIUM_PROMISE_MANAGER=0~~ feat(SELENIUM_PROMISE_MANAGER): Support SELENIUM_PROMISE_MANAGER=0 Jan 11, 2017

sjelin force-pushed the cf branch from 9ced31d to ebe26e2 Compare January 11, 2017 01:52

juliemr reviewed Jan 11, 2017

View reviewed changes

sjelin force-pushed the cf branch from ebe26e2 to 3740a14 Compare January 12, 2017 03:31

sjelin force-pushed the cf branch from 3740a14 to 6342f64 Compare January 12, 2017 03:55

sjelin mentioned this pull request Jan 12, 2017

feat(typescript): add types #79

Merged

juliemr removed the pr: needs review label Jan 13, 2017

sjelin merged commit 27b4850 into angular:jasminewd2 Jan 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(SELENIUM_PROMISE_MANAGER): Support `SELENIUM_PROMISE_MANAGER=0` #72

feat(SELENIUM_PROMISE_MANAGER): Support `SELENIUM_PROMISE_MANAGER=0` #72

sjelin commented Dec 17, 2016 •

edited

Loading

sjelin commented Dec 28, 2016

sjelin commented Jan 3, 2017

juliemr left a comment

juliemr Jan 4, 2017

sjelin Jan 4, 2017

juliemr Jan 4, 2017

juliemr Jan 4, 2017

sjelin Jan 4, 2017

sjelin commented Jan 11, 2017

juliemr Jan 11, 2017

juliemr Jan 11, 2017

sjelin Jan 12, 2017

juliemr Jan 11, 2017

juliemr Jan 11, 2017

sjelin Jan 12, 2017

sjelin commented Jan 12, 2017

juliemr commented Jan 13, 2017

		@@ -1,110 +1,110 @@
		var webdriver = require('selenium-webdriver');
		import {promise as wdpromise, WebElement} from 'selenium-webdriver';

feat(SELENIUM_PROMISE_MANAGER): Support SELENIUM_PROMISE_MANAGER=0 #72

feat(SELENIUM_PROMISE_MANAGER): Support SELENIUM_PROMISE_MANAGER=0 #72

Conversation

sjelin commented Dec 17, 2016 • edited Loading

sjelin commented Dec 28, 2016

sjelin commented Jan 3, 2017

juliemr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjelin commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjelin commented Jan 12, 2017

juliemr commented Jan 13, 2017

feat(SELENIUM_PROMISE_MANAGER): Support `SELENIUM_PROMISE_MANAGER=0` #72

feat(SELENIUM_PROMISE_MANAGER): Support `SELENIUM_PROMISE_MANAGER=0` #72

sjelin commented Dec 17, 2016 •

edited

Loading