report: reuse generalized clumping for perf category #9288

paulirish · 2019-06-26T16:54:34Z

I noticed that perf audits with warnings don't use the Passed audits but with warnings clump that @connorjclark added in #6989. Turns out most of the audits that can emit warnings are in perf and pwa, neither of which used the generalized clumping stuff in category-renderer.

This PR lets the perf cat use the clumping stuff. And makes sure we don't accidentally not render audits (which has been happening to User Timing for a while).

The major fix: any passed audits in perf category with warnings will be rendered in the pre-expanded clump for extra visibility.

patrickhulce

LGTM!

patrickhulce · 2019-06-26T18:42:54Z

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

@@ -68,7 +68,17 @@ describe('PerfCategoryRenderer', () => {
  it('renders the sections', () => {
    const categoryDOM = renderer.render(category, sampleResults.categoryGroups);
    const sections = categoryDOM.querySelectorAll('.lh-category > .lh-audit-group');
-    assert.equal(sections.length, 5);
+    const sectionTitles = Array.from(sections).map(el => el.className);
+    expect(sectionTitles).toMatchInlineSnapshot(`


patrickhulce · 2019-06-26T18:48:08Z

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

-    const passedAudits = category.auditRefs.filter(audit =>
-      audit.group && audit.group !== 'metrics' && audit.id !== 'performance-budget'
-        && Util.showAsPassed(audit.result));
+    const passedAudits = category.auditRefs.filter(


rename to passedAuditIds?

brendankenny

seems good to share the implementation

I noticed that perf audits with warnings don't use the Passed audits but with warnings clump that @connorjclark added in #6989.

can you add a test that those do get rendered with a warning now?

brendankenny · 2019-06-27T21:14:19Z

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

@@ -156,7 +156,7 @@ class PerformanceCategoryRenderer extends CategoryRenderer {

    // Filmstrip
    const timelineEl = this.dom.createChildOf(element, 'div', 'lh-filmstrip-container');
-    const thumbnailAudit = category.auditRefs.find(audit => audit.id === 'screenshot-thumbnails');
+    const thumbnailAudit = category.auditRefs.find(audit => audit.group === 'filmstrip');


this doesn't seem better since (here and below) the code really is looking for a particular audit, not whatever happens to be first in a group (or happens to be the only one in a group)?

brendankenny · 2019-06-27T21:35:14Z

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

+    const renderedAudits = [...metricAudits, thumbnailAudit, budgetAudit, ...opportunityAudits,
+      ...diagnosticAudits];
+    const unrenderedAudits = category.auditRefs.filter(ref => !renderedAudits.includes(ref));
+    const remainingAudits = unrenderedAudits.filter(ref => !!ref.group);


this seems like it'll be easy to forget to add things to the renderedAudits list and get them rendered twice.

What if we switched how we do the non-rendered audits and gave them an explicit group (report-invisible or a name equally as good) instead of no group?

It would reduce the mystery of how the audits that don't show up in the html report work and let this check just be whatever hasn't been rendered yet and isn't in that group.

this sounds good.

it means every auditRef needs a group. but that works for me.

the tricky thing is manual audits are currently defined by their scoreDisplayMode, and this should be a group.

but once that's cleaned up it makes sense to do that policy change.

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

brendankenny · 2019-06-27T21:49:12Z

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

+   * @param {LH.Result.Category.manualDescription} [manualDescription]
+   * @return {Element[]}
+   */
+  _splitAndRenderClumps(auditRefs, groupDefinitions = {}, manualDescription) {


doesn't the leading underscore still break when rolling to devtools (closure won't like perf-category-rendering trying to access it outside of this file?).

It's also pretty much what render() was before, so having it as a public method feels as ok as renderAuditGroup, renderClump, etc, so I say just go for

Suggested change

_splitAndRenderClumps(auditRefs, groupDefinitions = {}, manualDescription) {

renderClumps(auditRefs, groupDefinitions, manualDescription) {

also should move the really good ascii diagram + description to this method as this is the one actually doing it now

Co-authored-by: Brendan Kenny <bckenny@gmail.com>

googlebot · 2020-09-18T01:33:46Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

paulirish · 2020-09-18T01:28:26Z

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

+    const renderedAudits = [...metricAudits, thumbnailAudit, budgetAudit, ...opportunityAudits,
+      ...diagnosticAudits];
+    const unrenderedAudits = category.auditRefs.filter(ref => !renderedAudits.includes(ref));
+    const remainingAudits = unrenderedAudits.filter(ref => !!ref.group);


this sounds good.

it means every auditRef needs a group. but that works for me.

the tricky thing is manual audits are currently defined by their scoreDisplayMode, and this should be a group.

but once that's cleaned up it makes sense to do that policy change.

paulirish · 2020-09-18T01:35:34Z

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

+   * @param {Object<string, LH.Result.ReportGroup>} [groupDefinitions]
+   * @return {Element}
+   */
+  render(category, groupDefinitions = {}) {


(the diff is a little funny here because i moved the diagram... but the method signature and top 3 lines are identical to what it had been.)

git diff catches the changes as a "move", and calls out the new method of renderClumps a bit more clearer

connorjclark · 2020-09-21T22:31:52Z

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

-      const filmstripEl = this.detailsRenderer.render(thumbnailResult.details);
-      filmstripEl && timelineEl.appendChild(filmstripEl);
+    // We only expect one of these, but the renderer will support multiple
+    const thumbnailAudits = category.auditRefs.filter(audit => audit.group === 'filmstrip');


Is this breaking? v7? or: support as fallback the old way.

yeah this would break formatting the v6 reports in a new renderer. good call. i'll add a compat bit to also the screenshot-thumbnails audit

connorjclark

unsure if breaking change

paulirish added 3 commits June 26, 2019 09:40

report: reuse generalized clumping for perf category

1e86d5e

tests

151fc98

lint

27c3215

paulirish requested review from brendankenny and patrickhulce as code owners June 26, 2019 16:54

googlebot added the cla: yes label Jun 26, 2019

paulirish requested review from connorjclark and removed request for brendankenny and patrickhulce June 26, 2019 16:55

paulirish mentioned this pull request Jun 26, 2019

misc(build): create error-y LHR for the deploy #9283

Merged

patrickhulce approved these changes Jun 26, 2019

View reviewed changes

connorjclark approved these changes Jun 27, 2019

View reviewed changes

brendankenny requested changes Jun 27, 2019

View reviewed changes

brendankenny added the waiting4committer label Oct 25, 2019

paulirish added 2 commits September 17, 2020 18:05

Merge branch 'master' into perfclumps

3d2f41b

working

9eb5b08

paulirish requested a review from a team as a code owner September 18, 2020 01:10

vercel bot deployed to Preview September 18, 2020 01:10 View deployment

paulirish and others added 6 commits September 17, 2020 18:11

Apply suggestions from code review

670624b

Co-authored-by: Brendan Kenny <bckenny@gmail.com>

rename to public method

d72e53b

move the diagram

8f93171

render multiple filmstrips, potentially

865cbd4

feedback

ae3c77f

lint

c934c4b

paulirish force-pushed the perfclumps branch from dd19244 to c934c4b Compare September 18, 2020 01:33

vercel bot deployed to Preview September 18, 2020 01:33 View deployment

googlebot added cla: no and removed cla: yes labels Sep 18, 2020

paulirish commented Sep 18, 2020

View reviewed changes

paulirish added waiting4reviewer and removed waiting4committer labels Sep 21, 2020

connorjclark reviewed Sep 21, 2020

View reviewed changes

connorjclark self-requested a review September 21, 2020 22:45

connorjclark added waiting4committer and removed waiting4reviewer labels Oct 2, 2020

google-cla bot added cla: yes and removed cla: no labels Nov 9, 2021

connorjclark added the pr-bankruptcy label Apr 25, 2022

connorjclark closed this Apr 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: reuse generalized clumping for perf category #9288

report: reuse generalized clumping for perf category #9288

paulirish commented Jun 26, 2019

patrickhulce left a comment

patrickhulce Jun 26, 2019

patrickhulce Jun 26, 2019

brendankenny left a comment

brendankenny Jun 27, 2019 •

edited

Loading

brendankenny Jun 27, 2019 •

edited

Loading

paulirish Sep 18, 2020

brendankenny Jun 27, 2019

brendankenny Jun 27, 2019

googlebot commented Sep 18, 2020

paulirish Sep 18, 2020

paulirish Sep 18, 2020

connorjclark Sep 21, 2020 •

edited

Loading

paulirish Sep 21, 2020

connorjclark left a comment

	_splitAndRenderClumps(auditRefs, groupDefinitions = {}, manualDescription) {
	renderClumps(auditRefs, groupDefinitions, manualDescription) {

report: reuse generalized clumping for perf category #9288

report: reuse generalized clumping for perf category #9288

Conversation

paulirish commented Jun 26, 2019

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

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

brendankenny Jun 27, 2019 • 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

googlebot commented Sep 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark left a comment

Choose a reason for hiding this comment

brendankenny Jun 27, 2019 •

edited

Loading

brendankenny Jun 27, 2019 •

edited

Loading

connorjclark Sep 21, 2020 •

edited

Loading