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.

Sign up for GitHub

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

filterPasses -> validatePasses #608

Merged
merged 5 commits into from
Aug 24, 2016
Merged

filterPasses -> validatePasses #608

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8ab91aa
filterPasses -> validatePasses. Don't mutate the config
paulirish Aug 23, 2016
bff2480
addressing feedback
paulirish Aug 23, 2016
b59ffe0
cleanup config constructor.
paulirish Aug 23, 2016
60464e2
remove audit whitelist.
paulirish Aug 23, 2016
2152197
restore expandArtifacts
paulirish Aug 23, 2016
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
40 changes: 17 additions & 23 deletions lighthouse-core/config/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,29 +106,22 @@ function cleanTrace(trace) {
return trace;
}

function filterPasses(passes, audits, rootPath) {
function validatePasses(passes, audits, rootPath) {
if (!Array.isArray(passes)) {
Copy link
Member

Choose a reason for hiding this comment

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

why wouldnt it be an array?

Copy link
Member

Choose a reason for hiding this comment

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

oh, if it's null? it seems confusing to do the check in here instead of in getGatherersNeededByAudits

Copy link
Member

Choose a reason for hiding this comment

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

I forgot this isn't returning anything, just logging now, so that's fine, actually

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) {
Expand Down Expand Up @@ -291,13 +284,14 @@ class Config {
// 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._audits = null;
if (configJSON.audits) {
this._audits = expandAudits(filterAudits(configJSON.audits, auditWhitelist), this._configDir);
}
// validatePasses expects audits to have been expanded
validatePasses(configJSON.passes, this._audits, this._configDir);
this._passes = configJSON.passes ? Array.from(configJSON.passes) : null;

this._auditResults = configJSON.auditResults ? Array.from(configJSON.auditResults) : null;
this._artifacts = null;
if (configJSON.artifacts) {
Expand Down
7 changes: 1 addition & 6 deletions lighthouse-core/config/perf.example-custom.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,7 @@
"network": true,
"trace": true,
"loadPage": true,
"gatherers": [
"url",
"critical-request-chains",
"screenshots",
"speedline"
]
"gatherers": []
}],

"audits": [
Expand Down
16 changes: 0 additions & 16 deletions lighthouse-core/test/config/config-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: [{
Expand Down