core(lhr): remove debugString, add explantion/errorMessage #5132

patrickhulce · 2018-05-05T17:51:48Z

I am SUPER jazzed about this change. I think it cleans up the intention behind everything A LOT and is extra nice. There is no more debug string exposed at the audit level (manifest-parser still returns a debug string from gatherer but we can clean that up later). Existing debug strings got recategorized into one of

displayValue - for cases like appcache where we just communicated the name of the thing we found
errorMessage - when the message was a true error message/failure case
warnings - for when the message was just a warning. This is the biggest behavioral change in the PR, the warnings are now collected and displayed in the top-level warnings instead of popping the specific audit into failing. I request that you only consider if warnings should be exposed per-audit in the LHR for this PR and not discuss the display mechanism we can always change how the warnings manifest in the report later, so let's just focus on the breaking-LHR-relevant change.
explanation - the default case if the debugString didn't obviously fit into the other 3, these are currently displayed exactly as debugString was before, so no real impact other than name change. I have ideas for how to change the display but we can 🚲 🏠 that another day

NOTE: relies on #5128 and conflicts with #5130

vinamratasingal-zz · 2018-05-05T22:06:09Z

Not sure if my approval here is really needed but I'll give a LGTM anyways :P

paulirish

this is great. really like the semantic cleanup here.

Some of the new displayValue's don't appear to be making their way to the report:

before and after

paulirish · 2018-05-06T05:21:27Z

lighthouse-core/runner.js

