Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: basic implementation of command line config option #1080

Merged
merged 13 commits into from
Dec 13, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export function applyConfigToArgv({
`an unknown option: "${option}"`);
}
if (options[decamelizedOptName].type === undefined) {
// This means yargs option type wasn't not defined correctly
throw new WebExtError(
`Option: ${option} was defined without a type.`);
}
Expand Down Expand Up @@ -109,4 +110,4 @@ export function loadJSConfigFile(filePath: string): Object {
'Did you set module.exports = {...}?');
}
return configObject;
}
}
35 changes: 32 additions & 3 deletions src/program.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ import {UsageError} from './errors';
import {createLogger, consoleStream as defaultLogStream} from './util/logger';
import {coerceCLICustomPreference} from './firefox/preferences';
import {checkForUpdates as defaultUpdateChecker} from './util/updates';
import {
loadJSConfigFile as defaultLoadJSConfigFile,
applyConfigToArgv as defaultApplyConfigToArgv,
} from './config';

const log = createLogger(__filename);
const envPrefix = 'WEB_EXT';
Expand All @@ -20,13 +24,17 @@ type ProgramOptions = {|
absolutePackageDir?: string,
|}

export type VersionGetterFn = (absolutePackageDir: string) => string;

// TODO: add pipes to Flow type after https://github.com/facebook/flow/issues/2405 is fixed

type ExecuteOptions = {
checkForUpdates?: Function,
systemProcess?: typeof process,
logStream?: typeof defaultLogStream,
getVersion?: Function,
getVersion?: VersionGetterFn,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saintsebastian ❤️ thank you so much! this type annotation is way better!

applyConfigToArgv?: typeof defaultApplyConfigToArgv,
loadJSConfigFile?: typeof defaultLoadJSConfigFile,
shouldExitProgram?: boolean,
globalEnv?: string,
}
Expand Down Expand Up @@ -115,6 +123,8 @@ export class Program {
{
checkForUpdates = defaultUpdateChecker, systemProcess = process,
logStream = defaultLogStream, getVersion = defaultVersionGetter,
applyConfigToArgv = defaultApplyConfigToArgv,
loadJSConfigFile = defaultLoadJSConfigFile,
shouldExitProgram = true, globalEnv = WEBEXT_BUILD_ENV,
}: ExecuteOptions = {}
): Promise<void> {
Expand Down Expand Up @@ -145,7 +155,19 @@ export class Program {
});
}

await runCommand(argv, {shouldExitProgram});
let argvFromConfig = { ...argv };
if (argv.config) {
const configFileName = path.resolve(argv.config);
const configObject = loadJSConfigFile(configFileName);
argvFromConfig = applyConfigToArgv({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for correcting this. It also needs test coverage. When I comment out your fix, the tests all pass -- at least one should fail.

argv,
configFileName,
configObject,
options: this.options,
});
}

await runCommand(argvFromConfig, {shouldExitProgram});

} catch (error) {
if (!(error instanceof UsageError) || argv.verbose) {
Expand Down Expand Up @@ -194,7 +216,7 @@ export function defaultVersionGetter(
// TODO: add pipes to Flow type after https://github.com/facebook/flow/issues/2405 is fixed

type MainParams = {
getVersion?: Function,
getVersion?: VersionGetterFn,
commands?: Object,
argv: Array<any>,
runOptions?: Object,
Expand Down Expand Up @@ -265,6 +287,13 @@ Example: $0 --help run.
describe: 'Disable all features that require standard input',
type: 'boolean',
},
'config': {
alias: 'c',
describe: 'Path to the config file',
default: undefined,
requiresArg: true,
type: 'string',
},
});

program
Expand Down
31 changes: 0 additions & 31 deletions tests/unit/test.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -661,37 +661,6 @@ describe('config', () => {
'WebExtError: Option: apiUrl was defined without a type.');
});

it('throws an error when the type of one of them is in config' +
' missing', () => {
const params = makeArgv({
userCmd: ['sign'],
command: 'sign',
commandOpt: {
'api-url': {
requiresArg: true,
demand: false,
default: 'pretend-default-value-of-apiKey',
},
'api-key': {
requiresArg: true,
demand: false,
type: 'string',
default: 'pretend-default-value-of-apiKey',
},
},
});
const configObject = {
sign: {
apiUrl: 2,
apiKey: 'fake-api-key',
},
};
assert.throws(() => {
applyConf({...params, configObject});
}, WebExtError,
'WebExtError: Option: apiUrl was defined without a type.');
});

