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.

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

Jump to bottom

core: change 'not-applicable' to 'notApplicable' #6783

Merged
merged 9 commits into from
Jan 5, 2019
Merged

core: change 'not-applicable' to 'notApplicable' #6783

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
41977eb
Replaced not-applicable with notApplicable.
exterkamp Dec 12, 2018
ce31b4b
Add backward compat with not-applicable in viewer.
exterkamp Dec 12, 2018
d6b4f2f
Added underscore convergence to proto-preprocessor also.
exterkamp Dec 12, 2018
d44d806
Comment fixes and removing '.
exterkamp Dec 13, 2018
1347e73
fixed comments + changed clump class to .lh-clump--notapplicable and …
exterkamp Dec 20, 2018
fdf9755
Handled undefined scoreDisplayMode. Fixed missing score display mode…
exterkamp Jan 2, 2019
37a42f6
Changed the last few not-applicables.
exterkamp Jan 4, 2019
495884e
Removed guardrails, made tests follow types accurately.
exterkamp Jan 4, 2019
a7e119c
Haha whoops!
exterkamp Jan 5, 2019
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
26 changes: 13 additions & 13 deletions lighthouse-cli/test/smokehouse/offline-local/offline-expectations.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
14 changes: 7 additions & 7 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 @@ -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,18 +45,18 @@ 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',
Copy link
Member

Choose a reason for hiding this comment

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

need to update

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh we should get a puppeteer screenshot test in for the report to catch these css mismatches

Copy link
Member

Choose a reason for hiding this comment

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

oh we should get a puppeteer screenshot test in for the report to catch these css mismatches

good idea. Don't we also have a jsdom test or two looking for the not applicable results? Concerning they weren't found...

Copy link
Member

Choose a reason for hiding this comment

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

Concerning they weren't found...

oh, because the test selectors weren't updated either :)

But now I'm questioning if it should be updated :) We don't currently create the clump names dynamically, so it can be whatever we want. Commonly notApplicable in js goes to not-applicable in css, but do we care? Or is notapplicable fine? Keeping camelCase seems gross, though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would go for not-applicable makes it easier to read in my opinion

},
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
11 changes: 6 additions & 5 deletions lighthouse-core/report/html/renderer/util.js
View file
Open in desktop
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 @@ -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/test/audits/audit-test.js
View file
Open in desktop
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
View file
Open in desktop
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
6 changes: 3 additions & 3 deletions lighthouse-core/test/report/html/renderer/category-renderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ describe('CategoryRenderer', () => {
'.lh-clump--not-applicable .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,
notApplicableCount,
Expand Down Expand Up @@ -202,7 +202,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 +213,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 Down
5 changes: 2 additions & 3 deletions lighthouse-core/test/report/html/renderer/report-renderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ describe('ReportRenderer', () => {

let notApplicableCount = 0;
Object.values(clonedSampleResult.audits).forEach(audit => {
if (audit.scoreDisplayMode === 'not-applicable') {
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
if (audit.scoreDisplayMode === 'notApplicable') {
notApplicableCount++;
audit.scoreDisplayMode = 'not_applicable';
}
Expand All @@ -196,8 +196,7 @@ describe('ReportRenderer', () => {
const container = renderer._dom._document.body;
const reportElement = renderer.renderReport(sampleResults, container);
const notApplicableElementCount = reportElement
.querySelectorAll('.lh-audit--not-applicable').length;

.querySelectorAll('.lh-audit--notApplicable').length;
assert.strictEqual(notApplicableCount, notApplicableElementCount);
});
});
4 changes: 2 additions & 2 deletions lighthouse-core/test/report/html/renderer/util-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,12 +171,12 @@ describe('util helpers', () => {
});

describe('#prepareReportResult', () => {
it('corrects underscored `not-applicable` scoreDisplayMode', () => {
it('corrects underscored `notApplicable` scoreDisplayMode', () => {
const clonedSampleResult = JSON.parse(JSON.stringify(sampleResult));

let notApplicableCount = 0;
Object.values(clonedSampleResult.audits).forEach(audit => {
if (audit.scoreDisplayMode === 'not-applicable') {
if (audit.scoreDisplayMode === 'notApplicable') {
notApplicableCount++;
audit.scoreDisplayMode = 'not_applicable';
}
Expand Down
Loading