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
+56 −157
Merged

filterPasses -> validatePasses #608

Show file tree
Hide file tree
Changes from 1 commit
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
27 changes: 9 additions & 18 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,20 @@ function cleanTrace(trace) {
return trace;
}

function filterPasses(passes, audits, rootPath) {
function validatePasses(passes, audits, rootPath) {
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 => {
// Note if we are running gathers that are not needed by the audits listed in the config
Copy link
Member

Choose a reason for hiding this comment

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

s/Note/Log

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;
});
return passes;
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't return passes anymore

}

function getGatherersNeededByAudits(audits) {
Expand Down Expand Up @@ -294,9 +285,9 @@ class Config {
this._audits = configJSON.audits ? expandAudits(
filterAudits(configJSON.audits, auditWhitelist), this._configDir
) : null;
// filterPasses expects audits to have been expanded
// validatePasses expects audits to have been expanded
this._passes = configJSON.passes ?
filterPasses(configJSON.passes, this._audits, this._configDir) :
validatePasses(configJSON.passes, this._audits, this._configDir) :
Copy link
Member

Choose a reason for hiding this comment

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

this was also serving to make a copy of the passes array and the individual passes. We don't want to modify the config passed into us (for use as a node module and for the extension)

null;
this._auditResults = configJSON.auditResults ? Array.from(configJSON.auditResults) : null;
this._artifacts = null;
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