Noissue - Upgrade to selenium-webdriver 4.0.0-alpha

Noissue - Upgrade to selenium-webdriver 4.0.0-alpha

COLLABORATOR=sebastian@adblockplus.org

[hub, 2018-11-06 08:39:48]

We hope this should work better for MSEdge, even though this is alpha1. 

I have tested this change on macOS. Currently there is a test failure in Chrome
I need to verify.

[Sebastian Noack, 2018-11-06 09:02:33]

https://codereview.adblockplus.org/29938555/diff/29938556/package.json
File package.json (right):

https://codereview.adblockplus.org/29938555/diff/29938556/package.json#newcode25
package.json:25: "selenium-webdriver": "4.0.0-alpha1",
We might want to require ^4.0.0-alpha here.

https://codereview.adblockplus.org/29938555/diff/29938556/test/browsers/firef...
File test/browsers/firefox.js (right):

https://codereview.adblockplus.org/29938555/diff/29938556/test/browsers/firef...
test/browsers/firefox.js:43: .setParameter("path", devenvPath)
Nit: The indentation seems to be off here by 2 spaces.

https://codereview.adblockplus.org/29938555/diff/29938556/test/browsers/firef...
test/browsers/firefox.js:46: driver.execute(cmd);
Nit: I think we don't need the cmd variable anymore (previously it was
referenced twice, now it could be inlined).

https://codereview.adblockplus.org/29938555/diff/29938556/test/wrappers/pages.js
File test/wrappers/pages.js (right):

https://codereview.adblockplus.org/29938555/diff/29938556/test/wrappers/pages...
test/wrappers/pages.js:77: element.getRect()
You might want to use the loc variable (and maybe rename it) from.the outer
scope here rather than calling getRect() again.

[tlucas, 2018-11-07 08:57:16]

https://codereview.adblockplus.org/29938555/diff/29938556/package.json
File package.json (right):

https://codereview.adblockplus.org/29938555/diff/29938556/package.json#newcode25
package.json:25: "selenium-webdriver": "4.0.0-alpha1",
On 2018/11/06 09:02:33, Sebastian Noack wrote:
> We might want to require ^4.0.0-alpha here.

If not, there's still a typo: 4.0.0-alpha.1
                                         ^

[hub, 2018-11-07 17:13:37]

updated patch

https://codereview.adblockplus.org/29938555/diff/29938556/package.json
File package.json (right):

https://codereview.adblockplus.org/29938555/diff/29938556/package.json#newcode25
package.json:25: "selenium-webdriver": "4.0.0-alpha1",
On 2018/11/06 09:02:33, Sebastian Noack wrote:
> We might want to require ^4.0.0-alpha here.

I had started with that but I'm wondering of the API stability in the alpha
series. I'd hate that `alpha.2` break API.

https://codereview.adblockplus.org/29938555/diff/29938556/package.json#newcode25
package.json:25: "selenium-webdriver": "4.0.0-alpha1",
On 2018/11/07 08:57:16, tlucas wrote:
> On 2018/11/06 09:02:33, Sebastian Noack wrote:
> > We might want to require ^4.0.0-alpha here.
> 
> If not, there's still a typo: 4.0.0-alpha.1
>                                          ^

oops.

https://codereview.adblockplus.org/29938555/diff/29938556/test/browsers/firef...
File test/browsers/firefox.js (right):

https://codereview.adblockplus.org/29938555/diff/29938556/test/browsers/firef...
test/browsers/firefox.js:43: .setParameter("path", devenvPath)
On 2018/11/06 09:02:33, Sebastian Noack wrote:
> Nit: The indentation seems to be off here by 2 spaces.

Done.

https://codereview.adblockplus.org/29938555/diff/29938556/test/browsers/firef...
test/browsers/firefox.js:46: driver.execute(cmd);
On 2018/11/06 09:02:33, Sebastian Noack wrote:
> Nit: I think we don't need the cmd variable anymore (previously it was
> referenced twice, now it could be inlined).

Done.

https://codereview.adblockplus.org/29938555/diff/29938556/test/wrappers/pages.js
File test/wrappers/pages.js (right):

