From 71fd8f6bd78747759c151dc04bf3725a8ce2e207 Mon Sep 17 00:00:00 2001 From: Shubheksha Jalan Date: Tue, 7 Feb 2017 00:09:10 +0530 Subject: [PATCH 1/6] throw an error for invalid keys in config --- src/config.js | 5 +++-- tests/unit/test.config.js | 36 ++++++++++++++++++++++++++++++------ 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/config.js b/src/config.js index eba576809f..b289d7dd93 100644 --- a/src/config.js +++ b/src/config.js @@ -10,7 +10,7 @@ const log = createLogger(__filename); type ApplyConfigToArgvParams = {| argv: Object, - configObject: Object, + configObject?: Object, defaultValues: Object, |}; @@ -34,7 +34,8 @@ export function applyConfigToArgv({ if (!argv.hasOwnProperty(option)) { log.debug(`Ignoring configuration: ${option}=${configObject[option]} ` + 'because this is an unknown option'); - continue; + throw new UsageError(`The option ${option} is invalid.` + + ' Please fix your config and try again.'); } newArgv[option] = configObject[option]; } diff --git a/tests/unit/test.config.js b/tests/unit/test.config.js index 82761aa33b..5029d5446e 100644 --- a/tests/unit/test.config.js +++ b/tests/unit/test.config.js @@ -120,8 +120,11 @@ describe('config', () => { const configObject = { foo: '/configured/foo', }; - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); - assert.strictEqual(newArgv.sourceDir, 'default/value/option/definition'); + assert.throws(() => { + const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + assert.strictEqual(newArgv.sourceDir, + 'default/value/option/definition'); + }, UsageError, /Please fix your config and try again/); }); it('preserves value on the command line if not in config', () => { @@ -140,8 +143,10 @@ describe('config', () => { const configObject = { foo: '/configured/foo', }; - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); - assert.strictEqual(newArgv.sourceDir, cmdLineSrcDir); + assert.throws(() => { + const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + assert.strictEqual(newArgv.sourceDir, cmdLineSrcDir); + }, UsageError, /Please fix your config and try again/); }); it('uses a configured boolean value over an implicit default', () => { @@ -206,8 +211,10 @@ describe('config', () => { const configObject = { foo: true, }; - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); - assert.strictEqual(newArgv.sourceDir, cmdLineSrcDir); + assert.throws(() => { + const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + assert.strictEqual(newArgv.sourceDir, cmdLineSrcDir); + }, UsageError, /Please fix your config and try again/); }); it('uses a configured number value over a falsey default', () => { @@ -259,6 +266,23 @@ describe('config', () => { const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); assert.strictEqual(newArgv.sourceDir, '/configured/directory'); }); + + it('throws an error when an option is invalid', () => { + const {argv, defaultValues} = makeArgv({ + globalOpt: { + 'source-dir': { + type: 'string', + demand: false, + }, + }, + }); + const configObject = { + foo: 'fake/value/', + }; + assert.throws(() => { + applyConfigToArgv({argv, configObject, defaultValues}); + }, UsageError, /Please fix your config and try again/); + }); }); describe('loadJSConfigFile', () => { From f84d57412e4e3b6407b384f747296f9d05965e76 Mon Sep 17 00:00:00 2001 From: Shubheksha Jalan Date: Tue, 7 Feb 2017 02:19:29 +0530 Subject: [PATCH 2/6] add check for camel cased options --- src/config.js | 9 +++++++-- tests/unit/test.config.js | 6 +++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/config.js b/src/config.js index b289d7dd93..43deb3cc49 100644 --- a/src/config.js +++ b/src/config.js @@ -2,6 +2,7 @@ import path from 'path'; import requireUncached from 'require-uncached'; +import camelCase from 'camelcase'; import {createLogger} from './util/logger'; import {UsageError} from './errors'; @@ -10,7 +11,7 @@ const log = createLogger(__filename); type ApplyConfigToArgvParams = {| argv: Object, - configObject?: Object, + configObject: Object, defaultValues: Object, |}; @@ -23,7 +24,11 @@ export function applyConfigToArgv({ for (const option in configObject) { // we assume the value was set on the CLI if the default value is // not the same as that on the argv object as there is a very rare chance - // this happening + // of this happening + if (camelCase(option) !== option) { + throw new UsageError(`Please use camel case to specify ${option} ` + + 'in the config'); + } const wasValueSetOnCLI = typeof(argv[option]) !== 'undefined' && (argv[option] !== defaultValues[option]); if (wasValueSetOnCLI) { diff --git a/tests/unit/test.config.js b/tests/unit/test.config.js index 5029d5446e..79713c78fa 100644 --- a/tests/unit/test.config.js +++ b/tests/unit/test.config.js @@ -267,7 +267,7 @@ describe('config', () => { assert.strictEqual(newArgv.sourceDir, '/configured/directory'); }); - it('throws an error when an option is invalid', () => { + it('throws an error when an option is not camel cased', () => { const {argv, defaultValues} = makeArgv({ globalOpt: { 'source-dir': { @@ -277,11 +277,11 @@ describe('config', () => { }, }); const configObject = { - foo: 'fake/value/', + 'source-dir': 'fake/value/', }; assert.throws(() => { applyConfigToArgv({argv, configObject, defaultValues}); - }, UsageError, /Please fix your config and try again/); + }, UsageError, /Please use camel case to specify source-dir/); }); }); From d475a0e858990b6ffd4df78491df0d004cae3d14 Mon Sep 17 00:00:00 2001 From: Shubheksha Jalan Date: Tue, 7 Feb 2017 02:28:34 +0530 Subject: [PATCH 3/6] fix tests unnecessary errors --- package.json | 3 ++- src/config.js | 4 +++- tests/unit/test.config.js | 36 ++++++++++++++++++++---------------- 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/package.json b/package.json index 92d7dc67e4..d577f2cd54 100644 --- a/package.json +++ b/package.json @@ -51,6 +51,7 @@ "bunyan": "1.8.5", "camelcase": "4.0.0", "debounce": "1.0.0", + "decamelize": "1.2.0", "es6-error": "4.0.1", "es6-promisify": "5.0.0", "event-to-promise": "0.7.0", @@ -65,9 +66,9 @@ "parse-json": "2.2.0", "regenerator-runtime": "0.10.1", "require-uncached": "1.0.3", - "stream-to-promise": "2.2.0", "sign-addon": "0.2.1", "source-map-support": "0.4.11", + "stream-to-promise": "2.2.0", "tmp": "0.0.30", "update-notifier": "1.0.3", "watchpack": "1.2.0", diff --git a/src/config.js b/src/config.js index 43deb3cc49..d71df23143 100644 --- a/src/config.js +++ b/src/config.js @@ -3,6 +3,7 @@ import path from 'path'; import requireUncached from 'require-uncached'; import camelCase from 'camelcase'; +import decamelize from 'decamelize'; import {createLogger} from './util/logger'; import {UsageError} from './errors'; @@ -36,7 +37,8 @@ export function applyConfigToArgv({ `configuration: ${option}=${configObject[option]}`); continue; } - if (!argv.hasOwnProperty(option)) { + if (!argv.hasOwnProperty(option) && + !argv.hasOwnProperty(decamelize(option, '-'))) { log.debug(`Ignoring configuration: ${option}=${configObject[option]} ` + 'because this is an unknown option'); throw new UsageError(`The option ${option} is invalid.` + diff --git a/tests/unit/test.config.js b/tests/unit/test.config.js index 79713c78fa..e988dd0f3c 100644 --- a/tests/unit/test.config.js +++ b/tests/unit/test.config.js @@ -115,16 +115,17 @@ describe('config', () => { demand: false, default: 'default/value/option/definition', }, + 'artifacts-dir': { + type: 'string', + demand: false, + }, }, }); const configObject = { - foo: '/configured/foo', + artifactsDir: '/configured/artifacts/dir', }; - assert.throws(() => { - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); - assert.strictEqual(newArgv.sourceDir, - 'default/value/option/definition'); - }, UsageError, /Please fix your config and try again/); + const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + assert.strictEqual(newArgv.sourceDir, 'default/value/option/definition'); }); it('preserves value on the command line if not in config', () => { @@ -138,15 +139,17 @@ describe('config', () => { demand: false, default: 'default/value/option/definition', }, + 'artifacts-dir': { + type: 'string', + demand: false, + }, }, }); const configObject = { - foo: '/configured/foo', + artifactsDir: '/configured/artifacts/dir', }; - assert.throws(() => { - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); - assert.strictEqual(newArgv.sourceDir, cmdLineSrcDir); - }, UsageError, /Please fix your config and try again/); + const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + assert.strictEqual(newArgv.sourceDir, cmdLineSrcDir); }); it('uses a configured boolean value over an implicit default', () => { @@ -206,15 +209,16 @@ describe('config', () => { 'source-dir': { type: 'string', }, + 'verbose': { + type: 'boolean', + }, }, }); const configObject = { - foo: true, + verbose: true, }; - assert.throws(() => { - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); - assert.strictEqual(newArgv.sourceDir, cmdLineSrcDir); - }, UsageError, /Please fix your config and try again/); + const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + assert.strictEqual(newArgv.sourceDir, cmdLineSrcDir); }); it('uses a configured number value over a falsey default', () => { From ca9dd2ebe1f36b507ff6c6c546c8754f037e91ab Mon Sep 17 00:00:00 2001 From: Shubheksha Jalan Date: Tue, 7 Feb 2017 02:30:37 +0530 Subject: [PATCH 4/6] add tests for invalid option --- tests/unit/test.config.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/unit/test.config.js b/tests/unit/test.config.js index e988dd0f3c..d54164f2ff 100644 --- a/tests/unit/test.config.js +++ b/tests/unit/test.config.js @@ -287,6 +287,23 @@ describe('config', () => { applyConfigToArgv({argv, configObject, defaultValues}); }, UsageError, /Please use camel case to specify source-dir/); }); + + it('throws an error when an option is invalid', () => { + const {argv, defaultValues} = makeArgv({ + globalOpt: { + 'source-dir': { + type: 'string', + demand: false, + }, + }, + }); + const configObject = { + artifacts: 'fake/value/', + }; + assert.throws(() => { + applyConfigToArgv({argv, configObject, defaultValues}); + }, UsageError, /The option artifacts is invalid. Please fix your config/); + }); }); describe('loadJSConfigFile', () => { From 8b3d2c565000a79f1ddebe4e3727ddf7420c6008 Mon Sep 17 00:00:00 2001 From: Shubheksha Jalan Date: Tue, 7 Feb 2017 03:36:27 +0530 Subject: [PATCH 5/6] fix nits --- src/config.js | 15 +++++++-------- tests/unit/test.config.js | 9 ++++++--- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/config.js b/src/config.js index d71df23143..2e4c151307 100644 --- a/src/config.js +++ b/src/config.js @@ -14,12 +14,14 @@ type ApplyConfigToArgvParams = {| argv: Object, configObject: Object, defaultValues: Object, + configFileName?: string, |}; export function applyConfigToArgv({ argv, configObject, defaultValues = {}, + configFileName = '', }: ApplyConfigToArgvParams): Object { const newArgv = {...argv}; for (const option in configObject) { @@ -27,8 +29,8 @@ export function applyConfigToArgv({ // not the same as that on the argv object as there is a very rare chance // of this happening if (camelCase(option) !== option) { - throw new UsageError(`Please use camel case to specify ${option} ` + - 'in the config'); + throw new UsageError(`The config option "${option}" must be ` + + `specified in camel case: "${camelCase(option)}"`); } const wasValueSetOnCLI = typeof(argv[option]) !== 'undefined' && (argv[option] !== defaultValues[option]); @@ -37,12 +39,9 @@ export function applyConfigToArgv({ `configuration: ${option}=${configObject[option]}`); continue; } - if (!argv.hasOwnProperty(option) && - !argv.hasOwnProperty(decamelize(option, '-'))) { - log.debug(`Ignoring configuration: ${option}=${configObject[option]} ` + - 'because this is an unknown option'); - throw new UsageError(`The option ${option} is invalid.` + - ' Please fix your config and try again.'); + if (!argv.hasOwnProperty(decamelize(option, '-'))) { + throw new UsageError(`The config file at ${configFileName} specified ` + + `an unknown option: "${option}"`); } newArgv[option] = configObject[option]; } diff --git a/tests/unit/test.config.js b/tests/unit/test.config.js index d54164f2ff..97efdabb4e 100644 --- a/tests/unit/test.config.js +++ b/tests/unit/test.config.js @@ -285,7 +285,8 @@ describe('config', () => { }; assert.throws(() => { applyConfigToArgv({argv, configObject, defaultValues}); - }, UsageError, /Please use camel case to specify source-dir/); + }, UsageError, 'UsageError: The config option "source-dir" must be ' + + 'specified in camel case: "sourceDir"'); }); it('throws an error when an option is invalid', () => { @@ -297,12 +298,14 @@ describe('config', () => { }, }, }); + const configFileName = 'fake/path/to/config'; const configObject = { artifacts: 'fake/value/', }; assert.throws(() => { - applyConfigToArgv({argv, configObject, defaultValues}); - }, UsageError, /The option artifacts is invalid. Please fix your config/); + applyConfigToArgv({argv, configObject, defaultValues, configFileName}); + }, UsageError, 'UsageError: The config file at fake/path/to/config ' + + 'specified an unknown option: "artifacts"'); }); }); From 21c6b6aba3399fff64ee9f0eea772eddba6a83cb Mon Sep 17 00:00:00 2001 From: Shubheksha Jalan Date: Tue, 7 Feb 2017 17:59:58 +0530 Subject: [PATCH 6/6] fix nits in tests --- src/config.js | 4 ++-- tests/unit/test.config.js | 37 +++++++++++++++++++++---------------- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/config.js b/src/config.js index 2e4c151307..f9ea09baef 100644 --- a/src/config.js +++ b/src/config.js @@ -14,14 +14,14 @@ type ApplyConfigToArgvParams = {| argv: Object, configObject: Object, defaultValues: Object, - configFileName?: string, + configFileName: string, |}; export function applyConfigToArgv({ argv, configObject, defaultValues = {}, - configFileName = '', + configFileName, }: ApplyConfigToArgvParams): Object { const newArgv = {...argv}; for (const option in configObject) { diff --git a/tests/unit/test.config.js b/tests/unit/test.config.js index 97efdabb4e..053b01a7d4 100644 --- a/tests/unit/test.config.js +++ b/tests/unit/test.config.js @@ -45,6 +45,11 @@ function makeArgv({ }; } +const applyConf = (params) => applyConfigToArgv({ + configFileName: 'some/path/to/config.js', + ...params, +}); + describe('config', () => { describe('applyConfigToArgv', () => { @@ -64,7 +69,7 @@ describe('config', () => { const configObject = { sourceDir: '/configured/source/dir', }; - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + const newArgv = applyConf({argv, configObject, defaultValues}); assert.strictEqual(newArgv.sourceDir, cmdLineSrcDir); }); @@ -82,7 +87,7 @@ describe('config', () => { const configObject = { sourceDir: '/configured/source/dir', }; - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + const newArgv = applyConf({argv, configObject, defaultValues}); assert.strictEqual(newArgv.sourceDir, configObject.sourceDir); }); @@ -102,7 +107,7 @@ describe('config', () => { const configObject = { sourceDir: '/configured/source/dir', }; - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + const newArgv = applyConf({argv, configObject, defaultValues}); assert.strictEqual(newArgv.sourceDir, cmdLineSrcDir); }); @@ -124,7 +129,7 @@ describe('config', () => { const configObject = { artifactsDir: '/configured/artifacts/dir', }; - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + const newArgv = applyConf({argv, configObject, defaultValues}); assert.strictEqual(newArgv.sourceDir, 'default/value/option/definition'); }); @@ -148,7 +153,7 @@ describe('config', () => { const configObject = { artifactsDir: '/configured/artifacts/dir', }; - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + const newArgv = applyConf({argv, configObject, defaultValues}); assert.strictEqual(newArgv.sourceDir, cmdLineSrcDir); }); @@ -165,7 +170,7 @@ describe('config', () => { const configObject = { overwriteFiles: true, }; - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + const newArgv = applyConf({argv, configObject, defaultValues}); assert.strictEqual(newArgv.overwriteFiles, true); }); @@ -181,7 +186,7 @@ describe('config', () => { const configObject = { overwriteFiles: true, }; - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + const newArgv = applyConf({argv, configObject, defaultValues}); assert.strictEqual(newArgv.overwriteFiles, true); }); @@ -197,7 +202,7 @@ describe('config', () => { const configObject = { overwriteFiles: false, }; - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + const newArgv = applyConf({argv, configObject, defaultValues}); assert.strictEqual(newArgv.overwriteFiles, true); }); @@ -217,7 +222,7 @@ describe('config', () => { const configObject = { verbose: true, }; - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + const newArgv = applyConf({argv, configObject, defaultValues}); assert.strictEqual(newArgv.sourceDir, cmdLineSrcDir); }); @@ -234,7 +239,7 @@ describe('config', () => { const configObject = { numberOfRetries: 1, }; - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + const newArgv = applyConf({argv, configObject, defaultValues}); assert.strictEqual(newArgv.numberOfRetries, 1); }); @@ -251,7 +256,7 @@ describe('config', () => { const configObject = { numberOfRetries: 1, }; - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + const newArgv = applyConf({argv, configObject, defaultValues}); assert.strictEqual(newArgv.numberOfRetries, 0); }); @@ -267,7 +272,7 @@ describe('config', () => { const configObject = { sourceDir: '/configured/directory', }; - const newArgv = applyConfigToArgv({argv, configObject, defaultValues}); + const newArgv = applyConf({argv, configObject, defaultValues}); assert.strictEqual(newArgv.sourceDir, '/configured/directory'); }); @@ -284,7 +289,7 @@ describe('config', () => { 'source-dir': 'fake/value/', }; assert.throws(() => { - applyConfigToArgv({argv, configObject, defaultValues}); + applyConf({argv, configObject, defaultValues}); }, UsageError, 'UsageError: The config option "source-dir" must be ' + 'specified in camel case: "sourceDir"'); }); @@ -300,12 +305,12 @@ describe('config', () => { }); const configFileName = 'fake/path/to/config'; const configObject = { - artifacts: 'fake/value/', + artifactsDir: 'fake/artifacts/dir', }; assert.throws(() => { - applyConfigToArgv({argv, configObject, defaultValues, configFileName}); + applyConf({argv, configObject, defaultValues, configFileName}); }, UsageError, 'UsageError: The config file at fake/path/to/config ' + - 'specified an unknown option: "artifacts"'); + 'specified an unknown option: "artifactsDir"'); }); });