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): convert additional base artifacts #12594

Merged
merged 9 commits into from
Jun 3, 2021
Merged
Show file tree
Hide file tree
Changes from 8 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
6 changes: 6 additions & 0 deletions lighthouse-core/fraggle-rock/config/default-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ const artifacts = {
FormElements: '',
FullPageScreenshot: '',
GlobalListeners: '',
HostFormFactor: '',
HostUserAgent: '',
IFrameElements: '',
ImageElements: '',
InstallabilityErrors: '',
Expand Down Expand Up @@ -57,6 +59,8 @@ for (const key of Object.keys(artifacts)) {
const defaultConfig = {
artifacts: [
// Artifacts which can be depended on come first.
{id: artifacts.HostUserAgent, gatherer: 'host-user-agent'},
{id: artifacts.HostFormFactor, gatherer: 'host-form-factor'},
{id: artifacts.DevtoolsLog, gatherer: 'devtools-log'},
{id: artifacts.Trace, gatherer: 'trace'},

Expand Down Expand Up @@ -109,6 +113,8 @@ const defaultConfig = {
cpuQuietThresholdMs: 1000,
artifacts: [
// Artifacts which can be depended on come first.
artifacts.HostUserAgent,
artifacts.HostFormFactor,
artifacts.DevtoolsLog,
artifacts.Trace,

Expand Down
2 changes: 0 additions & 2 deletions lighthouse-core/fraggle-rock/config/filters.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ const Audit = require('../../audits/audit.js');
const baseArtifactKeySource = {
fetchTime: '',
LighthouseRunWarnings: '',
HostFormFactor: '',
HostUserAgent: '',
BenchmarkIndex: '',
settings: '',
Timing: '',
Expand Down
11 changes: 2 additions & 9 deletions lighthouse-core/fraggle-rock/gather/base-artifacts.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
const log = require('lighthouse-logger');
const isEqual = require('lodash.isequal');
const {
getBrowserVersion,
getBenchmarkIndex,
getEnvironmentWarnings,
} = require('../../gather/driver/environment.js');
Expand All @@ -19,12 +18,6 @@ const {
* @return {Promise<LH.BaseArtifacts>}
*/
async function getBaseArtifacts(config, driver) {
const HostUserAgent = (await getBrowserVersion(driver.defaultSession)).userAgent;

// Whether Lighthouse was run on a mobile device (i.e. not on a desktop machine).
const HostFormFactor =
HostUserAgent.includes('Android') || HostUserAgent.includes('Mobile') ? 'mobile' : 'desktop';

const BenchmarkIndex = await getBenchmarkIndex(driver.executionContext);

return {
Expand All @@ -34,13 +27,13 @@ async function getBaseArtifacts(config, driver) {
LighthouseRunWarnings: [],
settings: config.settings,
// Environment artifacts that can always be computed.
HostFormFactor,
HostUserAgent,
BenchmarkIndex,
// Contextual artifacts whose collection changes based on gather mode.
URL: {requestedUrl: '', finalUrl: ''},
PageLoadError: null,
// Artifacts that have been replaced by regular gatherers in Fraggle Rock.
HostFormFactor: 'mobile',
HostUserAgent: '',
Stacks: [],
NetworkUserAgent: '',
WebAppManifest: null,
Expand Down
31 changes: 31 additions & 0 deletions lighthouse-core/gather/gatherers/host-form-factor.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* @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';

const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');
const HostUserAgent = require('./host-user-agent.js');

class HostFormFactor extends FRGatherer {
static symbol = Symbol('HostFormFactor');

/** @type {LH.Gatherer.GathererMeta<'HostUserAgent'>} */
meta = {
symbol: HostFormFactor.symbol,
supportedModes: ['snapshot', 'timespan', 'navigation'],
dependencies: {HostUserAgent: HostUserAgent.symbol},
};

/**
* @param {LH.Gatherer.FRTransitionalContext<'HostUserAgent'>} context
* @return {Promise<LH.Artifacts['HostFormFactor']>}
*/
async getArtifact(context) {
const userAgent = context.dependencies.HostUserAgent;
Copy link
Collaborator

Choose a reason for hiding this comment

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

time to start a breaking changes v9 issue to merge these :)

return userAgent.includes('Android') || userAgent.includes('Mobile') ? 'mobile' : 'desktop';
}
}

module.exports = HostFormFactor;
29 changes: 29 additions & 0 deletions lighthouse-core/gather/gatherers/host-user-agent.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* @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';

const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');
const {getBrowserVersion} = require('../driver/environment.js');

class HostUserAgent extends FRGatherer {
static symbol = Symbol('HostUserAgent');

/** @type {LH.Gatherer.GathererMeta} */
meta = {
symbol: HostUserAgent.symbol,
supportedModes: ['snapshot', 'timespan', 'navigation'],
};

/**
* @param {LH.Gatherer.FRTransitionalContext} context
* @return {Promise<LH.Artifacts['HostUserAgent']>}
*/
getArtifact(context) {
return getBrowserVersion(context.driver.defaultSession).then(v => v.userAgent);
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
}
}

module.exports = HostUserAgent;
2 changes: 1 addition & 1 deletion lighthouse-core/test/fraggle-rock/config/filters-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ describe('Fraggle Rock Config Filtering', () => {
class SnapshotWithBase extends BaseAudit {
static meta = {
id: 'snapshot',
requiredArtifacts: /** @type {any} */ (['Snapshot', 'URL', 'HostUserAgent']),
requiredArtifacts: /** @type {any} */ (['Snapshot', 'URL']),
adamraine marked this conversation as resolved.
Show resolved Hide resolved
...auditMeta,
};
}
Expand Down
32 changes: 5 additions & 27 deletions lighthouse-core/test/fraggle-rock/gather/base-artifacts-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,40 +15,17 @@ const {initializeConfig} = require('../../../fraggle-rock/config/config.js');
const {createMockDriver} = require('./mock-driver.js');
const LighthouseError = require('../../../lib/lh-error.js');

/** @param {{userAgent: string}} params */
function getMockDriverForArtifacts({userAgent}) {
function getMockDriverForArtifacts() {
const driverMock = createMockDriver();
driverMock._session.sendCommand.mockResponse('Browser.getVersion', {
userAgent,
product: '',
});

driverMock._executionContext.evaluate.mockResolvedValue(500);

return driverMock;
}

describe('getBaseArtifacts', () => {
let driverMock = getMockDriverForArtifacts({userAgent: 'Desktop Chrome'});
let driverMock = getMockDriverForArtifacts();

beforeEach(() => {
driverMock = getMockDriverForArtifacts({userAgent: 'Desktop Chrome'});
});

it('should fetch the desktop user agent', async () => {
const {config} = initializeConfig(undefined, {gatherMode: 'navigation'});
const artifacts = await getBaseArtifacts(config, driverMock.asDriver());
expect(artifacts.HostUserAgent).toEqual('Desktop Chrome');
expect(artifacts.HostFormFactor).toEqual('desktop');
});

it('should fetch the mobile user agent', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we'll probably want to bring these tests back when we have the breaking change for 9.0 to merge those

driverMock = getMockDriverForArtifacts({userAgent: 'Mobile Chrome'});

const {config} = initializeConfig(undefined, {gatherMode: 'navigation'});
const artifacts = await getBaseArtifacts(config, driverMock.asDriver());
expect(artifacts.HostUserAgent).toEqual('Mobile Chrome');
expect(artifacts.HostFormFactor).toEqual('mobile');
driverMock = getMockDriverForArtifacts();
});

it('should fetch benchmark index', async () => {
Expand All @@ -72,7 +49,7 @@ describe('finalizeArtifacts', () => {

beforeEach(async () => {
const {config} = initializeConfig(undefined, {gatherMode: 'navigation'});
const driver = getMockDriverForArtifacts({userAgent: 'Desktop Chrome'}).asDriver();
const driver = getMockDriverForArtifacts().asDriver();
baseArtifacts = await getBaseArtifacts(config, driver);
baseArtifacts.URL = {requestedUrl: 'http://example.com', finalUrl: 'https://example.com'};
gathererArtifacts = {};
Expand All @@ -81,6 +58,7 @@ describe('finalizeArtifacts', () => {
it('should merge the two objects', () => {
baseArtifacts.LighthouseRunWarnings = [{i18nId: '1', formattedDefault: 'Yes'}];
gathererArtifacts.LighthouseRunWarnings = [{i18nId: '2', formattedDefault: 'No'}];
gathererArtifacts.HostUserAgent = 'Desktop Chrome';

const winningError = new LighthouseError(LighthouseError.errors.NO_LCP);
baseArtifacts.PageLoadError = new LighthouseError(LighthouseError.errors.NO_FCP);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ describe('NavigationRunner', () => {

it('should collect base artifacts', async () => {
const {baseArtifacts} = await runner._setup({driver, config, requestedUrl});
expect(baseArtifacts).toMatchObject({HostUserAgent: 'Chrome', URL: {requestedUrl}});
Copy link
Member Author

Choose a reason for hiding this comment

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

HostUserAgent is not initialized in _setup anymore

expect(baseArtifacts).toMatchObject({URL: {requestedUrl}});
});

it('should prepare the target for navigation', async () => {
Expand All @@ -123,8 +123,8 @@ describe('NavigationRunner', () => {
});

it('should prepare the target for navigation *after* base artifact collection', async () => {
mockDriver._session.sendCommand.mockReset();
mockDriver._session.sendCommand.mockRejectedValue(new Error('Not available'));
mockDriver._executionContext.evaluate.mockReset();
mockDriver._executionContext.evaluate.mockRejectedValue(new Error('Not available'));
const setupPromise = runner._setup({driver, config, requestedUrl});
await expect(setupPromise).rejects.toThrowError(/Not available/);
expect(mocks.prepareMock.prepareTargetForNavigationMode).not.toHaveBeenCalled();
Expand Down
10 changes: 5 additions & 5 deletions types/artifacts.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,6 @@ declare global {
fetchTime: string;
/** A set of warnings about unexpected things encountered while loading and testing the page. */
LighthouseRunWarnings: Array<string | IcuMessage>;
/** Device which Chrome is running on. */
HostFormFactor: 'desktop'|'mobile';
/** The user agent string of the version of Chrome used. */
HostUserAgent: string;
/** The benchmark index that indicates rough device class. */
BenchmarkIndex: number;
/** An object containing information about the testing configuration used by Lighthouse. */
Expand All @@ -72,6 +68,10 @@ declare global {
* The set of base artifacts that were replaced by standard gatherers in Fraggle Rock.
*/
export interface LegacyBaseArtifacts {
/** Device which Chrome is running on. */
HostFormFactor: 'desktop'|'mobile';
/** The user agent string of the version of Chrome used. */
HostUserAgent: string;
/** The user agent string that Lighthouse used to load the page. Set to the empty string if unknown. */
NetworkUserAgent: string;
/** Information on detected tech stacks (e.g. JS libraries) used by the page. */
Expand Down Expand Up @@ -114,7 +114,7 @@ declare global {
* Artifacts provided by the default gatherers. Augment this interface when adding additional
* gatherers. Changes to these artifacts are not considered a breaking Lighthouse change.
*/
export interface GathererArtifacts extends PublicGathererArtifacts {
export interface GathererArtifacts extends PublicGathererArtifacts,LegacyBaseArtifacts {
/** The results of running the aXe accessibility tests on the page. */
Accessibility: Artifacts.Accessibility;
/** Array of all anchors on the page. */
Expand Down
9 changes: 1 addition & 8 deletions types/gatherer.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,7 @@ declare global {
trace?: Trace;
}

export type PhaseArtifact = |
LH.GathererArtifacts[keyof LH.GathererArtifacts] |
LH.Artifacts['devtoolsLogs'] |
LH.Artifacts['traces'] |
LH.Artifacts['WebAppManifest'] |
LH.Artifacts['InstallabilityErrors'] |
LH.Artifacts['Stacks'];
export type PhaseResultNonPromise = void|PhaseArtifact
Comment on lines -72 to -79
Copy link
Member Author

Choose a reason for hiding this comment

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

Only difference here should be NetworkUserAgent

export type PhaseResultNonPromise = void | LH.GathererArtifacts[keyof LH.GathererArtifacts];
export type PhaseResult = PhaseResultNonPromise | Promise<PhaseResultNonPromise>

export type GatherMode = 'snapshot'|'timespan'|'navigation';
Expand Down