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

core(fr): separate audit phase for flows #13623

Merged
merged 14 commits into from
Mar 1, 2022
Merged

core(fr): separate audit phase for flows #13623

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c116fe9
core(fr): separate audit phase for flows
adamraine Feb 3, 2022
86e90fb
async
adamraine Feb 4, 2022
6ec57d6
fix test
adamraine Feb 4, 2022
a8da3ea
check
adamraine Feb 4, 2022
ed43694
test
adamraine Feb 4, 2022
ae2ef25
test
adamraine Feb 4, 2022
6486482
test
adamraine Feb 4, 2022
32efb46
rn
adamraine Feb 7, 2022
335df52
Merge branch 'master' into fr-flow-audit-phase
adamraine Feb 7, 2022
495fb26
test
adamraine Feb 8, 2022
16377e7
Merge branch 'master' into fr-flow-audit-phase
adamraine Feb 11, 2022
3273315
Merge branch 'master' into fr-flow-audit-phase
adamraine Mar 1, 2022
88b9e85
no cache
adamraine Mar 1, 2022
b0c9e46
bring back
adamraine Mar 1, 2022
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
38 changes: 35 additions & 3 deletions lighthouse-core/fraggle-rock/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@
*/
'use strict';

const {snapshot} = require('./gather/snapshot-runner.js');
const {startTimespan} = require('./gather/timespan-runner.js');
const {navigation} = require('./gather/navigation-runner.js');
const {snapshot: snapshot_} = require('./gather/snapshot-runner.js');
const {startTimespan: startTimespan_} = require('./gather/timespan-runner.js');
const {navigation: navigation_} = require('./gather/navigation-runner.js');
const Runner = require('../runner.js');
const UserFlow = require('./user-flow.js');

/**
Expand All @@ -18,6 +19,37 @@ async function startFlow(page, options) {
return new UserFlow(page, options);
}

/**
* @param {Parameters<navigation_>} params
* @return {Promise<LH.RunnerResult|undefined>}
*/
async function navigation(...params) {
const gatherResult = await navigation_(...params);
return Runner.audit(gatherResult.artifacts, gatherResult.runnerOptions);
}

/**
* @param {Parameters<snapshot_>} params
* @return {Promise<LH.RunnerResult|undefined>}
*/
async function snapshot(...params) {
const gatherResult = await snapshot_(...params);
return Runner.audit(gatherResult.artifacts, gatherResult.runnerOptions);
}

/**
* @param {Parameters<startTimespan_>} params
* @return {Promise<{endTimespan: () => Promise<LH.RunnerResult|undefined>}>}
*/
async function startTimespan(...params) {
const {endTimespan: endTimespan_} = await startTimespan_(...params);
const endTimespan = async () => {
const gatherResult = await endTimespan_();
return Runner.audit(gatherResult.artifacts, gatherResult.runnerOptions);
};
return {endTimespan};
}

