cli: only launch Chrome when running against localhost

[tkindy]

Summary
This PR fixes the CLI so it doesn't try to launch a local Chrome instance when given a remote hostname to connect to. This allows Lighthouse runs via the CLI in environments that don't have Chrome installed (such as in CI pipelines).

Related Issues/PRs

Closes #12137

[tkindy]

I'm not sure how to test this. I don't have much experience with Jest; is it possible to assert that a code path wasn't taken (i.e. that we don't try to launch a local Chrome instance)?

[connorjclark]

I'm not sure how to test this. I don't have much experience with Jest; is it possible to assert that a code path wasn't taken (i.e. that we don't try to launch a local Chrome instance)?

I think you could mock the launch function of the chrome-launcher module, and assert it isn't called in a test.

[RegattaNick]

@connorjclark I can test this, as I encountered this exact issue, though I cannot perform regression testing to ensure it works in other scenarios.

[RegattaNick]

I hit this

lighthouse https://google.com --port=9514 --hostname=XX.XX.XX.XX
  config:warn IFrameElements gatherer requested, however no audit requires it. +0ms
  config:warn FormElements gatherer requested, however no audit requires it. +1ms
  status Connecting to browser +84ms
  CriConnection:warn Cannot create new tab; reusing open tab. +8ms
  status Disconnecting from browser... +2ms
  CriConnection:error sendRawMessage() was called without an established connection. +1ms
  GatherRunner disconnect:error sendRawMessage() was called without an established connection. +1ms
Unable to connect to Chrome

[patrickhulce]

@RegattaNick are you trying to use a selenium/webdriver server or something as the host? That error path happens when the port that's exposed isn't an actual remote debuggable Chromium instance.

[RegattaNick]

@RegattaNick are you trying to use a selenium/webdriver server or something as the host? That error path happens when the port that's exposed isn't an actual remote debuggable Chromium instance.

You know whats funny, thats how I ended up here (after a couple hours digging around), but I learned pretty swiftly that you can't do that directly.

So heres the thing - I have a chromedriver instance running on port 9515 and a full raw chrome instance running with that port open on 9514. (CDriver works fine!)

EDIT:

Turns out I also needed to use
--remote-debugging-address=0.0.0.0 as a flag on the chrome instance I was running!

PR Looks good.

[RegattaNick]

For whatever my approval is worth...

[tkindy]

Thanks for the advice on mocking, @connorjclark! I'm running into some trouble trying to write the test still, though.

Currently, I've just mocked the call to ChromeLauncher.launch so I can later assert that it wasn't called. However, I run into further problems where the Lighthouse run attempts to continue even though it doesn't have a real Chrome instance to connect to. The test ends up spinning its wheels forever, seemingly waiting for something. I tried debugging through to figure out where the indefinite wait is but couldn't find it after some time.

For this test, I really just want to avoid running Lighthouse altogether; I just need to see if it launches Chrome beforehand. I figured I could mock out the actual Lighthouse call, but the following code didn't seem to work:

jest.doMock(
  '../../../lighthouse-core/index.js',
  () => () => Promise.resolve()
);

Any ideas on how to get around this? Thanks!

[connorjclark]

I think we actually want to use jest spies here. But, chrome-launcher is made in such a way for spys do not work. (the module export object is this es5-ied commonjs mess, and jest spits out an error "can't overrwrite property with just a getter". I share the code that should work, if that module is ever fixed... @paulirish suggested that might be possible)

Anyway, in the meantime.... we have no tests for this file launch chrome anyways, so I guess it isn't the worst thing to forgo writing a test here. Let's just delete it.

diff --git a/lighthouse-cli/test/cli/run-test.js b/lighthouse-cli/test/cli/run-test.js
index 3c965eb5a..c0182d62c 100644
--- a/lighthouse-cli/test/cli/run-test.js
+++ b/lighthouse-cli/test/cli/run-test.js
@@ -5,6 +5,14 @@
  */
 'use strict';
 
+// Map plugin name to fixture since not actually installed in node_modules/.
+jest.mock('lighthouse-plugin-simple', () => {
+  // eslint-disable-next-line max-len
+  return require('../../../lighthouse-core/test/fixtures/config-plugins/lighthouse-plugin-simple/plugin-simple.js');
+}, {virtual: true});
+
+const launchChromeFn = jest.spyOn(require('chrome-launcher'), 'launch');
+
 /* eslint-env jest */
 const assert = require('assert').strict;
 const path = require('path');
@@ -19,12 +27,6 @@ const fastConfig = {
   },
 };
 
-// Map plugin name to fixture since not actually installed in node_modules/.
-jest.mock('lighthouse-plugin-simple', () => {
-  // eslint-disable-next-line max-len
-  return require('../../../lighthouse-core/test/fixtures/config-plugins/lighthouse-plugin-simple/plugin-simple.js');
-}, {virtual: true});
-
 const getFlags = require('../../cli-flags.js').getFlags;
 
 describe('CLI run', function() {
@@ -183,16 +185,10 @@ describe('Parsing --chrome-flags', () => {
 });
 
 it('doesn\'t launch a local Chrome when given an external hostname', async () => {
-  const launch = require('chrome-launcher').launch;
-  jest.doMock('chrome-launcher', () => ({
-    launch: jest.fn(() => Promise.reject()),
-  }));
-
-  /** @type {!jest.MockedFunction<typeof launch>} */
-  const mockLaunch = (launch);
+  launchChromeFn.mockImplementation(() => Promise.reject());
 
   const url = 'chrome://version';
   await run.runLighthouse(url, getFlags(`${url} --hostname=192.168.1.1`), fastConfig);
 
-  expect(mockLaunch.mock).toBeCalledTimes(0);
+  expect(launchChromeFn).toBeCalledTimes(0);
 });

[tkindy]

Thanks for taking a look @connorjclark! I'll delete the test.

[tkindy]

@connorjclark just wanted to make sure you knew this was ready for another review. Thanks! 😄

[connorjclark]

LGTM, thanks!

[RegattaNick]

Noice - thanks guys