clean up of extensible audit loading and error handling

lighthouse-cli/index.js

@@ -138,8 +138,8 @@ const flags = cli;
 let config = null;
 if (cli.configPath) {
   // Resolve the config file path relative to where cli was called.
-  const configPath = path.resolve(process.cwd(), cli.configPath);
-  config = require(configPath);
+  cli.configPath = path.resolve(process.cwd(), cli.configPath);
+  config = require(cli.configPath);
 } else if (cli.perf) {
   config = perfOnlyConfig;
 }

lighthouse-core/config/config.js

@@ -16,6 +16,7 @@
  */
 'use strict';
 
+const defaultConfigPath = './default.json';
 const defaultConfig = require('./default.json');
 const recordsFromLogs = require('../lib/network-recorder').recordsFromLogs;
 
@@ -154,46 +155,71 @@ function getGatherersNeededByAudits(audits) {
   }, new Set());
 }
 
-function requireAudits(audits, rootPath) {
+/**
+ * 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;
   }
   const Runner = require('../runner');
   const coreList = Runner.getAuditList();
 
   return audits.map(audit => {
-    let AuditClass;
+    let requirePath;
 
-    // Firstly see if the audit is in Lighthouse itself.
+    // First, see if the audit is a Lighthouse core audit.
     const coreAudit = coreList.find(a => a === `${audit}.js`);
-    let requirePath = `../audits/${audit}`;
-
-    // If not, see if it can be found another way.
-    if (!coreAudit) {
-      // Firstly try and see if the audit resolves naturally through the usual means.
-      try {
-        require.resolve(audit);
-
-        // If the above works, update the path to the absolute value provided.
-        requirePath = audit;
-      } catch (requireError) {
-        // If that fails, try and find it relative to any config path provided.
-        if (rootPath) {
-          requirePath = path.resolve(rootPath, audit);
-        }
-      }
+    if (coreAudit) {
+      requirePath = `../audits/${audit}`;
+    } else {
+      // Otherwise, attempt to find it elsewhere.
+      requirePath = resolveAudit(audit, configPath);
     }
 
-    // Now try and require it in. If this fails then the audit file isn't where we expected it.
-    try {
-      AuditClass = require(requirePath);
-    } catch (requireError) {
-      AuditClass = null;
-    }
-
-    if (!AuditClass) {
-      throw new Error(`Unable to locate audit: ${audit} (tried at ${requirePath})`);
-    }
+    const AuditClass = require(requirePath);
 
     // Confirm that the audit appears valid.
     assertValidAudit(audit, AuditClass);
@@ -274,18 +300,22 @@ function expandArtifacts(artifacts) {
   return artifacts;
 }
 
-/**
- * @return {!Config}
- */
 class Config {
   /**
    * @constructor
-   * @param{Object} config
+   * @param {!LighthouseConfig} configJSON
+   * @param {string=} configPath The absolute path to the config file, if there is one.
    */
   constructor(configJSON, configPath) {
     if (!configJSON) {
       configJSON = defaultConfig;
+      configPath = path.resolve(__dirname, defaultConfigPath);
+    }
+
+    if (configPath && !path.isAbsolute(configPath)) {
+      throw new Error('configPath must be an absolute path.');
     }
+
     // We don't want to mutate the original config object
     configJSON = JSON.parse(JSON.stringify(configJSON));
     // Store the directory of the config path, if one was provided.

lighthouse-core/test/config/config-test.js

@@ -159,41 +159,84 @@ describe('Config', () => {
     return assert.equal(typeof config.audits[0], 'function');
   });
 
-  it('throws when it audit is not found', () => {
+  it('throws when an audit is not found', () => {
     return assert.throws(_ => new Config({
       audits: ['/fake-path/non-existent-audit']
-    }));
+    }), /locate audit/);
+  });
+
+  it('throws on a non-absolute config path', () => {
+    const configPath = '../../config/default.json';
+
+    return assert.throws(_ => new Config({
+      audits: []
+    }, configPath), /absolute path/);
   });
 
-  it('loads an audit relative to a config', () => {
+  it('loads an audit relative to a config path', () => {
+    const configPath = __filename;
+
     return assert.doesNotThrow(_ => new Config({
       audits: ['../fixtures/valid-custom-audit']
-    }, __filename));
+    }, configPath));
+  });
+
+  it('loads an audit from node_modules/', () => {
+    return assert.throws(_ => new Config({
+      // Use a lighthouse dep as a stand in for a module.
+      audits: ['chrome-remote-interface']
+    }), function(err) {
+      // Should throw an audit validation error, but *not* an audit not found error.
+      return !/locate audit/.test(err) && /audit\(\) method/.test(err);
+    });
+  });
+
+  it('loads an audit relative to the working directory', () => {
+    // Construct an audit URL relative to current working directory, regardless
+    // of where test was started from.
+    const absoluteAuditPath = path.resolve(__dirname, '../fixtures/valid-custom-audit');
+    assert.doesNotThrow(_ => require.resolve(absoluteAuditPath));
+    const relativePath = path.relative(process.cwd(), absoluteAuditPath);
+
+    return assert.doesNotThrow(_ => new Config({
+      audits: [relativePath]
+    }));
+  });
+
+  it('throws but not for missing audit when audit has a dependency error', () => {
+    return assert.throws(_ => new Config({
+      audits: [path.resolve(__dirname, '../fixtures/invalid-audits/require-error.js')]
+    }), function(err) {
+      // We're expecting not to find parent class Audit, so only reject on our
+      // own custom locate audit error, not the usual MODULE_NOT_FOUND.
+      return !/locate audit/.test(err) && err.code === 'MODULE_NOT_FOUND';
+    });
   });
 
   it('throws when it finds invalid audits', () => {
+    const basePath = path.resolve(__dirname, '../fixtures/invalid-audits');
     assert.throws(_ => new Config({
-      audits: ['../test/fixtures/invalid-audits/missing-audit']
+      audits: [basePath + '/missing-audit']
     }), /audit\(\) method/);
 
     assert.throws(_ => new Config({
-      audits: ['../test/fixtures/invalid-audits/missing-category']
+      audits: [basePath + '/missing-category']
     }), /meta.category property/);
 
     assert.throws(_ => new Config({
-      audits: ['../test/fixtures/invalid-audits/missing-name']
+      audits: [basePath + '/missing-name']
     }), /meta.name property/);
 
     assert.throws(_ => new Config({
-      audits: ['../test/fixtures/invalid-audits/missing-description']
+      audits: [basePath + '/missing-description']
     }), /meta.description property/);
 
     assert.throws(_ => new Config({
-      audits: ['../test/fixtures/invalid-audits/missing-required-artifacts']
+      audits: [basePath + '/missing-required-artifacts']
     }), /meta.requiredArtifacts property/);
 
     return assert.throws(_ => new Config({
-      audits: ['../test/fixtures/invalid-audits/missing-generate-audit-result']
+      audits: [basePath + '/missing-generate-audit-result']
     }), /generateAuditResult\(\) method/);
   });
 

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

@@ -0,0 +1,35 @@
+/**
+ *
+ * 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 LighthouseAudit = require('../terrible/path/come/on/audit');
+
+class RequireErrorAudit extends LighthouseAudit {
+  static get meta() {
+    return {
+      name: 'require-error',
+      category: 'Custom',
+      description: 'Require Error',
+      requiredArtifacts: ['HTML']
+    };
+  }
+
+  static audit() {}
+}
+
+module.exports = RequireErrorAudit;