Skip to content

report: reuse generalized clumping for perf category #9288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
paulirish wants to merge 11 commits into from
Closed

report: reuse generalized clumping for perf category #9288

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1e86d5e
report: reuse generalized clumping for perf category
paulirish Jun 26, 2019
151fc98
tests
paulirish Jun 26, 2019
27c3215
lint
paulirish Jun 26, 2019
3d2f41b
Merge branch 'master' into perfclumps
paulirish Sep 18, 2020
9eb5b08
working
paulirish Sep 18, 2020
670624b
Apply suggestions from code review
paulirish Sep 18, 2020
d72e53b
rename to public method
paulirish Sep 18, 2020
8f93171
move the diagram
paulirish Sep 18, 2020
865cbd4
render multiple filmstrips, potentially
paulirish Sep 18, 2020
ae3c77f
feedback
paulirish Sep 18, 2020
c934c4b
lint
paulirish Sep 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lighthouse-core/config/default-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,14 +395,14 @@ const defaultConfig = {
{id: 'performance-budget', weight: 0, group: 'budgets'},
{id: 'resource-summary', weight: 0, group: 'diagnostics'},
{id: 'third-party-summary', weight: 0, group: 'diagnostics'},
{id: 'screenshot-thumbnails', weight: 0, group: 'filmstrip'},
// Audits past this point don't belong to a group and will not be shown automatically
{id: 'network-requests', weight: 0},
{id: 'network-rtt', weight: 0},
{id: 'network-server-latency', weight: 0},
{id: 'main-thread-tasks', weight: 0},
{id: 'diagnostics', weight: 0},
{id: 'metrics', weight: 0},
{id: 'screenshot-thumbnails', weight: 0},
{id: 'final-screenshot', weight: 0},
],
},
Expand Down
26 changes: 21 additions & 5 deletions lighthouse-core/report/html/renderer/category-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,22 @@ class CategoryRenderer {
this.createPermalinkSpan(element, category.id);
element.appendChild(this.renderCategoryHeader(category, groupDefinitions));

const clumpElems = this._splitAndRenderClumps(category.auditRefs, groupDefinitions,
category.manualDescription);
element.append(...clumpElems);
return element;
}

/**
*
* @param {LH.ReportResult.AuditRef[]} auditRefs
* @param {Object<string, LH.Result.ReportGroup>} [groupDefinitions]
* @param {LH.Result.Category.manualDescription} [manualDescription]
* @return {Element[]}
*/
_splitAndRenderClumps(auditRefs, groupDefinitions = {}, manualDescription) {
Copy link
Contributor

@brendankenny brendankenny Jun 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Contributor

@brendankenny brendankenny Jun 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const clumpElems = [];

// Top level clumps for audits, in order they will appear in the report.
/** @type {Map<TopLevelClumpId, Array<LH.ReportResult.AuditRef>>} */
const clumps = new Map();
Expand All @@ -423,7 +439,7 @@ class CategoryRenderer {
clumps.set('notApplicable', []);

// Sort audits into clumps.
for (const auditRef of category.auditRefs) {
for (const auditRef of auditRefs) {
const clumpId = this._getClumpIdForAuditRef(auditRef);
const clump = /** @type {Array<LH.ReportResult.AuditRef>} */ (clumps.get(clumpId)); // already defined
clump.push(auditRef);
Expand All @@ -437,16 +453,16 @@ class CategoryRenderer {
if (clumpId === 'failed') {
const clumpElem = this.renderUnexpandableClump(auditRefs, groupDefinitions);
clumpElem.classList.add(`lh-clump--failed`);
element.appendChild(clumpElem);
clumpElems.push(clumpElem);
continue;
}

const description = clumpId === 'manual' ? category.manualDescription : undefined;
const description = clumpId === 'manual' ? manualDescription : undefined;
const clumpElem = this.renderClump(clumpId, {auditRefs, description});
element.appendChild(clumpElem);
clumpElems.push(clumpElem);
}

return element;
return clumpElems;
}

/**
Expand Down
23 changes: 9 additions & 14 deletions lighthouse-core/report/html/renderer/performance-category-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Contributor

@brendankenny brendankenny Jun 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

const thumbnailResult = thumbnailAudit && thumbnailAudit.result;
if (thumbnailResult && thumbnailResult.details) {
timelineEl.id = thumbnailResult.id;
Expand All @@ -165,7 +165,7 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
}

// Budgets
const budgetAudit = category.auditRefs.find(audit => audit.id === 'performance-budget');
const budgetAudit = category.auditRefs.find(audit => audit.group === 'budgets');
if (budgetAudit && budgetAudit.result.details) {
const table = this.detailsRenderer.render(budgetAudit.result.details);
if (table) {
Expand Down Expand Up @@ -220,19 +220,14 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
element.appendChild(groupEl);
}

// Passed audits
const passedAudits = category.auditRefs
.filter(audit => (audit.group === 'load-opportunities' || audit.group === 'diagnostics') &&
Util.showAsPassed(audit.result));
// Everything else (passed, passed with warnings, n/a)
const renderedAudits = [...metricAudits, thumbnailAudit, budgetAudit, ...opportunityAudits,
...diagnosticAudits];
const unrenderedAudits = category.auditRefs.filter(ref => !renderedAudits.includes(ref));
const remainingAudits = unrenderedAudits.filter(ref => !!ref.group);
Copy link
Contributor

@brendankenny brendankenny Jun 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@paulirish paulirish Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (!passedAudits.length) return element;

const clumpOpts = {
auditRefs: passedAudits,
groupDefinitions: groups,
};
const passedElem = this.renderClump('passed', clumpOpts);
element.appendChild(passedElem);
const clumpElems = this._splitAndRenderClumps(remainingAudits, groups);
element.append(...clumpElems);
return element;
}
}
Expand Down
27 changes: 22 additions & 5 deletions lighthouse-core/test/report/html/renderer/performance-category-renderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(`
Copy link
Collaborator

@patrickhulce patrickhulce Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

Array [
"lh-audit-group lh-audit-group--metrics",
"lh-audit-group lh-audit-group--budgets",
"lh-audit-group lh-audit-group--load-opportunities",
"lh-audit-group lh-audit-group--diagnostics",
"lh-clump lh-audit-group lh-clump--passed",
"lh-clump lh-audit-group lh-clump--notapplicable",
]
`);
});

it('renders the metrics', () => {
Expand Down Expand Up @@ -150,11 +160,18 @@ describe('PerfCategoryRenderer', () => {
const categoryDOM = renderer.render(category, sampleResults.categoryGroups);
const passedSection = categoryDOM.querySelector('.lh-category > .lh-clump--passed');

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

@patrickhulce patrickhulce Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to passedAuditIds?

audit =>
audit.group &&
audit.group !== 'metrics' &&
audit.id !== 'performance-budget' &&
Util.showAsPassed(audit.result) &&
audit.result.scoreDisplayMode !== 'notApplicable'
).map(ref => ref.id);

const passedElements = passedSection.querySelectorAll('.lh-audit');
assert.equal(passedElements.length, passedAudits.length);
const passedElemIds = Array.from(passedElements).map(el => el.id);
expect(passedAudits).toEqual(passedElemIds);
});

// Unsupported by perf cat renderer right now.
Expand Down