report: audit warnings are not top-level #5270

paulirish · 2018-05-19T01:05:35Z

They're now reported along with the audit.

Examples:

You can see one example of this in the deployment. (But these other warning cases are pretty esoteric so i've created an artifact bundle that reproduces them.)

fixes #5153

patrickhulce

much less noisy 👍 thanks for picking this up! :D

patrickhulce · 2018-05-21T16:35:22Z

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

+    if (!warnings || warnings.length === 0) return auditEl;
+
+    // Add list of warnings or singular warning
+    const warningsEl = this.dom.createChildOf(titleEl, 'div', 'lh-debug lh-debug--warnings');


do we have any cases of warning + explanation? if so, does it still look okish :)

yup! looks ok

here's explanation + warnings + displayValue all at once

patrickhulce · 2018-05-21T16:37:30Z

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

@@ -114,23 +125,6 @@ describe('CategoryRenderer', () => {
    category.description = prevDesc;
  });

-  // TODO(phulce): revisit if top-level warnings approach is too noisy
-  it.skip('renders audits with warnings as failed', () => {


do we want to render an audit with warnings as failed? or keep it in passed?

good policy question!

Let's see. here's the possible warnings we have right now (that are placed into Passed)

I see the argument for these being visible by default, however in the case of timing/decode, there's not much a user can do about them. Either way, to be visible by default, they'd have to be placed in "Failed" and either we keep the green checkmark or fail them. Both are pretty weird.

So I'm comfy with saying that warnings are orthogonal to passing/failing and default report visibility. wdyt?

Yeah I don't think there's much anyone can do about them either, I'm fine to hide especially now that we've got Sentry tracking 'em for Httparchive runs 👍

oh right, I was good with warnings and failures being independent of each other, but I forgot that means that passing audits with warnings would then be collapsed by default. That definitely takes away from the usefulness of warnings since that means they'll essentially never be seen if the audit is marked as passing.

I can kind of see an argument for hiding some warnings normally, not much the user can do about them, but things like "your manifest technically works but you have weird stuff in there that's getting fallbacks" seems worth bringing up even though the audit passed (and even the warnings about lighthouse's shortcomings might be relevant, which is why we made them warnings in the first place e.g. we can't actually verify all your external links or your images are encoded well except for this list of 20 images, which, while you can't make lighthouse fix, you can verify them yourself).

So while I would say that the existence of warnings should be independent of failure state, I would say that it's definitely not independent of whether or not to show them, and marking the audit as failed doesn't seem like a good way to handle that.

My suggestion in #5273 (comment) was to use warnings as a way to get attention to an issue without penalizing the score. So I would definitely prefer for the warning to be visible by default (not collapsed). I understand that warnings may also be used for things outside of the developer's control so collapsing may make more sense. Is this something that can be configured by the audit at runtime, rather than forcing all warnings to behave the same?

I'd also love to see different UI treatment for warnings; not a green check and not a red exclamation. Could it be a yellow/orange exclamation to distinguish it that something's amiss?

Here're some ideas to visually distinguish the orthogonal warnings from audits.

~~I think A is similar to the top-level warning treatment. Probably consider changing the text to dark for legibility.~~

Update: B seems to make things more visible (recommended).

And for warnings that are associated with passed audits (green checkmarks), how about collating them right below the Passed Audits zippy. So those warnings have visibility and draw attention while keeping the passed audits collapsed.

Updated:

Sg. broke this into it's own issue: #5327 ("make sure warnings within Passed Audits are visible")

patrickhulce · 2018-05-21T16:38:20Z

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

-      assert.ok(/Links.*unsafe/.test(warningEls[0].textContent), 'did not add warning text');
+      assert.strictEqual(warningEls.length, sampleResults.runWarnings.length);
+      for (const warningEl of warningEls) {
+        assert.ok(/Links.*unsafe/.test(warningEl.textContent), 'did not add warning text');


shouldn't this just be deleted now that warningEls should be length 0?

patrickhulce · 2018-05-21T16:40:15Z

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

@@ -33,7 +33,9 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
    valueEl.textContent = Util.formatDisplayValue(audit.result.displayValue);

    const descriptionEl = this.dom.find('.lh-metric__description', tmpl);
-    descriptionEl.appendChild(this.dom.convertMarkdownLinkSnippets(audit.result.description));
+    if (audit.result.description) {


a little confused by removing the ? from description but making this conditional... what's up with that?

good catch. it's assumed to be there now.

brendankenny · 2018-05-21T19:59:07Z

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

@@ -114,23 +125,6 @@ describe('CategoryRenderer', () => {
    category.description = prevDesc;
  });

-  // TODO(phulce): revisit if top-level warnings approach is too noisy
-  it.skip('renders audits with warnings as failed', () => {


oh right, I was good with warnings and failures being independent of each other, but I forgot that means that passing audits with warnings would then be collapsed by default. That definitely takes away from the usefulness of warnings since that means they'll essentially never be seen if the audit is marked as passing.

I can kind of see an argument for hiding some warnings normally, not much the user can do about them, but things like "your manifest technically works but you have weird stuff in there that's getting fallbacks" seems worth bringing up even though the audit passed (and even the warnings about lighthouse's shortcomings might be relevant, which is why we made them warnings in the first place e.g. we can't actually verify all your external links or your images are encoded well except for this list of 20 images, which, while you can't make lighthouse fix, you can verify them yourself).

So while I would say that the existence of warnings should be independent of failure state, I would say that it's definitely not independent of whether or not to show them, and marking the audit as failed doesn't seem like a good way to handle that.

brendankenny · 2018-05-21T20:07:22Z

lighthouse-core/audits/viewport.js

    }
    if (Object.keys(parsedProps.invalidValues).length) {
-      warnings.push(`Invalid values found: ${JSON.stringify(parsedProps.invalidValues)}.`);
+      warnings.push(`Invalid values found: ${JSON.stringify(parsedProps.invalidValues)}`);


tests for these will need to be updated (they assert a . in there :)

brendankenny · 2018-05-21T20:09:09Z

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

+      score: 1,
+    };
+    const auditDOM = renderer.renderAudit({id: 'foo', score: 1, result: auditResult});
+    assert.ok(auditDOM.querySelector('.lh-debug--warnings'), 'did not render debug message');


want to test at least that the warning text is in there as well? And probably want a plural/bulleted test in addition

brendankenny · 2018-05-21T20:11:32Z

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

@@ -135,8 +135,7 @@ describe('ReportRenderer', () => {
      const output = renderer.renderReport(sampleResults, container);

      const warningEls = output.querySelectorAll('.lh-run-warnings > ul > li');
-      assert.strictEqual(warningEls.length, 1);
-      assert.ok(/Links.*unsafe/.test(warningEls[0].textContent), 'did not add warning text');
+      assert.strictEqual(warningEls.length, sampleResults.runWarnings.length);


there's already a "no lighthouseRunWarnings" test above this, so probably want to make a const warningResults = Object.assign({}, sampleResults, {runWarnings: ['Warnings are great!']}); or whatever and pass that in

brendankenny · 2018-05-21T20:11:57Z

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

 /**
 * @typedef {{
      id: string,
      score: (number|null),
      weight: number,
      group?: string,
-      result: AuditResultJSON
+      result: LH.Audit.Result


paulirish · 2018-05-23T21:11:16Z

@brendankenny PTAL
@hwikyounglee does this look right to you?

here's a report with a bunch of errors: http://lh-warningsredux0.surge.sh/errors.html Open up Perf > Passed to see more warnings.

FYI Explanations & warnings are now the same font-size as audit title text:

brendankenny

LGTM

brendankenny · 2018-05-24T01:00:13Z

FYI Explanations & warnings are now the same font-size as audit title text:

this is a little hard to read now to me, but fine with whatever is decided. It looks like in the earlier screenshots there were still bullets and indentation, though?

brendankenny · 2018-05-24T01:00:30Z

oh, that one was a single warning, not a multi-warning

paulirish added 3 commits May 18, 2018 17:07

dont emit warnings to toplevel

5f6f86d

renderer side of warnings.

aa249d4

tests: address phulce's skipped test.

05f3922

paulirish requested review from brendankenny, patrickhulce and vinamratasingal-zz as code owners May 19, 2018 01:05

paulirish deployed to PR staging May 19, 2018 01:05 View deployment

patrickhulce reviewed May 21, 2018

View reviewed changes

patrick feeedback.

078c2b0

paulirish deployed to PR staging May 21, 2018 17:19 View deployment

brendankenny requested changes May 21, 2018

View reviewed changes

patrickhulce mentioned this pull request May 21, 2018

SEO - Page is blocked from indexing - robots.txt #5273

Closed

paulirish requested a review from hwikyounglee May 22, 2018 19:55

wip new viz treatment to warnings.

949b591

paulirish mentioned this pull request May 23, 2018

Report: make sure warnings within Passed Audits are visible #5327

Closed

paulirish added 3 commits May 23, 2018 13:55

warnings match visual spec.

836d9ef

byebye lh-debug

f484b62

feedback on tests.

0f0165a

paulirish requested a review from ebidel as a code owner May 23, 2018 21:07

paulirish deployed to PR staging May 23, 2018 21:07 View deployment

paulirish added the 3.0 label May 23, 2018

brendankenny approved these changes May 24, 2018

View reviewed changes

paulirish merged commit 4820a0b into master May 24, 2018

paulirish deleted the warningsredux branch May 24, 2018 01:02

paulirish removed 3.0 labels Dec 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: audit warnings are not top-level #5270

report: audit warnings are not top-level #5270

paulirish commented May 19, 2018 •

edited

Loading

patrickhulce left a comment

patrickhulce May 21, 2018

paulirish May 21, 2018

patrickhulce May 21, 2018

paulirish May 21, 2018

patrickhulce May 21, 2018

brendankenny May 21, 2018

rviscomi May 21, 2018

hwikyounglee May 22, 2018 •

edited

Loading

hwikyounglee May 22, 2018 •

edited

Loading

paulirish May 23, 2018

patrickhulce May 21, 2018

paulirish May 21, 2018

patrickhulce May 21, 2018

paulirish May 21, 2018

brendankenny May 21, 2018

brendankenny May 21, 2018

brendankenny May 21, 2018

brendankenny May 21, 2018

brendankenny May 21, 2018

paulirish commented May 23, 2018

brendankenny left a comment

brendankenny commented May 24, 2018

brendankenny commented May 24, 2018

report: audit warnings are not top-level #5270

report: audit warnings are not top-level #5270

Conversation

paulirish commented May 19, 2018 • edited Loading

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

hwikyounglee May 22, 2018 • edited Loading

Choose a reason for hiding this comment

hwikyounglee May 22, 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

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

paulirish commented May 23, 2018

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny commented May 24, 2018

brendankenny commented May 24, 2018

paulirish commented May 19, 2018 •

edited

Loading

hwikyounglee May 22, 2018 •

edited

Loading

hwikyounglee May 22, 2018 •

edited

Loading