misc(build): create error-y LHR for the deploy #9283

paulirish · 2019-06-25T22:10:28Z

will aid in looking at some of @connorjclark's recent PRs.

paulirish · 2019-06-25T22:38:34Z

@connorjclark this one doesn't add any runWarnings.... it probably should. got a suggestion for what to add in there?

connorjclark · 2019-06-25T23:26:07Z

"Something went wrong with recording the trace over your page load. Please run Lighthouse again. (NO_FCP)"

connorjclark · 2019-06-25T23:26:17Z

a classic

patrickhulce · 2019-06-26T01:33:33Z

got a suggestion for what to add in there?

We can just throw any string into the LighthouseRunWarnings artifact, right?

patrickhulce

LGTM!

lighthouse-core/scripts/build-report-for-autodeployment.js

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

googlebot · 2019-06-26T15:39:12Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

paulirish · 2019-06-26T15:39:39Z

We can just throw any string into the LighthouseRunWarnings artifact, right?

yup. just wanted something realistic. ;)

brendankenny

This seems like it could be pretty fragile. What do you think about checking in error artifacts? If the pageLoadError-defaultPass trace and devtoolsLog are deleted, they should be quite small.

brendankenny · 2019-06-26T16:20:38Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

@@ -220,6 +220,8 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
      element.appendChild(groupEl);
    }

+    // TODO: Handle passed w/ warnings in a unique clump like non-perf categories do


This is ~~chicken nuggets~~ an issue

delete then? it doesn't really have anything to do with this PR and is redundant with that PR

lighthouse-core/scripts/build-report-for-autodeployment.js

brendankenny · 2019-06-26T16:25:06Z

lighthouse-core/scripts/build-report-for-autodeployment.js

+
+/**
+ * Generate an LHR with errors for the renderer to display
+ * We'll write an "empty" artifacts file to disk, only to use it in auditMode


Needs periods

brendankenny · 2019-06-26T16:26:48Z

lighthouse-core/scripts/build-report-for-autodeployment.js

+    },
+  };
+  // @ts-ignore driver isn't mocked out completely
+  const artifacts = await GatherRunner.initializeBaseArtifacts(options);


One downside here is that ts-ignore is going to make this invisible to any breaking changes made to initializeBaseArtifacts, and since it's an internal method that could pretty easily happen

how about throwing a ts-ignore/cast on the driver object instead?

brendankenny · 2019-06-26T16:27:46Z

lighthouse-core/scripts/build-report-for-autodeployment.js

+  mkdirp(TMP);
+  fs.writeFileSync(`${TMP}/artifacts.json`, JSON.stringify(artifacts), 'utf-8');
+  const errorRunnerResult = await lighthouse(url, {auditMode: TMP});
+  const errorLhr = /** @type {LH.RunnerResult} */ (errorRunnerResult).lhr;


Why does this need a cast?

because lighthouse can return RunnerResult | undefined (i think because of -G?)

because lighthouse can return RunnerResult | undefined (i think because of -G?)

ah, oh right. I prefer the real undefined check over the cast personally, but not a big deal in this case

paulirish · 2019-06-27T01:15:37Z

What do you think about checking in error artifacts?

I was thinking that (and i've done something like that locally for months) but calling GatherRunner.initializeBaseArtifacts gives me the same thing, without as much maintenance cost.

Not sure if you were suggesting it, but also adding that reverse engineering how to structure artifacts to get these warnings also is not fun.

brendankenny · 2019-06-26T23:58:25Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

@@ -220,6 +220,8 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
      element.appendChild(groupEl);
    }

+    // TODO: Handle passed w/ warnings in a unique clump like non-perf categories do


delete then? it doesn't really have anything to do with this PR and is redundant with that PR

brendankenny · 2019-06-27T00:01:12Z

lighthouse-core/scripts/build-report-for-autodeployment.js

+  mkdirp(TMP);
+  fs.writeFileSync(`${TMP}/artifacts.json`, JSON.stringify(artifacts), 'utf-8');
+  const errorRunnerResult = await lighthouse(url, {auditMode: TMP});
+  const errorLhr = /** @type {LH.RunnerResult} */ (errorRunnerResult).lhr;