https://codereview.adblockplus.org/29938555/diff/29938556/test/wrappers/pages...
test/wrappers/pages.js:77: element.getRect()
On 2018/11/06 09:02:33, Sebastian Noack wrote:
> You might want to use the loc variable (and maybe rename it) from.the outer
> scope here rather than calling getRect() again.

Done.

[Sebastian Noack, 2018-11-07 18:35:49]

https://codereview.adblockplus.org/29938555/diff/29939616/package.json
File package.json (right):

https://codereview.adblockplus.org/29938555/diff/29939616/package.json#newcode25
package.json:25: "selenium-webdriver": "4.0.0-alpha.1",
^4.0.0-alpha (as I initially suggested) woul work too. The difference is that it
will automatically update to 4.0.1-alpha or 4.0.0-alpha.2 as sooner as it
becomes available, which is more consistent with how we require other packages.

https://codereview.adblockplus.org/29938555/diff/29939616/test/wrappers/pages.js
File test/wrappers/pages.js (right):

https://codereview.adblockplus.org/29938555/diff/29939616/test/wrappers/pages...
test/wrappers/pages.js:66: () => element.getRect().then(loc =>
Nit: Perhaps rename loc to rect?

[hub, 2018-11-08 19:02:22]

Updated patch.


There is still a failure on Chrome (time out), I need to resolve.

https://codereview.adblockplus.org/29938555/diff/29939616/package.json
File package.json (right):

https://codereview.adblockplus.org/29938555/diff/29939616/package.json#newcode25
package.json:25: "selenium-webdriver": "4.0.0-alpha.1",
On 2018/11/07 18:35:49, Sebastian Noack wrote:
> ^4.0.0-alpha (as I initially suggested) woul work too. The difference is that
it
> will automatically update to 4.0.1-alpha or 4.0.0-alpha.2 as sooner as it
> becomes available, which is more consistent with how we require other
packages.

OK. we'll see what we do when the alpha break APIs

https://codereview.adblockplus.org/29938555/diff/29939616/test/wrappers/pages.js
File test/wrappers/pages.js (right):

https://codereview.adblockplus.org/29938555/diff/29939616/test/wrappers/pages...
test/wrappers/pages.js:66: () => element.getRect().then(loc =>
On 2018/11/07 18:35:49, Sebastian Noack wrote:
> Nit: Perhaps rename loc to rect?

Done.

[Sebastian Noack, 2018-11-08 21:39:43]

LGTM

[Sebastian Noack, 2018-11-08 22:02:06]

Another more thing, I just noticed, Selenium 4.0.0 requires Node.js 8.9 or
higher, we should update our README accordingly.

[hub, 2018-11-09 20:16:32]

updated patch

[Sebastian Noack, 2018-11-09 22:18:52]

I added a new patch set, fixing the test failure on Chromium.

(The changes in package.json are due to rebasing.)

[hub, 2018-11-09 22:23:31]

https://codereview.adblockplus.org/29938555/diff/29941678/test/wrappers/pages.js
File test/wrappers/pages.js (right):

https://codereview.adblockplus.org/29938555/diff/29941678/test/wrappers/pages...
test/wrappers/pages.js:70: lastScreenshot]).then(([loc]) =>
you might have taken the wrong version of the patch to rebase on, as this was
supposed to be `rect` instead of `loc` (as per your suggestion in the review)

[Sebastian Noack, 2018-11-09 22:26:53]

https://codereview.adblockplus.org/29938555/diff/29941678/test/wrappers/pages.js
File test/wrappers/pages.js (right):

https://codereview.adblockplus.org/29938555/diff/29941678/test/wrappers/pages...
test/wrappers/pages.js:70: lastScreenshot]).then(([loc]) =>
On 2018/11/09 22:23:31, hub wrote:
> you might have taken the wrong version of the patch to rebase on, as this was
> supposed to be `rect` instead of `loc` (as per your suggestion in the review)

Oops, I indeed missed your last patch set. Fixed.

[hub, 2018-11-09 22:31:55]

LGTM