it('throws an error when type of unrelated sub option is invalid', () => {
const program = new Program(['run']);

Expand Down
87 changes: 83 additions & 4 deletions tests/unit/test.program.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,20 @@ import {assert} from 'chai';

import {defaultVersionGetter, main, Program} from '../../src/program';
import commands from '../../src/cmd';
import {onlyInstancesOf, UsageError} from '../../src/errors';
import {
onlyInstancesOf,
UsageError,
} from '../../src/errors';
import {
createFakeProcess,
fake,
makeSureItFails,
ErrorWithCode,
} from './helpers';
import {ConsoleStream} from '../../src/util/logger';

import {
consoleStream, // instance is imported to inspect logged messages
ConsoleStream,
} from '../../src/util/logger';

describe('program.Program', () => {

Expand All @@ -26,7 +31,7 @@ describe('program.Program', () => {
const absolutePackageDir = path.join(__dirname, '..', '..');
return program.execute(
absolutePackageDir, {
getVersion: () => spy(),
getVersion: () => 'not-a-real-version',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

argh... This is why it is better to use a more specific type signature instead of Function as a flow type signature :-)

Do you mind to file a follow up issue related to a more specific type signature for getVersion in "src/program.js" (or we can check how many flow errors it produces, if they are not too much we can fix it in this patch, on the contrary we can move it into a separate issue and pull request)

A reasonable flow type could be just () => string (a function which takes no parameters and returns a string).

checkForUpdates: spy(),
systemProcess: fakeProcess,
shouldExitProgram: false,
Expand Down Expand Up @@ -217,6 +222,33 @@ describe('program.Program', () => {
});
});

it('logs UsageErrors into console', () => {
// Clear console stream from previous messages and start recording
consoleStream.stopCapturing();
consoleStream.flushCapturedLogs();
consoleStream.startCapturing();

const program = new Program(['thing']).command('thing', '', () => {
throw new UsageError('some error');
});
program.setGlobalOptions({
verbose: {
type: 'boolean',
},
});
return execProgram(program)
.then(makeSureItFails())
.catch(onlyInstancesOf(UsageError, (error) => {
const {capturedMessages} = consoleStream;
// Stop recording
consoleStream.stopCapturing();
assert.match(error.message, /some error/);
assert.ok(
capturedMessages.some((message) => message.match(/some error/)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great test! Thanks

));
}));
});

it('throws an error about unknown commands', () => {
return execProgram(new Program(['nope']))
.then(makeSureItFails())
Expand Down Expand Up @@ -430,6 +462,53 @@ describe('program.main', () => {
assert.strictEqual(options.shouldExitProgram, false);
});
});

it('applies options from the specified config file', () => {
const fakeCommands = fake(commands, {
lint: () => Promise.resolve(),
});
const fakePath = 'path/to/web-ext-config.js';
const configObject = {
prop: 'prop',
};
const resolvedFakePath = path.resolve(fakePath);
const expectedArgv = {
_: ['lint'],
config: fakePath,
};
const fakeLoadJSConfigFile = sinon.spy(() => {
return configObject;
});
const fakeApplyConfigToArgv = sinon.spy(() => {
return expectedArgv;
});

return execProgram(
['lint', '--config', fakePath],
{
commands: fakeCommands,
runOptions: {
applyConfigToArgv: fakeApplyConfigToArgv,
loadJSConfigFile: fakeLoadJSConfigFile,
},
})
.then(() => {
const options = fakeCommands.lint.firstCall.args[0];
assert.strictEqual(options.config, fakePath);
sinon.assert.calledOnce(fakeLoadJSConfigFile);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to see the sinon.assert.calledOnce and sinon.assert.calledWith related to the same spy grouped together.

sinon.assert.calledWith(fakeLoadJSConfigFile,
sinon.match(resolvedFakePath));
sinon.assert.calledOnce(fakeApplyConfigToArgv);
sinon.assert.calledWith(fakeApplyConfigToArgv, sinon.match({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to check all parameters for the call to fakeApplyConfigToArgv because Flow isn't going to catch all configuration problem, especially the options parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume options will always be the same (set in program.js), but i added a check for argv.

configFileName: resolvedFakePath,
configObject,
argv: {
_: ['lint'],
config: fakePath,
},
}));
});
});
});

describe('program.defaultVersionGetter', () => {
Expand Down