Lighthouse: DevTools webtests improvements

[patrickhulce]

Summary

Now that we've started to bring the DevTools testing into core we can start to make them even stronger :)

Miscellaneous thoughts:

• [ ] Move LighthouseTestRunner into core as well so we can make changes to the shared utilities.
• [ ] Strengthen audit assertions, for example we had to disable all perf assertions in the past because screenshots were flaky. It would be awesome if we could flip the script to get Chromium's perspective to consider flaky screenshot collection a bug and fix that instead of disabling the tests that point it out 😃
• [ ] Assertions on the rendering of the report. AFAIK all our current tests assert on the LHR are obtained from a spy on internal methods. Some sort of testing that the complete rendering pipeline works would be great for testing PRs like #11411
• [ ] A package.json script that would build and open the latest Lighthouse in DevTools for manual inspection.
• [x] fix caching TTL https://github.com/GoogleChrome/lighthouse/issues/11414#issuecomment-691294975
• [ ] remove the TestExpectations that marks one of the tests flaky (probably not flaky anymore)
• [ ] startup errors should show error and not timeout https://github.com/GoogleChrome/lighthouse/pull/10630#issuecomment-694400741
• [ ] only put explanation in snapshots when audit fails
• [ ] don't hide output of http server (blocked port hangs)
• [ ] git pull devtools checkout in .tmp #11717 #11678

[patrickhulce]

cc @connorjclark I'm sure he has more ideas too :)

[brendankenny]

Assertions on the rendering of the report

heh, I was going to suggest the same after seeing #11411 passing without the upstream changes landing yet :)

[connorjclark]

Yeah, we should do all these things.

Assertions on the rendering of the report. AFAIK all our current tests assert on the LHR are obtained from a spy on internal methods.

We have these:

https://github.com/GoogleChrome/lighthouse/blob/master/third-party/chromium-webtests/webtests/http/tests/devtools/lighthouse/lighthouse-view-trace-run.js#L34
https://github.com/GoogleChrome/lighthouse/blob/master/third-party/chromium-webtests/webtests/http/tests/devtools/lighthouse/lighthouse-successful-run.js#L83
https://github.com/GoogleChrome/lighthouse/blob/master/third-party/chromium-webtests/webtests/http/tests/devtools/lighthouse/lighthouse-export-run.js#L39

Some sort of testing that the complete rendering pipeline works would be great for testing PRs like #11411

Right, I'd expect them to fail now, because we do have rendering tests. I think devtools doesn't run their layout tests in their repo yet. @TimvdLippe can you confirm if you run the tests in tests/webtests on trybots? That's where all the Lighthouse layout tests are.

I just ran yarn test-devtools and get:

ninja: Entering directory `out/Default'
ninja: error: '../../front_end/third_party/lighthouse/report-assets/report.d.ts', needed by 'resources/inspector/third_party/lighthouse/locales/ar-XB.json', missing and no known rule to make it
error Command failed with exit code 1.

Which I assume is also related to these changes. (EDIT: yea, here's a pr: https://github.com/GoogleChrome/lighthouse/pull/11418 )

[patrickhulce]

We have these:

Ah thanks yeah I forgot about those. That caching seems to explain why they didn't fail then haha. I wish we could also cache on some hash of devtools files to avoid going an entire week without learning about it (are we sure the cached assets fall out after a week no matter what or just a week after they're not used? their policy language makes me think these might last forever given our frequency of runs), but I guess that's a circular problem with the cache to begin with sooooo, no go. Darn.

[brendankenny]

That caching seems to explain why they didn't fail then haha

wait, but why didn't #11411 break the renderer since it gets rid of Runtime.cachedResources and uses globalThis.EXPORTED_CACHED_RESOURCES_ONLY_FOR_LIGHTHOUSE but the cached devtools code presumably wouldn't have that new object yet?

[patrickhulce]

wait, but why didn't #11411 break the renderer since it gets rid of Runtime.cachedResources and uses globalThis.EXPORTED_CACHED_RESOURCES_ONLY_FOR_LIGHTHOUSE but the cached devtools code presumably wouldn't have that new object yet?

I don't know. At the time I said that I hadn't looked at #11417 yet 😆 I had it in my head that we wouldn't have rebuilt for devtools at all if the hash key wasn't changed (like how the lantern tests work), but now I'm seeing it should've been run with a mismatched old DevTools, so I have the same question as you.

[connorjclark]

I wish we could also cache on some hash of devtools files to avoid going an entire week without learning about it

Already on it #11417

(are we sure the cached assets fall out after a week no matter what or just a week after they're not used? their policy language makes me think these might last forever given our frequency of runs)

darn! totally right. This is quite important–we should write to disk a file that only changes every weeks (with some time maths or sumthin).

[patrickhulce]

Already on it #11417

I meant a hash of DevTools changes in Chromium like a hypothetical something that would have invalidated our cache when https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2398829/10/front_end/third_party/lighthouse/report-assets/report-generator.js landed. But figuring that out is the same as doing the work that we're caching, so it's moot haha

[TimvdLippe]

@connorjclark The webtests in the repository are running. For example, for https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2398829the CQ run was https://ci.chromium.org/p/devtools-frontend/builders/try/devtools_frontend_linux_blink_light_rel/6883? and step 31 ran the tests. The results are at https://test-results.appspot.com/data/layout_results/devtools_frontend_linux_blink_light_rel/6883/webkit_layout_from_devtools%20%28with%20patch%29/layout-test-results/results.html which includes all LightHouse webtests.

Before I fixed the LightHouse integration with the global hack, the tests were also blocking submission: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2398829/9 is the patchset and https://ci.chromium.org/p/devtools-frontend/builders/try/devtools_frontend_linux_blink_light_rel/6838? the CQ run

[brendankenny]

wdyt about dropping the need for wget?

Sucks to have to install it so some scripts can basically curl some things :)

I don't see any unusual things going on, seems like it would be a simple replacement?