fix(cli): more consistent chromePath handling (#284)

Author: patrickhulce

packages/cli/src/collect/collect.js

@@ -7,7 +7,7 @@
 
 const path = require('path');
 const yargsParser = require('yargs-parser');
-const ChromeLauncher = require('chrome-launcher').Launcher;
+const {determineChromePath} = require('../utils.js');
 const FallbackServer = require('./fallback-server.js');
 const LighthouseRunner = require('./lighthouse-runner.js');
 const PuppeteerManager = require('./puppeteer-manager.js');
@@ -41,10 +41,7 @@ function buildCommand(yargs) {
     },
     chromePath: {
       description: 'The path to the Chrome or Chromium executable to use for collection.',
-      default:
-        process.env.CHROME_PATH ||
-        new PuppeteerManager(naiveOptions).getChromiumPath() ||
-        ChromeLauncher.getInstallations()[0],
+      default: determineChromePath(naiveOptions),
     },
     puppeteerScript: {
       description:

packages/cli/src/collect/puppeteer-manager.js

@@ -25,7 +25,7 @@ class PuppeteerManager {
    * setting of `collect.chromePath`.
    * @return {typeof import('puppeteer')|undefined}
    */
-  _requirePuppeteer() {
+  static _requirePuppeteer() {
     try {
       // eslint-disable-next-line import/no-extraneous-dependencies
       return require('puppeteer');
@@ -43,7 +43,7 @@ class PuppeteerManager {
     if (this._browser) return this._browser;
 
     // Delay require to only run after user requests puppeteer functionality.
-    const puppeteer = this._requirePuppeteer();
+    const puppeteer = PuppeteerManager._requirePuppeteer();
     if (!puppeteer) {
       throw new Error(`Unable to require 'puppeteer' for script, have you run 'npm i puppeteer'?`);
     }
@@ -65,13 +65,16 @@ class PuppeteerManager {
     return !!this._options.puppeteerScript;
   }
 
-  /** @return {string|undefined} */
-  getChromiumPath() {
+  /**
+   * @param {{puppeteerScript?: string}} options
+   * @return {string|undefined}
+   */
+  static getChromiumPath(options) {
     // If we're not using puppeteer, return undefined.
-    if (!this._options.puppeteerScript) return undefined;
+    if (!options.puppeteerScript) return undefined;
 
     // Otherwise, check to see if the expected puppeteer download exists.
-    const puppeteer = this._requirePuppeteer();
+    const puppeteer = PuppeteerManager._requirePuppeteer();
     const chromiumPath = puppeteer && puppeteer.executablePath();
     return chromiumPath && fs.existsSync(chromiumPath) ? chromiumPath : undefined;
   }

packages/cli/src/healthcheck/healthcheck.js

@@ -5,8 +5,7 @@
  */
 'use strict';
 
-const fs = require('fs');
-const ChromeLauncher = require('chrome-launcher').Launcher;
+const {canAccessPath, determineChromePath} = require('../utils.js');
 const ApiClient = require('@lhci/utils/src/api-client.js');
 const {loadAndParseRcFile, resolveRcFilePath} = require('@lhci/utils/src/lighthouserc.js');
 const {
@@ -65,8 +64,8 @@ const checks = [
     failureLabel: 'Chrome installation not found',
     shouldTest: () => true,
     test: opts => {
-      if (opts.chromePath) return fs.existsSync(opts.chromePath);
-      return ChromeLauncher.getInstallations().length > 0;
+      if (opts.chromePath) return canAccessPath(opts.chromePath);
+      return canAccessPath(determineChromePath(opts) || '');
     },
   },
   {

packages/cli/src/utils.js

@@ -5,6 +5,10 @@
  */
 'use strict';
 
+const fs = require('fs');
+const PuppeteerManager = require('./collect/puppeteer-manager.js');
+const ChromeLauncher = require('chrome-launcher').Launcher;
+
 /** @param {string} dependency */
 function assertOptionalDependency(dependency) {
   try {
@@ -19,4 +23,49 @@ function assertOptionalDependency(dependency) {
   }
 }
 
-module.exports = {assertOptionalDependency};
+/** @param {string} filePath @return {boolean} */
+function canAccessPath(filePath) {
+  try {
+    fs.accessSync(filePath);
+    return true;
+  } catch (_) {
+    return false;
+  }
+}
+
+/**
+ * ChromeLauncher has an annoying API that *throws* instead of returning an empty array.
+ * Also enables testing of environments that don't have Chrome globally installed in tests.
+ *
+ * @return {string[]}
+ */
+function getChromeInstallationsSafe() {
+  if (process.env.LHCITEST_IGNORE_CHROME_INSTALLATIONS) return [];
+
+  try {
+    return ChromeLauncher.getInstallations();
+  } catch (err) {
+    return [];
+  }
+}
+
+/**
+ * @param {Partial<LHCI.CollectCommand.Options>} options
+ * @return {string|undefined}
+ */
+function determineChromePath(options) {
+  return (
+    options.chromePath ||
+    process.env.CHROME_PATH ||
+    PuppeteerManager.getChromiumPath(options) ||
+    getChromeInstallationsSafe()[0] ||
+    undefined
+  );
+}
+
+module.exports = {
+  assertOptionalDependency,
+  canAccessPath,
+  determineChromePath,
+  getChromeInstallationsSafe,
+};

packages/cli/test/collect-puppeteer.test.js

@@ -49,4 +49,23 @@ describe('Lighthouse CI collect CLI with puppeteer', () => {
     `);
     expect(status).toEqual(0);
   }, 180000);
+
+  it('should not fail on providing defaults without Chrome installations', () => {
+    const {stdout, stderr, status} = runCLI(['collect', '--help'], {
+      cwd: autorunDir,
+      env: {LHCITEST_IGNORE_CHROME_INSTALLATIONS: '1'},
+    });
+
+    // Make sure there is no default chromePath found
+    const chromePathHelp = stdout.match(/--chromePath.*\n.*\n.*/);
+    expect(chromePathHelp).toMatchInlineSnapshot(`
+      Array [
+        "--chromePath               The path to the Chrome or Chromium executable to
+                                   use for collection.
+        --puppeteerScript          The path to a script that manipulates the browser",
+      ]
+    `);
+    expect(stderr).toMatchInlineSnapshot(`""`);
+    expect(status).toEqual(0);
+  });
 });

packages/cli/test/healthcheck.test.js

@@ -7,7 +7,10 @@
 
 /* eslint-env jest */
 
+const os = require('os');
+const fs = require('fs');
 const path = require('path');
+const rimraf = require('rimraf');
 const {runCLI} = require('./test-utils.js');
 
 describe('Lighthouse CI healthcheck CLI', () => {
@@ -114,4 +117,70 @@ describe('Lighthouse CI healthcheck CLI', () => {
       expect(stderr).toMatchInlineSnapshot(`""`);
     });
   });
+
+  describe('chrome installation', () => {
+    let fixtureDir;
+
+    beforeEach(() => {
+      fixtureDir = path.join(os.tmpdir(), fs.mkdtempSync('lhcihealthcheck'));
+      fs.mkdirSync(fixtureDir, {recursive: true});
+    });
+
+    afterEach(() => {
+      rimraf.sync(fixtureDir);
+    });
+
+    it('should find a chrome installation installed on the system', () => {
+      const {stdout, stderr, status} = runCLI(['healthcheck', '--fatal'], {
+        cwd: fixtureDir,
+      });
+
+      expect(stdout).toMatchInlineSnapshot(`
+        "✅  .lighthouseci/ directory writable
+        ⚠️   Configuration file not found
+        ✅  Chrome installation found
+        Healthcheck passed!
+        "
+      `);
+      expect(stderr).toMatchInlineSnapshot(`""`);
+      expect(status).toEqual(0);
+    });
+
+    it('should not find a chrome installation when ignored', () => {
+      const {stdout, stderr, status} = runCLI(['healthcheck', '--fatal'], {
+        cwd: fixtureDir,
+        env: {LHCITEST_IGNORE_CHROME_INSTALLATIONS: '1'},
+      });
+
+      expect(stdout).toMatchInlineSnapshot(`
+        "✅  .lighthouseci/ directory writable
+        ⚠️   Configuration file not found
+        ❌  Chrome installation not found
+        Healthcheck failed!
+        "
+      `);
+      expect(stderr).toMatchInlineSnapshot(`""`);
+      expect(status).toEqual(1);
+    });
+
+    it('should find a chrome installation when ignored but passed explicitly via config', () => {
+      const ci = {collect: {chromePath: fixtureDir}};
+      fs.writeFileSync(path.join(fixtureDir, '.lighthouserc.json'), JSON.stringify({ci}));
+
+      const {stdout, stderr, status} = runCLI(['healthcheck', '--fatal'], {
+        cwd: fixtureDir,
+        env: {LHCI_NO_LIGHTHOUSERC: undefined, LHCITEST_IGNORE_CHROME_INSTALLATIONS: '1'},
+      });
+
+      expect(stdout).toMatchInlineSnapshot(`
+        "✅  .lighthouseci/ directory writable
+        ✅  Configuration file found
+        ✅  Chrome installation found
+        Healthcheck passed!
+        "
+      `);
+      expect(stderr).toMatchInlineSnapshot(`""`);
+      expect(status).toEqual(0);
+    });
+  });
 });

packages/cli/test/test-utils.js

@@ -111,6 +111,7 @@ function waitForCondition(fn, label) {
 function getCleanEnvironment(extraEnvVars) {
   const cleanEnv = {
     ...process.env,
+    CHROME_PATH: undefined,
     LHCI_GITHUB_TOKEN: undefined,
     LHCI_GITHUB_APP_TOKEN: undefined,
     LHCI_CANARY_SERVER_URL: undefined,