+
+        if (audit.warnings.length) {
+          const prefixedWarnings = audit.warnings.map(msg => `${audit.description}: ${msg}`);
+          lighthouseRunWarnings.push(...prefixedWarnings);


all warnings are pushed to the toplevel warnings?

relatedly: i haven't seen a top level warning in the report in a long time. :)

all warnings are pushed to the toplevel warnings?

correct, I figure we can always change this without breaking LHR changes later if we don't like it/prefer to flag the individual audits, buuuuuut as you point out...

relatedly: i haven't seen a top level warning in the report in a long time. :)

agreed I can't remember seeing one naturally, so seems like a good time to try this out

only thing I can think of we might want to change is that the warnings entries are objects like {message: string} in the LHR so we can later add {level: } etc if we want

paulirish · 2018-05-06T05:23:42Z

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

@@ -99,7 +99,8 @@ describe('PerfCategoryRenderer', () => {
    assert.ok(oppSparklineElement.title, 'did not set tooltip on sparkline');
  });

-  it('renders the performance opportunities with a debug string', () => {
+  // TODO(phulce): verify these don't happen anymore
+  it.skip('renders the performance opportunities with a debug string', () => {


in a branch of mine, i handle this case.

cool beans, in this case it's referring to the fact that it should exclusively be an "error" and that an explanation isn't really possible anymore (the error case is covered by the case below)

so, verified?

yes indeed, removed

paulirish · 2018-05-06T05:24:32Z

lighthouse-core/test/results/sample_v2.json

-      "debugString": "Audit error: Required RobotsTxt gatherer did not run.",
+      "displayValue": "",
+      "errorMessage": "Audit error: Required RobotsTxt gatherer did not run.",
+      "warnings": [],


can we leave warnings undefined if there are none?
same thing with displayValue, tbh. :)

yeah sgtm 👍 @brendankenny agree?

@brendankenny agree?

yeah, sounds good

patrickhulce · 2018-05-06T18:00:33Z

Hmmm strange @paulirish they're working fine for me, is this merged with some of your branches perhaps and mixing with the single-use displayValue bug?

patrickhulce · 2018-05-06T18:05:28Z

@paulirish diff is ~100 lines smaller with undefined warnings/displayValue 🎉

paulirish

I dig this. Over to brendan.

brendankenny · 2018-05-07T18:41:56Z

travis/appveyor seem real confused

patrickhulce · 2018-05-07T18:57:52Z

travis/appveyor seem real confused

yeah I don't think they rebuild on change of base, just force pushed

brendankenny

overally the change looks good and I like how much clearer things are for our bug vs their bug vs just a result from the audit :) A few major things for how we handle these different outputs:

it's great to start using displayValue for things that are display values, but it does expose a weakness with how we display them. It's definitely harder to scan across to the value for the audit vs just having it right below
old:

new:

(and it's even wider across when not shrunk by github)
Separating the warnings from the audits seems like a regression from the status quo in most of the warnings cases added here. Having a list of images we can't reencode at the top of the report or (in the case of sample_v2.json) a warning that some target=_blank anchors may be outgoing but we can't tell, seems like noise at the top of the report and is lost as valuable context at the individual audit level (where you probably want to be warned that the audit may have been unable to comprehensively run its test).

Maybe having log levels for warnings make sense, and only things that audits flag as super important get moved to the top-level box, and everything else stays at the audit level?

brendankenny · 2018-05-07T18:57:07Z

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

@@ -99,7 +99,8 @@ describe('PerfCategoryRenderer', () => {
    assert.ok(oppSparklineElement.title, 'did not set tooltip on sparkline');
  });

-  it('renders the performance opportunities with a debug string', () => {
+  // TODO(phulce): verify these don't happen anymore
+  it.skip('renders the performance opportunities with a debug string', () => {


so, verified?

brendankenny · 2018-05-07T18:59:28Z

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

@@ -113,7 +113,8 @@ describe('CategoryRenderer', () => {
    category.description = prevDesc;
  });

-  it('renders audits with debugString as failed', () => {
+  // TODO(phulce): decide what to do about these cases


whats' the decision?

updated the comment. this is what's up for debate re: toplevel warnings

currently the test simply doesn't apply anymore since all warnings are stuck into toplevel for display

brendankenny · 2018-05-07T19:19:13Z

lighthouse-core/audits/seo/robots-txt.js

@@ -182,7 +182,7 @@ class RobotsTxt extends Audit {
    if (!status) {
      return {
        rawValue: false,
-        debugString: 'Lighthouse was unable to download your robots.txt file',
+        explanation: 'Lighthouse was unable to download your robots.txt file',


is this a failure case on our part or the site's?

depends on what you mean by "our part" it could be because the server aborted the request/bad server/etc or it could be because the network failed, either way it seems to belong in explanation instead of errorMessage since there's nothing we would change code-wise, it's expected behavior

I guess this is the nuance we should expose so the user has some recourse (either file a bug on lighthouse or know how to start debugging their site). Really a bug on robots-txt gatherer so fine to leave

brendankenny · 2018-05-07T19:23:13Z

lighthouse-core/audits/dobetterweb/external-anchors-use-rel-noopener.js

-          debugString = 'Lighthouse was unable to determine the destination ' +
-              'of some anchor tags. If they are not used as hyperlinks, ' +
-              'consider removing the _blank target.';
+          // TODO(phulce): make this message better with anchor.outerHTML


old message had a lot more explanation :)

brendankenny · 2018-05-07T19:28:39Z

lighthouse-core/audits/audit.js

@@ -167,7 +166,7 @@ class Audit {
      result.rawValue = true;
    }

-    if (result.error) {
+    if (result.errorMessage) {


time to rework re: #5128 (comment) ?

discussed in person, todo assigned to @brendankenny :)

brendankenny · 2018-05-07T19:41:13Z

some of the TODOs clearly seem like for the future (e.g. debugString in gatherer output), but others seem like the needed to be decided soonish/in this PR? Sorry if i misclassified :)

patrickhulce · 2018-05-07T20:35:33Z

some of the TODOs clearly seem like for the future (e.g. debugString in gatherer output), but others seem like the needed to be decided soonish/in this PR? Sorry if i misclassified :)

yeah mostly just called out for discussion which it has sparked so 👍

only one remaining is in response to your other comments which we just discussed in person

It's definitely harder to scan across to the value for the audit vs just having it right below

We've definitely entered highly subjective territory here because I disagree 😄 Either way, we can always change where we put/color/highlight displayValue later on so we'll pickup discussion of best display value designs in a separate issue.

patrickhulce · 2018-05-07T20:37:45Z

filed #5153 @brendankenny 👍

brendankenny · 2018-05-07T20:39:55Z

we can always change where we put/color/highlight displayValue later on so we'll pickup discussion of best display value designs in a separate issue.

yeah, it was just a comment on the display of displayValue. Out of scope with this PR

brendankenny

warnings/displayValue discussion moved to #5153

My only other concern would be making a warning out of e.g. every image in uses-optimized-images.js, but we can take that up in #5153ish

…ome#5132)

patrickhulce requested review from brendankenny, ebidel, paulirish and vinamratasingal-zz as code owners May 5, 2018 17:51

vinamratasingal-zz approved these changes May 5, 2018

View reviewed changes

paulirish requested changes May 6, 2018

View reviewed changes

paulirish approved these changes May 6, 2018

View reviewed changes

patrickhulce mentioned this pull request May 7, 2018

core(lhr): support null scores #5128

Merged

core(lhr): remove debugString, add explantion/errorMessage

d56f04f

patrickhulce force-pushed the no_debug_string branch from fc9a3cb to c5d522e Compare May 7, 2018 18:26

patrickhulce changed the base branch from null_scores to master May 7, 2018 18:27

fixes

5757ed4

patrickhulce force-pushed the no_debug_string branch from c5d522e to 5757ed4 Compare May 7, 2018 18:57

brendankenny requested changes May 7, 2018

View reviewed changes

feedback/cleanup

1f69616

patrickhulce mentioned this pull request May 7, 2018

Redesign displayValue/warnings #5153

Closed

added todo

c14c6f6

paulirish mentioned this pull request May 7, 2018

[meta] 2018 I/O Burndown #5008

Closed

82 tasks

brendankenny approved these changes May 7, 2018

View reviewed changes

Merge branch 'master' into no_debug_string

ce50e27

brendankenny merged commit 2c473e7 into master May 7, 2018

brendankenny deleted the no_debug_string branch May 7, 2018 22:20

kdzwinel pushed a commit to kdzwinel/lighthouse that referenced this pull request Aug 16, 2018

core(lhr): remove debugString, add explantion/errorMessage (GoogleChr…

a63fb43

…ome#5132)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(lhr): remove debugString, add explantion/errorMessage #5132

core(lhr): remove debugString, add explantion/errorMessage #5132

patrickhulce commented May 5, 2018 •

edited by paulirish

Loading

vinamratasingal-zz commented May 5, 2018

paulirish left a comment

paulirish May 6, 2018

patrickhulce May 6, 2018 •

edited

Loading

paulirish May 6, 2018

patrickhulce May 6, 2018 •

edited

Loading

brendankenny May 7, 2018

patrickhulce May 7, 2018

paulirish May 6, 2018

patrickhulce May 6, 2018

brendankenny May 7, 2018 •

edited

Loading

patrickhulce commented May 6, 2018 •

edited

Loading

patrickhulce commented May 6, 2018

paulirish left a comment

brendankenny commented May 7, 2018

patrickhulce commented May 7, 2018

brendankenny left a comment

brendankenny May 7, 2018

brendankenny May 7, 2018

patrickhulce May 7, 2018

brendankenny May 7, 2018

patrickhulce May 7, 2018

brendankenny May 7, 2018

brendankenny May 7, 2018

patrickhulce May 7, 2018

brendankenny May 7, 2018

patrickhulce May 7, 2018

brendankenny commented May 7, 2018

patrickhulce commented May 7, 2018

patrickhulce commented May 7, 2018

brendankenny commented May 7, 2018

brendankenny left a comment

core(lhr): remove debugString, add explantion/errorMessage #5132

core(lhr): remove debugString, add explantion/errorMessage #5132

Conversation

patrickhulce commented May 5, 2018 • edited by paulirish Loading

vinamratasingal-zz commented May 5, 2018

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce May 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce May 6, 2018 • 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

Choose a reason for hiding this comment

brendankenny May 7, 2018 • edited Loading

Choose a reason for hiding this comment

patrickhulce commented May 6, 2018 • edited Loading

patrickhulce commented May 6, 2018

paulirish left a comment

Choose a reason for hiding this comment

brendankenny commented May 7, 2018

patrickhulce commented May 7, 2018

brendankenny 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

brendankenny commented May 7, 2018

patrickhulce commented May 7, 2018

patrickhulce commented May 7, 2018

brendankenny commented May 7, 2018

brendankenny left a comment

Choose a reason for hiding this comment

patrickhulce commented May 5, 2018 •

edited by paulirish

Loading

patrickhulce May 6, 2018 •

edited

Loading

patrickhulce May 6, 2018 •

edited

Loading

brendankenny May 7, 2018 •

edited

Loading

patrickhulce commented May 6, 2018 •

edited

Loading