clean up and unification of custom audit/gatherer loading

lighthouse-core/config/config.js

@@ -155,51 +155,6 @@ function getGatherersNeededByAudits(audits) {
   }, new Set());
 }
 
-/**
- * Resolves the location of the specified audit and returns a string path that
- * can then be loaded via `require()`. Throws an error if no audit is found.
- * @param {string} audit
- * @param {string=} configPath The absolute path to the config file, if there is one.
- * @return {string}
- * @throws {Error}
- */
-function resolveAudit(audit, configPath) {
-  // First try straight `require()`. Unlikely to be specified relative to this
-  // file, but adds support for Lighthouse audits in npm modules as `require()`
-  // walks up parent directories looking inside any node_modules/ present. Also
-  // handles absolute paths.
-  try {
-    require.resolve(audit);
-    return audit;
-  } catch (e) {}
-
-  // See if the audit resolves relative to the current working directory. Most
-  // useful to handle the case of invoking Lighthouse as a module, since then
-  // the config is an object and so has no path.
-  const cwdPath = path.resolve(process.cwd(), audit);
-  try {
-    require.resolve(cwdPath);
-    return cwdPath;
-  } catch (e) {}
-
-  const errorString = `Unable to locate audit: ${audit} (tried to require() ` +
-      `from '${__dirname}' and load from '${cwdPath}'`;
-  if (!configPath) {
-    throw new Error(errorString + ')');
-  }
-
-  // Finally, try looking up relative to the config file path. Just like the
-  // relative path passed to `require()` is found relative to the file it's in,
-  // this allows audit paths to be specified relative to the config file.
-  const relativePath = path.resolve(configPath, audit);
-  try {
-    require.resolve(relativePath);
-    return relativePath;
-  } catch (requireError) {}
-
-  throw new Error(errorString + ` and '${relativePath}')`);
-}
-
 function requireAudits(audits, configPath) {
   if (!audits) {
     return null;
@@ -208,15 +163,12 @@ function requireAudits(audits, configPath) {
   const coreList = Runner.getAuditList();
 
   return audits.map(audit => {
-    let requirePath;
-
     // First, see if the audit is a Lighthouse core audit.
     const coreAudit = coreList.find(a => a === `${audit}.js`);
-    if (coreAudit) {
-      requirePath = `../audits/${audit}`;
-    } else {
-      // Otherwise, attempt to find it elsewhere.
-      requirePath = resolveAudit(audit, configPath);
+    let requirePath = `../audits/${audit}`;
+    if (!coreAudit) {
+      // Otherwise, attempt to find it elsewhere. This throws if not found.
+      requirePath = Runner.resolvePlugin(audit, configPath, 'audit');
     }
 
     const AuditClass = require(requirePath);

lighthouse-core/gather/gather-runner.js

@@ -273,42 +273,19 @@ class GatherRunner {
       });
   }
 
-  static getGathererClass(gatherer, rootPath) {
+  static getGathererClass(gatherer, configPath) {
     const Runner = require('../runner');
     const coreList = Runner.getGathererList();
 
-    let GathererClass;
-
-    // First see if it is a core gatherer in Lighthouse itself.
+    // First, see if the gatherer is a Lighthouse core gatherer.
     const coreGatherer = coreList.find(a => a === `${gatherer}.js`);
     let requirePath = `./gatherers/${gatherer}`;
-
-    // If not, see if it can be found another way.
     if (!coreGatherer) {
-      // Firstly try and see if the gatherer resolves naturally through the usual means.
-      try {
-        require.resolve(gatherer);
-
-        // If the above works, update the path to the absolute value provided.
-        requirePath = gatherer;
-      } catch (requireError) {
-        // If that fails, try and find it relative to any config path provided.
-        if (rootPath) {
-          requirePath = path.resolve(rootPath, gatherer);
-        }
-      }
+      // Otherwise, attempt to find it elsewhere. This throws if not found.
+      requirePath = Runner.resolvePlugin(gatherer, configPath, 'gatherer');
     }
 
-    // Now try and require it in. If this fails then the audit file isn't where we expected it.
-    try {
-      GathererClass = require(requirePath);
-    } catch (requireError) {
-      GathererClass = null;
-    }
-
-    if (!GathererClass) {
-      throw new Error(`Unable to locate gatherer: ${gatherer} (tried at: ${requirePath})`);
-    }
+    const GathererClass = require(requirePath);
 
     // Confirm that the gatherer appears valid.
     this.assertValidGatherer(gatherer, GathererClass);

lighthouse-core/runner.js

@@ -153,6 +153,52 @@ class Runner {
         .readdirSync(path.join(__dirname, './gather/gatherers'))
         .filter(f => /\.js$/.test(f));
   }
+
+  /**
+   * Resolves the location of the specified plugin and returns an absolute
+   * string path to the file. Used for loading custom audits and gatherers.
+   * Throws an error if no plugin is found.
+   * @param {string} plugin
+   * @param {string=} configDir The absolute path to the directory of the config file, if there is one.
+   * @param {string=} category Optional plugin category (e.g. 'audit') for better error messages.
+   * @return {string}
+   * @throws {Error}
+   */
+  static resolvePlugin(plugin, configDir, category) {
+    // First try straight `require()`. Unlikely to be specified relative to this
+    // file, but adds support for Lighthouse plugins in npm modules as
+    // `require()` walks up parent directories looking inside any node_modules/
+    // present. Also handles absolute paths.
+    try {
+      return require.resolve(plugin);
+    } catch (e) {}
+
+    // See if the plugin resolves relative to the current working directory.
+    // Most useful to handle the case of invoking Lighthouse as a module, since
+    // then the config is an object and so has no path.
+    const cwdPath = path.resolve(process.cwd(), plugin);
+    try {
+      return require.resolve(cwdPath);
+    } catch (e) {}
+
+    const errorString = 'Unable to locate ' +
+        (category ? `${category}: ` : '') +
+        `${plugin} (tried to require() from '${__dirname}' and load from '${cwdPath}'`;
+
+    if (!configDir) {
+      throw new Error(errorString + ')');
+    }
+
+    // Finally, try looking up relative to the config file path. Just like the
+    // relative path passed to `require()` is found relative to the file it's
+    // in, this allows plugin paths to be specified relative to the config file.
+    const relativePath = path.resolve(configDir, plugin);
+    try {
+      return require.resolve(relativePath);
+    } catch (requireError) {}
+
+    throw new Error(errorString + ` and '${relativePath}')`);
+  }
 }
 
 module.exports = Runner;

lighthouse-core/test/fixtures/invalid-gatherers/require-error.js

@@ -0,0 +1,24 @@
+/**
+ *
+ * Copyright 2016 Google Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+'use strict';
+
+// NOTE: this require path does not resolve correctly.
+const LighthouseGatherer = require('../terrible/path/no/seriously/gatherer');
+
+class CustomGatherer extends LighthouseGatherer {}
+
+module.exports = CustomGatherer;

lighthouse-core/test/gather/gather-runner-test.js

@@ -313,6 +313,49 @@ describe('GatherRunner', function() {
     return assert.doesNotThrow(_ => GatherRunner.getGathererClass('valid-custom-gatherer', root));
   });
 
+  it('throws when a gatherer is not found', () => {
+    return assert.throws(_ => GatherRunner.getGathererClass(
+        '/fake-path/non-existent-gatherer'), /locate gatherer/);
+  });
+
+  it('loads a gatherer relative to a config path', () => {
+    const configPath = __dirname;
+
+    return assert.doesNotThrow(_ =>
+        GatherRunner.getGathererClass('../fixtures/valid-custom-gatherer', configPath));
+  });
+
+  it('loads a gatherer from node_modules/', () => {
+    return assert.throws(_ => GatherRunner.getGathererClass(
+        // Use a lighthouse dep as a stand in for a module.
+        'chrome-remote-interface'
+    ), function(err) {
+      // Should throw a gatherer validation error, but *not* a gatherer not found error.
+      return !/locate gatherer/.test(err) && /beforePass\(\) method/.test(err);
+    });
+  });
+
+  it('loads a gatherer relative to the working directory', () => {
+    // Construct a gatherer URL relative to current working directory,
+    // regardless of where test was started from.
+    const absoluteGathererPath = path.resolve(__dirname, '../fixtures/valid-custom-gatherer');
+    assert.doesNotThrow(_ => require.resolve(absoluteGathererPath));
+    const relativeGathererPath = path.relative(process.cwd(), absoluteGathererPath);
+
+    return assert.doesNotThrow(_ =>
+        GatherRunner.getGathererClass(relativeGathererPath));
+  });
+
+  it('throws but not for missing gatherer when it has a dependency error', () => {
+    const gathererPath = path.resolve(__dirname, '../fixtures/invalid-gatherers/require-error.js');
+    return assert.throws(_ => GatherRunner.getGathererClass(gathererPath),
+        function(err) {
+          // We're expecting not to find parent class Gatherer, so only reject on
+          // our own custom locate gatherer error, not the usual MODULE_NOT_FOUND.
+          return !/locate gatherer/.test(err) && err.code === 'MODULE_NOT_FOUND';
+        });
+  });
+
   it('throws for invalid gatherers', () => {
     const root = path.resolve(__dirname, '../fixtures/invalid-gatherers');