From b6ed4320c4119a7af9039771df1f3b80d203cc22 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Tue, 20 Sep 2016 17:54:26 -0700 Subject: [PATCH 1/2] clean up of extensible audit loading and error handling --- lighthouse-cli/index.js | 4 +- lighthouse-core/config/config.js | 64 ++++++++++--------- lighthouse-core/test/config/config-test.js | 53 ++++++++++++--- .../fixtures/invalid-audits/require-error.js | 35 ++++++++++ 4 files changed, 115 insertions(+), 41 deletions(-) create mode 100644 lighthouse-core/test/fixtures/invalid-audits/require-error.js diff --git a/lighthouse-cli/index.js b/lighthouse-cli/index.js index 67899c19309e..37f4248e7d63 100755 --- a/lighthouse-cli/index.js +++ b/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; } diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js index 14a77aaf3aac..13bca209bfa3 100644 --- a/lighthouse-core/config/config.js +++ b/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,7 +155,7 @@ function getGatherersNeededByAudits(audits) { }, new Set()); } -function requireAudits(audits, rootPath) { +function requireAudits(audits, configPath) { if (!audits) { return null; } @@ -162,38 +163,39 @@ function requireAudits(audits, rootPath) { 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 in Lighthouse itself. 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. + if (coreAudit) { + requirePath = `../audits/${audit}`; + } else { + // If not, see if it can be found another way: + // See if the audit resolves relative to the current working directory. + const cwdPath = path.resolve(process.cwd(), audit); try { - require.resolve(audit); - - // If the above works, update the path to the absolute value provided. - requirePath = audit; + // If resolving this path works, update the path to be used. + require.resolve(cwdPath); + requirePath = cwdPath; } catch (requireError) { - // If that fails, try and find it relative to any config path provided. - if (rootPath) { - requirePath = path.resolve(rootPath, audit); + // Also try relative to the config path, if provided. + if (!configPath) { + throw new Error(`Unable to locate audit: ${audit} (tried at '${cwdPath}')`); + } + + const relativePath = path.resolve(configPath, audit); + try { + // If resolving this path works, update the path to be used. + require.resolve(relativePath); + requirePath = relativePath; + } catch (requireError) { + throw new Error(`Unable to locate audit: ${audit} ` + + `(tried at '${cwdPath}' and '${relativePath}')`); } } } - // 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 +276,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. diff --git a/lighthouse-core/test/config/config-test.js b/lighthouse-core/test/config/config-test.js index 0aa57af8648d..8609d84fe4ec 100644 --- a/lighthouse-core/test/config/config-test.js +++ b/lighthouse-core/test/config/config-test.js @@ -159,41 +159,74 @@ 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 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/); }); diff --git a/lighthouse-core/test/fixtures/invalid-audits/require-error.js b/lighthouse-core/test/fixtures/invalid-audits/require-error.js new file mode 100644 index 000000000000..b73ff625e349 --- /dev/null +++ b/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 ValidCustomAudit extends LighthouseAudit { + static get meta() { + return { + name: 'valid-audit', + category: 'Custom', + description: 'Valid Audit', + requiredArtifacts: ['HTML'] + }; + } + + static audit() {} +} + +module.exports = ValidCustomAudit; From aa1355b12adb48c7ccd3422eebfa0bcbb84c780b Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Wed, 21 Sep 2016 16:18:40 -0700 Subject: [PATCH 2/2] add custom audit loading from node_modules/, documentation --- lighthouse-core/config/config.js | 72 ++++++++++++------- lighthouse-core/test/config/config-test.js | 10 +++ .../fixtures/invalid-audits/require-error.js | 8 +-- 3 files changed, 62 insertions(+), 28 deletions(-) diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js index 13bca209bfa3..d4d3b4ed4934 100644 --- a/lighthouse-core/config/config.js +++ b/lighthouse-core/config/config.js @@ -155,6 +155,51 @@ 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; @@ -165,34 +210,13 @@ function requireAudits(audits, configPath) { return audits.map(audit => { let requirePath; - // First, 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`); if (coreAudit) { requirePath = `../audits/${audit}`; } else { - // If not, see if it can be found another way: - // See if the audit resolves relative to the current working directory. - const cwdPath = path.resolve(process.cwd(), audit); - try { - // If resolving this path works, update the path to be used. - require.resolve(cwdPath); - requirePath = cwdPath; - } catch (requireError) { - // Also try relative to the config path, if provided. - if (!configPath) { - throw new Error(`Unable to locate audit: ${audit} (tried at '${cwdPath}')`); - } - - const relativePath = path.resolve(configPath, audit); - try { - // If resolving this path works, update the path to be used. - require.resolve(relativePath); - requirePath = relativePath; - } catch (requireError) { - throw new Error(`Unable to locate audit: ${audit} ` + - `(tried at '${cwdPath}' and '${relativePath}')`); - } - } + // Otherwise, attempt to find it elsewhere. + requirePath = resolveAudit(audit, configPath); } const AuditClass = require(requirePath); diff --git a/lighthouse-core/test/config/config-test.js b/lighthouse-core/test/config/config-test.js index 8609d84fe4ec..40d8de2f1fcd 100644 --- a/lighthouse-core/test/config/config-test.js +++ b/lighthouse-core/test/config/config-test.js @@ -181,6 +181,16 @@ describe('Config', () => { }, 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. diff --git a/lighthouse-core/test/fixtures/invalid-audits/require-error.js b/lighthouse-core/test/fixtures/invalid-audits/require-error.js index b73ff625e349..2f7543542134 100644 --- a/lighthouse-core/test/fixtures/invalid-audits/require-error.js +++ b/lighthouse-core/test/fixtures/invalid-audits/require-error.js @@ -19,12 +19,12 @@ // NOTE: this require path does not resolve correctly. const LighthouseAudit = require('../terrible/path/come/on/audit'); -class ValidCustomAudit extends LighthouseAudit { +class RequireErrorAudit extends LighthouseAudit { static get meta() { return { - name: 'valid-audit', + name: 'require-error', category: 'Custom', - description: 'Valid Audit', + description: 'Require Error', requiredArtifacts: ['HTML'] }; } @@ -32,4 +32,4 @@ class ValidCustomAudit extends LighthouseAudit { static audit() {} } -module.exports = ValidCustomAudit; +module.exports = RequireErrorAudit;