Skip to content

Commit

Permalink
Add linting for all icons in manifest (mozilla#2677)
Browse files Browse the repository at this point in the history
  • Loading branch information
amccausl committed Jul 10, 2019
1 parent e356c51 commit 3b5bd84
Show file tree
Hide file tree
Showing 3 changed files with 221 additions and 16 deletions.
72 changes: 56 additions & 16 deletions src/parsers/manifestjson.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ async function getImageMetadata(io, iconPath) {
return probeImageSize(data);
}

function getNormalizedExtension(_path) {
return path
.extname(_path)
.substring(1)
.toLowerCase();
}

export default class ManifestJSONParser extends JSONParser {
constructor(
jsonString,
Expand Down Expand Up @@ -499,22 +506,58 @@ export default class ManifestJSONParser extends JSONParser {
}

validateIcons() {
const icons = [];

if (this.parsedJSON.icons) {
Object.keys(this.parsedJSON.icons).forEach((size) => {
icons.push([size, this.parsedJSON.icons[size]]);
});
}

// Check for default_icon key at each of the action properties
['browser_action', 'page_action', 'sidebar_action'].forEach((key) => {
if (this.parsedJSON[key] && this.parsedJSON[key].default_icon) {
if (typeof this.parsedJSON[key].default_icon === 'string') {
icons.push([null, this.parsedJSON[key].default_icon]);
} else {
Object.keys(this.parsedJSON[key].default_icon).forEach((size) => {
icons.push([size, this.parsedJSON[key].default_icon[size]]);
});
}
}
});

// Check for the theme_icons from the browser_action
if (
this.parsedJSON.browser_action &&
this.parsedJSON.browser_action.theme_icons
) {
this.parsedJSON.browser_action.theme_icons.forEach((icon) => {
if (icon.dark) {
icons.push([icon.size, icon.dark]);
}
if (icon.light) {
icons.push([icon.size, icon.light]);
}
});
}

const promises = [];
const { icons } = this.parsedJSON;
Object.keys(icons).forEach((size) => {
const _path = normalizePath(icons[size]);
const errorIcons = [];
icons.forEach(([size, iconPath]) => {
const _path = normalizePath(iconPath);
if (!Object.prototype.hasOwnProperty.call(this.io.files, _path)) {
this.collector.addError(messages.manifestIconMissing(_path));
this.isValid = false;
if (!errorIcons.includes(_path)) {
this.collector.addError(messages.manifestIconMissing(_path));
this.isValid = false;
errorIcons.push(_path);
}
} else if (
!IMAGE_FILE_EXTENSIONS.includes(
_path
.split('.')
.pop()
.toLowerCase()
)
!IMAGE_FILE_EXTENSIONS.includes(getNormalizedExtension(_path))
) {
this.collector.addWarning(messages.WRONG_ICON_EXTENSION);
if (!errorIcons.includes(_path)) {
this.collector.addWarning(messages.WRONG_ICON_EXTENSION);
}
} else {
promises.push(this.validateIcon(_path, size));
}
Expand All @@ -524,10 +567,7 @@ export default class ManifestJSONParser extends JSONParser {

async validateThemeImage(imagePath, manifestPropName) {
const _path = normalizePath(imagePath);
const ext = path
.extname(imagePath)
.substring(1)
.toLowerCase();
const ext = getNormalizedExtension(imagePath);

const fileExists = this.validateFileExistsInPackage(
_path,
Expand Down
6 changes: 6 additions & 0 deletions tests/unit/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,12 @@ export function assertHasMatchingError(errors, expected) {
expect(errors.some((error) => isMatch(error, expected))).toBe(true);
}

export function assertHasMatchingErrorCount(errors, expected, count) {
expect(Array.isArray(errors)).toBe(true);
expect(errors.length).toBeGreaterThan(0);
expect(errors.filter((error) => isMatch(error, expected)).length).toBe(count);
}

export function getStreamableIO(files) {
return {
files,
Expand Down
159 changes: 159 additions & 0 deletions tests/unit/parsers/test.manifestjson.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import * as messages from 'messages';

import {
assertHasMatchingError,
assertHasMatchingErrorCount,
validManifestJSON,
validDictionaryManifestJSON,
validLangpackManifestJSON,
Expand Down Expand Up @@ -1219,6 +1220,164 @@ describe('ManifestJSONParser', () => {
});
});

it('adds an error if the browser_action icon is not in the package', async () => {
const addonLinter = new Linter({ _: ['bar'] });
const json = validManifestJSON({
browser_action: {
default_icon: {
32: 'icons/icon-32.png',
64: 'icons/icon-64.png',
},
},
});
const files = {
'icons/icon-32.png': EMPTY_PNG.toString('binary'),
};
const manifestJSONParser = new ManifestJSONParser(
json,
addonLinter.collector,
{ io: getStreamableIO(files) }
);

await manifestJSONParser.validateIcons();
expect(manifestJSONParser.isValid).toBeFalsy();
assertHasMatchingError(addonLinter.collector.errors, {
code: messages.MANIFEST_ICON_NOT_FOUND,
message:
'An icon defined in the manifest could not be found in the package.',
description: 'Icon could not be found at "icons/icon-64.png".',
});
});

it('adds an error if the browser_action theme icon is not in the package', async () => {
const addonLinter = new Linter({ _: ['bar'] });
const json = validManifestJSON({
browser_action: {
theme_icons: [
{
light: 'icons/light-32.png',
dark: 'icons/dark-32.png',
size: 32,
},
],
},
});
const files = {
'icons/light-32.png': EMPTY_PNG.toString('binary'),
};
const manifestJSONParser = new ManifestJSONParser(
json,
addonLinter.collector,
{ io: getStreamableIO(files) }
);

await manifestJSONParser.validateIcons();
expect(manifestJSONParser.isValid).toBeFalsy();
assertHasMatchingError(addonLinter.collector.errors, {
code: messages.MANIFEST_ICON_NOT_FOUND,
message:
'An icon defined in the manifest could not be found in the package.',
description: 'Icon could not be found at "icons/dark-32.png".',
});
});

it('adds an error if the page_action icon is not in the package', async () => {
const addonLinter = new Linter({ _: ['bar'] });
const json = validManifestJSON({
page_action: {
default_icon: {
32: 'icons/icon-32.png',
64: 'icons/icon-64.png',
},
},
});
const files = {
'icons/icon-32.png': EMPTY_PNG.toString('binary'),
};
const manifestJSONParser = new ManifestJSONParser(
json,
addonLinter.collector,
{ io: getStreamableIO(files) }
);

await manifestJSONParser.validateIcons();
expect(manifestJSONParser.isValid).toBeFalsy();
assertHasMatchingError(addonLinter.collector.errors, {
code: messages.MANIFEST_ICON_NOT_FOUND,
message:
'An icon defined in the manifest could not be found in the package.',
description: 'Icon could not be found at "icons/icon-64.png".',
});
});

it('adds an error if the sidebar_action icon is not in the package', async () => {
const addonLinter = new Linter({ _: ['bar'] });
const json = validManifestJSON({
sidebar_action: {
default_icon: {
32: 'icons/icon-32.png',
64: 'icons/icon-64.png',
},
},
});
const files = {
'icons/icon-32.png': EMPTY_PNG.toString('binary'),
};
const manifestJSONParser = new ManifestJSONParser(
json,
addonLinter.collector,
{ io: getStreamableIO(files) }
);

await manifestJSONParser.validateIcons();
expect(manifestJSONParser.isValid).toBeFalsy();
assertHasMatchingError(addonLinter.collector.errors, {
code: messages.MANIFEST_ICON_NOT_FOUND,
message:
'An icon defined in the manifest could not be found in the package.',
description: 'Icon could not be found at "icons/icon-64.png".',
});
});

it('filters duplicate errors if a missing icon is reused', async () => {
const addonLinter = new Linter({ _: ['bar'] });
const json = validManifestJSON({
browser_action: {
default_icon: {
32: 'icons/icon-32.png',
64: 'icons/icon-64.png',
},
},
sidebar_action: {
default_icon: {
32: 'icons/icon-32.png',
64: 'icons/icon-64.png',
},
},
});
const files = {
'icons/icon-32.png': EMPTY_PNG.toString('binary'),
};
const manifestJSONParser = new ManifestJSONParser(
json,
addonLinter.collector,
{ io: getStreamableIO(files) }
);

await manifestJSONParser.validateIcons();
expect(manifestJSONParser.isValid).toBeFalsy();
assertHasMatchingErrorCount(
addonLinter.collector.errors,
{
code: messages.MANIFEST_ICON_NOT_FOUND,
message:
'An icon defined in the manifest could not be found in the package.',
description: 'Icon could not be found at "icons/icon-64.png".',
},
1
);
});

it('adds a warning if the icon does not have a valid extension', async () => {
const addonLinter = new Linter({ _: ['bar'] });
const json = validManifestJSON({
Expand Down

0 comments on commit 3b5bd84

Please sign in to comment.