because lighthouse can return RunnerResult | undefined (i think because of -G?)

ah, oh right. I prefer the real undefined check over the cast personally, but not a big deal in this case

brendankenny · 2019-06-27T01:16:24Z

lighthouse-core/scripts/build-report-for-autodeployment.js

+  const artifacts = await GatherRunner.initializeBaseArtifacts(options);
+  // Add in a global runWarning
+  artifacts.LighthouseRunWarnings.push(`Something went wrong with recording the trace over your
+  page load. Please run Lighthouse again. (NO_FCP)`);


What do you think about checking in error artifacts? If the pageLoadError-defaultPass trace and devtoolsLog are deleted, they should be quite small.

could just replace all of the above with

/** @type {LH.BaseArtifacts} */ const artifacts = { fetchTime: '2019-06-26T23:56:58.381Z', LighthouseRunWarnings: [ `Something went wrong with recording the trace over your page load. Please run Lighthouse again. (NO_FCP)`, // eslint-disable-line max-len ], TestedAsMobileDevice: true, HostUserAgent: 'Mozilla/5.0 ErrorUserAgent Chrome/66', NetworkUserAgent: 'Mozilla/5.0 ErrorUserAgent Chrome/66', BenchmarkIndex: 1000, WebAppManifest: null, Stacks: [], settings: defaultSettings, URL: { requestedUrl: 'http://fakeurl.com', finalUrl: 'http://fakeurl.com', }, Timing: [], PageLoadError: null, devtoolsLogs: {}, traces: {}, };

and then continue as below. Only a little bit longer and the type assertion keeps everything in sync.

brendankenny

Nice! LGTM

googlebot added the cla: yes label Jun 25, 2019

paulirish requested a review from connorjclark June 25, 2019 22:10

connorjclark approved these changes Jun 25, 2019

View reviewed changes

paulirish changed the base branch from deployvariants to master June 25, 2019 22:23

misc(build): create error-y LHR for the deploy

efd41c9

paulirish force-pushed the deployvariant-error branch from 11bc9cd to efd41c9 Compare June 25, 2019 22:23

vercel bot deployed to staging June 25, 2019 22:23 View deployment

patrickhulce approved these changes Jun 26, 2019

View reviewed changes

lighthouse-core/scripts/build-report-for-autodeployment.js Outdated Show resolved Hide resolved

add runwarning

10d1e03

vercel bot deployed to staging June 26, 2019 15:36 View deployment

Update lighthouse-core/scripts/build-report-for-autodeployment.js

f36c343

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

googlebot added cla: no and removed cla: yes labels Jun 26, 2019

vercel bot deployed to staging June 26, 2019 15:39 View deployment

paulirish added 2 commits June 26, 2019 09:05

add in audit warnings, too

fbf2c9f

less obj.assign for stronger typing

9245c3e

vercel bot deployed to staging June 26, 2019 16:11 View deployment

brendankenny requested changes Jun 26, 2019

View reviewed changes

some feedback

3bc962f

vercel bot deployed to staging June 26, 2019 23:45 View deployment

paulirish added 3 commits June 26, 2019 18:11

cast the driver instead

1e28415

lint

68f6736

Merge branch 'master' into deployvariant-error

de55da5

vercel bot deployed to staging June 27, 2019 01:14 View deployment

brendankenny reviewed Jun 27, 2019

View reviewed changes

drop other cast

aa1b39b

vercel bot deployed to staging June 27, 2019 01:17 View deployment

feedback

76920df

vercel bot deployed to staging June 28, 2019 00:36 View deployment

paulirish added cla: yes and removed cla: no labels Jun 28, 2019

This comment has been minimized.

Sign in to view

brendankenny approved these changes Jun 28, 2019

View reviewed changes

paulirish merged commit 0da300a into master Jun 29, 2019

paulirish deleted the deployvariant-error branch June 29, 2019 07:02

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc(build): create error-y LHR for the deploy #9283

misc(build): create error-y LHR for the deploy #9283

This comment has been minimized.

misc(build): create error-y LHR for the deploy #9283

misc(build): create error-y LHR for the deploy #9283

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment