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: change 'not-applicable' to 'notApplicable' #6783

Merged
merged 9 commits into from
Jan 5, 2019
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
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
if (audit.scoreDisplayMode === 'not-applicable' || audit.scoreDisplayMode === 'not_applicable') {
audit.scoreDisplayMode = 'notApplicable';
}
}
// Drop raw values. #6199
Expand Down
23 changes: 13 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 */
exterkamp marked this conversation as resolved.
Show resolved Hide resolved

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,10 @@ 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}`);
if (typeof scoreDisplayMode === 'string') {
Copy link
Member

Choose a reason for hiding this comment

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

wait...how would it be possible for it not to be? That seems bad if it happens. Just in tests maybe (hopefully)?

Copy link
Member

Choose a reason for hiding this comment

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

from https://travis-ci.org/GoogleChrome/lighthouse/builds/470305380#L816, it looks like just the unit tests. We should fix those (is that what fdf9755 did?) and then not change the code here, I think. Better to trust our type system and fail fast if we defined/enforced things poorly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah fdf9755 fixed the unit tests and added guard rails for if the scoreDisplayMode was undefined. Good point tho, I removed the guardrails and made all the tests actually represent reality!

element.classList.add(`lh-audit--${scoreDisplayMode.toLowerCase()}`);
}
return element;
}

Expand Down Expand Up @@ -279,7 +282,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 +373,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 +400,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
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
if (audit.scoreDisplayMode === 'not_applicable' || audit.scoreDisplayMode === 'not-applicable') {
audit.scoreDisplayMode = 'notApplicable';
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
}
}

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 @@ -603,7 +603,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
4 changes: 2 additions & 2 deletions lighthouse-core/test/lib/proto-preprocessor-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ describe('processing for proto', () => {
const input = {
'audits': {
'critical-request-chains': {
'scoreDisplayMode': 'not-applicable',
'scoreDisplayMode': 'notApplicable',
Copy link
Member

Choose a reason for hiding this comment

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

ha, sorry, this one we want

Copy link
Member Author

Choose a reason for hiding this comment

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

delete not-applicable
don't delete not-applicable

I just can't impress you brendan 😭

'rawValue': 14.3,
'displayValue': ['hello %d', 123],
},
Expand All @@ -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
46 changes: 34 additions & 12 deletions lighthouse-core/test/report/html/renderer/category-renderer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,19 +59,41 @@ 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('handles an audit with no scoreDisplayMode', () => {
const audit1 = renderer.renderAudit({
score: 0,
result: {
description: 'help text',
explanation: 'A reason',
title: 'Audit title',
},
});
assert.ok(audit1.classList.contains('lh-audit--fail'));
});

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'},
score: 0,
result: {
scoreDisplayMode: 'binary',
description: 'help text',
explanation: 'A reason',
title: 'Audit title',
},
});
assert.ok(audit1.querySelector('.lh-audit-explanation'));

const audit2 = renderer.renderAudit({
scoreDisplayMode: 'binary', score: 0,
result: {description: 'help text', title: 'Audit title'},
score: 0,
result: {
scoreDisplayMode: 'binary',
description: 'help text',
title: 'Audit title',
},
});
assert.ok(!audit2.querySelector('.lh-audit-explanation'));
});
Expand Down Expand Up @@ -158,19 +180,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);
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
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 +224,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 +235,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 +255,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