Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds support for custom audits and gatherers #593

Merged
merged 2 commits into from
Aug 20, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 86 additions & 13 deletions lighthouse-core/config/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const SpeedlineGatherer = require('../gather/gatherers/speedline');

const GatherRunner = require('../gather/gather-runner');
const log = require('../lib/log');
const path = require('path');

// cleanTrace is run to remove duplicate TracingStartedInPage events,
// and to change TracingStartedInBrowser events into TracingStartedInPage.
Expand Down Expand Up @@ -107,7 +108,7 @@ function cleanTrace(trace) {
return trace;
}

function filterPasses(passes, audits) {
function filterPasses(passes, audits, rootPath) {
const requiredGatherers = getGatherersNeededByAudits(audits);

// Make sure we only have the gatherers that are needed by the audits
Expand All @@ -116,12 +117,8 @@ function filterPasses(passes, audits) {
const freshPass = Object.assign({}, pass);

freshPass.gatherers = freshPass.gatherers.filter(gatherer => {
try {
const GathererClass = GatherRunner.getGathererClass(gatherer);
return requiredGatherers.has(GathererClass.name);
} catch (requireError) {
throw new Error(`Unable to locate gatherer: ${gatherer}`);
}
const GathererClass = GatherRunner.getGathererClass(gatherer, rootPath);
return requiredGatherers.has(GathererClass.name);
});

return freshPass;
Expand Down Expand Up @@ -171,16 +168,82 @@ function filterAudits(audits, auditWhitelist) {
return filteredAudits;
}

function expandAudits(audits) {
function expandAudits(audits, rootPath) {
const Runner = require('../runner');

return audits.map(audit => {
// Firstly see if the audit is in Lighthouse itself.
const list = Runner.getAuditList();
const coreAudit = list.find(a => a === `${audit}.js`);

// Assume it's a core audit first.
let requirePath = path.resolve(__dirname, `../audits/${audit}`);
let AuditClass;

// 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);
}
}
}

// Now try and require it in. If this fails then the audit file isn't where we expected it.
try {
return require(`../audits/${audit}`);
AuditClass = require(requirePath);
} catch (requireError) {
throw new Error(`Unable to locate audit: ${audit}`);
AuditClass = null;
}

if (!AuditClass) {
throw new Error(`Unable to locate audit: ${audit} (tried at ${requirePath})`);
}

// Confirm that the audit appears valid.
assertValidAudit(audit, AuditClass);

return AuditClass;
});
}

function assertValidAudit(audit, auditDefinition) {
if (typeof auditDefinition.audit !== 'function') {
throw new Error(`${audit} has no audit() method.`);
}

if (typeof auditDefinition.meta.name !== 'string') {
throw new Error(`${audit} has no meta.name property, or the property is not a string.`);
}

if (typeof auditDefinition.meta.category !== 'string') {
throw new Error(`${audit} has no meta.category property, or the property is not a string.`);
}

if (typeof auditDefinition.meta.description !== 'string') {
throw new Error(`${audit} has no meta.description property, or the property is not a string.`);
}

if (!Array.isArray(auditDefinition.meta.requiredArtifacts)) {
throw new Error(
`${audit} has no meta.requiredArtifacts property, or the property is not an array.`
);
}

if (typeof auditDefinition.generateAuditResult !== 'function') {
throw new Error(
`${audit} has no generateAuditResult() method. Did you inherit from the proper base class?`
);
}
}

function expandArtifacts(artifacts, includeSpeedline) {
const expandedArtifacts = Object.assign({}, artifacts);

Expand Down Expand Up @@ -236,16 +299,21 @@ class Config {
* @constructor
* @param{Object} config
*/
constructor(configJSON, auditWhitelist) {
constructor(configJSON, auditWhitelist, configPath) {
if (!configJSON) {
configJSON = defaultConfig;
}

// 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)
filterAudits(configJSON.audits, auditWhitelist), this._configDir
) : null;
// filterPasses expects audits to have been expanded
this._passes = configJSON.passes ? filterPasses(configJSON.passes, this._audits) : null;
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) {
Expand All @@ -256,6 +324,11 @@ class Config {
this._aggregations = configJSON.aggregations ? Array.from(configJSON.aggregations) : null;
}

/** @type {string} */
get configDir() {
return this._configDir;
}

/** @type {Array<!Pass>} */
get passes() {
return this._passes;
Expand Down
95 changes: 90 additions & 5 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

const log = require('../lib/log.js');
const Audit = require('../audits/audit');
const path = require('path');

/**
* Class that drives browser to load the page and runs gatherer lifecycle hooks.
Expand Down Expand Up @@ -189,7 +190,29 @@ class GatherRunner {
const driver = options.driver;
const tracingData = {traces: {}};

passes = GatherRunner.instantiateGatherers(passes);
if (typeof options.url !== 'string' || options.url.length === 0) {
return Promise.reject(new Error('You must provide a url to the driver'));
}

if (typeof options.flags === 'undefined') {
options.flags = {};
}

if (typeof options.config === 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check should be in Runner

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? The Gatherer needs the config, it should be the one to assert that it exists.

return Promise.reject(new Error('You must provide a config'));
}

// Default mobile emulation and page loading to true.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This set of checks duplicates what's already in runner. : https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/runner.js#L29-L44

I think we should only check where neccessary and not end up with all the duplication. If we can't keep straight if we need a safety check, then we probably need to untangle a bit. (I'm thinking of our refactor for traceEvents a bit ago. There's a few more in the wings.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. This looks like an artifact of my refactoring perhaps, rather than something that should be there.

// The extension will switch these off initially.
if (typeof options.flags.mobile === 'undefined') {
options.flags.mobile = true;
}

if (typeof options.flags.loadPage === 'undefined') {
options.flags.loadPage = true;
}

passes = this.instantiateGatherers(passes, options.config.configDir);

return driver.connect()
.then(_ => GatherRunner.setupDriver(driver, options))
Expand Down Expand Up @@ -222,26 +245,88 @@ class GatherRunner {
const artifacts = Object.assign({}, tracingData);
passes.forEach(pass => {
pass.gatherers.forEach(gatherer => {
if (typeof gatherer.artifact === 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe if ('artifact' in gatherer) to account for gatherers that may someday want to return an undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I want a gatherer to return undefined as it's easily mistakable for a failure to set artifact. If you want to say error then a -1 would do it.

throw new Error(`${gatherer.constructor.name} failed to provide an artifact.`);
}

artifacts[gatherer.name] = gatherer.artifact;
});
});
return artifacts;
});
}

static getGathererClass(gatherer) {
return require(`./gatherers/${gatherer}`);
static getGathererClass(gatherer, rootPath) {
const Runner = require('../runner');
const list = Runner.getGathererList();
const coreGatherer = list.find(a => a === `${gatherer}.js`);

// Assume it's a core gatherer first.
let requirePath = path.resolve(__dirname, `./gatherers/${gatherer}`);
let GathererClass;

// 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);
}
}
}

// 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})`);
}

// Confirm that the gatherer appears valid.
this.assertValidGatherer(gatherer, GathererClass);

return GathererClass;
}

static assertValidGatherer(gatherer, GathererDefinition) {
const gathererInstance = new GathererDefinition();

if (typeof gathererInstance.beforePass !== 'function') {
throw new Error(`${gatherer} has no beforePass() method.`);
}

if (typeof gathererInstance.pass !== 'function') {
throw new Error(`${gatherer} has no pass() method.`);
}

if (typeof gathererInstance.afterPass !== 'function') {
throw new Error(`${gatherer} has no afterPass() method.`);
}

if (typeof gathererInstance.artifact !== 'object') {
throw new Error(`${gatherer} has no artifact property.`);
}
}

static instantiateGatherers(passes) {
static instantiateGatherers(passes, rootPath) {
return passes.map(pass => {
pass.gatherers = pass.gatherers.map(gatherer => {
// If this is already instantiated, don't do anything else.
if (typeof gatherer !== 'string') {
return gatherer;
}

const GathererClass = GatherRunner.getGathererClass(gatherer);
const GathererClass = GatherRunner.getGathererClass(gatherer, rootPath);
return new GathererClass();
});

Expand Down
4 changes: 3 additions & 1 deletion lighthouse-core/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ module.exports = function(url, flags, configJSON) {
}

// Use ConfigParser to generate a valid config file
const config = new Config(configJSON, flags.auditWhitelist);
const config = new Config(configJSON, flags.auditWhitelist, flags.configPath);

const driver = new ChromeProtocol();

Expand All @@ -73,3 +73,5 @@ module.exports = function(url, flags, configJSON) {

module.exports.getAuditList = Runner.getAuditList;
module.exports.traceCategories = require('./gather/drivers/driver').traceCategories;
module.exports.Audit = require('./audits/audit');
module.exports.Gatherer = require('./gather/gatherers/gatherer');
10 changes: 10 additions & 0 deletions lighthouse-core/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,16 @@ class Runner {
.readdirSync(path.join(__dirname, './audits'))
.filter(f => /\.js$/.test(f));
}

/**
* Returns list of gatherer names for external querying.
* @return {!Array<string>}
*/
static getGathererList() {
return fs
.readdirSync(path.join(__dirname, './gather/gatherers'))
.filter(f => /\.js$/.test(f));
}
}

module.exports = Runner;
10 changes: 10 additions & 0 deletions lighthouse-core/test/audits/audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ class B extends Audit {
static get meta() {
return {};
}

static audit() {}
}

describe('Audit', () => {
Expand All @@ -37,6 +39,14 @@ describe('Audit', () => {
assert.doesNotThrow(_ => B.meta);
});

it('throws if an audit does not override audit()', () => {
assert.throws(_ => A.audit());
});

it('does not throw if an audit overrides audit()', () => {
assert.doesNotThrow(_ => B.audit());
});

it('throws if an audit does generate a result with a value', () => {
assert.throws(_ => A.generateAuditResult({}));
});
Expand Down
38 changes: 38 additions & 0 deletions lighthouse-core/test/config/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,44 @@ describe('Config', () => {
return assert.equal(typeof config.audits[0], 'function');
});

it('throws when it audit is not found', () => {
return assert.throws(_ => new Config({
audits: ['/fake-path/non-existent-audit']
}));
});

it('loads an audit relative to a config', () => {
return assert.doesNotThrow(_ => new Config({
audits: ['../fixtures/valid-custom-audit']
}, null, __filename));
});

it('throws when it finds invalid audits', () => {
assert.throws(_ => new Config({
audits: ['../test/fixtures/invalid-audits/missing-audit']
}), /audit\(\) method/);

assert.throws(_ => new Config({
audits: ['../test/fixtures/invalid-audits/missing-category']
}), /meta.category property/);

assert.throws(_ => new Config({
audits: ['../test/fixtures/invalid-audits/missing-name']
}), /meta.name property/);

assert.throws(_ => new Config({
audits: ['../test/fixtures/invalid-audits/missing-description']
}), /meta.description property/);

assert.throws(_ => new Config({
audits: ['../test/fixtures/invalid-audits/missing-required-artifacts']
}), /meta.requiredArtifacts property/);

return assert.throws(_ => new Config({
audits: ['../test/fixtures/invalid-audits/missing-generate-audit-result']
}), /generateAuditResult\(\) method/);
});

it('expands artifacts', () => {
const config = new Config({
artifacts: {
Expand Down
Loading