From 1fb77ae1cd52b102b06f7fca9c129791ec9145f8 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Tue, 23 Aug 2016 17:30:44 -0700 Subject: [PATCH] filterPasses -> validatePasses (#608) * filterPasses -> validatePasses. Don't mutate the config * cleanup config constructor. * remove audit whitelist. --- lighthouse-cli/index.js | 10 -- lighthouse-core/config/config.js | 93 ++++++------------- .../config/perf.example-custom.json | 7 +- lighthouse-core/test/config/config-test.js | 41 ++------ lighthouse-core/test/runner-test.js | 61 ++++-------- readme.md | 1 - 6 files changed, 56 insertions(+), 157 deletions(-) diff --git a/lighthouse-cli/index.js b/lighthouse-cli/index.js index 354c7e6a5072..819c4abbf163 100755 --- a/lighthouse-cli/index.js +++ b/lighthouse-cli/index.js @@ -50,7 +50,6 @@ const cli = yargs 'load-page', 'save-assets', 'save-artifacts', - 'audit-whitelist', 'list-all-audits', 'list-trace-categories', 'config-path' @@ -60,7 +59,6 @@ const cli = yargs 'load-page': 'Loads the page', 'save-assets': 'Save the trace contents & screenshots to disk', 'save-artifacts': 'Save all gathered artifacts to disk', - 'audit-whitelist': 'Comma separated list of audits to run', 'list-all-audits': 'Prints a list of all available audits and exits', 'list-trace-categories': 'Prints a list of all required trace categories and exits', 'config-path': 'The absolute path to the config JSON.' @@ -92,7 +90,6 @@ Example: --output-path=./lighthouse-results.html` // default values .default('mobile', true) .default('load-page', true) - .default('audit-whitelist', 'all') .default('output', Printer.OUTPUT_MODE.pretty) .default('output-path', 'stdout') .check(argv => { @@ -138,13 +135,6 @@ if (cli.verbose) { flags.logLevel = 'error'; } -// Normalize audit whitelist. -if (!flags.auditWhitelist || flags.auditWhitelist === 'all') { - flags.auditWhitelist = null; -} else { - flags.auditWhitelist = new Set(flags.auditWhitelist.split(',').map(a => a.toLowerCase())); -} - // kick off a lighthouse run lighthouse(url, flags, config) .then(results => Printer.write(results, outputMode, outputPath)) diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js index 5c47a0e18d05..fe521f48d66b 100644 --- a/lighthouse-core/config/config.js +++ b/lighthouse-core/config/config.js @@ -106,29 +106,22 @@ function cleanTrace(trace) { return trace; } -function filterPasses(passes, audits, rootPath) { +function validatePasses(passes, audits, rootPath) { + if (!Array.isArray(passes)) { + return; + } const requiredGatherers = getGatherersNeededByAudits(audits); - // Make sure we only have the gatherers that are needed by the audits - // that have been listed in the config. - const filteredPasses = passes.map(pass => { - const freshPass = Object.assign({}, pass); - - freshPass.gatherers = freshPass.gatherers.filter(gatherer => { + // Log if we are running gathers that are not needed by the audits listed in the config + passes.forEach(pass => { + pass.gatherers.forEach(gatherer => { const GathererClass = GatherRunner.getGathererClass(gatherer, rootPath); const isGatherRequiredByAudits = requiredGatherers.has(GathererClass.name); if (isGatherRequiredByAudits === false) { - log.warn('config', `Skipping ${GathererClass.name} gatherer as no audit requires it.`); + log.warn('config', `${GathererClass.name} is included, however no audit requires it.`); } - return isGatherRequiredByAudits; }); - - return freshPass; - }) - - // Now remove any passes which no longer have gatherers. - .filter(p => p.gatherers.length > 0); - return filteredPasses; + }); } function getGatherersNeededByAudits(audits) { @@ -144,33 +137,10 @@ function getGatherersNeededByAudits(audits) { }, new Set()); } -function filterAudits(audits, auditWhitelist) { - // If there is no whitelist, assume all. - if (!auditWhitelist) { - return Array.from(audits); - } - - const rejected = []; - const filteredAudits = audits.filter(a => { - const auditName = a.toLowerCase(); - const inWhitelist = auditWhitelist.has(auditName); - - if (!inWhitelist) { - rejected.push(auditName); - } - - return inWhitelist; - }); - - if (rejected.length) { - log.log('info', 'Running these audits:', `${filteredAudits.join(', ')}`); - log.log('info', 'Skipping these audits:', `${rejected.join(', ')}`); +function requireAudits(audits, rootPath) { + if (!audits) { + return null; } - - return filteredAudits; -} - -function expandAudits(audits, rootPath) { const Runner = require('../runner'); return audits.map(audit => { @@ -247,8 +217,9 @@ function assertValidAudit(audit, auditDefinition) { } function expandArtifacts(artifacts) { - const expandedArtifacts = Object.assign({}, artifacts); - + if (!artifacts) { + return null; + } // currently only trace logs and performance logs should be imported if (artifacts.traces) { Object.keys(artifacts.traces).forEach(key => { @@ -264,15 +235,14 @@ function expandArtifacts(artifacts) { } trace = cleanTrace(trace); - expandedArtifacts.traces[key] = trace; + artifacts.traces[key] = trace; }); } - if (artifacts.performanceLog) { - expandedArtifacts.networkRecords = recordsFromLogs(require(artifacts.performanceLog)); + artifacts.networkRecords = recordsFromLogs(require(artifacts.performanceLog)); } - return expandedArtifacts; + return artifacts; } /** @@ -283,27 +253,24 @@ class Config { * @constructor * @param{Object} config */ - constructor(configJSON, auditWhitelist, configPath) { + constructor(configJSON, configPath) { if (!configJSON) { configJSON = defaultConfig; } - + // 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. this._configDir = configPath ? path.dirname(configPath) : undefined; - this._audits = configJSON.audits ? expandAudits( - filterAudits(configJSON.audits, auditWhitelist), this._configDir - ) : null; - // filterPasses expects audits to have been expanded - this._passes = configJSON.passes ? - filterPasses(configJSON.passes, this._audits, this._configDir) : - null; - this._auditResults = configJSON.auditResults ? Array.from(configJSON.auditResults) : null; - this._artifacts = null; - if (configJSON.artifacts) { - this._artifacts = expandArtifacts(configJSON.artifacts); - } - this._aggregations = configJSON.aggregations ? Array.from(configJSON.aggregations) : null; + this._passes = configJSON.passes || null; + this._auditResults = configJSON.auditResults || null; + this._aggregations = configJSON.aggregations || null; + + this._audits = requireAudits(configJSON.audits, this._configDir); + this._artifacts = expandArtifacts(configJSON.artifacts); + + // validatePasses must follow after audits are required + validatePasses(configJSON.passes, this._audits, this._configDir); } /** @type {string} */ diff --git a/lighthouse-core/config/perf.example-custom.json b/lighthouse-core/config/perf.example-custom.json index 7c4033260900..7939d45abc29 100644 --- a/lighthouse-core/config/perf.example-custom.json +++ b/lighthouse-core/config/perf.example-custom.json @@ -3,12 +3,7 @@ "network": true, "trace": true, "loadPage": true, - "gatherers": [ - "url", - "critical-request-chains", - "screenshots", - "speedline" - ] + "gatherers": [] }], "audits": [ diff --git a/lighthouse-core/test/config/config-test.js b/lighthouse-core/test/config/config-test.js index 44dfc6a4319c..0dab4cace0e4 100644 --- a/lighthouse-core/test/config/config-test.js +++ b/lighthouse-core/test/config/config-test.js @@ -51,22 +51,6 @@ describe('Config', () => { /Unable to locate/); }); - it('filters gatherers from passes when no audits require them', () => { - const config = new Config({ - passes: [{ - gatherers: [ - 'url', - 'html', - 'viewport' - ] - }], - - audits: ['viewport'] - }); - - assert.equal(config.passes[0].gatherers.length, 1); - }); - it('doesn\'t mutate old gatherers when filtering passes', () => { const configJSON = { passes: [{ @@ -99,22 +83,13 @@ describe('Config', () => { }]; const config = new Config(configJSON); - assert.notEqual(config, configJSON); - assert.ok(config.aggregations); - assert.ok(config.auditResults); - assert.deepStrictEqual(config.aggregations, configJSON.aggregations); - assert.notEqual(config.aggregations, configJSON.aggregations); - assert.notEqual(config.auditResults, configJSON.auditResults); - assert.deepStrictEqual(config.auditResults, configJSON.auditResults); - }); - - it('returns filtered audits when a whitelist is given', () => { - const config = new Config({ - audits: ['is-on-https'] - }, new Set(['b'])); - - assert.ok(Array.isArray(config.audits)); - return assert.equal(config.audits.length, 0); + assert.notEqual(config, configJSON, 'Objects are strictly different'); + assert.ok(config.aggregations, 'Aggregations array exists'); + assert.ok(config.auditResults, 'Audits array exists'); + assert.deepStrictEqual(config.aggregations, configJSON.aggregations, 'Aggregations match'); + assert.notEqual(config.aggregations, configJSON.aggregations, 'Aggregations not same object'); + assert.notEqual(config.auditResults, configJSON.auditResults, 'Audits not same object'); + assert.deepStrictEqual(config.auditResults, configJSON.auditResults, 'Audits match'); }); it('expands audits', () => { @@ -136,7 +111,7 @@ describe('Config', () => { it('loads an audit relative to a config', () => { return assert.doesNotThrow(_ => new Config({ audits: ['../fixtures/valid-custom-audit'] - }, null, __filename)); + }, __filename)); }); it('throws when it finds invalid audits', () => { diff --git a/lighthouse-core/test/runner-test.js b/lighthouse-core/test/runner-test.js index e0c412e8e932..2b561aff16a5 100644 --- a/lighthouse-core/test/runner-test.js +++ b/lighthouse-core/test/runner-test.js @@ -25,9 +25,6 @@ const path = require('path'); describe('Runner', () => { it('expands gatherers', () => { const url = 'https://example.com'; - const flags = { - auditWhitelist: null - }; const config = new Config({ passes: [{ gatherers: ['https'] @@ -37,31 +34,25 @@ describe('Runner', () => { ] }); - return Runner.run(fakeDriver, {url, config, flags}).then(_ => { + return Runner.run(fakeDriver, {url, config}).then(_ => { assert.ok(typeof config.passes[0].gatherers[0] === 'object'); }); }); it('throws when given neither passes nor artifacts', () => { const url = 'https://example.com'; - const flags = { - auditWhitelist: null - }; const config = new Config({ audits: [ 'is-on-https' ] - }, flags.auditWhitelist); + }); - return assert.throws(_ => Runner.run(fakeDriver, {url, config, flags}), + return assert.throws(_ => Runner.run(fakeDriver, {url, config}), /The config must provide passes/); }); it('accepts existing artifacts', () => { const url = 'https://example.com'; - const flags = { - auditWhitelist: null - }; const config = new Config({ audits: [ 'is-on-https' @@ -70,16 +61,13 @@ describe('Runner', () => { artifacts: { HTTPS: true } - }, flags.auditWhitelist); + }); - return assert.doesNotThrow(_ => Runner.run({}, {url, config, flags})); + return assert.doesNotThrow(_ => Runner.run({}, {url, config})); }); it('accepts trace artifacts as paths and outputs appropriate data', () => { const url = 'https://example.com'; - const flags = { - auditWhitelist: null - }; const config = new Config({ audits: [ @@ -91,9 +79,9 @@ describe('Runner', () => { [Audit.DEFAULT_TRACE]: path.join(__dirname, '/fixtures/traces/trace-user-timings.json') } } - }, flags.auditWhitelist); + }); - return Runner.run({}, {url, config, flags}).then(results => { + return Runner.run({}, {url, config}).then(results => { assert.equal(results[0].rawValue, 2); assert.equal(results[0].name, 'user-timings'); }); @@ -101,9 +89,6 @@ describe('Runner', () => { it('fails gracefully with empty artifacts object', () => { const url = 'https://example.com'; - const flags = { - auditWhitelist: null - }; const config = new Config({ audits: [ @@ -112,9 +97,9 @@ describe('Runner', () => { artifacts: { } - }, flags.auditWhitelist); + }); - return Runner.run({}, {url, config, flags}).then(results => { + return Runner.run({}, {url, config}).then(results => { assert.equal(results[0].rawValue, -1); assert(results[0].debugString); }); @@ -122,9 +107,6 @@ describe('Runner', () => { it('accepts performance logs as an artifact', () => { const url = 'https://example.com'; - const flags = { - auditWhitelist: null - }; const config = new Config({ audits: [ 'critical-request-chains' @@ -133,9 +115,9 @@ describe('Runner', () => { artifacts: { performanceLog: path.join(__dirname, '/fixtures/perflog.json') } - }, flags.auditWhitelist); + }); - return Runner.run({}, {url, config, flags}).then(results => { + return Runner.run({}, {url, config}).then(results => { assert.equal(results[0].rawValue, 9); assert.equal(results[0].name, 'critical-request-chains'); }); @@ -143,24 +125,18 @@ describe('Runner', () => { it('throws when given neither audits nor auditResults', () => { const url = 'https://example.com'; - const flags = { - auditWhitelist: null - }; const config = new Config({ passes: [{ gatherers: ['https'] }] - }, flags.auditWhitelist); + }); - return assert.throws(_ => Runner.run(fakeDriver, {url, config, flags}), + return assert.throws(_ => Runner.run(fakeDriver, {url, config}), /The config must provide passes/); }); it('accepts existing auditResults', () => { const url = 'https://example.com'; - const flags = { - auditWhitelist: null - }; const config = new Config({ auditResults: { HTTPS: true @@ -182,16 +158,13 @@ describe('Runner', () => { } }] }] - }, flags.auditWhitelist); + }); - return assert.doesNotThrow(_ => Runner.run(fakeDriver, {url, config, flags})); + return assert.doesNotThrow(_ => Runner.run(fakeDriver, {url, config})); }); it('returns an aggregation', () => { const url = 'https://example.com'; - const flags = { - auditWhitelist: null - }; const config = new Config({ auditResults: [{ name: 'is-on-https', @@ -216,9 +189,9 @@ describe('Runner', () => { } }] }] - }, flags.auditWhitelist); + }); - return Runner.run(fakeDriver, {url, config, flags}).then(results => { + return Runner.run(fakeDriver, {url, config}).then(results => { assert.equal(results.initialUrl, url); assert.equal(results.audits['is-on-https'].name, 'is-on-https'); assert.equal(results.aggregations[0].score[0].overall, 1); diff --git a/readme.md b/readme.md index 4387c6b3ed1b..69dae9152e0d 100644 --- a/readme.md +++ b/readme.md @@ -108,7 +108,6 @@ Configuration: --load-page Loads the page [default: true] --save-assets Save the trace contents & screenshots to disk [boolean] --save-artifacts Save all gathered artifacts to disk [boolean] - --audit-whitelist Comma separated list of audits to run [default: "all"] --list-all-audits Prints a list of all available audits and exits [boolean] --list-trace-categories Prints a list of all required trace categories and exits [boolean] --config-path The absolute path to the config JSON.