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

paulirish · 2018-03-07T23:27:56Z

Changes

reportCategories no longer contains real auditResults (aka de-dupe all audit results from ☂️ 👣 Things to fix on next breaking change #4333)
- The categories still keep their .score prop. Eg: reportCategories.performance.score
- Each audit's score moves from reportCategories.performance['audit-name'] (which now just holds weight and group) over to audits['audit-name']
Remove LHR.score, the overallScore

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

ref #4614

fixes #1999

This reverts commit 72c8a6e.

patrickhulce

nice! let's keep this chain amovin' we've got lots of #4333 to land :D

patrickhulce · 2018-03-09T16:28:36Z

lighthouse-core/runner.js

      if (opts.config.categories) {
-        report = Runner._scoreAndCategorize(opts, resultsById);
+        Runner._scoreAndCategorize(opts, resultsById);
+        // COMPAT: after dropping Node 6, upgrade to use Object.values


does this mean we can do it now? :)

patrickhulce · 2018-03-09T16:30:02Z

lighthouse-core/scoring.js

      const category = config.categories[categoryId];
      category.id = categoryId;

-      const audits = category.audits.map(audit => {
+      category.audits.forEach(audit => {
        const result = resultsByAuditId[audit.id];
        // Cast to number to catch `null` and undefined when audits error


should all of these line just be moved into audit.js then to keep all the score mapping in the same place?

more a comment for #4690 I suppose

Aye. It actually already happens in that PR: https://github.com/GoogleChrome/lighthouse/pull/4690/files#diff-488f5da33160a651526463bdb951e5deL31

brendankenny · 2018-03-09T20:09:04Z

smokehouse failures?

brendankenny

this is looking good to me so far. As discussed, we'll want to smoosh the audit results into a copy of the LHR so the canonical one still exists untouched.

something for the future: as we add more typechecking, we may want to split AuditJSON to a version with result (only used inside the report renderer) and a version without (used everywhere else) so that we can more easily reason about which we have at some point in time and so we don’t have to put in a bunch of checks to keep tsc happy :)

brendankenny · 2018-03-09T20:11:27Z

lighthouse-core/runner.js

      if (opts.config.categories) {
-        report = Runner._scoreAndCategorize(opts, resultsById);
+        Runner._scoreAndCategorize(opts, resultsById);
+        // COMPAT: after dropping Node 6, upgrade to use Object.values


paulirish · 2018-03-09T23:37:35Z

As discussed, we'll want to smoosh the audit results into a copy of the LHR so the canonical one still exists untouched.

Done. added a test.

something for the future: as we add more typechecking, we may want to split AuditJSON to a version with result (only used inside the report renderer) and a version without (used everywhere else) so that we can more easily reason about which we have at some point in time and so we don’t have to put in a bunch of checks to keep tsc happy :)

YESSSSSSS. So want this.

Here was my typechecking spike, validating the outgoing LHR against some real types. Works! 95e19a5...typechecking

This and #4690 are updated and ready

patrickhulce

let's get this show on the 🚗 💨

brendankenny

LGTM. Just the merge issue and two questions remaining 🌊➡️🛁

brendankenny · 2018-03-12T23:51:20Z

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

@@ -155,24 +157,39 @@ class ReportRenderer {
    perfCategoryRenderer.setTemplateContext(this._templateContext);

    const categories = reportSection.appendChild(this._dom.createElement('div', 'lh-categories'));
+
+    ReportRenderer.smooshAuditResultsIntoCategories(report.audits, report.reportCategories);


does this make more sense up in renderReport before passing into _renderReport? Then internally it'll always be the smooshed version and we won't have to remember not to use reportCategories above this line

brendankenny · 2018-03-12T23:57:04Z

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

+ *     result: (ReportRenderer.AuditResultJSON|undefined)
+ * }}
+ */
+ReportRenderer.AuditJSON; // eslint-disable-line no-unused-expressions


defined below (merge conflict, I think)

brendankenny · 2018-03-12T23:58:26Z

lighthouse-core/runner.js

@@ -108,8 +109,7 @@ class Runner {
        audits: resultsById,
        artifacts: runResults.artifacts,
        runtimeConfig: Runner.getRuntimeConfig(opts.flags),
-        score: report ? report.score : 0,
-        reportCategories: report ? report.categories : [],
+        reportCategories: categories,


before this defaulted to an empty array. Will it cause problems anywhere as undefined (and removed from the JSON) instead?

brendankenny · 2018-03-12T23:59:17Z

lighthouse-core/scoring.js

   * @param {!Object<string, {score: ?number|boolean|undefined, notApplicable: boolean, informative: boolean}>} resultsByAuditId
   * @return {{score: number, categories: !Array<{audits: !Array<{score: number, result: !Object}>}>}}
   */
  static scoreAllCategories(config, resultsByAuditId) {
-    const categories = Object.keys(config.categories).map(categoryId => {
-      const category = config.categories[categoryId];
+    for (const [categoryId, category] of Object.entries(config.categories)) {


paulirish · 2018-03-13T00:21:37Z

merge when green.

devtools-bot added the waiting4reviewer label Mar 7, 2018

This was referenced Mar 8, 2018

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

Merged

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

Merged

paulirish changed the base branch from newdetails to master March 9, 2018 02:47

paulirish added 3 commits March 8, 2018 18:52

make reportCategories shallow. move score to AuditResultJSON

539ec76

This reverts commit 72c8a6e.

extract scoringMode changes.

47669a6

regen sample_v2 json

e24e3c7

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

paulirish requested review from brendankenny, patrickhulce and vinamratasingal-zz as code owners March 9, 2018 02:53

patrickhulce reviewed Mar 9, 2018

View reviewed changes

brendankenny changed the title ~~core(lhr): make reportCategories shallow. move score to AuditResultJSON~~ core(lhr): make reportCategories shallow. move audit scores to AuditResultJSON Mar 9, 2018

brendankenny changed the title ~~core(lhr): make reportCategories shallow. move audit scores to AuditResultJSON~~ core(lhr): make reportCategories shallow; move audit scores to AuditResultJSON Mar 9, 2018

brendankenny requested changes Mar 9, 2018

View reviewed changes

paulirish added 4 commits March 9, 2018 14:00

object.values/entries, wassup.

ef10f4f

smoosh. what a time to be alive.

cf4ae95

temporary hack to retain boolean scores for the moment.

010f2fd

don't mutate the LHR within the renderer

e4784a2

tempoary: cast boolean scores when creating arithmetic mean score.

fddb987

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

patrickhulce approved these changes Mar 12, 2018

View reviewed changes

patrickhulce assigned brendankenny Mar 12, 2018

Merge branch 'master' into shallowCategories

5e1337d

brendankenny approved these changes Mar 13, 2018

View reviewed changes

feedback.

c2e25b5

placate ts

b4ac5f0

paulirish force-pushed the shallowCategories branch from f0cb8e6 to b4ac5f0 Compare March 13, 2018 00:47

paulirish merged commit 4d5696d into master Mar 13, 2018

paulirish removed the waiting4reviewer label Mar 15, 2018

paulirish deleted the shallowCategories branch April 27, 2018 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

paulirish commented Mar 7, 2018 •

edited

Loading

patrickhulce left a comment

patrickhulce Mar 9, 2018

brendankenny Mar 9, 2018

patrickhulce Mar 9, 2018

patrickhulce Mar 9, 2018

paulirish Mar 9, 2018

brendankenny commented Mar 9, 2018

brendankenny left a comment

brendankenny Mar 9, 2018

paulirish commented Mar 9, 2018

patrickhulce left a comment

brendankenny left a comment

brendankenny Mar 12, 2018

brendankenny Mar 12, 2018

brendankenny Mar 12, 2018

brendankenny Mar 12, 2018

paulirish commented Mar 13, 2018

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

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

Conversation

paulirish commented Mar 7, 2018 • edited Loading

Changes

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

brendankenny commented Mar 9, 2018

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Mar 9, 2018

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

paulirish commented Mar 13, 2018

paulirish commented Mar 7, 2018 •

edited

Loading