core(fr): add base fraggle rock snapshot runner by patrickhulce · Pull Request #11748 · GoogleChrome/lighthouse

patrickhulce · 2020-12-02T18:39:39Z

Summary
Builds on the work of #11623 #11633 #11685 #11742 to add the base fraggle rock runner for snapshot mode. A very basic form of snapshot mode is now runnable with this PR. See the end-to-end puppeteer tests for the example.

const puppeteer = require('puppeteer')
const {snapshot} = require('lighthouse/lighthouse-core/fraggle-rock/api.js')
 
const browser = await puppeteer.launch()
const page = await browser.newPage()
await page.goto('http://example.com')
await page.click('body')
const {lhr, artifacts} = await snapshot({page})
console.log('A11y score was', lhr.categories.accessibility.score)

Related Issues/PRs
See #11622 and #11313 for the larger context.

connorjclark

quick pass but this LGTM

connorjclark · 2020-12-02T18:50:22Z

@@ -227,6 +232,7 @@ if (require.main === module) {
  console.log(`offline: listening on http://localhost:${offlinePort}`);


one day we should split this off into a smoke-static-server-entry.js or something ..

brendankenny · 2020-12-03T21:14:55Z

        options?: {};
      } | {
-        implementation: typeof Gatherer;
+        implementation: ClassOf<Gatherer.GathererInstance>;


What does changing this do? typeof Gatherer already is a ClassOf<Gatherer.GathererInstance>

see #11748 (comment)

brendankenny · 2020-12-03T21:18:05Z

+    type PhaseResult_ = void|LH.GathererArtifacts[keyof LH.GathererArtifacts]
+    export type PhaseResult = PhaseResult_ | Promise<PhaseResult_>
+
+    export interface GathererInstance {


same here. The js class is the base class and the interface. Duplicating it here doesn't seem to do more with the type and adds to maintenance/distancing types from jsdocs, etc

Decoupling the implementation and the interface is the goal here to distinguish the new from the old and differentiate which gatherers support which models of execution in a relatively trivial and lightweight way.

Is there a counter proposal in there I'm missing for how incremental transition for gatherers will be supported? I'm happy to move the docs to the interface if that's what you're asking.

brendankenny · 2020-12-03T21:19:02Z

+        implementation: ClassOf<Gatherer.GathererInstance>;
        options?: {};
      } | {
-        instance: InstanceType<typeof Gatherer>;


this is working around an old bug and should be able to just be instance: Gatherer, fwiw

brendankenny · 2020-12-03T21:25:51Z

+const puppeteer = require('puppeteer');
+const StaticServer = require('../../../lighthouse-cli/test/fixtures/static-server.js').Server;
+
+jest.setTimeout(90 * 1000);


90_000 y'all, step into the future :)

brendankenny · 2020-12-03T21:29:23Z

@@ -0,0 +1,84 @@
+/**


if this ends up expanding to a bunch of new smoke tests, are you thinking about moving them somewhere else/called somehow else instead of in the midst of all the (mostly) unit tests?

called somehow else, yes definitely 👍

moved somewhere else, no wasn't planning on it.

brendankenny · 2020-12-03T21:43:48Z

+        artifacts[artifactName] = artifact;
+      }
+
+      return /** @type {LH.Artifacts} */ ({...baseArtifacts, ...artifacts}); // Cast to drop Partial<>


I assume you're already thinking about how these types are mostly useless (sometimes worse than useless) in a world where we can't usually assume they're all actually there...

Important to note this section was just copied from our existing gathering so it's not a degradation :)

I have not invested significant time on ideating how to improve the types here, no. I don't really consider the pre-existing temporary lie of LH.Artifacts at the runner level to be high on the list of type migrations that need to be done to get FR off the ground. Do you think this type flaw we have today is much more important in the FR world to warrant addressing soon?

Co-authored-by: Brendan Kenny <bckenny@gmail.com>

adamraine · 2020-12-07T18:39:00Z

+ * @param {LH.Gatherer.GathererInstance} gatherer
+ * @return {gatherer is LH.Gatherer.FRGathererInstance}
+ */
+function isFRGatherer(gatherer) {


Are all FR gatherers going to support snapshot mode?

No, not all, see full compat table for which ones, but the next step after #11759 is the TODO that's here about using gatherer.meta to annotate whether a gatherer supports it or not.

The name isFRGatherer makes it sound like this will detect FR gatherers, but it's used to detect snapshot support on L60. WDYT of renaming this to something like isFRSnapshotGatherer?

The name isFRGatherer makes it sound like this will detect FR gatherers

Excellent, that's exactly what it's supposed to be doing :) purely a type guard on whether a gatherer instance supports the FR gatherer properties.

but it's used to detect snapshot support on L60

I consider this function not detecting snapshot support at all which is why the TODO is there. Any suggestion for alternative wording there to make it clearer? Snapshot detection support is 2-3 PRs away from being fixed and won't affect any results in the meantime.

I think this issue has more to do with me misunderstanding the comment on L59 than the comment's wording. It's fine to leave as is :)

core(fr): add base fraggle rock snapshot runner

14fdb68

patrickhulce requested a review from a team as a code owner December 2, 2020 18:39

patrickhulce requested review from adamraine and removed request for a team December 2, 2020 18:39

google-cla Bot added the cla: yes label Dec 2, 2020

devtools-bot assigned adamraine Dec 2, 2020

devtools-bot added the waiting4reviewer label Dec 2, 2020

connorjclark reviewed Dec 2, 2020

View reviewed changes

feedback

a79b8a7

vercel Bot deployed to Preview December 2, 2020 21:35 View deployment

patrickhulce mentioned this pull request Dec 3, 2020

core(config): refactor config cloning for fraggle rock #11759

Merged

brendankenny reviewed Dec 3, 2020

View reviewed changes

Update lighthouse-core/fraggle-rock/api.js

7525e87

Co-authored-by: Brendan Kenny <bckenny@gmail.com>

vercel Bot deployed to Preview December 7, 2020 16:54 View deployment

future

a7a860d

vercel Bot deployed to Preview December 7, 2020 16:55 View deployment

adamraine approved these changes Dec 7, 2020

View reviewed changes

bump estarget for typescript API recognition

13ba78c

vercel Bot deployed to Preview December 7, 2020 19:37 View deployment

patrickhulce removed the waiting4reviewer label Dec 8, 2020

patrickhulce merged commit 80b9db0 into master Dec 8, 2020

patrickhulce deleted the fr_runner branch December 8, 2020 23:13

patrickhulce mentioned this pull request Jan 11, 2021

Flow Support (Fraggle Rock) #11313

Closed

		@@ -227,6 +232,7 @@ if (require.main === module) {
		console.log(`offline: listening on http://localhost:${offlinePort}`);

Conversation

patrickhulce commented Dec 2, 2020

Uh oh!

connorjclark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants