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): move HostUserAgent/FormFactor back to base artifacts #12969

Merged
merged 5 commits into from
Sep 8, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,7 @@ const status403 = {
// Note: most scores are null (audit error) because the page 403ed.
requestedUrl: BASE_URL + 'seo-failure-cases.html?status_code=403',
finalUrl: BASE_URL + 'seo-failure-cases.html?status_code=403',
userAgent: /Chrom(e|ium)/, // Ensure we still collect base artifacts when page fails to load.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bring back the assertion that was previously implicit that userAgent could be collected on failed runs

runtimeError: {
code: 'ERRORED_DOCUMENT_REQUEST',
message: /Status code: 403/,
Expand Down
6 changes: 0 additions & 6 deletions lighthouse-core/fraggle-rock/config/default-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,6 @@ const artifacts = {
FullPageScreenshot: '',
GatherContext: '',
GlobalListeners: '',
HostFormFactor: '',
HostUserAgent: '',
IFrameElements: '',
ImageElements: '',
InstallabilityErrors: '',
Expand Down Expand Up @@ -85,8 +83,6 @@ 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 @@ -142,8 +138,6 @@ 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: 2 additions & 0 deletions lighthouse-core/fraggle-rock/config/filters.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ const baseArtifactKeySource = {
Timing: '',
URL: '',
PageLoadError: '',
HostFormFactor: '',
HostUserAgent: '',
};

const baseArtifactKeys = Object.keys(baseArtifactKeySource);
Expand Down
7 changes: 5 additions & 2 deletions lighthouse-core/fraggle-rock/gather/base-artifacts.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
const log = require('lighthouse-logger');
const isEqual = require('lodash.isequal');
const {
getBrowserVersion,
getBenchmarkIndex,
getEnvironmentWarnings,
} = require('../../gather/driver/environment.js');
Expand All @@ -19,6 +20,7 @@ const {
*/
async function getBaseArtifacts(config, driver) {
const BenchmarkIndex = await getBenchmarkIndex(driver.executionContext);
const {userAgent} = await getBrowserVersion(driver.defaultSession);

return {
// Meta artifacts.
Expand All @@ -28,12 +30,13 @@ async function getBaseArtifacts(config, driver) {
settings: config.settings,
// Environment artifacts that can always be computed.
BenchmarkIndex,
HostUserAgent: userAgent,
HostFormFactor: userAgent.includes('Android') || userAgent.includes('Mobile') ?
'mobile' : 'desktop',
// 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
14 changes: 9 additions & 5 deletions lighthouse-core/fraggle-rock/gather/navigation-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const NetworkRecords = require('../../computed/network-records.js');
* @property {LH.Config.FRConfig} config
* @property {LH.Config.NavigationDefn} navigation
* @property {string} requestedUrl
* @property {LH.FRBaseArtifacts} baseArtifacts
* @property {Map<string, LH.ArbitraryEqualityMap>} computedCache
*/

Expand Down Expand Up @@ -177,6 +178,7 @@ async function _navigation(navigationContext) {
computedCache: navigationContext.computedCache,
artifactDefinitions: navigationContext.navigation.artifacts,
artifactState,
baseArtifacts: navigationContext.baseArtifacts,
settings: navigationContext.config.settings,
};

Expand All @@ -192,10 +194,10 @@ async function _navigation(navigationContext) {
}

/**
* @param {{driver: Driver, config: LH.Config.FRConfig, requestedUrl: string; computedCache: NavigationContext['computedCache']}} args
* @param {{driver: Driver, config: LH.Config.FRConfig, requestedUrl: string; baseArtifacts: LH.FRBaseArtifacts, computedCache: NavigationContext['computedCache']}} args
* @return {Promise<{artifacts: Partial<LH.FRArtifacts & LH.FRBaseArtifacts>}>}
*/
async function _navigations({driver, config, requestedUrl, computedCache}) {
async function _navigations({driver, config, requestedUrl, baseArtifacts, computedCache}) {
if (!config.navigations) throw new Error('No navigations configured');

/** @type {Partial<LH.FRArtifacts & LH.FRBaseArtifacts>} */
Expand All @@ -209,6 +211,7 @@ async function _navigations({driver, config, requestedUrl, computedCache}) {
navigation,
requestedUrl,
config,
baseArtifacts,
computedCache,
};

Expand Down Expand Up @@ -253,9 +256,10 @@ async function navigation(options) {
return Runner.run(
async () => {
const driver = new Driver(page);
const {baseArtifacts} = await _setup({driver, config, requestedUrl});
const {artifacts} = await _navigations({driver, config, requestedUrl, computedCache});
await _cleanup({driver, config, requestedUrl});
const context = {driver, config, requestedUrl};
const {baseArtifacts} = await _setup(context);
const {artifacts} = await _navigations({...context, baseArtifacts, computedCache});
await _cleanup(context);

return finalizeArtifacts(baseArtifacts, artifacts);
},
Expand Down
3 changes: 3 additions & 0 deletions lighthouse-core/fraggle-rock/gather/runner-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
* @property {import('./driver.js')} driver
* @property {Array<LH.Config.AnyArtifactDefn>} artifactDefinitions
* @property {ArtifactState} artifactState
* @property {LH.FRBaseArtifacts} baseArtifacts
* @property {LH.Gatherer.FRGatherPhase} phase
* @property {LH.Gatherer.GatherMode} gatherMode
* @property {Map<string, LH.ArbitraryEqualityMap>} computedCache
Expand Down Expand Up @@ -68,6 +69,7 @@ async function collectPhaseArtifacts(options) {
driver,
artifactDefinitions,
artifactState,
baseArtifacts,
phase,
gatherMode,
computedCache,
Expand All @@ -91,6 +93,7 @@ async function collectPhaseArtifacts(options) {
url: await driver.url(),
gatherMode,
driver,
baseArtifacts,
dependencies,
computedCache,
settings,
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/fraggle-rock/gather/snapshot-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ async function snapshot(options) {
phase: 'getArtifact',
gatherMode: 'snapshot',
driver,
baseArtifacts,
artifactDefinitions,
artifactState,
computedCache,
Expand Down
3 changes: 2 additions & 1 deletion lighthouse-core/fraggle-rock/gather/timespan-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,14 @@ async function startTimespan(options) {
const computedCache = new Map();
const artifactDefinitions = config.artifacts || [];
const requestedUrl = await options.page.url();
const baseArtifacts = await getBaseArtifacts(config, driver);
const artifactState = getEmptyArtifactState();
/** @type {Omit<import('./runner-helpers.js').CollectPhaseArtifactOptions, 'phase'>} */
const phaseOptions = {
driver,
artifactDefinitions,
artifactState,
baseArtifacts,
computedCache,
gatherMode: 'timespan',
settings: config.settings,
Expand All @@ -48,7 +50,6 @@ async function startTimespan(options) {
const finalUrl = await options.page.url();
return Runner.run(
async () => {
const baseArtifacts = await getBaseArtifacts(config, driver);
baseArtifacts.URL.requestedUrl = requestedUrl;
baseArtifacts.URL.finalUrl = finalUrl;

Expand Down
31 changes: 0 additions & 31 deletions lighthouse-core/gather/gatherers/host-form-factor.js

This file was deleted.

29 changes: 0 additions & 29 deletions lighthouse-core/gather/gatherers/host-user-agent.js

This file was deleted.

9 changes: 4 additions & 5 deletions lighthouse-core/gather/gatherers/script-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ const NetworkRequest = require('../../lib/network-request.js');
const pageFunctions = require('../../lib/page-functions.js');
const {fetchResponseBodyFromCache} = require('../driver/network.js');
const DevtoolsLog = require('./devtools-log.js');
const HostFormFactor = require('./host-form-factor.js');

/* global getNodeDetails */

Expand Down Expand Up @@ -67,10 +66,10 @@ async function runInSeriesOrParallel(values, promiseMapper, runInSeries) {
* @fileoverview Gets JavaScript file contents.
*/
class ScriptElements extends FRGatherer {
/** @type {LH.Gatherer.GathererMeta<'DevtoolsLog'|'HostFormFactor'>} */
/** @type {LH.Gatherer.GathererMeta<'DevtoolsLog'>} */
meta = {
supportedModes: ['timespan', 'navigation'],
dependencies: {DevtoolsLog: DevtoolsLog.symbol, HostFormFactor: HostFormFactor.symbol},
dependencies: {DevtoolsLog: DevtoolsLog.symbol},
}

/**
Expand Down Expand Up @@ -138,11 +137,11 @@ class ScriptElements extends FRGatherer {
}

/**
* @param {LH.Gatherer.FRTransitionalContext<'DevtoolsLog'|'HostFormFactor'>} context
* @param {LH.Gatherer.FRTransitionalContext<'DevtoolsLog'>} context
*/
async getArtifact(context) {
const devtoolsLog = context.dependencies.DevtoolsLog;
const formFactor = context.dependencies.HostFormFactor;
const formFactor = context.baseArtifacts.HostFormFactor;
const networkRecords = await NetworkRecords.request(devtoolsLog, context);
return this._getArtifact(context, networkRecords, formFactor);
}
Expand Down
14 changes: 11 additions & 3 deletions lighthouse-core/test/fraggle-rock/api-test-pptr.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ const StaticServer = require('../../../lighthouse-cli/test/fixtures/static-serve

jest.setTimeout(90_000);

/**
* Some audits can be notApplicable based on machine timing information.
* Exclude these audits from applicability comparisons. */
const FLAKY_AUDIT_IDS_APPLICABILITY = new Set(['long-tasks']);

/**
* @param {LH.Result} lhr
*/
Expand All @@ -25,7 +30,10 @@ function getAuditsBreakdown(lhr) {
);

const notApplicableAudits = auditResults.filter(
audit => audit.scoreDisplayMode === 'notApplicable'
audit => (
audit.scoreDisplayMode === 'notApplicable' &&
!FLAKY_AUDIT_IDS_APPLICABILITY.has(audit.id)
)
);

const informativeAudits = applicableAudits.filter(
Expand All @@ -38,7 +46,7 @@ function getAuditsBreakdown(lhr) {

const failedAudits = applicableAudits.filter(audit => audit.score !== null && audit.score < 1);

return {auditResults, erroredAudits, failedAudits, informativeAudits, notApplicableAudits};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this wasn't used outside of this function and the new invariant means its count could be flaky, so avoiding accidental usage.

return {auditResults, erroredAudits, failedAudits, notApplicableAudits};
}

describe('Fraggle Rock API', () => {
Expand Down Expand Up @@ -174,7 +182,7 @@ describe('Fraggle Rock API', () => {
const {auditResults, erroredAudits, notApplicableAudits} = getAuditsBreakdown(result.lhr);
expect(auditResults.length).toMatchInlineSnapshot(`50`);

expect(notApplicableAudits.length).toMatchInlineSnapshot(`22`);
expect(notApplicableAudits.length).toMatchInlineSnapshot(`23`);
expect(notApplicableAudits.map(audit => audit.id)).toContain('server-response-time');

// TODO(FR-COMPAT): Reduce this number by handling the error, making N/A, or removing timespan support.
Expand Down
11 changes: 11 additions & 0 deletions lighthouse-core/test/fraggle-rock/gather/base-artifacts-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ const LighthouseError = require('../../../lib/lh-error.js');
function getMockDriverForArtifacts() {
const driverMock = createMockDriver();
driverMock._executionContext.evaluate.mockResolvedValue(500);
driverMock._session.sendCommand.mockResponse('Browser.getVersion', {
userAgent: 'Mozilla/5.0 (Macintosh; Intel Mac OS X) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.159 Safari/537.36', // eslint-disable-line max-len
product: 'Chrome/92.0.4515.159',
});
return driverMock;
}

Expand All @@ -34,6 +38,13 @@ describe('getBaseArtifacts', () => {
expect(artifacts.BenchmarkIndex).toEqual(500);
});

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

it('should return settings', async () => {
const {config} = initializeConfig(undefined, {gatherMode: 'navigation'});
const artifacts = await getBaseArtifacts(config, driverMock.asDriver());
Expand Down
19 changes: 19 additions & 0 deletions lighthouse-core/test/fraggle-rock/gather/mock-driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,13 +128,31 @@ function mockDriverModule(driverProvider) {
};
}

/**
* @returns {LH.FRBaseArtifacts}
*/
function createMockBaseArtifacts() {
return {
fetchTime: new Date().toISOString(),
URL: {finalUrl: 'https://example.com', requestedUrl: 'https://example.com'},
PageLoadError: null,
settings: defaultSettings,
BenchmarkIndex: 500,
LighthouseRunWarnings: [],
Timing: [],
HostFormFactor: 'desktop',
HostUserAgent: 'Chrome/93.0.1449.0',
};
}

function createMockContext() {
return {
driver: createMockDriver(),
url: 'https://example.com',
gatherMode: 'navigation',
computedCache: new Map(),
dependencies: {},
baseArtifacts: createMockBaseArtifacts(),
settings: defaultSettings,

/** @return {LH.Gatherer.FRTransitionalContext} */
Expand Down Expand Up @@ -208,5 +226,6 @@ module.exports = {
createMockPage,
createMockSession,
createMockGathererInstance,
createMockBaseArtifacts,
createMockContext,
};
Loading