From 825663d206fb864d12ede0fdd47cbe83e623fd54 Mon Sep 17 00:00:00 2001 From: Yaroslav Admin Date: Thu, 17 Dec 2020 00:38:04 +0100 Subject: [PATCH] feat(cli): error out on unexpected options or parameters This should make CLI more helpful as it will error out early and users can see that they have passed a wrong option instead of guessing why it does not have any effect. Notes: - units tests use same parser configuration as production code (hence changes to tests) - logic and test case for _ typos in option names was removed as this is covered by yargs strict mode now - added documentation for couple of existing options as otherwise they are considered unknown and error out (but they do exist and were found in the unit tests) BREAKING CHANGE: Karma is more strict and will error out if unknown option or argument is passed to CLI. --- lib/cli.js | 52 ++++++++++++++++++------- test/e2e/cli.feature | 90 +++++++++++++++++++++++++++++++++++++------ test/unit/cli.spec.js | 59 +++++++++++----------------- 3 files changed, 140 insertions(+), 61 deletions(-) diff --git a/lib/cli.js b/lib/cli.js index 448f56eb6..0aac17e9a 100644 --- a/lib/cli.js +++ b/lib/cli.js @@ -1,7 +1,6 @@ 'use strict' const path = require('path') -const assert = require('assert') const yargs = require('yargs') const fs = require('graceful-fs') @@ -10,12 +9,9 @@ const helper = require('./helper') const constant = require('./constants') function processArgs (argv, options, fs, path) { - // TODO(vojta): warn/throw when unknown argument (probably mispelled) Object.getOwnPropertyNames(argv).forEach(function (name) { let argumentValue = argv[name] if (name !== '_' && name !== '$0') { - assert(!name.includes('_'), `Bad argument: ${name} did you mean ${name.replace('_', '-')}`) - if (Array.isArray(argumentValue)) { argumentValue = argumentValue.pop() // If the same argument is defined multiple times, override. } @@ -99,7 +95,7 @@ function processArgs (argv, options, fs, path) { options.refresh = options.refresh === 'true' } - let configFile = argv._.shift() + let configFile = argv.configFile if (!configFile) { // default config file (if exists) @@ -151,13 +147,13 @@ function describeRoot () { 'Run --help with particular command to see its description and available options.\n\n' + 'Usage:\n' + ' $0 ') - .command('init', 'Initialize a config file.', describeInit) - .command('start', 'Start the server / do a single run.', describeStart) - .command('run', 'Trigger a test run.', describeRun) - .command('stop', 'Stop the server.', describeStop) + .command('init [configFile]', 'Initialize a config file.', describeInit) + .command('start [configFile]', 'Start the server / do a single run.', describeStart) + .command('run [configFile]', 'Trigger a test run.', describeRun) + .command('stop [configFile]', 'Stop the server.', describeStop) .command('completion', 'Shell completion for karma.', describeCompletion) .demandCommand(1, 'Command not specified.') - .strictCommands() + .strict() .describe('help', 'Print usage and options.') .describe('version', 'Print current version.') } @@ -168,8 +164,11 @@ function describeInit (yargs) { 'INIT - Initialize a config file.\n\n' + 'Usage:\n' + ' $0 init [configFile]') - .strictCommands(false) .version(false) + .positional('configFile', { + describe: 'Name of the generated Karma configuration file', + type: 'string' + }) .describe('log-level', ' Level of logging.') .describe('colors', 'Use colors when reporting and printing logs.') .describe('no-colors', 'Do not use colors when reporting or printing logs.') @@ -183,6 +182,10 @@ function describeStart (yargs) { ' $0 start [configFile]') .strictCommands(false) .version(false) + .positional('configFile', { + describe: 'Path to the Karma configuration file', + type: 'string' + }) .describe('port', ' Port where the server is running.') .describe('auto-watch', 'Auto watch source files and run on change.') .describe('detached', 'Detach the server.') @@ -200,6 +203,10 @@ function describeStart (yargs) { .describe('no-fail-on-empty-test-suite', 'Do not fail on empty test suite.') .describe('fail-on-failing-test-suite', 'Fail on failing test suite.') .describe('no-fail-on-failing-test-suite', 'Do not fail on failing test suite.') + .option('format-error', { + describe: 'A path to a file that exports the format function.', + type: 'string' + }) } function describeRun (yargs) { @@ -208,8 +215,11 @@ function describeRun (yargs) { 'RUN - Run the tests (requires running server).\n\n' + 'Usage:\n' + ' $0 run [configFile] [-- ]') - .strictCommands(false) .version(false) + .positional('configFile', { + describe: 'Path to the Karma configuration file', + type: 'string' + }) .describe('port', ' Port where the server is listening.') .describe('no-refresh', 'Do not re-glob all the patterns.') .describe('fail-on-empty-test-suite', 'Fail on empty test suite.') @@ -217,6 +227,18 @@ function describeRun (yargs) { .describe('log-level', ' Level of logging.') .describe('colors', 'Use colors when reporting and printing logs.') .describe('no-colors', 'Do not use colors when reporting or printing logs.') + .option('removed-files', { + describe: 'Comma-separated paths to removed files. Useful when automatic file watching is disabled.', + type: 'string' + }) + .option('changed-files', { + describe: 'Comma-separated paths to changed files. Useful when automatic file watching is disabled.', + type: 'string' + }) + .option('added-files', { + describe: 'Comma-separated paths to added files. Useful when automatic file watching is disabled.', + type: 'string' + }) } function describeStop (yargs) { @@ -225,8 +247,11 @@ function describeStop (yargs) { 'STOP - Stop the server (requires running server).\n\n' + 'Usage:\n' + ' $0 stop [configFile]') - .strictCommands(false) .version(false) + .positional('configFile', { + describe: 'Path to the Karma configuration file', + type: 'string' + }) .describe('port', ' Port where the server is listening.') .describe('log-level', ' Level of logging.') } @@ -237,7 +262,6 @@ function describeCompletion (yargs) { 'COMPLETION - Bash/ZSH completion for karma.\n\n' + 'Installation:\n' + ' $0 completion >> ~/.bashrc') - .strictCommands(false) .version(false) } diff --git a/test/e2e/cli.feature b/test/e2e/cli.feature index 2913f23f2..476d29a8e 100644 --- a/test/e2e/cli.feature +++ b/test/e2e/cli.feature @@ -15,11 +15,11 @@ Feature: CLI karma Commands: - karma init Initialize a config file. - karma start Start the server / do a single run. - karma run Trigger a test run. - karma stop Stop the server. - karma completion Shell completion for karma. + karma init [configFile] Initialize a config file. + karma start [configFile] Start the server / do a single run. + karma run [configFile] Trigger a test run. + karma stop [configFile] Stop the server. + karma completion Shell completion for karma. Options: --help Print usage and options. [boolean] @@ -45,17 +45,62 @@ Feature: CLI karma Commands: - karma init Initialize a config file. - karma start Start the server / do a single run. - karma run Trigger a test run. - karma stop Stop the server. - karma completion Shell completion for karma. + karma init [configFile] Initialize a config file. + karma start [configFile] Start the server / do a single run. + karma run [configFile] Trigger a test run. + karma stop [configFile] Stop the server. + karma completion Shell completion for karma. Options: --help Print usage and options. [boolean] --version Print current version. [boolean] - Unknown command: strat + Unknown argument: strat + """ + + Scenario: Error when option is unknown + When I execute Karma with arguments: "start --invalid-option" + Then the stderr is exactly: + """ + Karma - Spectacular Test Runner for JavaScript. + + START - Start the server / do a single run. + + Usage: + karma start [configFile] + + Positionals: + configFile Path to the Karma configuration file [string] + + Options: + --help Print usage and options. [boolean] + --port Port where the server is running. + --auto-watch Auto watch source files and run on change. + --detached Detach the server. + --no-auto-watch Do not watch source files. + --log-level Level + of logging. + --colors Use colors when reporting and printing logs. + --no-colors Do not use colors when reporting or printing + logs. + --reporters List of reporters (available: dots, progress, + junit, growl, coverage). + --browsers List of browsers to start (eg. --browsers + Chrome,ChromeCanary,Firefox). + --capture-timeout Kill browser if does not capture in + given time [ms]. + --single-run Run the test when browsers captured and exit. + --no-single-run Disable single-run. + --report-slower-than Report tests that are slower than + given time [ms]. + --fail-on-empty-test-suite Fail on empty test suite. + --no-fail-on-empty-test-suite Do not fail on empty test suite. + --fail-on-failing-test-suite Fail on failing test suite. + --no-fail-on-failing-test-suite Do not fail on failing test suite. + --format-error A path to a file that exports the format + function. [string] + + Unknown arguments: invalid-option, invalidOption """ Scenario: Init command help @@ -69,6 +114,9 @@ Feature: CLI Usage: karma init [configFile] + Positionals: + configFile Name of the generated Karma configuration file [string] + Options: --help Print usage and options. [boolean] --log-level Level of logging. @@ -87,6 +135,9 @@ Feature: CLI Usage: karma start [configFile] + Positionals: + configFile Path to the Karma configuration file [string] + Options: --help Print usage and options. [boolean] --port Port where the server is running. @@ -112,6 +163,8 @@ Feature: CLI --no-fail-on-empty-test-suite Do not fail on empty test suite. --fail-on-failing-test-suite Fail on failing test suite. --no-fail-on-failing-test-suite Do not fail on failing test suite. + --format-error A path to a file that exports the format + function. [string] """ Scenario: Run command help @@ -125,6 +178,9 @@ Feature: CLI Usage: karma run [configFile] [-- ] + Positionals: + configFile Path to the Karma configuration file [string] + Options: --help Print usage and options. [boolean] --port Port where the server is listening. @@ -136,6 +192,15 @@ Feature: CLI --colors Use colors when reporting and printing logs. --no-colors Do not use colors when reporting or printing logs. + --removed-files Comma-separated paths to removed files. Useful + when automatic file watching is disabled. + [string] + --changed-files Comma-separated paths to changed files. Useful + when automatic file watching is disabled. + [string] + --added-files Comma-separated paths to added files. Useful + when automatic file watching is disabled. + [string] """ Scenario: Stop command help @@ -149,6 +214,9 @@ Feature: CLI Usage: karma stop [configFile] + Positionals: + configFile Path to the Karma configuration file [string] + Options: --help Print usage and options. [boolean] --port Port where the server is listening. diff --git a/test/unit/cli.spec.js b/test/unit/cli.spec.js index 8d9223256..222e92c04 100644 --- a/test/unit/cli.spec.js +++ b/test/unit/cli.spec.js @@ -1,6 +1,5 @@ 'use strict' -const yargs = require('yargs') const path = require('path') const mocks = require('mocks') @@ -34,7 +33,7 @@ describe('cli', () => { } const processArgs = (args, opts) => { - const argv = yargs.parse(args) + const argv = m.describeRoot().parse(args) return e.processArgs(argv, opts || {}, fsMock, pathMock) } @@ -63,14 +62,14 @@ describe('cli', () => { describe('processArgs', () => { it('should override if multiple options given', () => { // yargs parses --port 123 --port 456 as port = [123, 456] which makes no sense - const options = processArgs(['some.conf', '--port', '12', '--log-level', 'info', '--port', '34', '--log-level', 'debug']) + const options = processArgs(['start', 'some.conf', '--port', '12', '--log-level', 'info', '--port', '34', '--log-level', 'debug']) expect(options.port).to.equal(34) expect(options.logLevel).to.equal('DEBUG') }) it('should return camelCased options', () => { - const options = processArgs(['some.conf', '--port', '12', '--single-run']) + const options = processArgs(['start', 'some.conf', '--port', '12', '--single-run']) expect(options.configFile).to.exist expect(options.port).to.equal(12) @@ -79,41 +78,40 @@ describe('cli', () => { it('should parse options without configFile and set default', () => { setCWD('/cwd') - const options = processArgs(['--auto-watch', '--auto-watch-interval', '10']) + const options = processArgs(['start', '--auto-watch']) expect(path.resolve(options.configFile)).to.equal(path.resolve('/cwd/karma.conf.js')) expect(options.autoWatch).to.equal(true) - expect(options.autoWatchInterval).to.equal(10) }) it('should set default karma.conf.coffee config file if exists', () => { setCWD('/cwd2') - const options = processArgs(['--port', '10']) + const options = processArgs(['start', '--port', '10']) expect(path.resolve(options.configFile)).to.equal(path.resolve('/cwd2/karma.conf.coffee')) }) it('should set default karma.conf.ts config file if exists', () => { setCWD('/cwd3') - const options = processArgs(['--port', '10']) + const options = processArgs(['start', '--port', '10']) expect(path.resolve(options.configFile)).to.equal(path.resolve('/cwd3/karma.conf.ts')) }) it('should not set default config if neither exists', () => { setCWD('/') - const options = processArgs([]) + const options = processArgs(['start']) expect(options.configFile).to.equal(null) }) it('should parse auto-watch, colors, singleRun to boolean', () => { - let options = processArgs(['--auto-watch', 'false', '--colors', 'false', '--single-run', 'false']) + let options = processArgs(['start', '--auto-watch', 'false', '--colors', 'false', '--single-run', 'false']) expect(options.autoWatch).to.equal(false) expect(options.colors).to.equal(false) expect(options.singleRun).to.equal(false) - options = processArgs(['--auto-watch', 'true', '--colors', 'true', '--single-run', 'true']) + options = processArgs(['start', '--auto-watch', 'true', '--colors', 'true', '--single-run', 'true']) expect(options.autoWatch).to.equal(true) expect(options.colors).to.equal(true) @@ -121,63 +119,64 @@ describe('cli', () => { }) it('should replace log-level constants', () => { - let options = processArgs(['--log-level', 'debug']) + let options = processArgs(['start', '--log-level', 'debug']) expect(options.logLevel).to.equal(constant.LOG_DEBUG) - options = processArgs(['--log-level', 'error']) + options = processArgs(['start', '--log-level', 'error']) expect(options.logLevel).to.equal(constant.LOG_ERROR) - options = processArgs(['--log-level', 'warn']) + options = processArgs(['start', '--log-level', 'warn']) expect(options.logLevel).to.equal(constant.LOG_WARN) - options = processArgs(['--log-level', 'foo']) + options = processArgs(['start', '--log-level', 'foo']) expect(mockery.process.exit).to.have.been.calledWith(1) - options = processArgs(['--log-level']) + options = processArgs(['start', '--log-level']) expect(mockery.process.exit).to.have.been.calledWith(1) }) it('should parse format-error into a function', () => { // root export - let options = processArgs(['--format-error', '../../test/unit/fixtures/format-error-root']) + let options = processArgs(['start', '--format-error', '../../test/unit/fixtures/format-error-root']) const formatErrorRoot = require('../../test/unit/fixtures/format-error-root') expect(options.formatError).to.equal(formatErrorRoot) // property export - options = processArgs(['--format-error', '../../test/unit/fixtures/format-error-property']) + options = processArgs(['start', '--format-error', '../../test/unit/fixtures/format-error-property']) const formatErrorProperty = require('../../test/unit/fixtures/format-error-property').formatError expect(options.formatError).to.equal(formatErrorProperty) }) it('should parse browsers into an array', () => { - const options = processArgs(['--browsers', 'Chrome,ChromeCanary,Firefox']) + const options = processArgs(['start', '--browsers', 'Chrome,ChromeCanary,Firefox']) expect(options.browsers).to.deep.equal(['Chrome', 'ChromeCanary', 'Firefox']) }) it('should resolve configFile to absolute path', () => { setCWD('/cwd') - const options = processArgs(['some/config.js']) + const options = processArgs(['start', 'some/config.js']) expect(path.resolve(options.configFile)).to.equal(path.resolve('/cwd/some/config.js')) }) it('should parse report-slower-than to a number', () => { - let options = processArgs(['--report-slower-than', '2000']) + let options = processArgs(['start', '--report-slower-than', '2000']) expect(options.reportSlowerThan).to.equal(2000) - options = processArgs(['--no-report-slower-than']) + options = processArgs(['start', '--no-report-slower-than']) expect(options.reportSlowerThan).to.equal(0) }) it('should cast reporters to array', () => { - let options = processArgs(['--reporters', 'dots,junit']) + let options = processArgs(['start', '--reporters', 'dots,junit']) expect(options.reporters).to.deep.equal(['dots', 'junit']) - options = processArgs(['--reporters', 'dots']) + options = processArgs(['start', '--reporters', 'dots']) expect(options.reporters).to.deep.equal(['dots']) }) it('should parse removed/added/changed files to array', () => { const options = processArgs([ + 'run', '--removed-files', 'r1.js,r2.js', '--changed-files', 'ch1.js,ch2.js', '--added-files', 'a1.js,a2.js' @@ -187,18 +186,6 @@ describe('cli', () => { expect(options.addedFiles).to.deep.equal(['a1.js', 'a2.js']) expect(options.changedFiles).to.deep.equal(['ch1.js', 'ch2.js']) }) - - it('should error on args with underscores', () => { - let expectedException - try { - const options = processArgs(['--no_browsers']) - expectedException = 'Should have thrown but got ' + options - } catch (e) { - expectedException = e - } finally { - expect(expectedException + '').to.includes('Bad argument: no_browsers did you mean no-browsers') - } - }) }) describe('parseClientArgs', () => {