Skip to content
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.

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

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

Merged
merged 11 commits into from
Mar 13, 2018
1 change: 0 additions & 1 deletion lighthouse-core/config/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,6 @@ module.exports = {
},
'pwa': {
name: 'Progressive Web App',
weight: 1,
description: 'These checks validate the aspects of a Progressive Web App, as specified by the baseline [PWA Checklist](https://developers.google.com/web/progressive-web-apps/checklist).',
audits: [
{id: 'service-worker', weight: 1},
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/report/v2/renderer/category-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class CategoryRenderer {
scoreEl.classList.add('lh-score--manual');
}

return this._populateScore(tmpl, audit.score, scoringMode, title, description);
return this._populateScore(tmpl, audit.result.score, scoringMode, title, description);
}

/**
Expand Down Expand Up @@ -294,7 +294,7 @@ class CategoryRenderer {

if (audit.result.notApplicable) {
group.notApplicable.push(audit);
} else if (audit.score === 100 && !audit.result.debugString) {
} else if (audit.result.score === 100 && !audit.result.debugString) {
group.passed.push(audit);
} else {
group.failed.push(audit);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
_renderTimelineMetricAudit(audit, scale) {
const tmpl = this.dom.cloneTemplate('#tmpl-lh-timeline-metric', this.templateContext);
const element = this.dom.find('.lh-timeline-metric', tmpl);
element.classList.add(`lh-timeline-metric--${Util.calculateRating(audit.score)}`);
element.classList.add(`lh-timeline-metric--${Util.calculateRating(audit.result.score)}`);

const titleEl = this.dom.find('.lh-timeline-metric__title', tmpl);
titleEl.textContent = audit.result.description;
Expand Down Expand Up @@ -49,7 +49,7 @@ class PerformanceCategoryRenderer extends CategoryRenderer {

const element = this.dom.createElement('details', [
'lh-perf-hint',
`lh-perf-hint--${Util.calculateRating(audit.score)}`,
`lh-perf-hint--${Util.calculateRating(audit.result.score)}`,
'lh-expandable-details',
].join(' '));

Expand Down Expand Up @@ -152,7 +152,7 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
element.appendChild(metricAuditsEl);

const hintAudits = category.audits
.filter(audit => audit.group === 'perf-hint' && audit.score < 100)
.filter(audit => audit.group === 'perf-hint' && audit.result.score < 100)
.sort((auditA, auditB) => auditB.result.rawValue - auditA.result.rawValue);
if (hintAudits.length) {
const maxWaste = Math.max(...hintAudits.map(audit => audit.result.rawValue));
Expand All @@ -164,7 +164,7 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
}

const infoAudits = category.audits
.filter(audit => audit.group === 'perf-info' && audit.score < 100);
.filter(audit => audit.group === 'perf-info' && audit.result.score < 100);
if (infoAudits.length) {
const infoAuditsEl = this.renderAuditGroup(groups['perf-info'], {expandable: false});
infoAudits.forEach(item => infoAuditsEl.appendChild(this.renderAudit(item)));
Expand All @@ -174,7 +174,7 @@ class PerformanceCategoryRenderer extends CategoryRenderer {

const passedElements = category.audits
.filter(audit => (audit.group === 'perf-hint' || audit.group === 'perf-info') &&
audit.score === 100)
audit.result.score === 100)
.map(audit => this.renderAudit(audit));

if (!passedElements.length) return element;
Expand Down
22 changes: 18 additions & 4 deletions lighthouse-core/report/v2/renderer/report-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,24 +155,39 @@ class ReportRenderer {
perfCategoryRenderer.setTemplateContext(this._templateContext);

const categories = reportSection.appendChild(this._dom.createElement('div', 'lh-categories'));

ReportRenderer.assignAuditResultsIntoCategories(report.audits, report.reportCategories);

for (const category of report.reportCategories) {
if (scoreHeader) {
scoreHeader.appendChild(categoryRenderer.renderScoreGauge(category));
}

let renderer = categoryRenderer;

if (category.id === 'performance') {
renderer = perfCategoryRenderer;
}

categories.appendChild(renderer.render(category, report.reportGroups));
}

reportSection.appendChild(this._renderReportFooter(report));

return container;
}

/**
* Place the AuditResult into the auditDfn (which has just weight & group)
* @param {!Object<string, !ReportRenderer.AuditResultJSON>} audits
* @param {!Array<!ReportRenderer.CategoryJSON>} reportCategories
*/
static assignAuditResultsIntoCategories(audits, reportCategories) {
for (const category of reportCategories) {
category.audits.forEach(auditMeta => {
const result = audits[auditMeta.id];
auditMeta.result = result;
});
}
}
}

