Pass remote debugging port for chrome #39

anil-alt · 2018-06-25T19:58:13Z

Set '--remote-debugging-port' in testium config, which would be consumed by lighthouse to talk with testium.

Line of Thought:

Step 1: [Indidvidual apps] Add flag --remote-debugging-port to .testiumrc in the App's repo

Step 2: [testium-core] Use this flag to pass to chromedriver/chrome by either directly using the port number or by finding next available port. Also update the testium property, if new port number was used

Step 3: [testium-driver-wd] Add getPort method as wd chain methods to fetch the testium property

  wd.addPromiseChainMethod('getPort', function getPort() {
    let debugPort = testium.config.get('chrome.--remote-debugging-port');
    debug('Remote dubugging port set as', debugPort);
    return debugPort;
  });

Step 4: [Individual apps or itier-accessibility] Port fetch from browser.getPort would be used to pass it to lighthouse

 browser.getPort()
    .then(port => lighthouse(url, Object.assign({}, opts, { port }), config))
    .then(results => new Promise(res => res(results.audits)));

jkrems · 2018-07-02T20:38:46Z

lib/processes.js

+      if (debugPort) {
+        findOpenPort().then(function (port) {
+          args.push('--remote-debugging-port=' + port);
+          config.set('chrome.--remote-debugging-port', port);


This feels like a weird name for an option. Why not remoteDebuggingPort?

jkrems · 2018-07-02T20:39:43Z

lib/processes.js

@@ -97,6 +97,15 @@ function launchAllProcesses(config) {
        args.push('--headless');
      }
      if (process.getuid() === 0) args.push('--no-sandbox');
+
+      var debugPort = config.get('chrome.--remote-debugging-port');


Any reason why this isn't the default? It feels weird that people have to opt-in to enable this feature..?

Did you mean to always pass the port and set in the config? If yes, this was my initial thought -
In most cases, port number won't be used in the tests. Also, I was not very sure that, if we should always set the port even if it would be assigned by chromedriver.

But it makes sense now, if we are exposing API like getPort from testium. We should make it return in consistent way

IIRC there was some chat to just always read it from the port file written by Chrome. Didn't that work out?

@jkrems Unfortunately, that didn't work. Not sure if that was due to system issue (some process errors out writing to hidden directory/files). Looking at chromelauncher code, this option didn't seem bad to me :)

So setting this option is compatible with chromedriver after all? If so - SGTM. Is there any way we can verify that Chrome has in fact been started with that option?

Captured this SS from activity Monitor for the chrome instance. Port number generated programmatically was- 59196. Not sure, if anything is obvious here :(

jkrems · 2018-07-02T20:54:42Z

Some thoughts:

I think browser.getChromeDevtoolsPort() should just work when using chrome. Having to remember to set a global option seems like it would lead to confusion in the future.
Implied in 1: I think .getPort is a little too generic for what that method does.
I think testium.getChromeDevtoolsPort() should exist and throw a helpful error message when trying to use it outside of Chrome. Accessing the config directly from -wd makes it harder to tell where things are going wrong.

In the future I would hope that we could even get a driver instance exposed from testium so that we can reuse more logic. AFAICT it should be possible to create a reusable connection (e.g. that could be used to implement better/more reliable APIs) outside of lighthouse and pass that in. So hopefully a future API would be testium.getChromeDevtoolsConnection() rather than ...Port().

anil-alt · 2018-07-02T23:22:34Z

@jkrems What is the best place to add getChromeDevtoolsPort method? So far, I was thinking to add in testium-driver-wd

jkrems · 2018-07-03T14:31:42Z

I think the "real" implementation should definitely live in -core. We can expose it from -wd but I wouldn't want it to have deep knowledge about how it's implemented. So I'd prefer the -wd implementation to be not much more than return testium.getChromeDevtoolsPort();. That way we have all the pieces in one place and can iterate here without having to keep changes in sync.

anil-alt · 2018-07-03T19:33:03Z

@jkrems addressed the comments in last commit, except throwing error in the case port is not set. Should we just log the message for the failure case?

jkrems · 2018-07-03T19:36:38Z

lib/processes.js

@@ -97,6 +103,17 @@ function launchAllProcesses(config) {
        args.push('--headless');
      }
      if (process.getuid() === 0) args.push('--no-sandbox');
+
+      var debugPort = config.get('chrome.remoteDebuggingPort', null);


Since this already runs only when we're using chrome - any reason not to default this to true instead of null?

@jkrems This assumes that if the port has been set (with actual value) in .testiumrc, it would address that. Otherwise would discover the new port and update the config.

Are you suggesting that to remove the check and find new port anyway?

Is there value in making the port configurable? That seems to encourage hard-coding values which might lead to problems when running multiple test suites etc..

Oh yes. I didn't think about that. I'll remove the checks

jkrems

One question, otherwise LGTM

jkrems · 2018-07-03T19:37:37Z

lib/init.js

    testium = {
      close: close,
      config: config,
      getInitialUrl: getInitialUrl,
-      getNewPageUrl: _.partial(getNewPageUrl, config.get('proxy.targetUrl'))
+      getNewPageUrl: _.partial(getNewPageUrl, config.get('proxy.targetUrl')),
+      getChromeDevtoolsPort: getChromeDevtoolsPort


nit: it looks like those were sorted alphabetically before.

jkrems

🎉

jkrems · 2018-07-03T21:15:56Z

Thanks for digging into this & sorry for the long back-and-forth! :)

anil-alt · 2018-07-03T21:46:53Z

@jkrems Thanks for the review :)

How do I release new version now? I believe one with 'write access' can do

jkrems · 2018-07-03T21:48:20Z

These libraries are under nlm, so the new version got released once this got merged!

This reverts commit decfd60, reversing changes made to d006155.

Revert "Merge pull request #39 from anil-groupon/pass-debug-port"

feat: Pass remote debugging port for chrome

209ebae

jkrems reviewed Jul 2, 2018

View reviewed changes

chore: handle review comments

2274697

anil-alt force-pushed the pass-debug-port branch from b746d3c to 2274697 Compare July 2, 2018 23:22

chore: PR review comment

c942a21

jkrems reviewed Jul 3, 2018

View reviewed changes

jkrems approved these changes Jul 3, 2018

View reviewed changes

jkrems reviewed Jul 3, 2018

View reviewed changes

chore: remove check for the config flag

115d0c7

anil-alt force-pushed the pass-debug-port branch from 0d35459 to 115d0c7 Compare July 3, 2018 21:03

jkrems approved these changes Jul 3, 2018

View reviewed changes

jkrems changed the title ~~feat: Pass remote debugging port for chrome~~ Pass remote debugging port for chrome Jul 3, 2018

jkrems merged commit decfd60 into testiumjs:master Jul 3, 2018

anil-alt deleted the pass-debug-port branch July 3, 2018 21:22

jkrems pushed a commit that referenced this pull request Jul 5, 2018

fix: Revert "Merge pull request #39 from anil-groupon/pass-debug-port"

a7b2116

This reverts commit decfd60, reversing changes made to d006155.

jkrems added a commit that referenced this pull request Jul 5, 2018

Merge pull request #40 from testiumjs/jkrems/feature/master/revert-port

5168cb6

Revert "Merge pull request #39 from anil-groupon/pass-debug-port"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass remote debugging port for chrome #39

Pass remote debugging port for chrome #39

anil-alt commented Jun 25, 2018 •

edited

Loading

jkrems Jul 2, 2018

jkrems Jul 2, 2018

anil-alt Jul 2, 2018

jkrems Jul 2, 2018

anil-alt Jul 2, 2018

jkrems Jul 2, 2018

anil-alt Jul 2, 2018

jkrems commented Jul 2, 2018

anil-alt commented Jul 2, 2018

jkrems commented Jul 3, 2018

anil-alt commented Jul 3, 2018 •

edited

Loading

jkrems Jul 3, 2018

anil-alt Jul 3, 2018

jkrems Jul 3, 2018

anil-alt Jul 3, 2018

jkrems left a comment

jkrems Jul 3, 2018

jkrems left a comment

jkrems commented Jul 3, 2018

anil-alt commented Jul 3, 2018

jkrems commented Jul 3, 2018 •

edited

Loading

Pass remote debugging port for chrome #39

Pass remote debugging port for chrome #39

Conversation

anil-alt commented Jun 25, 2018 • edited Loading

Line of Thought:

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

Choose a reason for hiding this comment

jkrems commented Jul 2, 2018

anil-alt commented Jul 2, 2018

jkrems commented Jul 3, 2018

anil-alt commented Jul 3, 2018 • edited Loading

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

jkrems left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkrems left a comment

Choose a reason for hiding this comment

jkrems commented Jul 3, 2018

anil-alt commented Jul 3, 2018

jkrems commented Jul 3, 2018 • edited Loading

anil-alt commented Jun 25, 2018 •

edited

Loading

anil-alt commented Jul 3, 2018 •

edited

Loading

jkrems commented Jul 3, 2018 •

edited

Loading