testiumjs / testium-core

Juggling the bits and pieces to run integration tests
BSD 3-Clause "New" or "Revised" License
4 stars 7 forks source link

Pass remote debugging port for chrome #39

Closed anil-alt closed 6 years ago

anil-alt commented 6 years ago

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 commented 6 years ago

Some thoughts:

  1. 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.
  2. Implied in 1: I think .getPort is a little too generic for what that method does.
  3. 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 commented 6 years ago

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

jkrems commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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

anil-alt commented 6 years ago

@jkrems Thanks for the review :)

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

jkrems commented 6 years ago

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