module.exports = {
snapshot,
startTimespan,
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/fraggle-rock/gather/navigation-runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ async function _cleanup({requestedUrl, driver, config}) {

/**
* @param {{url: string, page: import('puppeteer').Page, config?: LH.Config.Json, configContext?: LH.Config.FRContext}} options
* @return {Promise<LH.RunnerResult|undefined>}
* @return {Promise<LH.Gatherer.FRGatherResult>}
*/
async function navigation(options) {
const {url, page, configContext = {}} = options;
Expand All @@ -301,7 +301,7 @@ async function navigation(options) {
},
runnerOptions
);
return Runner.audit(artifacts, runnerOptions);
return {artifacts, runnerOptions};
}

module.exports = {
Expand Down
7 changes: 5 additions & 2 deletions lighthouse-core/fraggle-rock/gather/snapshot-runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ const {
const {initializeConfig} = require('../config/config.js');
const {getBaseArtifacts, finalizeArtifacts} = require('./base-artifacts.js');

/** @param {{page: import('puppeteer').Page, config?: LH.Config.Json, configContext?: LH.Config.FRContext}} options */
/**
* @param {{page: import('puppeteer').Page, config?: LH.Config.Json, configContext?: LH.Config.FRContext}} options
* @return {Promise<LH.Gatherer.FRGatherResult>}
*/
async function snapshot(options) {
const {configContext = {}} = options;
const {config} = initializeConfig(options.config, {...configContext, gatherMode: 'snapshot'});
Expand Down Expand Up @@ -53,7 +56,7 @@ async function snapshot(options) {
},
runnerOptions
);
return Runner.audit(artifacts, runnerOptions);
return {artifacts, runnerOptions};
}

module.exports = {
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/fraggle-rock/gather/timespan-runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const {getBaseArtifacts, finalizeArtifacts} = require('./base-artifacts.js');

/**
* @param {{page: import('puppeteer').Page, config?: LH.Config.Json, configContext?: LH.Config.FRContext}} options
* @return {Promise<{endTimespan(): Promise<LH.RunnerResult|undefined>}>}
* @return {Promise<{endTimespan(): Promise<LH.Gatherer.FRGatherResult>}>}
*/
async function startTimespan(options) {
const {configContext = {}} = options;
Expand Down Expand Up @@ -66,7 +66,7 @@ async function startTimespan(options) {
},
runnerOptions
);
return Runner.audit(artifacts, runnerOptions);
return {artifacts, runnerOptions};
},
};
}
Expand Down
84 changes: 55 additions & 29 deletions lighthouse-core/fraggle-rock/user-flow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@ const {generateFlowReportHtml} = require('../../report/generator/report-generato
const {snapshot} = require('./gather/snapshot-runner.js');
const {startTimespan} = require('./gather/timespan-runner.js');
const {navigation} = require('./gather/navigation-runner.js');
const Runner = require('../runner.js');

/** @typedef {Parameters<snapshot>[0]} FrOptions */
/** @typedef {Omit<FrOptions, 'page'> & {name?: string}} UserFlowOptions */
/** @typedef {Omit<FrOptions, 'page'> & {stepName?: string}} StepOptions */
/** @typedef {{gatherResult: LH.Gatherer.FRGatherResult, name: string}} StepArtifact */

class UserFlow {
/**
Expand All @@ -24,8 +26,10 @@ class UserFlow {
this.options = {page, ...options};
/** @type {string|undefined} */
this.name = options?.name;
/** @type {LH.FlowResult.Step[]} */
this.steps = [];
/** @type {StepArtifact[]} */
this.stepArtifacts = [];
/** @type {LH.FlowResult|undefined} */
this.flowResult = undefined;
}

/**
Expand All @@ -38,12 +42,12 @@ class UserFlow {
}

/**
* @param {LH.Result} lhr
* @param {LH.Artifacts} artifacts
* @return {string}
*/
_getDefaultStepName(lhr) {
const shortUrl = this._shortenUrl(lhr.finalUrl);
switch (lhr.gatherMode) {
_getDefaultStepName(artifacts) {
const shortUrl = this._shortenUrl(artifacts.URL.finalUrl);
switch (artifacts.GatherContext.gatherMode) {
case 'navigation':
return `Navigation report (${shortUrl})`;
case 'timespan':
Expand All @@ -67,7 +71,8 @@ class UserFlow {
}

// On repeat navigations, we want to disable storage reset by default (i.e. it's not a cold load).
const isSubsequentNavigation = this.steps.some(step => step.lhr.gatherMode === 'navigation');
const isSubsequentNavigation = this.stepArtifacts
.some(step => step.gatherResult.artifacts.GatherContext.gatherMode === 'navigation');
if (isSubsequentNavigation) {
if (settingsOverrides.disableStorageReset === undefined) {
settingsOverrides.disableStorageReset = true;
Expand All @@ -87,16 +92,15 @@ class UserFlow {
async navigate(url, stepOptions) {
if (this.currentTimespan) throw Error('Timespan already in progress');

const result = await navigation(this._getNextNavigationOptions(url, stepOptions));
if (!result) throw Error('Navigation returned undefined');
const gatherResult = await navigation(this._getNextNavigationOptions(url, stepOptions));

const providedName = stepOptions?.stepName;
this.steps.push({
lhr: result.lhr,
name: providedName || this._getDefaultStepName(result.lhr),
this.stepArtifacts.push({
gatherResult,
name: providedName || this._getDefaultStepName(gatherResult.artifacts),
});

return result;
return gatherResult;
}

/**
Expand All @@ -114,17 +118,16 @@ class UserFlow {
if (!this.currentTimespan) throw Error('No timespan in progress');

const {timespan, options} = this.currentTimespan;
const result = await timespan.endTimespan();
const gatherResult = await timespan.endTimespan();
this.currentTimespan = undefined;
if (!result) throw Error('Timespan returned undefined');

const providedName = options?.stepName;
this.steps.push({
lhr: result.lhr,
name: providedName || this._getDefaultStepName(result.lhr),
this.stepArtifacts.push({
gatherResult,
name: providedName || this._getDefaultStepName(gatherResult.artifacts),
});

return result;
return gatherResult;
}

/**
Expand All @@ -134,26 +137,49 @@ class UserFlow {
if (this.currentTimespan) throw Error('Timespan already in progress');

const options = {...this.options, ...stepOptions};
const result = await snapshot(options);
if (!result) throw Error('Snapshot returned undefined');
const gatherResult = await snapshot(options);
if (!gatherResult) throw Error('Snapshot returned undefined');

const providedName = stepOptions?.stepName;
this.steps.push({
lhr: result.lhr,
name: providedName || this._getDefaultStepName(result.lhr),
this.stepArtifacts.push({
gatherResult,
name: providedName || this._getDefaultStepName(gatherResult.artifacts),
});

return result;
return gatherResult;
}

/**
* @returns {Promise<LH.FlowResult>}
*/
async endFlow() {
if (this.flowResult) return this.flowResult;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like if the user calls getFlowResult, followed by more gathering, then getFlowResult again, they will get a cached result and no indication that something they did was wrong.

Should either guard against that (throw if already called getFlowResult), or just not cache. If the latter, might be good to rename to createFlowResult.

also nit: rename getUserFlowReport?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should either guard against that (throw if already called getFlowResult), or just not cache. If the latter, might be good to rename to createFlowResult.

I'm thinking we remove the cache.

also nit: rename getUserFlowReport?

This function returns a FlowResult object not a string containing the user flow report.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant getUserFlowResult.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it makes more sense since the object type is FlowResult. I also think it's pretty clear that "flow" refers to a "user flow" and not some other type of flow. The constructor for UserFlow isn't exposed on the api either.

if (!this.stepArtifacts.length) {
throw Error('Need at least one step before ending the flow');
}
const url = new URL(this.stepArtifacts[0].gatherResult.artifacts.URL.finalUrl);
const flowName = this.name || `User flow (${url.hostname})`;

/** @type {LH.FlowResult['steps']} */
const steps = [];
for (const {gatherResult, name} of this.stepArtifacts) {
const result = await Runner.audit(gatherResult.artifacts, gatherResult.runnerOptions);
if (!result) throw new Error(`Step "${name}" did not return a result`);
steps.push({lhr: result.lhr, name});
}

this.flowResult = {steps, name: flowName};
return this.flowResult;
}

/**
* @return {LH.FlowResult}
*/
getFlowResult() {
if (!this.steps.length) throw Error('Need at least one step before getting the flow result');
const url = new URL(this.steps[0].lhr.finalUrl);
const name = this.name || `User flow (${url.hostname})`;
return {steps: this.steps, name};
if (!this.flowResult) {
throw Error('Must end the flow before getting the result');
}
return this.flowResult;
}

/**
Expand Down
2 changes: 2 additions & 0 deletions lighthouse-core/scripts/update-flow-fixtures.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ async function waitForImagesToLoad(page) {

await flow.navigate('https://www.mikescerealshack.co/corrections');

await flow.endFlow();

const flowResult = flow.getFlowResult();
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason to have separate functions for endFlow and getFlowResult?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to make getFlowResult async but that concern is kinda thin. I'm up for either.


fs.writeFileSync(
Expand Down
8 changes: 8 additions & 0 deletions types/gatherer.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,14 @@ declare module Gatherer {
settings: Config.Settings;
}

interface FRGatherResult {
artifacts: Artifacts;
runnerOptions: {
config: Config.FRConfig;
computedCache: Map<string, ArbitraryEqualityMap>
}
}

interface PassContext {
gatherMode: 'navigation';
/** The url of the currently loaded page. If the main document redirects, this will be updated to keep track. */
Expand Down