if (typeof module !== 'undefined' && module.exports) {
Expand All @@ -184,7 +199,6 @@ if (typeof module !== 'undefined' && module.exports) {
/**
* @typedef {{
* id: string,
* score: number,
* weight: number,
* group: (string|undefined),
* result: (ReportRenderer.AuditResultJSON|undefined)
Expand All @@ -205,6 +219,7 @@ ReportRenderer.AuditJSON; // eslint-disable-line no-unused-expressions
* scoringMode: string,
* extendedInfo: Object,
* error: boolean,
* score: number,
* details: (!DetailsRenderer.DetailsJSON|undefined),
* }}
*/
Expand All @@ -214,7 +229,6 @@ ReportRenderer.AuditResultJSON; // eslint-disable-line no-unused-expressions
* @typedef {{
* name: string,
* id: string,
* weight: number,
* score: number,
* description: string,
* audits: !Array<!ReportRenderer.AuditJSON>
Expand Down
11 changes: 6 additions & 5 deletions lighthouse-core/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,11 @@ class Runner {
const resultsById = {};
for (const audit of runResults.auditResults) resultsById[audit.name] = audit;

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

Choose a reason for hiding this comment

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

does this mean we can do it now? :)

Copy link
Member

Choose a reason for hiding this comment

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

do it!

categories = Object.keys(opts.config.categories).map(key => opts.config.categories[key]);
}

return {
Expand All @@ -108,8 +110,7 @@ class Runner {
audits: resultsById,
artifacts: runResults.artifacts,
runtimeConfig: Runner.getRuntimeConfig(opts.flags),
score: report ? report.score : 0,
reportCategories: report ? report.categories : [],
reportCategories: categories,
Copy link
Member

Choose a reason for hiding this comment

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

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

reportGroups: opts.config.groups,
};
}).catch(err => {
Expand Down Expand Up @@ -192,7 +193,7 @@ class Runner {
* @param {{}} resultsById
*/
static _scoreAndCategorize(opts, resultsById) {
return ReportScoring.scoreAllCategories(opts.config, resultsById);
ReportScoring.scoreAllCategories(opts.config, resultsById);
}

/**
Expand Down
43 changes: 24 additions & 19 deletions lighthouse-core/scoring.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,30 +13,33 @@ class ReportScoring {
* @return {number}
*/
static arithmeticMean(items) {
const results = items.reduce((result, item) => {
const score = Number(item.score) || 0;
const weight = Number(item.weight) || 0;
return {
weight: result.weight + weight,
sum: result.sum + score * weight,
};
}, {weight: 0, sum: 0});
const results = items.reduce(
(result, item) => {
const score = Number(item.score) || 0;
const weight = Number(item.weight) || 0;
return {
weight: result.weight + weight,
sum: result.sum + score * weight,
};
},
{weight: 0, sum: 0}
);

return (results.sum / results.weight) || 0;
return results.sum / results.weight || 0;
}

/**
* Returns the report JSON object with computed scores.
* @param {{categories: !Object<string, {id: string|undefined, weight: number|undefined, audits: !Array<{id: string, weight: number|undefined}>}>}} config
* @param {{categories: !Object<string, {id: string|undefined, weight: number|undefined, score: number|undefined, audits: !Array<{id: string, weight: number|undefined}>}>}} config
* @param {!Object<{score: ?number|boolean|undefined}>} resultsByAuditId
* @return {{score: number, categories: !Array<{audits: !Array<{score: number, result: !Object}>}>}}
* @void
*/
static scoreAllCategories(config, resultsByAuditId) {
const categories = Object.keys(config.categories).map(categoryId => {
Object.keys(config.categories).forEach(categoryId => {
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

more a comment for #4690 I suppose

Copy link
Member Author

Choose a reason for hiding this comment

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

let auditScore = Number(result.score) || 0;
Expand All @@ -53,15 +56,17 @@ class ReportScoring {
result.informative = true;
}

return Object.assign({}, audit, {result, score: auditScore});
result.score = auditScore;
});

const categoryScore = ReportScoring.arithmeticMean(audits);
return Object.assign({}, category, {audits, score: categoryScore});
const scores = category.audits.map(audit => ({
score: resultsByAuditId[audit.id].score,
weight: audit.weight,
}));
const categoryScore = ReportScoring.arithmeticMean(scores);
// mutate config.categories[].score
category.score = categoryScore;
});

const overallScore = ReportScoring.arithmeticMean(categories);
return {score: overallScore, categories};
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const DetailsRenderer = require('../../../../report/v2/renderer/details-renderer
const CriticalRequestChainRenderer = require(
'../../../../report/v2/renderer/crc-details-renderer.js');
const CategoryRenderer = require('../../../../report/v2/renderer/category-renderer.js');
const ReportRenderer = require('../../../../report/v2/renderer/report-renderer.js');
const sampleResults = require('../../../results/sample_v2.json');

const TEMPLATE_FILE = fs.readFileSync(__dirname + '/../../../../report/v2/templates.html', 'utf8');
Expand All @@ -33,6 +34,9 @@ describe('CategoryRenderer', () => {
const dom = new DOM(document);
const detailsRenderer = new DetailsRenderer(dom);
renderer = new CategoryRenderer(dom, detailsRenderer);

ReportRenderer.assignAuditResultsIntoCategories(sampleResults.audits,
sampleResults.reportCategories);
});

after(() => {
Expand Down Expand Up @@ -178,7 +182,7 @@ describe('CategoryRenderer', () => {
it.skip('renders the failed audits grouped by group', () => {
const categoryDOM = renderer.render(category, sampleResults.reportGroups);
const failedAudits = category.audits.filter(audit => {
return audit.score !== 100 && !audit.result.notApplicable;
return audit.result.score !== 100 && !audit.result.notApplicable;
});
const failedAuditTags = new Set(failedAudits.map(audit => audit.group));

Expand All @@ -188,10 +192,10 @@ describe('CategoryRenderer', () => {

it('renders the passed audits grouped by group', () => {
const categoryDOM = renderer.render(category, sampleResults.reportGroups);

const passedAudits = category.audits.filter(audit =>
!audit.result.notApplicable && audit.score === 100);
!audit.result.notApplicable && audit.result.score === 100);
const passedAuditTags = new Set(passedAudits.map(audit => audit.group));

const passedAuditGroups = categoryDOM.querySelectorAll('.lh-passed-audits .lh-audit-group');
assert.equal(passedAuditGroups.length, passedAuditTags.size);
});
Expand Down Expand Up @@ -219,7 +223,7 @@ describe('CategoryRenderer', () => {
it('doesnt create a passed section if there were 0 passed', () => {
const origCategory = sampleResults.reportCategories.find(c => c.id === 'pwa');
const category = JSON.parse(JSON.stringify(origCategory));
category.audits.forEach(audit => audit.score = 0);
category.audits.forEach(audit => audit.result.score = 0);
const elem = renderer.render(category, sampleResults.reportGroups);
const passedAudits = elem.querySelectorAll('.lh-passed-audits > .lh-audit');
const failedAudits = elem.querySelectorAll('.lh-failed-audits > .lh-audit');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const DetailsRenderer = require('../../../../report/v2/renderer/details-renderer
const CriticalRequestChainRenderer = require(
'../../../../report/v2/renderer/crc-details-renderer.js');
const CategoryRenderer = require('../../../../report/v2/renderer/category-renderer.js');
const ReportRenderer = require('../../../../report/v2/renderer/report-renderer.js');
const sampleResults = require('../../../results/sample_v2.json');

const TEMPLATE_FILE = fs.readFileSync(__dirname + '/../../../../report/v2/templates.html', 'utf8');
Expand All @@ -37,6 +38,8 @@ describe('PerfCategoryRenderer', () => {
const dom = new DOM(document);
const detailsRenderer = new DetailsRenderer(dom);
renderer = new PerformanceCategoryRenderer(dom, detailsRenderer);
ReportRenderer.assignAuditResultsIntoCategories(sampleResults.audits,
sampleResults.reportCategories);
});

after(() => {
Expand Down Expand Up @@ -81,7 +84,7 @@ describe('PerfCategoryRenderer', () => {
const categoryDOM = renderer.render(category, sampleResults.reportGroups);

const hintAudits = category.audits.filter(audit => audit.group === 'perf-hint' &&
audit.score !== 100);
audit.result.score !== 100);
const hintElements = categoryDOM.querySelectorAll('.lh-perf-hint');
assert.equal(hintElements.length, hintAudits.length);

Expand All @@ -99,7 +102,7 @@ describe('PerfCategoryRenderer', () => {
group: 'perf-hint',
result: {
rawValue: 100, debugString: 'Yikes!', description: 'Bug',
helpText: '',
helpText: '', score: 32,
details: {summary: {wastedMs: 3223}},
},
};
Expand Down Expand Up @@ -134,7 +137,8 @@ describe('PerfCategoryRenderer', () => {
score: 0,
group: 'perf-hint',
result: {
rawValue: 100, description: 'Bug', helpText: '',
rawValue: 100, description: 'Bug',
helpText: '', score: 32,
},
};

Expand All @@ -149,7 +153,7 @@ describe('PerfCategoryRenderer', () => {
const diagnosticSection = categoryDOM.querySelectorAll('.lh-category > .lh-audit-group')[2];

const diagnosticAudits = category.audits.filter(audit => audit.group === 'perf-info' &&
audit.score !== 100);
audit.result.score !== 100);
const diagnosticElements = diagnosticSection.querySelectorAll('.lh-audit');
assert.equal(diagnosticElements.length, diagnosticAudits.length);
});
Expand All @@ -159,7 +163,8 @@ describe('PerfCategoryRenderer', () => {
const passedSection = categoryDOM.querySelector('.lh-category > .lh-passed-audits');

const passedAudits = category.audits.filter(audit =>
audit.group && audit.group !== 'perf-metric' && audit.score === 100);
audit.group && audit.group !== 'perf-metric' &&
audit.result.score === 100);
const passedElements = passedSection.querySelectorAll('.lh-audit');
assert.equal(passedElements.length, passedAudits.length);
});
Expand Down
Loading