Upgrade to support electron 4 (2) #361

alexstrat · 2019-05-17T16:49:01Z

Finish #354 by implementing the workaround of #346 (comment)

Implement workaround found https://bitbucket.org/chromiumembedded/cef/issues/2476/cef-webdriver-instructions-will-need-to

alexstrat · 2019-05-17T16:50:06Z

test/application-test.js

@@ -137,7 +137,7 @@ describe('application loading', function () {
    it('gets the render process console logs and clears them', function () {
      return app.client.waitUntilWindowLoaded()
        .getRenderProcessLogs().then(function (logs) {
-          expect(logs.length).to.equal(3)
+          expect(logs.length).to.equal(4)


I have not checked, but probably an Electron warning is fired.

alexstrat · 2019-05-17T16:51:20Z

test/multi-window-test.js

@@ -24,15 +24,15 @@ describe('multiple windows', function () {
  it('launches the application', function () {
    return app.client
      .getWindowCount().should.eventually.equal(2)
-      .windowByIndex(1)
+      .windowByIndex(0)


Not sure why the order has changed, but don't think this change breaks the idea of the test anyway 🤷‍♂️

Actually, it looks like we can't rely on the order of windowhandles

https://github.com/webdriverio/webdriverio/blob/567f8778bc8a66a2a68ad9e12da2c81e86fabe47/packages/webdriver/protocol/webdriver.json#L192

So I rewrote the test to be independent of the order of the returned windows.

alexstrat · 2019-05-17T19:44:37Z

Failing on Windows CI:

 1) window commands
       webContents.selectAll()
         selects all the text on the page:
     AssertionError: expected '' to include 'Hello'

Have no clue so far: is getSelectedText or selectAll not working?

Edit: was actually failing since previous bump of version: #334

MarshallOfSound · 2019-05-22T09:06:33Z

lib/application.js

@@ -149,6 +149,11 @@ Application.prototype.createClient = function () {
      args.push('spectron-env-' + name + '=' + self.env[name])
    }

+    // Port to communicate on. Required starting with ChromeDriver v2.41.
+    // https://bitbucket.org/chromiumembedded/cef/issues/2476/cef-webdriver-instructions-will-need-to
+    const randomPort = Math.floor(Math.random() * (9999 - 9000) + 9000)


This is not the right port to use, we should be using port 0 and reading the DevToolsActivePort file to get the chosen port 🤔

From the link above:

It also doesn't work with CEF. The reason is that CEF's devtools implementation refuses to handle the remote-debugging-port=0 use case. See this code: https://bitbucket.org/chromiumembedded/cef/src/8e9d736cda2015b706e183bfd8f7fd63a2cd5396/libcef/browser/devtools_manager_delegate.cc#lines-75

So I considered that, in Electron (CEF right?), remote-debugging-port=0 won't work and I have not given it a try.

I can give it a try though if you think I should!

@alexstrat I fixed --remote-debugging-port=0 a while back in Electron -> electron/electron#17800

Oh yes! I missed that indeed!

However, it looks like the fix is only available for electron >=5 and this PR is for electron 4, right?

Should I either:

backport the fix in Electron 4

keep this random port solution until Spectron 7 (Electron 5) and add a comment here about it

try the remote-debugging-port=0 here and see if it works (is there a reason for it to work?)

I started looking into electron 5 and I'm not sure we need to read DevToolsActivePort actually. Indeed currently webdriver@4 does not use the devtools-service (no trace in dependencies, etc..) which is the only component that'll need access to devtools.

Indded, I was able to get electron 5 running just by removing args.push('remote-debugging-port=${randomPort}').

I chose to keep the random port solution until Spectron 7. #364

alexstrat · 2019-05-24T12:43:32Z

Ready for 2nd review! 👐

jonbeller · 2019-05-29T16:32:16Z

I am using spectron@5.0.0 with electron@4.0.4. What might be broken for me? Sorry if this is not the right place to ask!

See electron-userland#361 (comment)

codebytere · 2019-06-02T01:59:40Z

lib/application.js

@@ -153,6 +153,20 @@ Application.prototype.createClient = function () {
      args.push(arg)
    })

+    // Since ChromeDriver v2.41, ChromeDriver with CEF will only connect


Electron doesn't use CEF :) Do you mean this is broken here in a similar way it's broken in CEF?

Ah yes, it was not accurate, but yes, I guess it's similar behavior.

codebytere · 2019-06-02T02:00:47Z

Looks like

    webContents.selectAll()
      1) selects all the text on the page

is still failing 🤔

alexstrat · 2019-06-02T07:49:31Z

@codebytere yes, indeed, but it was failing since electron 3 #334 see 25d492d's Appeyor build.

I've done some research about it but I could not find a clue and I can hardly test in Windows. If you think it's necessary for merging this PR, I can have a look.

codebytere

we can investigate the windows spec failure in a follow-up

alexstrat · 2019-06-11T16:57:05Z

If ok, I think we can merge (i can't) and release?

See electron-userland#361 (comment)

This was originally employed to work around Spectron choosing a random port colliding with our back end. Spectron's choosing a random port was in turn used as an Electron workaround. The workarounds haven't been needed since Electron 5. We moved past Electron 4 at #381 . This effectively reverts #319 Other refs: electron-userland/spectron#361 electron-userland/spectron#364

shiftkey and others added 8 commits March 6, 2019 09:58

chore: bump package version

5fa1c21

chore: bump associated Electron version

9cccc68

chore: upgrade to electron-chromedriver 4.0.0

00343ac

chore: add new version to matrix

2ee6fef

chore: update chai and chai-as-promised

12ea743

Use random port

737db13

Implement workaround found https://bitbucket.org/chromiumembedded/cef/issues/2476/cef-webdriver-instructions-will-need-to

Minor fixes of tests

ed6868e

Lock

4643536

alexstrat commented May 17, 2019

View reviewed changes

alexstrat added 3 commits May 17, 2019 20:07

👕

55acabe

Make multi-window tests independant of the order of windowhandles()

1b2cede

Update to latest electron version

d1596ed

alexstrat mentioned this pull request May 21, 2019

[WIP] upgrade to support electron 4 #354

Closed

MarshallOfSound reviewed May 22, 2019

View reviewed changes

alexstrat mentioned this pull request May 24, 2019

Upgrade to support electron 5 #364

Merged

4 tasks

alexstrat added 2 commits May 24, 2019 14:40

Do not set random if it was already set by user

49c0c22

Make comment clearer

b8e8dbd

codebytere mentioned this pull request May 30, 2019

Electron 5 Support? #358

Open

alexstrat added a commit to alexstrat/spectron that referenced this pull request Jun 1, 2019

🤷‍♂️

60ce736

See electron-userland#361 (comment)

codebytere reviewed Jun 2, 2019

View reviewed changes

Make comment more accurate

766af85

codebytere approved these changes Jun 2, 2019

View reviewed changes

codebytere merged commit a64c809 into electron-userland:master Jun 20, 2019

alexstrat added a commit to alexstrat/spectron that referenced this pull request Jun 24, 2019

🤷‍♂️

4104a4f

See electron-userland#361 (comment)

mikesbrown mentioned this pull request Apr 16, 2020

test fix: remove explicit remote-debugging-port brimdata/zui#628

Merged

Upgrade to support electron 4 (2) #361

Upgrade to support electron 4 (2) #361

Uh oh!

Conversation

alexstrat commented May 17, 2019

Uh oh!

alexstrat May 17, 2019

Choose a reason for hiding this comment

Uh oh!

alexstrat May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexstrat May 17, 2019

Choose a reason for hiding this comment

Uh oh!

alexstrat commented May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarshallOfSound May 22, 2019

Choose a reason for hiding this comment

Uh oh!

alexstrat May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarshallOfSound May 22, 2019

Choose a reason for hiding this comment

Uh oh!

alexstrat May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexstrat May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexstrat May 24, 2019

Choose a reason for hiding this comment

Uh oh!

alexstrat commented May 24, 2019

Uh oh!

jonbeller commented May 29, 2019

Uh oh!

codebytere Jun 2, 2019

Choose a reason for hiding this comment

Uh oh!

alexstrat Jun 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codebytere commented Jun 2, 2019

Uh oh!

alexstrat commented Jun 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codebytere left a comment

Choose a reason for hiding this comment

Uh oh!

alexstrat commented Jun 11, 2019

Uh oh!

Uh oh!

alexstrat May 17, 2019 •

edited

Loading

alexstrat commented May 17, 2019 •

edited

Loading

alexstrat May 22, 2019 •

edited

Loading

alexstrat May 22, 2019 •

edited

Loading

alexstrat May 22, 2019 •

edited

Loading

alexstrat Jun 2, 2019 •

edited

Loading

alexstrat commented Jun 2, 2019 •

edited

Loading