Skip to content

Commit

Permalink
core(scoreDisplayMode): change 'not-applicable' to 'notApplicable' (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
exterkamp authored and brendankenny committed Jan 5, 2019
1 parent ca87ea3 commit c164e75
Show file tree
Hide file tree
Showing 20 changed files with 146 additions and 125 deletions.
2 changes: 1 addition & 1 deletion docs/understanding-results.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ An object containing the results of the audits, keyed by their name.
| rawValue | <code>boolean&#124;number</code> | The unscored value determined by the audit. Typically this will match the score if there's no additional information to impart. For performance audits, this value is typically a number indicating the metric value. |
| displayValue | `string` | The string to display in the report alongside audit results. If empty, nothing additional is shown. This is typically used to explain additional information such as the number and nature of failing items. |
| score | <code>number</code> | The scored value determined by the audit as a number `0-1`, representing displayed scores of 0-100. |
| scoreDisplayMode | <code>"binary"&#124;"numeric"&#124;"error"&#124;"manual"&#124;"not-applicable"&#124;"informative"</code> | A string identifying how the score should be interpreted for display i.e. is the audit pass/fail (score of 1 or 0), did it fail, should it be ignored, or are there shades of gray (scores between 0-1 inclusive). |
| scoreDisplayMode | <code>"binary"&#124;"numeric"&#124;"error"&#124;"manual"&#124;"notApplicable"&#124;"informative"</code> | A string identifying how the score should be interpreted for display i.e. is the audit pass/fail (score of 1 or 0), did it fail, should it be ignored, or are there shades of gray (scores between 0-1 inclusive). |
| details | `Object` | Extra information found by the audit necessary for display. The structure of this object varies from audit to audit. The structure of this object is somewhat stable between minor version bumps as this object is used to render the HTML report. |


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ module.exports = [
score: 1,
},
'user-timings': {
scoreDisplayMode: 'not-applicable',
scoreDisplayMode: 'notApplicable',
},
'critical-request-chains': {
scoreDisplayMode: 'not-applicable',
scoreDisplayMode: 'notApplicable',
},
'installable-manifest': {
score: 0,
Expand All @@ -75,22 +75,22 @@ module.exports = [
score: 0,
},
'aria-valid-attr': {
scoreDisplayMode: 'not-applicable',
scoreDisplayMode: 'notApplicable',
},
'aria-allowed-attr': {
scoreDisplayMode: 'not-applicable',
scoreDisplayMode: 'notApplicable',
},
'color-contrast': {
score: 1,
},
'image-alt': {
scoreDisplayMode: 'not-applicable',
scoreDisplayMode: 'notApplicable',
},
'label': {
scoreDisplayMode: 'not-applicable',
scoreDisplayMode: 'notApplicable',
},
'tabindex': {
scoreDisplayMode: 'not-applicable',
scoreDisplayMode: 'notApplicable',
},
'content-width': {
score: 1,
Expand Down Expand Up @@ -121,10 +121,10 @@ module.exports = [
score: 1,
},
'user-timings': {
scoreDisplayMode: 'not-applicable',
scoreDisplayMode: 'notApplicable',
},
'critical-request-chains': {
scoreDisplayMode: 'not-applicable',
scoreDisplayMode: 'notApplicable',
},
'installable-manifest': {
score: 1,
Expand All @@ -136,10 +136,10 @@ module.exports = [
score: 0,
},
'aria-valid-attr': {
scoreDisplayMode: 'not-applicable',
scoreDisplayMode: 'notApplicable',
},
'aria-allowed-attr': {
scoreDisplayMode: 'not-applicable',
scoreDisplayMode: 'notApplicable',
},
'color-contrast': {
score: 1,
Expand All @@ -148,10 +148,10 @@ module.exports = [
score: 0,
},
'label': {
scoreDisplayMode: 'not-applicable',
scoreDisplayMode: 'notApplicable',
},
'tabindex': {
scoreDisplayMode: 'not-applicable',
scoreDisplayMode: 'notApplicable',
},
'content-width': {
score: 1,
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-cli/test/smokehouse/seo/expectations.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ module.exports = [
},
'robots-txt': {
rawValue: true,
scoreDisplayMode: 'not-applicable',
scoreDisplayMode: 'notApplicable',
},
},
},
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class Audit {
BINARY: 'binary',
MANUAL: 'manual',
INFORMATIVE: 'informative',
NOT_APPLICABLE: 'not-applicable',
NOT_APPLICABLE: 'notApplicable',
ERROR: 'error',
};
}
Expand Down
11 changes: 7 additions & 4 deletions lighthouse-core/lib/proto-preprocessor.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,14 @@ function processForProto(result) {
Object.keys(reportJson.audits).forEach(auditName => {
const audit = reportJson.audits[auditName];

// Rewrite the 'not-applicable' scoreDisplayMode to 'not_applicable'. #6201
// Rewrite 'not-applicable' and 'not_applicable' scoreDisplayMode to 'notApplicable'. #6201, #6783.
if (audit.scoreDisplayMode) {
if (audit.scoreDisplayMode === 'not-applicable') {
// @ts-ignore Breaking the LH.Result type
audit.scoreDisplayMode = 'not_applicable';
// @ts-ignore ts properly flags this as invalid as it should not happen,
// but remains in preprocessor to protect from proto translation errors from
// old LHRs.
// eslint-disable-next-line max-len
if (audit.scoreDisplayMode === 'not-applicable' || audit.scoreDisplayMode === 'not_applicable') {
audit.scoreDisplayMode = 'notApplicable';
}
}
// Drop raw values. #6199
Expand Down
20 changes: 10 additions & 10 deletions lighthouse-core/report/html/renderer/category-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
/** @typedef {import('./report-renderer.js')} ReportRenderer */
/** @typedef {import('./details-renderer.js')} DetailsRenderer */
/** @typedef {import('./util.js')} Util */
/** @typedef {'failed'|'manual'|'passed'|'not-applicable'} TopLevelClumpId */
/** @typedef {'failed'|'manual'|'passed'|'notApplicable'} TopLevelClumpId */

class CategoryRenderer {
/**
Expand All @@ -45,20 +45,20 @@ class CategoryRenderer {
*/
get _clumpDisplayInfo() {
return {
'failed': {
failed: {
className: 'lh-clump--failed',
},
'manual': {
manual: {
title: Util.UIStrings.manualAuditsGroupTitle,
className: 'lh-clump--manual',
},
'passed': {
passed: {
title: Util.UIStrings.passedAuditsGroupTitle,
className: 'lh-clump--passed',
},
'not-applicable': {
notApplicable: {
title: Util.UIStrings.notApplicableAuditsGroupTitle,
className: 'lh-clump--not-applicable',
className: 'lh-clump--notapplicable',
},
};
}
Expand Down Expand Up @@ -153,7 +153,7 @@ class CategoryRenderer {
*/
_setRatingClass(element, score, scoreDisplayMode) {
const rating = Util.calculateRating(score, scoreDisplayMode);
element.classList.add(`lh-audit--${rating}`, `lh-audit--${scoreDisplayMode}`);
element.classList.add(`lh-audit--${rating}`, `lh-audit--${scoreDisplayMode.toLowerCase()}`);
return element;
}

Expand Down Expand Up @@ -279,7 +279,7 @@ class CategoryRenderer {

/**
* Renders a clump (a grouping of groups), under a status of failed, manual,
* passed, or not-applicable. The result ends up something like:
* passed, or notApplicable. The result ends up something like:
*
* clump (e.g. 'failed')
* ├── audit 1 (w/o group)
Expand Down Expand Up @@ -370,7 +370,7 @@ class CategoryRenderer {
*/
_getClumpIdForAuditRef(auditRef) {
const scoreDisplayMode = auditRef.result.scoreDisplayMode;
if (scoreDisplayMode === 'manual' || scoreDisplayMode === 'not-applicable') {
if (scoreDisplayMode === 'manual' || scoreDisplayMode === 'notApplicable') {
return scoreDisplayMode;
}

Expand All @@ -397,7 +397,7 @@ class CategoryRenderer {
clumps.set('failed', []);
clumps.set('manual', []);
clumps.set('passed', []);
clumps.set('not-applicable', []);
clumps.set('notApplicable', []);

// Sort audits into clumps.
for (const auditRef of category.auditRefs) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class PwaCategoryRenderer extends CategoryRenderer {

const auditRefs = category.auditRefs;

// Regular audits aren't split up into pass/fail/not-applicable clumps, they're
// Regular audits aren't split up into pass/fail/notApplicable clumps, they're
// all put in a top-level clump that isn't expandable/collapsable.
const regularAuditRefs = auditRefs.filter(ref => ref.result.scoreDisplayMode !== 'manual');
const auditsElem = this._renderAudits(regularAuditRefs, groupDefinitions);
Expand Down
13 changes: 7 additions & 6 deletions lighthouse-core/report/html/renderer/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,13 @@ class Util {
if (typeof clone.categories !== 'object') throw new Error('No categories provided.');
clone.reportCategories = Object.values(clone.categories);

// The proto process turns 'not-applicable' into 'not_applicable'. Correct this to support both.
// TODO: remove when underscore/hyphen proto issue is resolved. See #6371, #6201.
// Turn 'not-applicable' and 'not_applicable' into 'notApplicable' to support old reports.
// TODO: remove when underscore/hyphen proto issue is resolved. See #6371, #6201, #6783.
for (const audit of Object.values(clone.audits)) {
// @ts-ignore tsc rightly flags that this value shouldn't occur.
if (audit.scoreDisplayMode === 'not_applicable') {
audit.scoreDisplayMode = 'not-applicable';
// eslint-disable-next-line max-len
if (audit.scoreDisplayMode === 'not_applicable' || audit.scoreDisplayMode === 'not-applicable') {
audit.scoreDisplayMode = 'notApplicable';
}
}

Expand Down Expand Up @@ -143,7 +144,7 @@ class Util {
static showAsPassed(audit) {
switch (audit.scoreDisplayMode) {
case 'manual':
case 'not-applicable':
case 'notApplicable':
return true;
case 'error':
case 'informative':
Expand All @@ -163,7 +164,7 @@ class Util {
*/
static calculateRating(score, scoreDisplayMode) {
// Handle edge cases first, manual and not applicable receive 'pass', errored audits receive 'error'
if (scoreDisplayMode === 'manual' || scoreDisplayMode === 'not-applicable') {
if (scoreDisplayMode === 'manual' || scoreDisplayMode === 'notApplicable') {
return RATINGS.PASS.label;
} else if (scoreDisplayMode === 'error') {
return RATINGS.ERROR.label;
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/report/html/report-styles.css
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@
content: '';
background-image: var(--check-icon-url);
}
.lh-clump--not-applicable > summary .lh-audit-group__header::before {
.lh-clump--notapplicable > summary .lh-audit-group__header::before {
content: '';
background-image: var(--remove-circle-icon-url);
}
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/test/audits/audit-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ describe('Audit', () => {
const providedResult = {rawValue: true, notApplicable: true};
const result = Audit.generateAuditResult(B, providedResult);
assert.equal(result.score, null);
assert.equal(result.scoreDisplayMode, 'not-applicable');
assert.equal(result.scoreDisplayMode, 'notApplicable');
});

it('sets state of failed audits', () => {
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/test/lib/proto-preprocessor-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ describe('processing for proto', () => {
const expectation = {
'audits': {
'critical-request-chains': {
'scoreDisplayMode': 'not_applicable',
'scoreDisplayMode': 'notApplicable',
'displayValue': 'hello %d | 123',
},
},
Expand Down
49 changes: 33 additions & 16 deletions lighthouse-core/test/report/html/renderer/category-renderer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,27 +59,42 @@ describe('CategoryRenderer', () => {
assert.equal(title.textContent, auditRef.result.title);
assert.ok(description.querySelector('a'), 'audit help text contains coverted markdown links');
assert.ok(auditDOM.classList.contains('lh-audit--fail'));
assert.ok(auditDOM.classList.contains(`lh-audit--${auditRef.result.scoreDisplayMode}`));
assert.ok(
auditDOM.classList.contains(`lh-audit--${auditRef.result.scoreDisplayMode.toLowerCase()}`));
});

it('renders an audit explanation when appropriate', () => {
const audit1 = renderer.renderAudit({
scoreDisplayMode: 'binary', score: 0,
result: {description: 'help text', explanation: 'A reason', title: 'Audit title'},
result: {
title: 'Audit title',
explanation: 'A reason',
description: 'help text',
scoreDisplayMode: 'binary',
score: 0,
},
});
assert.ok(audit1.querySelector('.lh-audit-explanation'));

const audit2 = renderer.renderAudit({
scoreDisplayMode: 'binary', score: 0,
result: {description: 'help text', title: 'Audit title'},
result: {
title: 'Audit title',
description: 'help text',
scoreDisplayMode: 'binary',
score: 0,
},
});
assert.ok(!audit2.querySelector('.lh-audit-explanation'));
});

it('renders an informative audit', () => {
const auditDOM = renderer.renderAudit({
id: 'informative', score: 0,
result: {title: 'It informs', description: 'help text', scoreDisplayMode: 'informative'},
id: 'informative',
result: {
title: 'It informs',
description: 'help text',
scoreDisplayMode: 'informative',
score: 0,
},
});

assert.ok(auditDOM.matches('.lh-audit--informative'));
Expand All @@ -89,10 +104,11 @@ describe('CategoryRenderer', () => {
const auditResult = {
title: 'Audit',
description: 'Learn more',
scoreDisplayMode: 'informative',
warnings: ['It may not have worked!'],
score: 1,
};
const auditDOM = renderer.renderAudit({id: 'foo', score: 1, result: auditResult});
const auditDOM = renderer.renderAudit({id: 'foo', result: auditResult});
const warningEl = auditDOM.querySelector('.lh-warnings');
assert.ok(warningEl, 'did not render warning message');
assert.ok(warningEl.textContent.includes(auditResult.warnings[0]), 'warning message provided');
Expand All @@ -102,10 +118,11 @@ describe('CategoryRenderer', () => {
const auditResult = {
title: 'Audit',
description: 'Learn more',
scoreDisplayMode: 'informative',
warnings: ['It may not have worked!', 'You should read this, though'],
score: 1,
};
const auditDOM = renderer.renderAudit({id: 'foo', score: 1, result: auditResult});
const auditDOM = renderer.renderAudit({id: 'foo', result: auditResult});
const warningEl = auditDOM.querySelector('.lh-warnings');
assert.ok(warningEl, 'did not render warning message');
assert.ok(warningEl.textContent.includes(auditResult.warnings[0]), '1st warning provided');
Expand Down Expand Up @@ -158,19 +175,19 @@ describe('CategoryRenderer', () => {
const a11yCategory = sampleResults.reportCategories.find(cat => cat.id === 'accessibility');
const categoryDOM = renderer.render(a11yCategory, sampleResults.categoryGroups);
assert.ok(categoryDOM.querySelector(
'.lh-clump--not-applicable .lh-audit-group__summary'));
'.lh-clump--notapplicable .lh-audit-group__summary'));

const notApplicableCount = a11yCategory.auditRefs.reduce((sum, audit) =>
sum += audit.result.scoreDisplayMode === 'not-applicable' ? 1 : 0, 0);
sum += audit.result.scoreDisplayMode === 'notApplicable' ? 1 : 0, 0);
assert.equal(
categoryDOM.querySelectorAll('.lh-clump--not-applicable .lh-audit').length,
categoryDOM.querySelectorAll('.lh-clump--notapplicable .lh-audit').length,
notApplicableCount,
'score shows informative and dash icon'
);

const bestPracticeCat = sampleResults.reportCategories.find(cat => cat.id === 'best-practices');
const categoryDOM2 = renderer.render(bestPracticeCat, sampleResults.categoryGroups);
assert.ok(!categoryDOM2.querySelector('.lh-clump--not-applicable'));
assert.ok(!categoryDOM2.querySelector('.lh-clump--notapplicable'));
});

describe('category with groups', () => {
Expand Down Expand Up @@ -202,7 +219,7 @@ describe('CategoryRenderer', () => {
it.skip('renders the failed audits grouped by group', () => {
const categoryDOM = renderer.render(category, sampleResults.categoryGroups);
const failedAudits = category.auditRefs.filter(audit => {
return audit.result.score !== 1 && !audit.result.scoreDisplayMode === 'not-applicable';
return audit.result.score !== 1 && !audit.result.scoreDisplayMode === 'notApplicable';
});
const failedAuditTags = new Set(failedAudits.map(audit => audit.group));

Expand All @@ -213,7 +230,7 @@ describe('CategoryRenderer', () => {
it('renders the passed audits grouped by group', () => {
const categoryDOM = renderer.render(category, sampleResults.categoryGroups);
const passedAudits = category.auditRefs.filter(audit =>
audit.result.scoreDisplayMode !== 'not-applicable' && audit.result.score === 1);
audit.result.scoreDisplayMode !== 'notApplicable' && audit.result.score === 1);
const passedAuditTags = new Set(passedAudits.map(audit => audit.group));

const passedAuditGroups = categoryDOM.querySelectorAll('.lh-clump--passed .lh-audit-group');
Expand All @@ -233,7 +250,7 @@ describe('CategoryRenderer', () => {
const failedAudits = elem.querySelectorAll('.lh-clump--failed .lh-audit__index');
const manualAudits = elem.querySelectorAll('.lh-clump--manual .lh-audit__index');
const notApplicableAudits =
elem.querySelectorAll('.lh-clump--not-applicable .lh-audit__index');
elem.querySelectorAll('.lh-clump--notapplicable .lh-audit__index');

const assertAllTheIndices = (nodeList) => {
// Must be at least one for a decent test.
Expand Down
Loading

0 comments on commit c164e75

Please sign in to comment.