core(lhr): support null scores #5128

patrickhulce · 2018-05-05T00:04:11Z

followup to #5105, add support for null scores and forces all errors/informative/manual/not-applicable to be null

when category errors, the gauge shows '?' ~~I'll try to find a screenshot~~ :)

paulirish

love this one. some small ideas but on the whole it's feelin great

paulirish · 2018-05-05T19:14:37Z

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

+        return false;
+      case 'numeric':
+      case 'binary':
+        return audit.score === 1;


did you want to handle the case of numeric audits pass if above PASS_THRESHOLD?

yeah good call 👍

paulirish · 2018-05-05T19:15:02Z

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

+      case 'not-applicable':
+        return true;
+      case 'error':
+      case 'informative':


why do all informative audits fail?

so they show up as failing, it actually fits perfectly with our new scoreDisplayMode categories. if an informative audit has nothing to tell you it marks itself as not-applicable ;)

should record this idea somewhere (to the lhr/lhr-lite description of these, maybe?)

It's more an audit author concern since it's an immediate conclusion from the scoreDisplayMode that if an audit isn't not-applicable it means it was applicable :)

maybe in the custom audit docs?

paulirish · 2018-05-05T19:16:51Z

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

-    const perfCategory = sampleResults.reportCategories.find(cat => cat.id === 'performance');
-    const categoryDOM2 = renderer.render(perfCategory, sampleResults.reportGroups);
-    assert.ok(!categoryDOM2.querySelector('.lh-audit-group--notapplicable'));
+    const bestPracticeCat = sampleResults.reportCategories.find(cat => cat.id === 'best-practices');


just curious, why switching to BP? ah because usertimings is now n/a.

paulirish · 2018-05-05T20:27:45Z

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

@@ -275,12 +274,12 @@ class CategoryRenderer {
        group = auditsUngrouped;
      }

-      if (audit.result.scoreDisplayMode === 'not-applicable') {
-        group.notApplicable.push(audit);
-      } else if (audit.result.score === 1 && !audit.result.debugString) {


that logic on debugString seems rather incorrect tbh. glad it's gone.

paulirish · 2018-05-05T20:33:06Z

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

@@ -17,7 +17,8 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
    const element = this.dom.find('.lh-metric', tmpl);
    element.id = audit.result.name;
    // FIXME(paulirish): currently this sets a 'lh-metric--fail' class on error'd audits


can you also address this?

i'd prefer do not set a --fail class on these audits.

(I know i wrote it and put my own name. haha. So i'm asking super nicely with a cherry on top. :)

paulirish · 2018-05-05T20:34:13Z

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

-    this.dom.find('.lh-gauge__percentage', tmpl).textContent = scoreOutOf100;
+    const scoreOutOf100 = Math.round(numericScore * 100);
+    this.dom.find('.lh-gauge__percentage', tmpl).textContent = category.score === null ?
+      '?' : scoreOutOf100;


can you also apply a title attribute to that explains why its ?

done, text suggestion welcome right now "Errors occurred while auditing"

paulirish · 2018-05-05T20:36:01Z

lighthouse-core/scoring.js

   */
  static arithmeticMean(items) {
+    // If there is 1 null score, return a null average
+    if (items.some(item => item.score === null && item.weight !== 0)) return null;


wdyt about an early filter in this method to get rid of the weight===0 items. then we don't need the check in here PLUS L32.
ndb, tho

yeah sure sg 👍

paulirish · 2018-05-05T20:37:31Z

lighthouse-core/report/report-generator.js

@@ -73,7 +73,7 @@ class ReportGenerator {
      return category.audits.map(catAudit => {
        const audit = lhr.audits[catAudit.id];
        return [category.name, audit.name, audit.description, audit.scoreDisplayMode, audit.score]
-          .map(value => value.toString())
+          .map(value => value === null ? '0' : value.toString())


i dont know what the CSV consumers want but i feel like getting 0 is a little weird. empty string seems slightly better.

csv-validator requires that all values in the column are numbers (I tried 'null' as string originally), I have no preference here we can ditch csv-validator if we want to deviate?

cc @nourikhalass in case you have a preference for how null scores show up in CSV

I agree with @paulirish that 0 is a bit weird so empty string would be better. But as @patrickhulce says: csv-validator does not play along with that. Maybe its better to use -1 or omit the result entirely if the score is null.

I'll go with -1 for now since you're our primary CSV consumer 👍, PR welcome in the future if you think of a better canonical CSV-way to represent this :)

obvious other alternative is just stop validating the scores are numeric, how do you feel about that?

I think that's fine since the validation library leaves a lot to be desired. Maybe in the future I'll look at a better / more robust way of validating the CSV . So I think you'll be OK if you don't validate that scores are numeric.

Also thanks for including me in the loop!

paulirish · 2018-05-06T21:31:23Z

@brendankenny you wanna take a look?

brendankenny · 2018-05-06T23:45:31Z

looks like manual expectations need updating

brendankenny · 2018-05-06T23:55:28Z

lighthouse-core/audits/audit.js

@@ -162,18 +163,26 @@ class Audit {

    // If the audit was determined to not apply to the page, we'll reset it as informative only


this comment seems out of date now

brendankenny · 2018-05-07T00:01:34Z

lighthouse-core/audits/audit.js

      scoreDisplayMode = Audit.SCORING_MODES.NOT_APPLICABLE;
      result.rawValue = true;
    }

+    if (result.error) {


is it possible to rework this code to be a flow through a set of mutually exclusive states so it's easy to see the possibilities?

e.g. score is null, if error, it's always scoring mode error, else if it's been marked notApplicable, it's mode notApplicable, then manual, and then only if it's binary or numeric is a score calculated?

description seems like it might not fit the mold, but then maybe it's worth calling separately if it sits across the display modes differently than score does

this gets reworked immediately as part of #5132, fair to take up there?

fair to take up there?

yeah, SG

brendankenny · 2018-05-07T00:04:18Z

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

@@ -228,14 +225,21 @@ class CategoryRenderer {
    wrapper.href = `#${category.id}`;
    wrapper.classList.add(`lh-gauge__wrapper--${Util.calculateRating(category.score)}`);

+    const numericScore = Number(category.score);


should call out casting null to 0 here in a comment

brendankenny · 2018-05-07T00:05:03Z

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

@@ -259,11 +263,11 @@ class CategoryRenderer {
      notApplicable: !Array<!ReportRenderer.AuditJSON>}>} */ ({});
    const auditsUngrouped = {passed: [], failed: [], notApplicable: []};

-    nonManualAudits.forEach(audit => {
+    nonManualAudits.forEach(auditRef => {


brendankenny · 2018-05-07T00:07:26Z

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

+      case 'not-applicable':
+        return true;
+      case 'error':
+      case 'informative':


should record this idea somewhere (to the lhr/lhr-lite description of these, maybe?)

brendankenny · 2018-05-07T00:09:55Z

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

+   * @param {{score: (number|null), scoreDisplayMode: string}} audit
+   * @return {boolean}
+   */
+  static didAuditPass(audit) {


maybe this shouldn't be considered if the audit "passed" (we even call out informative audits as not being able to be interpreted as pass/fail lol), but I can't think of a better word and this is the way we divide the sections in the report. Maybe add a comment that this is specifically for that distinction?

yeah sure sg, I went back and forth on name, basically "shouldHideAudit" but not always

actually, I could flip and do "shouldConsiderFailing" or something?

brendankenny · 2018-05-07T00:12:23Z

lighthouse-core/scoring.js

          member.weight = 0;
        }
+


I think these members are auditRefs now if you want to do a drive by in this function :)

patrickhulce · 2018-05-07T02:16:25Z

looks like manual expectations need updating

whoops yeah fixed them on the wrong branch 😆

brendankenny

LGTM!

core(lhr): support null scores

13524c7

patrickhulce requested review from brendankenny, paulirish and vinamratasingal-zz as code owners May 5, 2018 00:04

patrickhulce mentioned this pull request May 5, 2018

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

Merged

paulirish requested changes May 5, 2018

View reviewed changes

patrickhulce added 3 commits May 5, 2018 23:23

feedback

b3bf789

tests

5977cc0

csv

f2edefa

paulirish approved these changes May 6, 2018

View reviewed changes

test

06f1a73

brendankenny requested changes May 7, 2018

View reviewed changes

feedback + smokes

aeb26dd

patrickhulce added 5 commits May 6, 2018 19:23

consistent smokes

158f777

comment

4d4c0e8

auditRef

3ca3776

Merge branch 'master' into null_scores

008ad51

go back on showWithPassed

f32de4e

brendankenny approved these changes May 7, 2018

View reviewed changes

patrickhulce merged commit be71709 into master May 7, 2018

brendankenny deleted the null_scores branch May 7, 2018 19:28

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

core(lhr): support null scores (GoogleChrome#5128)

1a63447

		@@ -162,18 +163,26 @@ class Audit {

		// If the audit was determined to not apply to the page, we'll reset it as informative only

core(lhr): support null scores #5128

core(lhr): support null scores #5128

Conversation

patrickhulce commented May 5, 2018 • edited Loading

paulirish 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish May 5, 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

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

paulirish commented May 6, 2018

brendankenny commented May 6, 2018

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

patrickhulce commented May 7, 2018

brendankenny left a comment

Choose a reason for hiding this comment

patrickhulce commented May 5, 2018 •

edited

Loading

patrickhulce May 6, 2018 •

edited

Loading

paulirish May 5, 2018 •

edited

Loading

patrickhulce May 6, 2018 •

edited

Loading

patrickhulce May 6, 2018 •

edited

Loading