From 1505933826aece27167f1fa7f5f37d5c67040924 Mon Sep 17 00:00:00 2001 From: Jia Hao Goh Date: Sun, 7 May 2017 15:49:15 +0800 Subject: [PATCH] Promisfy and parallelise config, add unit tests Instead of optionsMain exporting an async function, this commit changes it to return a promise instead. We split all the needed async helpers for this config builder into smaller promises, in `src/options/*`. Another side effect of this is that we perform all our async config inferring in parallel, which speeds up the nativefier CLI. Add proper unit tests as well for all of these promises. Switch to Jest for these unit tests, and we are temporarily running both Jest and mocha together in `npm test`. To refactor all the Mocha code to use Jest in a future commit. --- .eslintrc.yml | 3 + package.json | 21 ++++-- src/build/buildMain.js | 7 +- src/constants.js | 5 ++ src/infer/index.js | 4 ++ src/options/asyncConfig.js | 18 +++++ src/options/asyncConfig.test.js | 18 +++++ src/options/fields/icon.js | 15 +++++ src/options/fields/icon.test.js | 42 ++++++++++++ src/options/fields/index.js | 29 ++++++++ src/options/fields/index.test.js | 22 +++++++ src/options/fields/name.js | 23 +++++++ src/options/fields/name.test.js | 87 ++++++++++++++++++++++++ src/options/fields/userAgent.js | 9 +++ src/options/fields/userAgent.test.js | 18 +++++ src/options/optionsMain.js | 99 ++-------------------------- src/options/optionsMain.test.js | 18 +++++ src/utils/index.js | 3 + src/utils/sanitizeFilename.js | 17 +++++ src/utils/sanitizeFilename.test.js | 36 ++++++++++ 20 files changed, 394 insertions(+), 100 deletions(-) create mode 100644 src/constants.js create mode 100644 src/infer/index.js create mode 100644 src/options/asyncConfig.js create mode 100644 src/options/asyncConfig.test.js create mode 100644 src/options/fields/icon.js create mode 100644 src/options/fields/icon.test.js create mode 100644 src/options/fields/index.js create mode 100644 src/options/fields/index.test.js create mode 100644 src/options/fields/name.js create mode 100644 src/options/fields/name.test.js create mode 100644 src/options/fields/userAgent.js create mode 100644 src/options/fields/userAgent.test.js create mode 100644 src/options/optionsMain.test.js create mode 100644 src/utils/index.js create mode 100644 src/utils/sanitizeFilename.js create mode 100644 src/utils/sanitizeFilename.test.js diff --git a/.eslintrc.yml b/.eslintrc.yml index eeafada..15f66cb 100644 --- a/.eslintrc.yml +++ b/.eslintrc.yml @@ -1,4 +1,7 @@ extends: airbnb-base +env: + # TODO: find out how to turn this on only for src/**/*.test.js files + jest: true plugins: - import rules: diff --git a/package.json b/package.json index 9f96870..b778feb 100644 --- a/package.json +++ b/package.json @@ -12,7 +12,8 @@ "main": "lib/index.js", "scripts": { "dev-up": "npm install && (cd app && npm install) && npm run build", - "test": "gulp test", + "test": "jest && gulp test", + "jest": "jest", "tdd": "gulp tdd", "lint": "eslint .", "lint:fix": "eslint . --fix", @@ -31,7 +32,7 @@ "type": "git", "url": "git+https://github.com/jiahaog/nativefier.git" }, - "author": "", + "author": "Goh Jia Hao", "license": "MIT", "bugs": { "url": "https://github.com/jiahaog/nativefier/issues" @@ -57,13 +58,9 @@ "tmp": "0.0.31", "validator": "^7.0.0" }, - "babel": { - "presets": [ - "es2015" - ] - }, "devDependencies": { "babel-core": "^6.4.5", + "babel-jest": "^19.0.0", "babel-loader": "^6.2.1", "babel-preset-es2015": "^6.6.0", "babel-register": "^6.6.0", @@ -78,11 +75,21 @@ "gulp-mocha": "^4.3.0", "gulp-sourcemaps": "^2.6.0", "isparta": "^4.0.0", + "jest": "^19.0.2", + "regenerator-runtime": "^0.10.5", "require-dir": "^0.3.0", "run-sequence": "^1.1.5", "webpack-stream": "^3.1.0" }, "engines": { "node": ">= 4.0" + }, + "babel": { + "presets": [ + "es2015" + ] + }, + "jest": { + "testMatch": ["**/src/**/?(*.)(spec|test).js?(x)"] } } diff --git a/src/build/buildMain.js b/src/build/buildMain.js index 8b3eff8..afbc0d4 100644 --- a/src/build/buildMain.js +++ b/src/build/buildMain.js @@ -105,7 +105,12 @@ function buildMain(inpOptions, callback) { async.waterfall([ (callback) => { progress.tick('inferring'); - optionsFactory(options, callback); + optionsFactory(options) + .then((result) => { + callback(null, result); + }).catch((error) => { + callback(error); + }); }, (options, callback) => { progress.tick('copying'); diff --git a/src/constants.js b/src/constants.js new file mode 100644 index 0000000..6801592 --- /dev/null +++ b/src/constants.js @@ -0,0 +1,5 @@ +import path from 'path'; + +export const DEFAULT_APP_NAME = 'APP'; +export const ELECTRON_VERSION = '1.6.6'; +export const PLACEHOLDER_APP_DIR = path.join(__dirname, './../', 'app'); diff --git a/src/infer/index.js b/src/infer/index.js new file mode 100644 index 0000000..0495ef0 --- /dev/null +++ b/src/infer/index.js @@ -0,0 +1,4 @@ +export { default as inferIcon } from './inferIcon'; +export { default as inferOs } from './inferOs'; +export { default as inferTitle } from './inferTitle'; +export { default as inferUserAgent } from './inferUserAgent'; diff --git a/src/options/asyncConfig.js b/src/options/asyncConfig.js new file mode 100644 index 0000000..a9f1c06 --- /dev/null +++ b/src/options/asyncConfig.js @@ -0,0 +1,18 @@ +import fields from './fields'; + +function resultArrayToObject(fieldResults) { + return fieldResults.reduce((accumulator, value) => Object.assign({}, accumulator, value), {}); +} + +function inferredOptions(oldOptions, fieldResults) { + const newOptions = resultArrayToObject(fieldResults); + return Object.assign({}, oldOptions, newOptions); +} + +// Takes the options object and infers new values +// which may need async work +export default function (options) { + const tasks = fields(options); + return Promise.all(tasks) + .then(fieldResults => inferredOptions(options, fieldResults)); +} diff --git a/src/options/asyncConfig.test.js b/src/options/asyncConfig.test.js new file mode 100644 index 0000000..de0d108 --- /dev/null +++ b/src/options/asyncConfig.test.js @@ -0,0 +1,18 @@ +import asyncConfig from './asyncConfig'; +import fields from './fields'; + +jest.mock('./fields'); + +fields.mockImplementation(() => [Promise.resolve({ + someField: 'newValue', +})]); + +test('it should merge the result of the promise', () => { + const param = { another: 'field', someField: 'oldValue' }; + const expected = { another: 'field', someField: 'newValue' }; + + return asyncConfig(param).then((result) => { + expect(result).toEqual(expected); + }); +}); + diff --git a/src/options/fields/icon.js b/src/options/fields/icon.js new file mode 100644 index 0000000..cbd4a6a --- /dev/null +++ b/src/options/fields/icon.js @@ -0,0 +1,15 @@ +import log from 'loglevel'; +import { inferIcon } from './../../infer'; + +export default function ({ icon, targetUrl, platform }) { + // Icon is the path to the icon + if (icon) { + return icon; + } + + return inferIcon(targetUrl, platform) + .catch((error) => { + log.warn('Cannot automatically retrieve the app icon:', error); + return null; + }); +} diff --git a/src/options/fields/icon.test.js b/src/options/fields/icon.test.js new file mode 100644 index 0000000..3469299 --- /dev/null +++ b/src/options/fields/icon.test.js @@ -0,0 +1,42 @@ +import log from 'loglevel'; +import icon from './icon'; +import { inferIcon } from './../../infer'; + +jest.mock('./../../infer/inferIcon'); +jest.mock('loglevel'); + +const mockedResult = 'icon path'; + +describe('when the icon parameter is passed', () => { + test('it should return the icon parameter', () => { + expect(inferIcon).toHaveBeenCalledTimes(0); + + const params = { icon: './icon.png' }; + expect(icon(params)).toBe(params.icon); + }); +}); + +describe('when the icon parameter is not passed', () => { + test('it should call inferIcon', () => { + inferIcon.mockImplementationOnce(() => Promise.resolve(mockedResult)); + const params = { targetUrl: 'some url', platform: 'mac' }; + + return icon(params).then((result) => { + expect(result).toBe(mockedResult); + expect(inferIcon).toHaveBeenCalledWith(params.targetUrl, params.platform); + }); + }); + + describe('when inferIcon resolves with an error', () => { + test('it should handle the error', () => { + inferIcon.mockImplementationOnce(() => Promise.reject('some error')); + const params = { targetUrl: 'some url', platform: 'mac' }; + + return icon(params).then((result) => { + expect(result).toBe(null); + expect(inferIcon).toHaveBeenCalledWith(params.targetUrl, params.platform); + expect(log.warn).toHaveBeenCalledTimes(1); + }); + }); + }); +}); diff --git a/src/options/fields/index.js b/src/options/fields/index.js new file mode 100644 index 0000000..7956206 --- /dev/null +++ b/src/options/fields/index.js @@ -0,0 +1,29 @@ +import icon from './icon'; +import userAgent from './userAgent'; +import name from './name'; + +const fields = [{ + field: 'userAgent', + task: userAgent, +}, { + field: 'icon', + task: icon, +}, { + field: 'name', + task: name, +}]; + +// Modifies the result of each promise from a scalar +// value to a object containing its fieldname +function wrap(fieldName, promise, args) { + return promise(args) + .then(result => ({ + [fieldName]: result, + })); +} + +// Returns a list of promises which will all resolve +// with the following result: {[fieldName]: fieldvalue} +export default function (options) { + return fields.map(({ field, task }) => wrap(field, task, options)); +} diff --git a/src/options/fields/index.test.js b/src/options/fields/index.test.js new file mode 100644 index 0000000..36f0926 --- /dev/null +++ b/src/options/fields/index.test.js @@ -0,0 +1,22 @@ +import fields from './index'; +import icon from './icon'; +import userAgent from './userAgent'; +import name from './name'; + +jest.mock('./icon'); +jest.mock('./name'); +jest.mock('./userAgent'); + +const modules = [icon, userAgent, name]; +modules.forEach((module) => { + module.mockImplementation(() => Promise.resolve()); +}); + +test('it should return a list of promises', () => { + const result = fields({}); + expect(result).toHaveLength(3); + result.forEach((value) => { + expect(value).toBeInstanceOf(Promise); + }); +}); + diff --git a/src/options/fields/name.js b/src/options/fields/name.js new file mode 100644 index 0000000..eca888e --- /dev/null +++ b/src/options/fields/name.js @@ -0,0 +1,23 @@ +import log from 'loglevel'; +import { sanitizeFilename } from './../../utils'; +import { inferTitle } from './../../infer'; +import { DEFAULT_APP_NAME } from './../../constants'; + +function tryToInferName({ name, targetUrl }) { + // .length also checks if its the commanderJS function or a string + if (name && name.length > 0) { + return Promise.resolve(name); + } + + return inferTitle(targetUrl) + .then(pageTitle => (pageTitle || DEFAULT_APP_NAME)) + .catch((error) => { + log.warn(`Unable to automatically determine app name, falling back to '${DEFAULT_APP_NAME}'. Reason: ${error}`); + return DEFAULT_APP_NAME; + }); +} + +export default function ({ platform, name, targetUrl }) { + return tryToInferName({ name, targetUrl }) + .then(result => sanitizeFilename(platform, result)); +} diff --git a/src/options/fields/name.test.js b/src/options/fields/name.test.js new file mode 100644 index 0000000..6ccebff --- /dev/null +++ b/src/options/fields/name.test.js @@ -0,0 +1,87 @@ +import log from 'loglevel'; +import name from './name'; +import { DEFAULT_APP_NAME } from './../../constants'; +import { inferTitle } from './../../infer'; +import { sanitizeFilename } from './../../utils'; + +jest.mock('./../../infer/inferTitle'); +jest.mock('./../../utils/sanitizeFilename'); +jest.mock('loglevel'); + +sanitizeFilename.mockImplementation((_, filename) => filename); + +const mockedResult = 'mock name'; + +describe('well formed name parameters', () => { + const params = { name: 'appname', platform: 'something' }; + test('it should not call inferTitle', () => name(params).then((result) => { + expect(inferTitle).toHaveBeenCalledTimes(0); + expect(result).toBe(params.name); + })); + + test('it should call sanitize filename', () => name(params).then((result) => { + expect(sanitizeFilename).toHaveBeenCalledWith(params.platform, result); + })); +}); + +describe('bad name parameters', () => { + beforeEach(() => { + inferTitle.mockImplementationOnce(() => Promise.resolve(mockedResult)); + }); + + const params = { targetUrl: 'some url' }; + describe('when the name is undefined', () => { + test('it should call inferTitle', () => name(params).then(() => { + expect(inferTitle).toHaveBeenCalledWith(params.targetUrl); + })); + }); + + describe('when the name is an empty string', () => { + test('it should call inferTitle', () => { + const testParams = Object.assign({}, params, { name: '' }); + + return name(testParams).then(() => { + expect(inferTitle).toHaveBeenCalledWith(params.targetUrl); + }); + }); + }); + + test('it should call sanitize filename', () => name(params).then((result) => { + expect(sanitizeFilename).toHaveBeenCalledWith(params.platform, result); + })); +}); + +describe('handling inferTitle results', () => { + const params = { targetUrl: 'some url', name: '', platform: 'something' }; + test('it should return the result from inferTitle', () => { + inferTitle.mockImplementationOnce(() => Promise.resolve(mockedResult)); + + return name(params).then((result) => { + expect(result).toBe(mockedResult); + expect(inferTitle).toHaveBeenCalledWith(params.targetUrl); + }); + }); + + describe('when the returned pageTitle is falsey', () => { + test('it should return the default app name', () => { + inferTitle.mockImplementationOnce(() => Promise.resolve(null)); + + return name(params).then((result) => { + expect(result).toBe(DEFAULT_APP_NAME); + expect(inferTitle).toHaveBeenCalledWith(params.targetUrl); + }); + }); + }); + + describe('when inferTitle resolves with an error', () => { + test('it should return the default app name', () => { + inferTitle.mockImplementationOnce(() => Promise.reject('some error')); + + return name(params).then((result) => { + expect(result).toBe(DEFAULT_APP_NAME); + expect(inferTitle).toHaveBeenCalledWith(params.targetUrl); + expect(log.warn).toHaveBeenCalledTimes(1); + }); + }); + }); +}); diff --git a/src/options/fields/userAgent.js b/src/options/fields/userAgent.js new file mode 100644 index 0000000..df6d574 --- /dev/null +++ b/src/options/fields/userAgent.js @@ -0,0 +1,9 @@ +import { inferUserAgent } from './../../infer'; + +export default function ({ userAgent, electronVersion, platform }) { + if (userAgent) { + return userAgent; + } + + return inferUserAgent(electronVersion, platform); +} diff --git a/src/options/fields/userAgent.test.js b/src/options/fields/userAgent.test.js new file mode 100644 index 0000000..5ff1603 --- /dev/null +++ b/src/options/fields/userAgent.test.js @@ -0,0 +1,18 @@ +import userAgent from './userAgent'; +import { inferUserAgent } from './../../infer'; + +jest.mock('./../../infer/inferUserAgent'); + +test('when a userAgent parameter is passed', () => { + expect(inferUserAgent).toHaveBeenCalledTimes(0); + + const params = { userAgent: 'valid user agent' }; + expect(userAgent(params)).toBe(params.userAgent); +}); + +test('no userAgent parameter is passed', () => { + const params = { electronVersion: '123', platform: 'mac' }; + userAgent(params); + expect(inferUserAgent).toHaveBeenCalledWith(params.electronVersion, params.platform); +}); + diff --git a/src/options/optionsMain.js b/src/options/optionsMain.js index eabb7f5..8b03480 100644 --- a/src/options/optionsMain.js +++ b/src/options/optionsMain.js @@ -1,53 +1,19 @@ -import path from 'path'; -import _ from 'lodash'; -import async from 'async'; import log from 'loglevel'; -import sanitizeFilenameLib from 'sanitize-filename'; -import inferIcon from './../infer/inferIcon'; -import inferTitle from './../infer/inferTitle'; import inferOs from './../infer/inferOs'; -import inferUserAgent from './../infer/inferUserAgent'; import normalizeUrl from './normalizeUrl'; import packageJson from './../../package.json'; +import { ELECTRON_VERSION, PLACEHOLDER_APP_DIR } from './../constants'; +import asyncConfig from './asyncConfig'; const { inferPlatform, inferArch } = inferOs; -const PLACEHOLDER_APP_DIR = path.join(__dirname, '../../', 'app'); -const ELECTRON_VERSION = '1.6.6'; -const DEFAULT_APP_NAME = 'APP'; - -function sanitizeFilename(platform, str) { - let result = sanitizeFilenameLib(str); - - // remove all non ascii or use default app name - // eslint-disable-next-line no-control-regex - result = result.replace(/[^\x00-\x7F]/g, '') || DEFAULT_APP_NAME; - - // spaces will cause problems with Ubuntu when pinned to the dock - if (platform === 'linux') { - return _.kebabCase(result); - } - return result; -} - -function sanitizeOptions(options) { - const name = sanitizeFilename(options.platform, options.name); - return Object.assign({}, options, { name }); -} - -/** - * @callback optionsCallback - * @param error - * @param options augmented options - */ - /** * Extracts only desired keys from inpOptions and augments it with defaults - * @param inpOptions - * @param {optionsCallback} callback + * @param {Object} inpOptions + * @returns {Promise} */ -function optionsFactory(inpOptions, callback) { +export default function (inpOptions) { const options = { dir: PLACEHOLDER_APP_DIR, name: inpOptions.name, @@ -82,7 +48,7 @@ function optionsFactory(inpOptions, callback) { disableContextMenu: inpOptions.disableContextMenu, disableDevTools: inpOptions.disableDevTools, crashReporter: inpOptions.crashReporter, - // workaround for electron-packager#375 + // workaround for electron-packager#375 tmpdir: false, zoom: inpOptions.zoom || 1.0, internalUrls: inpOptions.internalUrls || null, @@ -119,57 +85,6 @@ function optionsFactory(inpOptions, callback) { options.height = options.maxHeight; } - async.waterfall([ - (callback) => { - if (options.userAgent) { - callback(); - return; - } - inferUserAgent(options.electronVersion, options.platform) - .then((userAgent) => { - options.userAgent = userAgent; - callback(); - }) - .catch(callback); - }, - (callback) => { - if (options.icon) { - callback(); - return; - } - inferIcon(options.targetUrl, options.platform) - .then((pngPath) => { - options.icon = pngPath; - callback(); - }) - .catch((error) => { - log.warn('Cannot automatically retrieve the app icon:', error); - callback(); - }); - }, - (callback) => { - // length also checks if its the commanderJS function or a string - if (options.name && options.name.length > 0) { - callback(); - return; - } - options.name = DEFAULT_APP_NAME; - - inferTitle(options.targetUrl).then((pageTitle) => { - options.name = pageTitle; - }).catch((error) => { - log.warn(`Unable to automatically determine app name, falling back to '${DEFAULT_APP_NAME}'. Reason: ${error}`); - }).then(() => { - callback(); - }); - }, - ], (error) => { - if (error) { - callback(error); - return; - } - callback(null, sanitizeOptions(options)); - }); + return asyncConfig(options); } -export default optionsFactory; diff --git a/src/options/optionsMain.test.js b/src/options/optionsMain.test.js new file mode 100644 index 0000000..370955a --- /dev/null +++ b/src/options/optionsMain.test.js @@ -0,0 +1,18 @@ +import optionsMain from './optionsMain'; +import asyncConfig from './asyncConfig'; + +jest.mock('./asyncConfig'); +const mockedAsyncConfig = { some: 'options' }; +asyncConfig.mockImplementation(() => Promise.resolve(mockedAsyncConfig)); + +test('it should call the async config', () => { + const params = { + targetUrl: 'http://example.com', + }; + return optionsMain(params).then((result) => { + expect(asyncConfig).toHaveBeenCalledWith(expect.objectContaining(params)); + expect(result).toEqual(mockedAsyncConfig); + }); +}); + +// TODO add more tests diff --git a/src/utils/index.js b/src/utils/index.js new file mode 100644 index 0000000..e36cf72 --- /dev/null +++ b/src/utils/index.js @@ -0,0 +1,3 @@ +// TODO remove the eslint disable when we have more than one +// eslint-disable-next-line import/prefer-default-export +export { default as sanitizeFilename } from './sanitizeFilename'; diff --git a/src/utils/sanitizeFilename.js b/src/utils/sanitizeFilename.js new file mode 100644 index 0000000..5f0068f --- /dev/null +++ b/src/utils/sanitizeFilename.js @@ -0,0 +1,17 @@ +import _ from 'lodash'; +import sanitizeFilenameLib from 'sanitize-filename'; +import { DEFAULT_APP_NAME } from './../constants'; + +export default function (platform, str) { + let result = sanitizeFilenameLib(str); + + // remove all non ascii or use default app name + // eslint-disable-next-line no-control-regex + result = result.replace(/[^\x00-\x7F]/g, '') || DEFAULT_APP_NAME; + + // spaces will cause problems with Ubuntu when pinned to the dock + if (platform === 'linux') { + return _.kebabCase(result); + } + return result; +} diff --git a/src/utils/sanitizeFilename.test.js b/src/utils/sanitizeFilename.test.js new file mode 100644 index 0000000..8d9d859 --- /dev/null +++ b/src/utils/sanitizeFilename.test.js @@ -0,0 +1,36 @@ +import sanitizeFilenameLib from 'sanitize-filename'; +import sanitizeFilename from './sanitizeFilename'; +import { DEFAULT_APP_NAME } from './../constants'; + +jest.mock('sanitize-filename'); +sanitizeFilenameLib.mockImplementation(str => str); + +test('it should call the sanitize-filename npm module', () => { + const param = 'abc'; + sanitizeFilename('', param); + expect(sanitizeFilenameLib).toHaveBeenCalledWith(param); +}); + +describe('replacing non ascii characters', () => { + const nonAscii = '�'; + test('it should return a result without non ascii cahracters', () => { + const param = `${nonAscii}abc`; + const expectedResult = 'abc'; + const result = sanitizeFilename('', param); + expect(result).toBe(expectedResult); + }); + + describe('when the result of replacing these characters is empty', () => { + const result = sanitizeFilename('', nonAscii); + expect(result).toBe(DEFAULT_APP_NAME); + }); +}); + +describe('when the platform is linux', () => { + test('it should return a kebab cased name', () => { + const param = 'some name'; + const expectedResult = 'some-name'; + const result = sanitizeFilename('linux', param); + expect(result).toBe(expectedResult); + }); +});