core(test): add user flows to generate some fixtures #15005

connorjclark · 2023-04-20T21:18:46Z

I picked a few traces and fleshed out a script to regenerate them.

This PR:

regenerate.js scripts in core/test/fixtures/artifacts to run user flows that update test fixtures
run all with sh core/test/scripts/regenerate-fixtures.sh
user flow scripts for: animation.json, trace-user-timings.json, site-with-redirect.json, video-embeds-m84.json (will rename in next PR to preserve git history)
deletes: devtools-homepage-w-screenshots-trace.json, network-records.json (not used)
changes some unit test asserts to inline snapshots, to make actually updating fixtures less cumbersome

Some user flows use existing pages copied over from the static server fixtures, or makes a new page. One (the redirects one) just runs on a real site (the same it originally came from), since that seemed more useful than a bare-bones test page ( /redirects-scripts.html?redirect=... works too but doesn't have a lot of main thread work).

We could choose to land this PR w/o any updates to the fixtures, just keep the user flow scripts (defer the actual update for when more stuff is done). Other reasons to defer would be if we want to somehow minify the fixtures, or use git-lfs, before we starting churning these big files. (EDIT: we decided to land the changes, and do not care about the file size)

This first pass has uncovered a possible bug with user flow + redirects, as seen by the interactive test currently failing. If we choose to drop changes in fixtures, we can land this PR w/o addressing the bug. (EDIT: this was a fluke)

https://docs.google.com/spreadsheets/d/1AaYzpzWnpXQ4JB5IZzOkTO9Zf5Sm0I8dsp7MhBgthMg/edit#gid=0

connorjclark · 2023-04-20T21:19:17Z

core/test/audits/third-party-facades-test.js

@@ -267,106 +267,124 @@ describe('Third party facades audit', () => {
    expect(results.displayValue).toBeDisplayString('2 facade alternatives available');
    expect(results.details.items[0].product).toBeDisplayString('YouTube Embedded Player (Video)');
    expect(results.details.items[1].product).toBeDisplayString('Vimeo Embedded Player (Video)');
-    expect(results.details.items).toMatchObject(


We could also just delete this?

connorjclark · 2023-04-20T21:23:13Z

core/test/gather/gatherers/trace-elements-test.js

-        animations: [
-          {failureReasonsMask: 8224, unsupportedProperties: ['width']},
-          {name: 'alpha', failureReasonsMask: 8224, unsupportedProperties: ['height']},
-          {name: 'beta', failureReasonsMask: 8224, unsupportedProperties: ['background-color']},


note: background-color is now supported (sometime shortly after this audit was made), but now font-size replaces it in the unsupported reasons? shrug

core/test/gather/gatherers/trace-elements-test.js

connorjclark · 2023-04-20T21:25:26Z

core/test/audits/mainthread-work-breakdown-test.js

@@ -101,6 +104,26 @@ describe('Performance: page execution timings audit', () => {
    assert.equal(output.score, 1);
  });

+  it('should compute the correct values for the load trace', async () => {


really any modern chrome trace will do here, which is why I didn't bother making a user flow for a basic page. instead just used animations here.

adamraine

First pass, but I like the idea of having generator scripts w/ verify functions

core/test/audits/non-composited-animations-test.js

core/test/audits/metrics/interactive-test.js

adamraine · 2023-04-20T23:52:31Z

core/test/audits/mainthread-work-breakdown-test.js

@@ -101,6 +104,26 @@ describe('Performance: page execution timings audit', () => {
    assert.equal(output.score, 1);
  });

+  it('should compute the correct values for the load trace', async () => {
+    assert(loadTrace.traceEvents.find(e => e.name === 'TracingStartedInBrowser'));


Why not put this assertion in the trace verify function?

Because that's pretty far removed from the one place that cares (this test)

Actually, are we really worried about this event being missing in any new traces? The check kinda makes sense for the old trace but if this one is being continually refreshed then I'm not sure we need it.

This trace will not be refreshed. We could remove the logic for the old event but only when all our test fixtures have been updated.

core/test/scripts/trace-fixtures/update-fixtures.js

core/test/gather/gatherers/trace-elements-test.js

core/test/scripts/trace-fixtures/redirect/user-flow.js

connorjclark · 2023-04-26T20:49:44Z

This first pass has uncovered a possible bug with user flow + redirects, as seen by the interactive test currently failing. If we choose to drop changes in fixtures, we can land this PR w/o addressing the bug.

Issue was that the user flow was simulated throttling, and that only waits 1s after cpu idle. Yet this trace was then being used in the interactive unit test for observed throttling, which assumes at least 5s of idle time at the end of the trace :)

Fixed by setting provided throttling in the user flow script.

adamraine · 2023-04-27T18:45:04Z

core/test/audits/mainthread-work-breakdown-test.js

@@ -101,6 +104,26 @@ describe('Performance: page execution timings audit', () => {
    assert.equal(output.score, 1);
  });

+  it('should compute the correct values for the load trace', async () => {
+    assert(loadTrace.traceEvents.find(e => e.name === 'TracingStartedInBrowser'));


Actually, are we really worried about this event being missing in any new traces? The check kinda makes sense for the old trace but if this one is being continually refreshed then I'm not sure we need it.

lighthouse-cli/test/fixtures/perf/animations.html

brendankenny · 2023-04-27T20:21:00Z

Can we bikeshed a bit?

If we ignore static-server's current requirements, in an ideal world each of these would be self contained units, easy to get your head around at the directory level (like performance-stories). Something like

core/test/fixtures/artifacts/
├─ animation/
│  ├─ devtoolslog.json
│  ├─ trace.json
│  ├─ regenerate.js
│  └─ page/
│     └─ animation.html
├─ redirects/
│ ...

this would also allow growth in page requirements (extra scripts, etc) and an easy place to colocate artifacts if/when we need them (devtoolslog, artifacts.json).

adamraine · 2023-04-27T20:26:36Z

It helps us organize better, but then we wouldn't be able to access them via yarn static-server.

brendankenny · 2023-04-27T20:41:32Z

These traces are central to so many tests, so better to pay for any static-server changes and reduce the annoyance of to authoring/maintaining test trace generation.

I was looking for where we've expanded the serving directories before to see how hard it would be, but had forgotten the server.baseDir hack/configuration that we use for all the *-test-pptr tests, so luckily this wouldn't be any work at all if we want to go this way.

connorjclark · 2023-04-27T20:46:07Z

perf/animation.html is an existing smoke page, in your proposal would you like the file to be duplicated as page.html?

self contained units

What are your thoughts on the redirect fixture (uses a real website at the moment, with real CPU work to test interactive audit)? Create a new page for the same purpose now, or defer until needed (when regenerating from the website no longer gives us what we need).

brendankenny · 2023-04-27T21:39:30Z

perf/animation.html is an existing smoke page, in your proposal would you like the file to be duplicated as page.html?

Yeah, I think it's worth it. The smoke test should be able to evolve, add extra things etc without worrying about altering a bunch of unit tests.

self contained units

What are your thoughts on the redirect fixture (uses a real website at the moment, with real CPU work to test interactive audit)? Create a new page for the same purpose now, or defer until needed (when regenerating from the website no longer gives us what we need).

I think in an ideal world we would recreate hermetic tests for all those, but in reality our most complex test sites aren't really approaching all the complexity you get in a real site (all the requests, all the cpu work, etc). Like we could never engineer a monstrosity like the youtube embed, and it's not worth trying.

(though by the same token, maybe we'll only want to refresh the traces for those sites like once every three years, or if there's a major change for a key trace event like navStart or a CWV event since there could be wild volatility in what results. That seems ok since for these we're just interested in a real trace, not a live one)

connorjclark · 2023-05-01T23:24:47Z

Moved the files to match the structure defined in #15005 (comment) - but will rename the trace/devtools output files in a follow up PR (simplest way to preserve file rename history in git).

core/test/fixtures/artifacts/update-trace-fixture.js

core(test): add user flows to generate some fixtures

4673e15

connorjclark requested a review from a team as a code owner April 20, 2023 21:18

connorjclark requested review from adamraine and removed request for a team April 20, 2023 21:18

connorjclark commented Apr 20, 2023

View reviewed changes

devtools-bot assigned adamraine Apr 20, 2023

devtools-bot added the waiting4reviewer label Apr 20, 2023

connorjclark commented Apr 20, 2023

View reviewed changes

core/test/gather/gatherers/trace-elements-test.js Show resolved Hide resolved

connorjclark commented Apr 20, 2023

View reviewed changes

dont use lighthouse pkg

559a0c3

vercel bot deployed to Preview April 20, 2023 21:37 View deployment

adamraine reviewed Apr 21, 2023

View reviewed changes

change lint thing

d608b26

vercel bot deployed to Preview April 25, 2023 18:40 View deployment

connorjclark force-pushed the fixture-scripts branch from 1503272 to d608b26 Compare April 25, 2023 18:54

vercel bot deployed to Preview April 25, 2023 19:15 View deployment

fix interactive test

d41d1cc

vercel bot deployed to Preview April 26, 2023 20:50 View deployment

fix snapshot

4e5f3bc

vercel bot deployed to Preview April 26, 2023 20:58 View deployment

fix animation fixture

2b75ae0

vercel bot deployed to Preview April 27, 2023 16:38 View deployment

revert test changes

a4706e0

vercel bot deployed to Preview April 27, 2023 16:43 View deployment

mv

40f90bb

vercel bot deployed to Preview April 27, 2023 16:47 View deployment

brendankenny mentioned this pull request Apr 27, 2023

core: use target type instead of session for oopif #15006

Merged

adamraine reviewed Apr 27, 2023

View reviewed changes

oops

b80bbe5

vercel bot deployed to Preview April 27, 2023 18:56 View deployment

adamraine approved these changes Apr 27, 2023

View reviewed changes

move to new folder structure

d0d244c

vercel bot deployed to Preview May 1, 2023 23:23 View deployment

bad c/p

044fcef

vercel bot deployed to Preview May 1, 2023 23:26 View deployment

adamraine reviewed May 1, 2023

View reviewed changes

core/test/fixtures/artifacts/update-trace-fixture.js Outdated Show resolved Hide resolved

rename

9838462

vercel bot deployed to Preview May 2, 2023 15:57 View deployment

connorjclark merged commit 14aa68d into main May 2, 2023

connorjclark deleted the fixture-scripts branch May 2, 2023 17:28

connorjclark mentioned this pull request May 2, 2023

core(test): move regenerated fixtures to new folders #15031

Merged

adamraine mentioned this pull request Jun 26, 2023

core(mainthread-work-breakdown): add TBT savings #15201

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(test): add user flows to generate some fixtures #15005

core(test): add user flows to generate some fixtures #15005

connorjclark commented Apr 20, 2023 •

edited

Loading

connorjclark Apr 20, 2023

connorjclark Apr 20, 2023

connorjclark Apr 20, 2023

adamraine left a comment

adamraine Apr 20, 2023

connorjclark Apr 21, 2023

adamraine Apr 27, 2023

connorjclark Apr 27, 2023 •

edited

Loading

connorjclark commented Apr 26, 2023 •

edited

Loading

adamraine Apr 27, 2023

brendankenny commented Apr 27, 2023

adamraine commented Apr 27, 2023

brendankenny commented Apr 27, 2023

connorjclark commented Apr 27, 2023

brendankenny commented Apr 27, 2023

connorjclark commented May 1, 2023

core(test): add user flows to generate some fixtures #15005

core(test): add user flows to generate some fixtures #15005

Conversation

connorjclark commented Apr 20, 2023 • edited Loading

connorjclark Apr 20, 2023

Choose a reason for hiding this comment

connorjclark Apr 20, 2023

Choose a reason for hiding this comment

connorjclark Apr 20, 2023

Choose a reason for hiding this comment

adamraine left a comment

Choose a reason for hiding this comment

adamraine Apr 20, 2023

Choose a reason for hiding this comment

connorjclark Apr 21, 2023

Choose a reason for hiding this comment

adamraine Apr 27, 2023

Choose a reason for hiding this comment

connorjclark Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

connorjclark commented Apr 26, 2023 • edited Loading

adamraine Apr 27, 2023

Choose a reason for hiding this comment

brendankenny commented Apr 27, 2023

adamraine commented Apr 27, 2023

brendankenny commented Apr 27, 2023

connorjclark commented Apr 27, 2023

brendankenny commented Apr 27, 2023

connorjclark commented May 1, 2023

connorjclark commented Apr 20, 2023 •

edited

Loading

connorjclark Apr 27, 2023 •

edited

Loading

connorjclark commented Apr 26, 2023 •

edited

Loading