From 3848c777b290b4c5ed6ad1af0436afe5f952b005 Mon Sep 17 00:00:00 2001 From: Luca Greco Date: Fri, 19 Jan 2018 20:19:52 +0100 Subject: [PATCH 1/6] fix: Do not treat an array of config values as a sub-command config section --- src/config.js | 7 +++++-- src/program.js | 6 +++--- tests/unit/test.config.js | 24 ++++++++++++++++++++++++ tests/unit/test.program.js | 2 +- 4 files changed, 33 insertions(+), 6 deletions(-) diff --git a/src/config.js b/src/config.js index 765d953f30..3d43d79944 100644 --- a/src/config.js +++ b/src/config.js @@ -32,14 +32,17 @@ export function applyConfigToArgv({ }: ApplyConfigToArgvParams): Object { let newArgv = {...argv}; - for (const option in configObject) { + for (const option of Object.keys(configObject)) { if (camelCase(option) !== option) { throw new UsageError( `The config option "${option}" must be ` + `specified in camel case: "${camelCase(option)}"`); } - if (typeof options[option] === 'object' && + // A config option cannot be a sub-command config + // object if it is an array. + if (!Array.isArray(configObject[option]) && + typeof options[option] === 'object' && typeof configObject[option] === 'object') { // Descend into the nested configuration for a sub-command. newArgv = applyConfigToArgv({ diff --git a/src/program.js b/src/program.js index 3cddefbbf1..3039d10d62 100644 --- a/src/program.js +++ b/src/program.js @@ -401,7 +401,7 @@ Example: $0 --help run. 'run against multiple targets.', default: 'firefox-desktop', demand: false, - type: 'string', + type: 'array', }, 'firefox': { alias: ['f', 'firefox-binary'], @@ -447,7 +447,7 @@ Example: $0 --help run. 'preference.', demand: false, requiresArg: true, - type: 'string', + type: 'array', coerce: coerceCLICustomPreference, }, 'start-url': { @@ -455,7 +455,7 @@ Example: $0 --help run. describe: 'Launch firefox at specified page', demand: false, requiresArg: true, - type: 'string', + type: 'array', }, 'browser-console': { alias: ['bc'], diff --git a/tests/unit/test.config.js b/tests/unit/test.config.js index b66add8e89..ef7a620d82 100644 --- a/tests/unit/test.config.js +++ b/tests/unit/test.config.js @@ -296,6 +296,30 @@ describe('config', () => { assert.strictEqual(argv.ignoreFiles, configObject.ignoreFiles); }); + it('does not mistake an array config values for a sub-command', + () => { + const params = makeArgv({ + userCmd: ['fakecommand'], + globalOpt: { + pref: { + demand: false, + type: 'string', + }, + }, + }); + + const configObject = { + pref: ['pref1=true', 'pref2=false'], + }; + + // TODO: expect a raised exception array is not a string + assert.throws( + () => applyConf({...params, configObject}), + UsageError, + 'type of "pref" incorrectly as "array" (expected type "string")' + ); + }); + it('uses CLI option over undefined configured option and default', () => { const cmdLineSrcDir = '/user/specified/source/dir/'; const params = makeArgv({ diff --git a/tests/unit/test.program.js b/tests/unit/test.program.js index d7db25ca29..9dbad8e152 100644 --- a/tests/unit/test.program.js +++ b/tests/unit/test.program.js @@ -449,7 +449,7 @@ describe('program.main', () => { .then(() => { sinon.assert.calledWithMatch( fakeCommands.run, - {startUrl: 'www.example.com'} + {startUrl: ['www.example.com']} ); }); }); From 1a54ce9ab002c2c4818467c4b0b880ec09900f32 Mon Sep 17 00:00:00 2001 From: Luca Greco Date: Tue, 23 Jan 2018 03:16:39 +0100 Subject: [PATCH 2/6] chore: apply changes to the test.config.js test based on review comments --- tests/unit/test.config.js | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/tests/unit/test.config.js b/tests/unit/test.config.js index ef7a620d82..d7d17e402f 100644 --- a/tests/unit/test.config.js +++ b/tests/unit/test.config.js @@ -303,7 +303,7 @@ describe('config', () => { globalOpt: { pref: { demand: false, - type: 'string', + type: 'array', }, }, }); @@ -312,12 +312,8 @@ describe('config', () => { pref: ['pref1=true', 'pref2=false'], }; - // TODO: expect a raised exception array is not a string - assert.throws( - () => applyConf({...params, configObject}), - UsageError, - 'type of "pref" incorrectly as "array" (expected type "string")' - ); + const resultArgv = applyConf({...params, configObject}); + assert.strictEqual(resultArgv.pref, configObject.pref); }); it('uses CLI option over undefined configured option and default', () => { From 025db7cc8b711cbd0f4141b493f407f268c99fea Mon Sep 17 00:00:00 2001 From: Luca Greco Date: Tue, 23 Jan 2018 03:31:04 +0100 Subject: [PATCH 3/6] chore: update flow types (and related test code changes) --- src/cmd/run.js | 4 ++-- src/firefox/preferences.js | 7 +------ tests/unit/test-cmd/test.run.js | 2 +- tests/unit/test-firefox/test.preferences.js | 14 +++++++------- 4 files changed, 11 insertions(+), 16 deletions(-) diff --git a/src/cmd/run.js b/src/cmd/run.js index df45f9baf2..70e55719ca 100644 --- a/src/cmd/run.js +++ b/src/cmd/run.js @@ -40,8 +40,8 @@ export type CmdRunParams = {| noReload: boolean, preInstall: boolean, sourceDir: string, - startUrl?: string | Array, - target?: string | Array, + startUrl?: Array, + target?: Array, // Android CLI options. adbBin?: string, diff --git a/src/firefox/preferences.js b/src/firefox/preferences.js index 985ec5cc27..657df42647 100644 --- a/src/firefox/preferences.js +++ b/src/firefox/preferences.js @@ -117,15 +117,10 @@ export function getPrefs( } export function coerceCLICustomPreference( - cliPrefs: string | Array + cliPrefs: Array ): FirefoxPreferences { const customPrefs = {}; - if (!Array.isArray(cliPrefs)) { - cliPrefs = [cliPrefs]; - } - - for (const pref of cliPrefs) { const prefsAry = pref.split('='); diff --git a/tests/unit/test-cmd/test.run.js b/tests/unit/test-cmd/test.run.js index d861e6a240..34a06693d0 100644 --- a/tests/unit/test-cmd/test.run.js +++ b/tests/unit/test-cmd/test.run.js @@ -73,7 +73,7 @@ describe('run', () => { it('passes startUrl parameter to Firefox when specified', async () => { const cmd = prepareRun(); - const expectedStartUrls = 'www.example.com'; + const expectedStartUrls = ['www.example.com']; const FirefoxDesktopExtensionRunner = sinon.spy(FakeExtensionRunner); await cmd.run({startUrl: expectedStartUrls}, diff --git a/tests/unit/test-firefox/test.preferences.js b/tests/unit/test-firefox/test.preferences.js index 170fee39e9..45993f9392 100644 --- a/tests/unit/test-firefox/test.preferences.js +++ b/tests/unit/test-firefox/test.preferences.js @@ -39,7 +39,7 @@ describe('firefox/preferences', () => { describe('coerceCLICustomPreference', () => { it('converts a single --pref cli option from string to object', () => { - const prefs = coerceCLICustomPreference('valid.preference=true'); + const prefs = coerceCLICustomPreference(['valid.preference=true']); assert.isObject(prefs); assert.equal(prefs['valid.preference'], true); }); @@ -54,23 +54,23 @@ describe('firefox/preferences', () => { }); it('converts boolean values', () => { - const prefs = coerceCLICustomPreference('valid.preference=true'); + const prefs = coerceCLICustomPreference(['valid.preference=true']); assert.equal(prefs['valid.preference'], true); }); it('converts number values', () => { - const prefs = coerceCLICustomPreference('valid.preference=455'); + const prefs = coerceCLICustomPreference(['valid.preference=455']); assert.equal(prefs['valid.preference'], 455); }); it('converts float values', () => { - const prefs = coerceCLICustomPreference('valid.preference=4.55'); + const prefs = coerceCLICustomPreference(['valid.preference=4.55']); assert.equal(prefs['valid.preference'], '4.55'); }); it('supports string values with "=" chars', () => { const prefs = coerceCLICustomPreference( - 'valid.preference=value=withequals=chars' + ['valid.preference=value=withequals=chars'] ); assert.equal(prefs['valid.preference'], 'value=withequals=chars'); }); @@ -87,13 +87,13 @@ describe('firefox/preferences', () => { it('throws an error for invalid or incomplete preferences', () => { assert.throws( - () => coerceCLICustomPreference('test.invalid.prop'), + () => coerceCLICustomPreference(['test.invalid.prop']), UsageError, 'UsageError: Incomplete custom preference: "test.invalid.prop". ' + 'Syntax expected: "prefname=prefvalue".' ); - assert.throws(() => coerceCLICustomPreference('*&%£=true'), + assert.throws(() => coerceCLICustomPreference(['*&%£=true']), UsageError, 'UsageError: Invalid custom preference name: *&%£'); }); From 6ded41520767c156913aedd1b6327dd5a8536af1 Mon Sep 17 00:00:00 2001 From: Luca Greco Date: Tue, 23 Jan 2018 12:23:25 +0100 Subject: [PATCH 4/6] fix: Ensured that verbose logs are enabled on verbose set from config --- src/program.js | 16 ++++++++++++++-- tests/unit/test.program.js | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/src/program.js b/src/program.js index 3039d10d62..bc3a37649c 100644 --- a/src/program.js +++ b/src/program.js @@ -144,11 +144,16 @@ export class Program { const runCommand = this.commands[cmd]; + let versionLogged = false; + if (argv.verbose) { logStream.makeVerbose(); log.info('Version:', getVersion(absolutePackageDir)); + versionLogged = true; } + let adjustedArgv = {...argv}; + try { if (cmd === undefined) { throw new UsageError('No sub-command was specified in the args'); @@ -162,7 +167,6 @@ export class Program { }); } - let adjustedArgv = {...argv}; const configFiles = []; if (argv.configDiscovery) { @@ -201,10 +205,18 @@ export class Program { }); }); + if (adjustedArgv.verbose) { + logStream.makeVerbose(); + + if (!versionLogged) { + log.info('Version:', getVersion(absolutePackageDir)); + } + } + await runCommand(adjustedArgv, {shouldExitProgram}); } catch (error) { - if (!(error instanceof UsageError) || argv.verbose) { + if (!(error instanceof UsageError) || adjustedArgv.verbose) { log.error(`\n${error.stack}\n`); } else { log.error(`\n${error}\n`); diff --git a/tests/unit/test.program.js b/tests/unit/test.program.js index 9dbad8e152..eaf8e503ba 100644 --- a/tests/unit/test.program.js +++ b/tests/unit/test.program.js @@ -674,6 +674,39 @@ describe('program.main', () => { // This should equal the final configured value. assert.equal(options.sourceDir, finalSourceDir); }); + + it('enables verbose more from config file', async () => { + const logStream = fake(new ConsoleStream()); + const fakeCommands = fake(commands, { + lint: () => Promise.resolve(), + }); + + const customConfig = path.resolve('custom/web-ext-config.js'); + + const finalSourceDir = path.resolve('final/source-dir'); + const loadJSConfigFile = makeConfigLoader({ + configObjects: { + [customConfig]: { + sourceDir: finalSourceDir, + verbose: true, + }, + }, + }); + + await execProgram( + ['lint', '--config', customConfig], + { + commands: fakeCommands, + runOptions: { + discoverConfigFiles: async () => [], + loadJSConfigFile, + logStream, + }, + } + ); + + sinon.assert.called(logStream.makeVerbose); + }); }); describe('program.defaultVersionGetter', () => { From 8f8e17c3a0c72dfd45b0356af3ef29edf54976ce Mon Sep 17 00:00:00 2001 From: Luca Greco Date: Tue, 23 Jan 2018 17:56:01 +0100 Subject: [PATCH 5/6] chore: minor tweak on test.program.js --- tests/unit/test.program.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/unit/test.program.js b/tests/unit/test.program.js index eaf8e503ba..77fbb9b221 100644 --- a/tests/unit/test.program.js +++ b/tests/unit/test.program.js @@ -683,11 +683,9 @@ describe('program.main', () => { const customConfig = path.resolve('custom/web-ext-config.js'); - const finalSourceDir = path.resolve('final/source-dir'); const loadJSConfigFile = makeConfigLoader({ configObjects: { [customConfig]: { - sourceDir: finalSourceDir, verbose: true, }, }, From 5fa17c808aab66beb4a3510bb33036eb4554dc49 Mon Sep 17 00:00:00 2001 From: Luca Greco Date: Tue, 23 Jan 2018 18:10:21 +0100 Subject: [PATCH 6/6] chore: refactoring out 'enable verbose mode' into a class method --- src/program.js | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/program.js b/src/program.js index bc3a37649c..cf24d172aa 100644 --- a/src/program.js +++ b/src/program.js @@ -50,6 +50,7 @@ export class Program { yargs: any; commands: { [key: string]: Function }; shouldExitProgram: boolean; + verboseEnabled: boolean; options: Object; constructor( @@ -69,6 +70,7 @@ export class Program { // config (See web-ext#469 for rationale). const yargsInstance = yargs(argv, absolutePackageDir); + this.verboseEnabled = false; this.shouldExitProgram = true; this.yargs = yargsInstance; this.yargs.strict(); @@ -121,6 +123,19 @@ export class Program { return this; } + enableVerboseMode( + logStream: typeof defaultLogStream, + version: string + ): void { + if (this.verboseEnabled) { + return; + } + + logStream.makeVerbose(); + log.info('Version:', version); + this.verboseEnabled = true; + } + async execute( absolutePackageDir: string, { @@ -135,21 +150,17 @@ export class Program { globalEnv = WEBEXT_BUILD_ENV, }: ExecuteOptions = {} ): Promise { - this.shouldExitProgram = shouldExitProgram; this.yargs.exitProcess(this.shouldExitProgram); const argv = this.yargs.argv; const cmd = argv._[0]; + const version = getVersion(absolutePackageDir); const runCommand = this.commands[cmd]; - let versionLogged = false; - if (argv.verbose) { - logStream.makeVerbose(); - log.info('Version:', getVersion(absolutePackageDir)); - versionLogged = true; + this.enableVerboseMode(logStream, version); } let adjustedArgv = {...argv}; @@ -206,11 +217,8 @@ export class Program { }); if (adjustedArgv.verbose) { - logStream.makeVerbose(); - - if (!versionLogged) { - log.info('Version:', getVersion(absolutePackageDir)); - } + // Ensure that the verbose is enabled when specified in a config file. + this.enableVerboseMode(logStream, version); } await runCommand(adjustedArgv, {shouldExitProgram});