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 all 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
5 changes: 4 additions & 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 @@ -356,6 +356,9 @@ const defaultConfig = {
title: str_(UIStrings.diagnosticsGroupTitle),
description: str_(UIStrings.diagnosticsGroupDescription),
},
'filmstrip': {
title: '',
},
'pwa-fast-reliable': {
title: str_(UIStrings.pwaFastReliableGroupTitle),
},
Expand Down Expand Up @@ -476,14 +479,14 @@ const defaultConfig = {
{id: 'non-composited-animations', weight: 0, group: 'diagnostics'},
{id: 'unsized-images', weight: 0, group: 'diagnostics'},
{id: 'large-javascript-libraries', 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
39 changes: 27 additions & 12 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 @@ -424,6 +424,22 @@ class CategoryRenderer {
}
}

/**
* @param {LH.ReportResult.Category} category
* @param {Object<string, LH.Result.ReportGroup>} [groupDefinitions]
* @return {Element}
*/
render(category, groupDefinitions = {}) {
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.

(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

image

const element = this.dom.createElement('div', 'lh-category');
this.createPermalinkSpan(element, category.id);
element.appendChild(this.renderCategoryHeader(category, groupDefinitions));

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

/**
* Renders a set of top level sections (clumps), under a status of failed, warning,
* manual, passed, or notApplicable. The result ends up something like:
Expand All @@ -442,14 +458,13 @@ class CategoryRenderer {
* ├── audit 2
* ├── …
* ⋮
* @param {LH.ReportResult.Category} category
* @param {Object<string, LH.Result.ReportGroup>} [groupDefinitions]
* @return {Element}
* @param {Array<LH.ReportResult.AuditRef>} auditRefs
* @param {Record<string, LH.Result.ReportGroup>} groupDefinitions
* @param {LH.Result.Category['manualDescription']} [manualDescription]
* @return {Array<Element>}
*/
render(category, groupDefinitions = {}) {
const element = this.dom.createElement('div', 'lh-category');
this.createPermalinkSpan(element, category.id);
element.appendChild(this.renderCategoryHeader(category, groupDefinitions));
renderClumps(auditRefs, groupDefinitions = {}, manualDescription) {
const clumpElems = [];

// Top level clumps for audits, in order they will appear in the report.
/** @type {Map<TopLevelClumpId, Array<LH.ReportResult.AuditRef>>} */
Expand All @@ -461,7 +476,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 @@ -475,16 +490,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
40 changes: 19 additions & 21 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 @@ -210,23 +210,26 @@ 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 thumbnailResult = thumbnailAudit && thumbnailAudit.result;
if (thumbnailResult && thumbnailResult.details) {
timelineEl.id = thumbnailResult.id;
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');
Copy link
Collaborator

@connorjclark connorjclark Sep 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@paulirish paulirish Sep 21, 2020

Choose a reason for hiding this comment

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

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

for (const thumbnailAudit of thumbnailAudits) {
const result = thumbnailAudit.result;
if (result && result.details) {
timelineEl.id = result.id;
const filmstripEl = this.detailsRenderer.render(result.details);
filmstripEl && timelineEl.appendChild(filmstripEl);
}
}

// Budgets
/** @type {Array<Element>} */
const budgetTableEls = [];
['performance-budget', 'timing-budget'].forEach((id) => {
const audit = category.auditRefs.find(audit => audit.id === id);
const budgetAudits = category.auditRefs.filter(audit => audit.group === 'budgets');
budgetAudits.forEach(audit => {
if (audit && audit.result.details) {
const table = this.detailsRenderer.render(audit.result.details);
if (table) {
table.id = id;
table.id = audit.id;
table.classList.add('lh-audit');
budgetTableEls.push(table);
}
Expand Down Expand Up @@ -281,19 +284,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));

if (!passedAudits.length) return element;
// Everything else (passed, passed with warnings, n/a)
const renderedAudits = [...metricAudits, ...thumbnailAudits, ...budgetAudits,
...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.


const clumpOpts = {
auditRefs: passedAudits,
groupDefinitions: groups,
};
const passedElem = this.renderClump('passed', clumpOpts);
element.appendChild(passedElem);
const clumpElems = this.renderClumps(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 @@ -69,7 +69,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 @@ -166,11 +176,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 passedAuditIds = category.auditRefs.filter(
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(passedAuditIds).toEqual(passedElemIds);
});

// Unsupported by perf cat renderer right now.
Expand Down
12 changes: 8 additions & 4 deletions lighthouse-core/test/results/sample_v2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4538,6 +4538,11 @@
"weight": 0,
"group": "diagnostics"
},
{
"id": "screenshot-thumbnails",
"weight": 0,
"group": "filmstrip"
},
{
"id": "network-requests",
"weight": 0
Expand All @@ -4562,10 +4567,6 @@
"id": "metrics",
"weight": 0
},
{
"id": "screenshot-thumbnails",
"weight": 0
},
{
"id": "final-screenshot",
"weight": 0
Expand Down Expand Up @@ -5111,6 +5112,9 @@
"title": "Diagnostics",
"description": "More information about the performance of your application. These numbers don't [directly affect](https://web.dev/performance-scoring/) the Performance score."
},
"filmstrip": {
"title": ""
},
"pwa-fast-reliable": {
"title": "Fast and reliable"
},
Expand Down