From 3b5bd8443c41e7d640acf7f13204c9f56f064650 Mon Sep 17 00:00:00 2001 From: Alex McCausland Date: Wed, 10 Jul 2019 00:41:21 -0400 Subject: [PATCH] Add linting for all icons in manifest (#2677) --- src/parsers/manifestjson.js | 72 ++++++++--- tests/unit/helpers.js | 6 + tests/unit/parsers/test.manifestjson.js | 159 ++++++++++++++++++++++++ 3 files changed, 221 insertions(+), 16 deletions(-) diff --git a/src/parsers/manifestjson.js b/src/parsers/manifestjson.js index 60e85909fbf..3e33be245bf 100644 --- a/src/parsers/manifestjson.js +++ b/src/parsers/manifestjson.js @@ -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, @@ -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)); } @@ -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, diff --git a/tests/unit/helpers.js b/tests/unit/helpers.js index 2e8e9b244c3..42d88e359b2 100644 --- a/tests/unit/helpers.js +++ b/tests/unit/helpers.js @@ -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, diff --git a/tests/unit/parsers/test.manifestjson.js b/tests/unit/parsers/test.manifestjson.js index 4e5c2c860a1..3c71b8f2641 100644 --- a/tests/unit/parsers/test.manifestjson.js +++ b/tests/unit/parsers/test.manifestjson.js @@ -9,6 +9,7 @@ import * as messages from 'messages'; import { assertHasMatchingError, + assertHasMatchingErrorCount, validManifestJSON, validDictionaryManifestJSON, validLangpackManifestJSON, @@ -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({