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

core(fr): filter configs by gather mode #11941

Merged
merged 3 commits into from
Jan 13, 2021
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
25 changes: 5 additions & 20 deletions lighthouse-core/fraggle-rock/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,11 @@

const Driver = require('./gather/driver.js');
const Runner = require('../runner.js');
const Config = require('../config/config.js');

/**
* @param {LH.Gatherer.GathererInstance|LH.Gatherer.FRGathererInstance} gatherer
* @return {gatherer is LH.Gatherer.FRGathererInstance}
*/
function isFRGatherer(gatherer) {
return 'meta' in gatherer;
}
const {initializeConfig} = require('./config/config.js');

/** @param {{page: import('puppeteer').Page, config?: LH.Config.Json}} options */
async function snapshot(options) {
const config = new Config(options.config);
const {config} = initializeConfig(options.config, {gatherMode: 'snapshot'});
const driver = new Driver(options.page);
await driver.connect();

Expand Down Expand Up @@ -47,20 +39,13 @@ async function snapshot(options) {
PageLoadError: null,
};

const gatherers = (config.passes || [])
.flatMap(pass => pass.gatherers);

/** @type {Partial<LH.GathererArtifacts>} */
const artifacts = {};

for (const {instance} of gatherers) {
if (!isFRGatherer(instance)) continue;
if (!instance.meta.supportedModes.includes('snapshot')) continue;

/** @type {keyof LH.GathererArtifacts} */
const artifactName = instance.name;
for (const {id, gatherer} of config.artifacts || []) {
const artifactName = /** @type {keyof LH.GathererArtifacts} */ (id);
const artifact = await Promise.resolve()
.then(() => instance.snapshot({gatherMode: 'snapshot', driver}))
.then(() => gatherer.instance.snapshot({gatherMode: 'snapshot', driver}))
.catch(err => err);

artifacts[artifactName] = artifact;
Expand Down
17 changes: 13 additions & 4 deletions lighthouse-core/fraggle-rock/config/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ const path = require('path');
const log = require('lighthouse-logger');
const Runner = require('../../runner.js');
const defaultConfig = require('./default-config.js');
const {isFRGathererDefn} = require('./validation.js');
const {filterConfigByGatherMode} = require('./filters.js');
const {
deepCloneConfigJson,
resolveSettings,
Expand Down Expand Up @@ -58,9 +60,14 @@ function resolveArtifactsToDefns(artifacts, configDir) {

const coreGathererList = Runner.getGathererList();
const artifactDefns = artifacts.map(artifactJson => {
const gatherer = resolveGathererToDefn(artifactJson.gatherer, coreGathererList, configDir);
if (!isFRGathererDefn(gatherer)) {
throw new Error(`${gatherer.instance.name} gatherer does not support Fraggle Rock`);
}

return {
id: artifactJson.id,
gatherer: resolveGathererToDefn(artifactJson.gatherer, coreGathererList, configDir),
gatherer,
};
});

Expand All @@ -70,7 +77,7 @@ function resolveArtifactsToDefns(artifacts, configDir) {

/**
* @param {LH.Config.Json|undefined} configJSON
* @param {{configPath?: string, settingsOverrides?: LH.SharedFlagsSettings}} context
* @param {{gatherMode: LH.Gatherer.GatherMode, configPath?: string, settingsOverrides?: LH.SharedFlagsSettings}} context
* @return {{config: LH.Config.FRConfig, warnings: string[]}}
*/
function initializeConfig(configJSON, context) {
Expand All @@ -87,18 +94,20 @@ function initializeConfig(configJSON, context) {
const artifacts = resolveArtifactsToDefns(configWorkingCopy.artifacts, configDir);

/** @type {LH.Config.FRConfig} */
const config = {
let config = {
artifacts,
audits: resolveAuditsToDefns(configWorkingCopy.audits, configDir),
categories: configWorkingCopy.categories || null,
groups: configWorkingCopy.groups || null,
settings,
};

// TODO(FR-COMPAT): filter config
// TODO(FR-COMPAT): validate navigations
// TODO(FR-COMPAT): validate audits
// TODO(FR-COMPAT): validate categories
// TODO(FR-COMPAT): filter config using onlyAudits/onlyCategories

config = filterConfigByGatherMode(config, context.gatherMode);

log.timeEnd(status);
return {config, warnings: []};
Expand Down
88 changes: 88 additions & 0 deletions lighthouse-core/fraggle-rock/config/filters.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/**
* @license Copyright 2021 The Lighthouse Authors. 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';

/**
* Filters an array of artifacts down to the set that supports the specified gather mode.
*
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a jsdoc description to these functions. It isn't obvious how each list is being filtered by the name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure thing 👍

* @param {LH.Config.FRConfig['artifacts']} artifacts
* @param {LH.Gatherer.GatherMode} mode
* @return {LH.Config.FRConfig['artifacts']}
*/
function filterArtifactsByGatherMode(artifacts, mode) {
if (!artifacts) return null;
return artifacts.filter(artifact => {
return artifact.gatherer.instance.meta.supportedModes.includes(mode);
});
}

/**
* Filters an array of audits down to the set that can be computed using only the specified artifacts.
*
* @param {LH.Config.FRConfig['audits']} audits
* @param {Array<LH.Config.ArtifactDefn>} availableArtifacts
* @return {LH.Config.FRConfig['audits']}
*/
function filterAuditsByAvailableArtifacts(audits, availableArtifacts) {
if (!audits) return null;

const availableAritfactIds = new Set(availableArtifacts.map(artifact => artifact.id));

return audits.filter(audit =>
audit.implementation.meta.requiredArtifacts.every(id => availableAritfactIds.has(id))
);
}

/**
* Filters a categories object and their auditRefs down to the set that can be computed using
* only the specified audits.
*
* @param {LH.Config.Config['categories']} categories
* @param {Array<LH.Config.AuditDefn>} availableAudits
* @return {LH.Config.Config['categories']}
*/
function filterCategoriesByAvailableAudits(categories, availableAudits) {
if (!categories) return categories;

const availableAuditIds = new Set(availableAudits.map(audit => audit.implementation.meta.id));

return Object.fromEntries(
Object.entries(categories).map(([categoryId, category]) => {
const filteredCategory = {
...category,
auditRefs: category.auditRefs.filter(ref => availableAuditIds.has(ref.id)),
};
return [categoryId, filteredCategory];
})
);
}

/**
* Filters a config's artifacts, audits, and categories down to the set that supports the specified gather mode.
*
* @param {LH.Config.FRConfig} config
* @param {LH.Gatherer.GatherMode} mode
* @return {LH.Config.FRConfig}
*/
function filterConfigByGatherMode(config, mode) {
const artifacts = filterArtifactsByGatherMode(config.artifacts, mode);
const audits = filterAuditsByAvailableArtifacts(config.audits, artifacts || []);
const categories = filterCategoriesByAvailableAudits(config.categories, audits || []);

return {
...config,
artifacts,
audits,
categories,
};
}

module.exports = {
filterConfigByGatherMode,
filterArtifactsByGatherMode,
filterAuditsByAvailableArtifacts,
filterCategoriesByAvailableAudits,
};
16 changes: 16 additions & 0 deletions lighthouse-core/fraggle-rock/config/validation.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* @license Copyright 2021 The Lighthouse Authors. 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';

/**
* @param {LH.Config.GathererDefn | LH.Config.FRGathererDefn} gathererDefn
* @return {gathererDefn is LH.Config.FRGathererDefn}
*/
function isFRGathererDefn(gathererDefn) {
return 'meta' in gathererDefn.instance;
}

module.exports = {isFRGathererDefn};
5 changes: 3 additions & 2 deletions lighthouse-core/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ const LHError = require('./lib/lh-error.js');

class Runner {
/**
* @param {(runnerData: {requestedUrl: string, config: Config, driverMock?: Driver}) => Promise<LH.Artifacts>} gatherFn
* @param {{config: Config, url?: string, driverMock?: Driver}} runOpts
* @template {LH.Config.Config | LH.Config.FRConfig} TConfig
* @param {(runnerData: {requestedUrl: string, config: TConfig, driverMock?: Driver}) => Promise<LH.Artifacts>} gatherFn
* @param {{config: TConfig, url?: string, driverMock?: Driver}} runOpts
* @return {Promise<LH.RunnerResult|undefined>}
*/
static async run(gatherFn, runOpts) {
Expand Down
13 changes: 8 additions & 5 deletions lighthouse-core/test/fraggle-rock/api-test-pptr.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,16 @@ describe('Fraggle Rock API', () => {
const accessibility = lhr.categories.accessibility;
expect(accessibility.score).toBeLessThan(1);

const auditResults = accessibility.auditRefs.map(ref => lhr.audits[ref.id]);
const auditResults = Object.values(lhr.audits);
// TODO(FR-COMPAT): This assertion can be removed when full compatibility is reached.
expect(auditResults.length).toMatchInlineSnapshot(`58`);

const irrelevantDisplayModes = new Set(['notApplicable', 'manual']);
const applicableAudits = auditResults
.filter(audit => !irrelevantDisplayModes.has(audit.scoreDisplayMode));
const applicableAudits = auditResults.filter(
audit => !irrelevantDisplayModes.has(audit.scoreDisplayMode)
);

const erroredAudits = applicableAudits
.filter(audit => audit.score === null);
const erroredAudits = applicableAudits.filter(audit => audit.score === null);
expect(erroredAudits).toHaveLength(0);

const failedAuditIds = applicableAudits
Expand Down
49 changes: 39 additions & 10 deletions lighthouse-core/test/fraggle-rock/config/config-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,31 @@
*/
'use strict';

const BaseGatherer = require('../../../fraggle-rock/gather/base-gatherer.js');
const {initializeConfig} = require('../../../fraggle-rock/config/config.js');

/* eslint-env jest */

describe('Fraggle Rock Config', () => {
const gatherMode = 'snapshot';

it('should throw if the config path is not absolute', () => {
const configFn = () => initializeConfig(undefined, {configPath: '../relative/path'});
const configFn = () =>
initializeConfig(undefined, {gatherMode, configPath: '../relative/path'});
expect(configFn).toThrow(/must be an absolute path/);
});

it('should not mutate the original input', () => {
const configJson = {artifacts: [{id: 'ImageElements', gatherer: 'image-elements'}]};
const {config} = initializeConfig(configJson, {});
expect(configJson).toEqual({artifacts: [{id: 'ImageElements', gatherer: 'image-elements'}]});
const configJson = {artifacts: [{id: 'Accessibility', gatherer: 'accessibility'}]};
const {config} = initializeConfig(configJson, {gatherMode});
expect(configJson).toEqual({artifacts: [{id: 'Accessibility', gatherer: 'accessibility'}]});
expect(config).not.toBe(configJson);
expect(config).not.toEqual(configJson);
expect(config.artifacts).toMatchObject([{gatherer: {path: 'image-elements'}}]);
expect(config.artifacts).toMatchObject([{gatherer: {path: 'accessibility'}}]);
});

it('should use default config when none passed in', () => {
const {config} = initializeConfig(undefined, {});
const {config} = initializeConfig(undefined, {gatherMode});
expect(config.settings).toMatchObject({formFactor: 'mobile'});
if (!config.audits) throw new Error('Did not define audits');
expect(config.audits.length).toBeGreaterThan(0);
Expand All @@ -34,7 +38,7 @@ describe('Fraggle Rock Config', () => {
it('should resolve settings with defaults', () => {
const {config} = initializeConfig(
{settings: {output: 'csv', maxWaitForFcp: 1234}},
{settingsOverrides: {maxWaitForFcp: 12345}}
{settingsOverrides: {maxWaitForFcp: 12345}, gatherMode}
);

expect(config.settings).toMatchObject({
Expand All @@ -45,11 +49,35 @@ describe('Fraggle Rock Config', () => {
});

it('should resolve artifact definitions', () => {
const configJson = {artifacts: [{id: 'Accessibility', gatherer: 'accessibility'}]};
const {config} = initializeConfig(configJson, {gatherMode});

expect(config).toMatchObject({
artifacts: [{id: 'Accessibility', gatherer: {path: 'accessibility'}}],
});
});

it('should throw on invalid artifact definitions', () => {
const configJson = {artifacts: [{id: 'ImageElements', gatherer: 'image-elements'}]};
const {config} = initializeConfig(configJson, {});
expect(() => initializeConfig(configJson, {gatherMode})).toThrow(/ImageElements gatherer/);
});

it('should filter configuration by gatherMode', () => {
const timespanGatherer = new BaseGatherer();
timespanGatherer.meta = {supportedModes: ['timespan']};

const configJson = {
artifacts: [
{id: 'Accessibility', gatherer: 'accessibility'},
{id: 'Timespan', gatherer: {instance: timespanGatherer}},
],
};

const {config} = initializeConfig(configJson, {gatherMode: 'snapshot'});
expect(config).toMatchObject({
artifacts: [{id: 'ImageElements', gatherer: {path: 'image-elements'}}],
artifacts: [
{id: 'Accessibility', gatherer: {path: 'accessibility'}},
],
});
});

Expand All @@ -59,6 +87,7 @@ describe('Fraggle Rock Config', () => {
it.todo('should adjust default pass options for throttling method');
it.todo('should normalize gatherer inputs');
it.todo('should require gatherers from their paths');
it.todo('should filter configuration');
it.todo('should filter configuration by inclusive settings');
it.todo('should filter configuration by exclusive settings');
it.todo('should validate audit/gatherer interdependencies');
});
Loading