core: record top-level warnings in LHR and display in report #3692

brendankenny · 2017-10-30T19:36:16Z

Adds a LighthouseRunWarnings artifact in gather-runner and a lighthouseRunWarnings in runner (which LighthouseRunWarnings is folded into when gather-runner is complete) to collect top-level warnings worthy of bringing to the user's attention (e.g. ran in Headless which doesn't support throttling). For now only gather-runner and runner can add to these, but should be easy to extend if we want to e.g. let gatherers or audits emit warnings they think are worthy of floating to the top. Happy to bikeshed on naming and exact flow of warning strings.

~~steals the styling in the HTML report from #2920, but not remotely attached to it :)~~

closes #2920 by adding a warning when running in Headless

brendankenny · 2017-10-30T19:39:58Z

lighthouse-core/gather/gather-runner.js

@@ -301,6 +319,9 @@ class GatherRunner {
  static collectArtifacts(gathererResults) {
    const artifacts = {};

+    // Nest LighthouseRunWarnings, if any, so they will be collected into artifact.
+    gathererResults.LighthouseRunWarnings = [gathererResults.LighthouseRunWarnings || []];


this ends up a bit weird. gatherResults expects an array of results for [beforePass, pass, afterPass] , the winner of which is selected just below. For adding warnings, however, that means everywhere a warning is added would have to do gathererResults.LighthouseRunWarnings[0].push('...'). This cheats it by keeping gathererResults.LighthouseRunWarnings a top level array everywhere (so it's just gathererResults.LighthouseRunWarnings.push('...')) and then nesting it into an array so it can be processed here

brendankenny · 2017-10-30T19:40:32Z

lighthouse-core/report/v2/renderer/report-renderer.js

+   */
+  _renderReportWarnings(report) {
+    if (!report.lighthouseRunWarnings || report.lighthouseRunWarnings.length === 0) {
+      return null;


alternatively this could return an empty div so the caller wouldn't need to check the results

I suppose I have a slight preference for empty div, but I don't feel terribly strongly if this is a pattern we can expect to employ elsewhere as well (I can't think of any off the top of my head, but maybe you borrowed from somewhere?)

I have a slight preference for empty div

done

patrickhulce

hurray finally a mechanism for top level warnings! :D

what do the expected flows look like for adding items to the warnings from audits and gatherers?

from audits: depend on the RunWarnings artifact and mutate it? feels a bit gross but super easy
from gatherers: add runWarnings to passData and give to gatherer afterPass?

patrickhulce · 2017-10-30T20:25:30Z

lighthouse-core/gather/gather-runner.js

@@ -301,6 +319,9 @@ class GatherRunner {
  static collectArtifacts(gathererResults) {
    const artifacts = {};

+    // Nest LighthouseRunWarnings, if any, so they will be collected into artifact.
+    gathererResults.LighthouseRunWarnings = [gathererResults.LighthouseRunWarnings || []];


shouldn't gathererResults.LighthouseRunWarnings always be set? if not, do we need a safeguard in warnOnHeadless too

shouldn't gathererResults.LighthouseRunWarnings always be set?

it should always be set, I was thinking of runner when I added that :)

patrickhulce · 2017-10-30T20:29:24Z

lighthouse-core/report/v2/renderer/report-renderer.js

@@ -202,6 +227,7 @@ ReportRenderer.GroupJSON; // eslint-disable-line no-unused-expressions
 *     timing: {total: number},
 *     initialUrl: string,
 *     url: string,
+ *     lighthouseRunWarnings: !Array<string>,


not sure how we should be modeling extensions to the report JSON here, the latest renderer needs to render older versions that might be missing runWarnings, which you account for, but it is nice to have the closure annotations match the current invariants

thoughts?

yeah, it should be optional. Fixed

patrickhulce · 2017-10-30T20:30:10Z

lighthouse-core/runner.js

@@ -150,6 +157,7 @@ class Runner {
          generatedTime: (new Date()).toJSON(),
          initialUrl: opts.initialUrl,
          url: opts.url,
+          lighthouseRunWarnings,


🚲 🏠 can we ditch the lighthouse prefix, it's the LHR so hopefully everything is lighthouse related

don't want to end up in a classic lighthouse-core, lighthouse-cli, lighthouse-* situation

patrickhulce · 2017-10-30T20:32:05Z

lighthouse-core/report/v2/report-styles.css

+  line-height: var(--header-line-height);
+}
+.lh-run-warnings::before {
+  display: inline-block;


~~interesting, what for?~~ oh are you reusing an icon from some other class?

oh are you reusing an icon from some other class?

yeah, the styling from lh-debug. Stolen from Eric's #2920

patrickhulce · 2017-10-30T20:34:19Z

lighthouse-core/report/v2/renderer/report-renderer.js

+   */
+  _renderReportWarnings(report) {
+    if (!report.lighthouseRunWarnings || report.lighthouseRunWarnings.length === 0) {
+      return null;


I suppose I have a slight preference for empty div, but I don't feel terribly strongly if this is a pattern we can expect to employ elsewhere as well (I can't think of any off the top of my head, but maybe you borrowed from somewhere?)

patrickhulce · 2017-10-30T20:35:37Z

lighthouse-core/gather/gather-runner.js

+   * @param {!GathererResults} gathererResults
+   * @return {!Promise<undefined>}
+   */
+  static warnOnHeadless(driver, gathererResults) {


nit: we just computed UserAgent the step before this, so could eliminate driver dep

brendankenny · 2017-10-30T22:21:53Z

what do the expected flows look like for adding items to the warnings from audits and gatherers?

I actually hadn't thought about gatherers hard enough. passData might make sense, though it's somewhat unfortunate for beforePass and pass not to have access. Not sure of other methods to get data out of a gatherer.

I was thinking the LighthouseRunWarnings artifact should be immutable after gathering is finished, which would allow GAR to recreate warnings correctly, for instance. Audits could just have yet another field in AuditResult to issue top-level warnings (hopefully rarely) and merged within runner to its local copy, not the artifact.

A separate idea from all of this would be some new logger method which would allow it to be stored somewhere. Runner could start and then stop recording, and append any collected super logs into runWarnings. This is nice since logger is already global and a singleton and we don't need yet another injected thing, and it is a kind of logging, but OTOH it's not reentrant and kind of ugly, so I decided to not pursue.

patrickhulce

lgtm!

patrickhulce · 2017-10-30T22:20:59Z

lighthouse-core/report/v2/templates.html

+<!-- Lighthouse run warnings -->
+<template id="tmpl-lh-run-warnings">
+  <div class="lh-run-warnings lh-debug">
+    <b>There were issues affecting this run of Lighthouse:</b>


is strong not the hip young tag it once was? 😆

vinamratasingal-zz

Brendan- small nit on UX. Can we make the error section of the report a box with a light yellow background? This will help separate the content out for end user. I also think having it red font makes it a bit scary, so changing it to a non-red color would be helpful. I can throw together a quick mock if that's useful.

Also- what's the product behavior with surfacing errors. How many errors can surface at a time, is there anyway that we can help prioritize, and do we want to have an opinion on how many errors to flash/what gets shown here?

brendankenny · 2017-10-30T22:52:42Z

updated styling:

vinamratasingal-zz

LGTM, thanks Brendan!

brendankenny · 2017-10-31T21:38:46Z

@paulirish if you want in

…hrome#3692)

brendankenny requested review from patrickhulce, paulirish and vinamratasingal-zz as code owners October 30, 2017 19:36

brendankenny commented Oct 30, 2017

View reviewed changes

brendankenny force-pushed the squirrelly branch from a00a06a to 554444d Compare October 30, 2017 19:49

patrickhulce reviewed Oct 30, 2017

View reviewed changes

patrickhulce added the waiting4committer label Oct 30, 2017

patrickhulce approved these changes Oct 30, 2017

View reviewed changes

brendankenny added waiting4reviewer and removed waiting4committer labels Oct 30, 2017

patrickhulce assigned paulirish and vinamratasingal-zz Oct 30, 2017

vinamratasingal-zz reviewed Oct 30, 2017

View reviewed changes

vinamratasingal-zz added waiting4committer and removed waiting4reviewer labels Oct 30, 2017

vinamratasingal-zz approved these changes Oct 30, 2017

View reviewed changes

vinamratasingal-zz added waiting4reviewer and removed waiting4committer labels Oct 30, 2017

brendankenny added 6 commits October 31, 2017 11:31

core: record top-level warnings in LHR and display in report

2cc37c8

add performance warning when running in Headless Chrome

7068f61

feedback

02cc020

STRONG

0a2d942

update report styling

68d895b

closure fix

d8a8c1f

brendankenny force-pushed the squirrelly branch from 4d23f41 to d8a8c1f Compare October 31, 2017 18:34

brendankenny unassigned vinamratasingal-zz Oct 31, 2017

paulirish approved these changes Oct 31, 2017

View reviewed changes

paulirish merged commit 6e8db91 into master Oct 31, 2017

paulirish deleted the squirrelly branch October 31, 2017 23:27

patrickhulce mentioned this pull request Nov 13, 2017

Way to surface non-critical errors in the report #1512

Closed

christhompson pushed a commit to christhompson/lighthouse that referenced this pull request Nov 28, 2017

core: record top-level warnings in LHR and display in report (GoogleC…

56878c8

…hrome#3692)

dependencies bot mentioned this pull request Dec 17, 2017

Update lighthouse in / from 2.5.0 to 2.7.0 chauncey-garrett/dotfiles#57

Open

paulirish removed the waiting4reviewer label Mar 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: record top-level warnings in LHR and display in report #3692

core: record top-level warnings in LHR and display in report #3692

brendankenny commented Oct 30, 2017 •

edited

Loading

brendankenny Oct 30, 2017

brendankenny Oct 30, 2017

patrickhulce Oct 30, 2017

brendankenny Oct 30, 2017

patrickhulce left a comment

patrickhulce Oct 30, 2017

brendankenny Oct 30, 2017

patrickhulce Oct 30, 2017

brendankenny Oct 30, 2017

patrickhulce Oct 30, 2017

brendankenny Oct 30, 2017

patrickhulce Oct 30, 2017

brendankenny Oct 30, 2017

patrickhulce Oct 30, 2017

patrickhulce Oct 30, 2017

brendankenny Oct 30, 2017

brendankenny commented Oct 30, 2017

patrickhulce left a comment

patrickhulce Oct 30, 2017

vinamratasingal-zz left a comment

brendankenny commented Oct 30, 2017

vinamratasingal-zz left a comment

brendankenny commented Oct 31, 2017

core: record top-level warnings in LHR and display in report #3692

core: record top-level warnings in LHR and display in report #3692

Conversation

brendankenny commented Oct 30, 2017 • edited Loading

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

patrickhulce left a 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

brendankenny commented Oct 30, 2017

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinamratasingal-zz left a comment

Choose a reason for hiding this comment

brendankenny commented Oct 30, 2017

vinamratasingal-zz left a comment

Choose a reason for hiding this comment

brendankenny commented Oct 31, 2017

brendankenny commented Oct 30, 2017 •

edited

Loading