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(user-flow): dry run flag #13837

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

core(user-flow): dry run flag #13837

wants to merge 15 commits into from

Conversation

adamraine
Copy link
Member

Shoutout to @BioPhoton and https://github.com/push-based/user-flow for this idea.

This allows the user to validate the user flow is performing the desired actions correctly before taking Lighthouse measurements. These speeds up the development process for user flow scripts since Lighthouse gathering can take a lot of time.

#11313

@adamraine adamraine requested a review from a team as a code owner April 8, 2022 19:09
@adamraine adamraine requested review from connorjclark and removed request for a team April 8, 2022 19:09
@brendankenny
Copy link
Member

For a faster development process you can use the --dryRun option to skip measurement and perform the interactions only
This is a multitude faster e.g. 3s vs 53s for a simple 2 step flow with navigation

something seems very wrong in that 53s case if LH overhead is 50s

this.name = options?.name;
this._name = options?.name;
/** @type {boolean|undefined} */
this._dryRun = options?.dryRun;
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this is on options why store it separately?

Copy link
Member Author

Choose a reason for hiding this comment

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

this._options are the options passed to each mode runner, the type doesn't include the dryRun and name flags.

The types for this file could definitely use some cleaning up though, but I'd rather do that in a separate PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The types for this file could definitely use some cleaning up though, but I'd rather do that in a separate PR.

Can you file an issue to track?

@adamraine
Copy link
Member Author

something seems very wrong in that 53s case if LH overhead is 50s

For one, page.goto is a quicker than our own gotoUrl function because we wait for certain network conditions.

Flow scripts can be arbitrarily long too. On our own flow fixtures test, dry run took the execution time from 48s to 13s (35s overhead). Since there are 4 flow steps in this test, we can estimate the LH overhead to be 8.75s per step which seems reasonable to me.

@brendankenny
Copy link
Member

Flow scripts can be arbitrarily long too. On our own flow fixtures test, dry run took the execution time from 48s to 13s (35s overhead). Since there are 4 flow steps in this test, we can estimate the LH overhead to be 8.75s per step which seems reasonable to me.

Just curious, what's the time breakdown on that? waitForFullyLoaded is a key candidate, right, though if they don't have their own waitFor* in their flow scripts the dry run might not go well.

I wonder how close dropping the audits and the waitFor conditions would get to that timing.

@adamraine
Copy link
Member Author

adamraine commented Apr 8, 2022

Step breakdown normally
nav 1: 11.428s
startTimespan: 1.465s
endTimespan: 5.671s
snap: 6.860s
nav 2: 9.566s

Impact of gotoURL specifically. This would include any waitFor* conditions:
First nav: 3.274s
Second nav: 2.542s

Step breakdown w/ dry run
nav 1: 1.356s
startTimespan: 277.94ms
endTimespan: 0.006ms
snap: 287.622ms
nav 2: 569.256ms

None of these timings include any auditing.

though if they don't have their own waitFor* in their flow scripts the dry run might not go well

We can try to use our own gotoURL function in place of page.goto() to make dry runs more representative of an actual LH run. It seems like gotoURL isn't where we spend most of our time during navigations, although that could be different for a page like https://cnn.com.

Edit:gotoURL is 13.838s on cnn out of 24.628s for the navigation gathering phase.

docs/user-flows.md Outdated Show resolved Hide resolved
@brendankenny
Copy link
Member

This still seems like a lot of infrastructure to support dry runs, adding a shadow version to every gather mode. It definitely makes any changes to the user flows API a lot more complicated.

Was the no-gatherers-or-audits idea a no go?

const options = {...this._options, ...stepOptions};

if (this._dryRun) {
await dryRun('timespan', options);
Copy link
Member

Choose a reason for hiding this comment

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

What happens to emulation when the driver disconnects? Does it persist or go away?

Copy link
Member Author

@adamraine adamraine Jun 22, 2022

Choose a reason for hiding this comment

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

https://bugs.chromium.org/p/chromium/issues/detail?id=1337089

TLDR it goes back to default, even if another session exists that is overriding the device emulation.

@adamraine
Copy link
Member Author

Was the no-gatherers-or-audits idea a no go?

It helps but it's not as good as using shadow runners:
Dry run smoke test with dryRun: false: 36s
Dry run smoke test with shadow runners: 9s
Dry run smoke test only running viewport audit: 14s

A "no-audits" solution still does stuff like collect base artifacts, but the difference is small enough where I'd be comfortable doing it.

@adamraine
Copy link
Member Author

Discussed, going to do the no-gatherers approach and close this. Will open an issue before closing...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants