core(lhr): strictly numeric scores, add scoreDisplayMode #4690

paulirish · 2018-03-05T08:20:51Z

Ensure all audits return a numeric score (0-1), no boolean nonsense.
scoreDisplayMode forces how it's displayed (renamed from scoringMode)
Remove displayValue fallback stuff that was introduced in Resolve audit result #487

This is a chain of PRs. Landing order is... 1st: newdetails (#4616). 2nd: shallowCategories (#4711). 3rd: scoring2.0 (#4690)

ref #4614

patrickhulce

quick, let's merge before there are conflicts 😆

patrickhulce · 2018-03-05T16:46:23Z

docs/understanding-results.md

@@ -57,7 +57,7 @@ An object containing the results of the audits, keyed by their name.
 | rawValue | <code>boolean&#124;number</code> | The unscored value determined by the audit. Typically this will match the score if there's no additional information to impart. For performance audits, this value is typically a number indicating the metric value. |
 | displayValue | `string` | The string to display in the report alongside audit results. If empty, nothing additional is shown. This is typically used to explain additional information such as the number and nature of failing items. |
 | score | <code>boolean&#124;number</code> | The scored value determined by the audit as either boolean or a number `0-100`. If the audit is a boolean, the implication is `score ? 100 : 0`. |
-| scoringMode | <code>"binary"&#124;"numeric"</code> | A string identifying how granular the score is meant to be indicating, i.e. is the audit pass/fail or are there shades of gray 0-100. *NOTE: This does not necessarily mean `typeof audit.score === audit.scoringMode`, an audit can have a score of 40 with a scoringMode of `"binary"` meant to indicate display should be failure.* |
+| scoreDisplayMode | <code>"binary"&#124;"numeric"</code> | A string identifying how granular the score is meant to be indicating, i.e. is the audit pass/fail or are there shades of gray 0-100. *NOTE: This does not necessarily mean `typeof audit.score === audit.scoreDisplayMode`, an audit can have a score of 40 with a scoreDisplayMode of `"binary"` meant to indicate display should be failure.* |


score should always be a number now, so entire caveat is unnecessary :)

patrickhulce · 2018-03-05T16:47:51Z

lighthouse-core/audits/audit.js

    score = Math.max(0, score);
-    return Math.round(score);
+    return Math.round(score * 100) / 100;


patrickhulce · 2018-03-05T16:50:24Z

lighthouse-core/scoring.js


-        result.score = auditScore;
+        if (!Number.isFinite(result.score)) {


do we need to check in both places?

i'd be fine with also doing this in generateAuditResult, however IMO there's not a big diff.

Many of our tests assert the result returned by audit()

Audit.generateAuditResult is called via Runner once each audit is done

Basically right after that, we call this scoreAllCategories method.

So ideally we'd have something in Audit class that wraps each audit's audit() method, but we don't.

lol I just made your point for you. 😊

patrickhulce · 2018-03-05T16:53:05Z

lighthouse-core/scoring.js

+ * @param {number} val
+ * @return {number}
+ */
+const clamp2decimals = val => Math.round(val * 100) / 100;


Not the biggest fan of this name, clampTo2Decimals?

paulirish · 2018-03-07T18:59:52Z

lighthouse-core/audits/audit.js

+      throw new Error(`Audit score for ${audit.meta.name} is > 1`);
+    }
+
+    const scoreDisplayMode = result.scoreDisplayMode || audit.meta.scoreDisplayMode ||


remove result.scoreDisplayMode and update the byteefficiency audit metas.

patrickhulce

couple nits, but we are so close! :D

patrickhulce · 2018-03-13T17:35:45Z

docs/understanding-results.md

@@ -56,8 +55,8 @@ An object containing the results of the audits, keyed by their name.
 | error | `boolean` | Set to true if there was an an exception thrown within the audit. The error message will be in `debugString`.
 | rawValue | <code>boolean&#124;number</code> | The unscored value determined by the audit. Typically this will match the score if there's no additional information to impart. For performance audits, this value is typically a number indicating the metric value. |
 | displayValue | `string` | The string to display in the report alongside audit results. If empty, nothing additional is shown. This is typically used to explain additional information such as the number and nature of failing items. |
-| score | <code>boolean&#124;number</code> | The scored value determined by the audit as either boolean or a number `0-100`. If the audit is a boolean, the implication is `score ? 100 : 0`. |
-| scoringMode | <code>"binary"&#124;"numeric"</code> | A string identifying how granular the score is meant to be indicating, i.e. is the audit pass/fail or are there shades of gray 0-100. *NOTE: This does not necessarily mean `typeof audit.score === audit.scoringMode`, an audit can have a score of 40 with a scoringMode of `"binary"` meant to indicate display should be failure.* |
+| score | <code>boolean&#124;number</code> | The scored value determined by the audit as a number `0-1`, representing displayed scores of 0-100. |


no longer boolean|number just number 🎉 :D

patrickhulce · 2018-03-13T17:36:19Z

docs/understanding-results.md

-| score | <code>boolean&#124;number</code> | The scored value determined by the audit as either boolean or a number `0-100`. If the audit is a boolean, the implication is `score ? 100 : 0`. |
-| scoringMode | <code>"binary"&#124;"numeric"</code> | A string identifying how granular the score is meant to be indicating, i.e. is the audit pass/fail or are there shades of gray 0-100. *NOTE: This does not necessarily mean `typeof audit.score === audit.scoringMode`, an audit can have a score of 40 with a scoringMode of `"binary"` meant to indicate display should be failure.* |
+| score | <code>boolean&#124;number</code> | The scored value determined by the audit as a number `0-1`, representing displayed scores of 0-100. |
+| scoreDisplayMode | <code>"binary"&#124;"numeric"</code> | A string identifying how granular the score is meant to be indicating, i.e. is the audit pass/fail (score of 1 or 0), or are there shades of gray (scores between 0-1 exclusive). |


why exclusive? we can still assign scores of 1 to numeric audits :)

patrickhulce · 2018-03-13T17:41:40Z

lighthouse-core/test/audits/audit-test.js

+    it('throws if an audit returns a score that\'s not a number', () => {
+      const re = /Invalid score/;
+      assert.throws(_ => Audit._normalizeAuditScore(B, {rawValue: true, score: NaN}), re);
+      assert.throws(_ => Audit._normalizeAuditScore(B, {rawValue: true, score: '50'}), /is > 1/);


wait why isn't this /Invalid score/?

this pair of assertions:
https://github.com/GoogleChrome/lighthouse/blob/scoring2.0/lighthouse-core/audits/audit.js#L127-L128

we assert its finite first, throw invalid score. otherwise we doublecheck we're finite but <= 1

yeah exactly so shouldn't we throw invalid score... I'm so confused haha

EDIT: OH wait, it's always clampTo2Decimals which casts to number, is that what we want? seems ok if it doesn't turn out to NaN

yeah exactly. :D clamp casts.
i fixed the tests.

so right now one could pass in a score of '0.7' and it'd get casted and be just fine. we could hypothetically throw but yeah i don't think its a big deal.

i fixed the tests.

patrickhulce

💥 🎉

brendankenny

review!

brendankenny · 2018-03-13T19:00:02Z

docs/architecture.md

@@ -9,9 +9,9 @@ _Some incomplete notes_
 * **Driver** - Interfaces with [Chrome Debugging Protocol](https://developer.chrome.com/devtools/docs/debugger-protocol)  ([API viewer](https://chromedevtools.github.io/debugger-protocol-viewer/))
 * **Gatherers** - Uses Driver to collect information about the page. Minimal post-processing.
  * **Artifacts** - output of a gatherer
-* **Audit** - Tests for a single feature/optimization/metric. Using the Artifacts as input, an audit evaluates a test and  resolves to a score which may be pass/fail/numeric. Formatting note: The meta description may contain markdown links and meta title may contain markdown code.
+* **Audit** - Tests for a single feature/optimization/metric. Using the Artifacts as input, an audit evaluates a test and resolves to a numeric score, described by scoreDisplayMode to be pass/fail or numeric. Formatting note: The meta description may contain markdown links and meta title may contain markdown code.


maybe move the scoreDisplayMode comment to the "formatting note"? It's pretty tangential to the rest of the sentence.

brendankenny · 2018-03-13T19:01:56Z

docs/scoring.md

@@ -36,30 +36,30 @@ The metric results are not weighted equally. Currently the weights are:
 * 1X - perceptual speed index
 * 1X - estimated input latency

-These weights were determined based on heuristics, and the Lighthouse team is working on formalizing this approach through more field data.  
+These weights were determined based on heuristics, and the Lighthouse team is working on formalizing this approach through more field data.


these weights are heuristics...

brendankenny · 2018-03-13T19:04:15Z

docs/understanding-results.md

-| score | <code>boolean&#124;number</code> | The scored value determined by the audit as either boolean or a number `0-100`. If the audit is a boolean, the implication is `score ? 100 : 0`. |
-| scoringMode | <code>"binary"&#124;"numeric"</code> | A string identifying how granular the score is meant to be indicating, i.e. is the audit pass/fail or are there shades of gray 0-100. *NOTE: This does not necessarily mean `typeof audit.score === audit.scoringMode`, an audit can have a score of 40 with a scoringMode of `"binary"` meant to indicate display should be failure.* |
+| score | <code>number</code> | The scored value determined by the audit as a number `0-1`, representing displayed scores of 0-100. |
+| scoreDisplayMode | <code>"binary"&#124;"numeric"</code> | A string identifying how granular the score is meant to be indicating, i.e. is the audit pass/fail (score of 1 or 0), or are there shades of gray (scores between 0-1 inclusive). |


maybe "A string identifying the score granularity, i.e. is the ...."

brendankenny · 2018-03-13T19:06:03Z

lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js

      },
      'link-blocking-first-paint': {
-        score: 100,
+        score: 1.00,


mixed 1.00s and 1s. I'd personally rather just embrace it and go with all 1 all the time, but should at least be consistent in a file and/or context

I personally feel the same about the trailing 0s on e.g. 0.80, too. Why hold on to the digit if we're not going to be doing 0-100 :)

very well! :)
fixed up all these. also standardized on a leading 0, so we don't have a mix of .25 and 0.25

brendankenny · 2018-03-13T19:15:12Z

lighthouse-core/audits/audit.js

    score = Math.max(0, score);
-    return Math.round(score);
+    return Math.round(score * 100) / 100;


clampTo2Decimals?

brendankenny · 2018-03-13T19:32:55Z

lighthouse-core/audits/audit.js

+      score,
+      scoreDisplayMode,
+      informative,
+      rawValue,


should rawValue be copying over the rawValue from the audit result? Should it have a different name if not?

looking at usage below, maybe these could have a name that indicates they're overrides

brendankenny · 2018-03-13T19:38:45Z

lighthouse-core/closure/typedefs/AuditResult.js

@@ -52,7 +52,7 @@ function AuditFullResult() {}
 AuditFullResult.prototype.score;


should be only {number} now?

brendankenny · 2018-03-13T19:39:22Z

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

@@ -207,9 +207,9 @@ if (typeof module !== 'undefined' && module.exports) {
 *     manual: (boolean|undefined),
 *     notApplicable: (boolean|undefined),
 *     debugString: (string|undefined),
- *     displayValue: string,
+ *     displayValue: (string|undefined),


what happened here?

brendankenny · 2018-03-13T19:40:48Z

lighthouse-core/scoring.js

-        }
-
-        const score = Number(itemScore) || 0;
+        const score = Number(item.score) || 0;


no more need for Number coercion?

brendankenny · 2018-03-13T20:00:19Z

lighthouse-core/audits/audit.js

+    }
+
+    if (!Number.isFinite(score)) throw new Error(`Invalid score: ${score}`);
+    if (score > 1) throw new Error(`Audit score for ${audit.meta.name} is > 1`);


worth checking < 0 as well?

brendankenny

LGTM!

paulirish added internals 3.0 labels Mar 5, 2018

devtools-bot added the waiting4reviewer label Mar 5, 2018

patrickhulce approved these changes Mar 5, 2018

View reviewed changes

patrickhulce reviewed Mar 5, 2018

View reviewed changes

paulirish changed the title ~~core: refactor scoring to be numeric only. introduce scoreDisplayMode~~ core(LHR): refactor scoring to be numeric only. introduce scoreDisplayMode Mar 7, 2018

paulirish changed the title ~~core(LHR): refactor scoring to be numeric only. introduce scoreDisplayMode~~ core(LHR): refactor scoring to be strictly numeric. introduce scoreDisplayMode Mar 7, 2018

paulirish changed the title ~~core(LHR): refactor scoring to be strictly numeric. introduce scoreDisplayMode~~ core(LHR): refactor scores to be strictly numeric, introduce scoreDisplayMode Mar 7, 2018

paulirish commented Mar 7, 2018

View reviewed changes

paulirish mentioned this pull request Mar 7, 2018

Improve LHR API shape for downstream consumers #4614

Closed

paulirish changed the title ~~core(LHR): refactor scores to be strictly numeric, introduce scoreDisplayMode~~ core(lhr): refactor scores to be strictly numeric, introduce scoreDisplayMode Mar 7, 2018

paulirish mentioned this pull request Mar 7, 2018

core(lhr): make reportCategories shallow; move audit scores to AuditResultJSON #4711

Merged

paulirish force-pushed the scoring2.0 branch from f582bfe to 6b05c4d Compare March 7, 2018 23:38

paulirish changed the base branch from newdetails to shallowCategories March 7, 2018 23:39

paulirish mentioned this pull request Mar 8, 2018

core(lhr): overhaul LHR details, introduce details.summary #4616

Merged

paulirish force-pushed the shallowCategories branch from 5d94217 to e24e3c7 Compare March 9, 2018 02:53

paulirish force-pushed the scoring2.0 branch from e35cc05 to 602c90a Compare March 9, 2018 03:14

paulirish requested review from brendankenny and vinamratasingal-zz as code owners March 9, 2018 03:14

paulirish force-pushed the scoring2.0 branch 3 times, most recently from 1584258 to bb099d3 Compare March 9, 2018 23:33

paulirish force-pushed the shallowCategories branch from 89c48f7 to fddb987 Compare March 9, 2018 23:38

paulirish force-pushed the scoring2.0 branch from bb099d3 to 3fb3572 Compare March 13, 2018 01:00

patrickhulce suggested changes Mar 13, 2018

View reviewed changes

paulirish added 2 commits March 13, 2018 11:33

core(lhr): strictly numeric scores. add scoreDisplayMode.

d3d192f

feedback.

688952a

paulirish force-pushed the scoring2.0 branch from 3fb3572 to 688952a Compare March 13, 2018 18:38

paulirish changed the base branch from shallowCategories to master March 13, 2018 18:39

paulirish changed the title ~~core(lhr): refactor scores to be strictly numeric, introduce scoreDisplayMode~~ core(lhr): strictly numeric scores, add scoreDisplayMode Mar 13, 2018

paulirish added 2 commits March 13, 2018 11:49

docs(pptr): score tweaks.

2f80735

don't test a score of string '50'

6be9f27

patrickhulce approved these changes Mar 13, 2018

View reviewed changes

patrickhulce assigned brendankenny Mar 13, 2018

clamp after checking isFinite

a1dcc40

brendankenny requested changes Mar 13, 2018

View reviewed changes

paulirish added 3 commits March 13, 2018 13:25

changes to _normalizeAuditScore from brendans feedback.

9fa100c

audits only return scores of number|undefined. no boolean.

5701fc0

1.00 -> 1 , .20 -> 0.2

107216c

brendankenny approved these changes Mar 14, 2018

View reviewed changes

brendankenny merged commit 1fc4557 into master Mar 14, 2018

brendankenny deleted the scoring2.0 branch March 14, 2018 00:59

patrickhulce mentioned this pull request Mar 14, 2018

☂️ 👣 Things to fix on next breaking change #4333

Closed

34 tasks

paulirish removed the waiting4reviewer label Mar 15, 2018

paulirish removed the 3.0 label Dec 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(lhr): strictly numeric scores, add scoreDisplayMode #4690

core(lhr): strictly numeric scores, add scoreDisplayMode #4690

paulirish commented Mar 5, 2018 •

edited

Loading

patrickhulce left a comment

patrickhulce Mar 5, 2018

patrickhulce Mar 5, 2018

patrickhulce Mar 5, 2018

paulirish Mar 5, 2018

paulirish Mar 5, 2018

patrickhulce Mar 5, 2018

paulirish Mar 5, 2018

paulirish Mar 7, 2018

patrickhulce left a comment

patrickhulce Mar 13, 2018

patrickhulce Mar 13, 2018

patrickhulce Mar 13, 2018

paulirish Mar 13, 2018

patrickhulce Mar 13, 2018 •

edited

Loading

paulirish Mar 13, 2018

patrickhulce left a comment

brendankenny left a comment

brendankenny Mar 13, 2018

brendankenny Mar 13, 2018

brendankenny Mar 13, 2018

brendankenny Mar 13, 2018

brendankenny Mar 13, 2018

paulirish Mar 13, 2018

brendankenny Mar 13, 2018

brendankenny Mar 13, 2018

brendankenny Mar 13, 2018

brendankenny Mar 13, 2018

brendankenny Mar 13, 2018

brendankenny Mar 13, 2018

brendankenny Mar 13, 2018

brendankenny left a comment


		result.score = auditScore;
		if (!Number.isFinite(result.score)) {

		@@ -52,7 +52,7 @@ function AuditFullResult() {}
		AuditFullResult.prototype.score;

core(lhr): strictly numeric scores, add scoreDisplayMode #4690

core(lhr): strictly numeric scores, add scoreDisplayMode #4690

Conversation

paulirish commented Mar 5, 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

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

patrickhulce Mar 13, 2018 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

paulirish commented Mar 5, 2018 •

edited

Loading

patrickhulce Mar 13, 2018 •

edited

Loading