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

@@ -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:

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)"]
   }
 }

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');

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');

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';

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));
+}

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);
+  });
+});
+

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;
+    });
+}

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);
+      });
+    });
+  });
+});

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));
+}

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);
+  });
+});
+

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));
+}

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);
+      });
+    });
+  });
+});

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);
+}

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);
+});
+

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;

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

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';

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;
+}

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 = '<27>';
+  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);
+  });
+});