From 72e3264af9cfc09b6c45eef06ba721259650afd4 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Thu, 22 Sep 2016 11:15:30 -0700 Subject: [PATCH] clean up and unification of custom audit/gatherer loading --- lighthouse-core/config/config.js | 56 ++----------------- lighthouse-core/gather/gather-runner.js | 33 ++--------- lighthouse-core/runner.js | 46 +++++++++++++++ .../invalid-gatherers/require-error.js | 24 ++++++++ .../test/gather/gather-runner-test.js | 43 ++++++++++++++ 5 files changed, 122 insertions(+), 80 deletions(-) create mode 100644 lighthouse-core/test/fixtures/invalid-gatherers/require-error.js diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js index d4d3b4ed4934..d09f7f8a4898 100644 --- a/lighthouse-core/config/config.js +++ b/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); diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 50ad1a70cf4b..a923cd5d73ac 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/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); diff --git a/lighthouse-core/runner.js b/lighthouse-core/runner.js index a79b36ffa876..ba4e2c3683f8 100644 --- a/lighthouse-core/runner.js +++ b/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; diff --git a/lighthouse-core/test/fixtures/invalid-gatherers/require-error.js b/lighthouse-core/test/fixtures/invalid-gatherers/require-error.js new file mode 100644 index 000000000000..bc63665dcedf --- /dev/null +++ b/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; diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 04e09693a08c..3382e2f8a118 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/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');