Skip to content

Commit 603bbc0

Browse files
devoto13devoto13
authoredDec 17, 2020
feat(cli): error out on unexpected options or parameters (#3589)
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.
·
v6.4.4v6.0.0
1 parent 7a3bd55 commit 603bbc0

File tree

3 files changed

+140
-61
lines changed

3 files changed

+140
-61
lines changed
 

‎lib/cli.js

+38-14
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
'use strict'
22

33
const path = require('path')
4-
const assert = require('assert')
54
const yargs = require('yargs')
65
const fs = require('graceful-fs')
76

@@ -10,12 +9,9 @@ const helper = require('./helper')
109
const constant = require('./constants')
1110

1211
function processArgs (argv, options, fs, path) {
13-
// TODO(vojta): warn/throw when unknown argument (probably mispelled)
1412
Object.getOwnPropertyNames(argv).forEach(function (name) {
1513
let argumentValue = argv[name]
1614
if (name !== '_' && name !== '$0') {
17-
assert(!name.includes('_'), `Bad argument: ${name} did you mean ${name.replace('_', '-')}`)
18-
1915
if (Array.isArray(argumentValue)) {
2016
argumentValue = argumentValue.pop() // If the same argument is defined multiple times, override.
2117
}
@@ -99,7 +95,7 @@ function processArgs (argv, options, fs, path) {
9995
options.refresh = options.refresh === 'true'
10096
}
10197

102-
let configFile = argv._.shift()
98+
let configFile = argv.configFile
10399

104100
if (!configFile) {
105101
// default config file (if exists)
@@ -151,13 +147,13 @@ function describeRoot () {
151147
'Run --help with particular command to see its description and available options.\n\n' +
152148
'Usage:\n' +
153149
' $0 <command>')
154-
.command('init', 'Initialize a config file.', describeInit)
155-
.command('start', 'Start the server / do a single run.', describeStart)
156-
.command('run', 'Trigger a test run.', describeRun)
157-
.command('stop', 'Stop the server.', describeStop)
150+
.command('init [configFile]', 'Initialize a config file.', describeInit)
151+
.command('start [configFile]', 'Start the server / do a single run.', describeStart)
152+
.command('run [configFile]', 'Trigger a test run.', describeRun)
153+
.command('stop [configFile]', 'Stop the server.', describeStop)
158154
.command('completion', 'Shell completion for karma.', describeCompletion)
159155
.demandCommand(1, 'Command not specified.')
160-
.strictCommands()
156+
.strict()
161157
.describe('help', 'Print usage and options.')
162158
.describe('version', 'Print current version.')
163159
}
@@ -168,8 +164,11 @@ function describeInit (yargs) {
168164
'INIT - Initialize a config file.\n\n' +
169165
'Usage:\n' +
170166
' $0 init [configFile]')
171-
.strictCommands(false)
172167
.version(false)
168+
.positional('configFile', {
169+
describe: 'Name of the generated Karma configuration file',
170+
type: 'string'
171+
})
173172
.describe('log-level', '<disable | error | warn | info | debug> Level of logging.')
174173
.describe('colors', 'Use colors when reporting and printing logs.')
175174
.describe('no-colors', 'Do not use colors when reporting or printing logs.')
@@ -183,6 +182,10 @@ function describeStart (yargs) {
183182
' $0 start [configFile]')
184183
.strictCommands(false)
185184
.version(false)
185+
.positional('configFile', {
186+
describe: 'Path to the Karma configuration file',
187+
type: 'string'
188+
})
186189
.describe('port', '<integer> Port where the server is running.')
187190
.describe('auto-watch', 'Auto watch source files and run on change.')
188191
.describe('detached', 'Detach the server.')
@@ -200,6 +203,10 @@ function describeStart (yargs) {
200203
.describe('no-fail-on-empty-test-suite', 'Do not fail on empty test suite.')
201204
.describe('fail-on-failing-test-suite', 'Fail on failing test suite.')
202205
.describe('no-fail-on-failing-test-suite', 'Do not fail on failing test suite.')
206+
.option('format-error', {
207+
describe: 'A path to a file that exports the format function.',
208+
type: 'string'
209+
})
203210
}
204211

205212
function describeRun (yargs) {
@@ -208,15 +215,30 @@ function describeRun (yargs) {
208215
'RUN - Run the tests (requires running server).\n\n' +
209216
'Usage:\n' +
210217
' $0 run [configFile] [-- <clientArgs>]')
211-
.strictCommands(false)
212218
.version(false)
219+
.positional('configFile', {
220+
describe: 'Path to the Karma configuration file',
221+
type: 'string'
222+
})
213223
.describe('port', '<integer> Port where the server is listening.')
214224
.describe('no-refresh', 'Do not re-glob all the patterns.')
215225
.describe('fail-on-empty-test-suite', 'Fail on empty test suite.')
216226
.describe('no-fail-on-empty-test-suite', 'Do not fail on empty test suite.')
217227
.describe('log-level', '<disable | error | warn | info | debug> Level of logging.')
218228
.describe('colors', 'Use colors when reporting and printing logs.')
219229
.describe('no-colors', 'Do not use colors when reporting or printing logs.')
230+
.option('removed-files', {
231+
describe: 'Comma-separated paths to removed files. Useful when automatic file watching is disabled.',
232+
type: 'string'
233+
})
234+
.option('changed-files', {
235+
describe: 'Comma-separated paths to changed files. Useful when automatic file watching is disabled.',
236+
type: 'string'
237+
})
238+
.option('added-files', {
239+
describe: 'Comma-separated paths to added files. Useful when automatic file watching is disabled.',
240+
type: 'string'
241+
})
220242
}
221243

222244
function describeStop (yargs) {
@@ -225,8 +247,11 @@ function describeStop (yargs) {
225247
'STOP - Stop the server (requires running server).\n\n' +
226248
'Usage:\n' +
227249
' $0 stop [configFile]')
228-
.strictCommands(false)
229250
.version(false)
251+
.positional('configFile', {
252+
describe: 'Path to the Karma configuration file',
253+
type: 'string'
254+
})
230255
.describe('port', '<integer> Port where the server is listening.')
231256
.describe('log-level', '<disable | error | warn | info | debug> Level of logging.')
232257
}
@@ -237,7 +262,6 @@ function describeCompletion (yargs) {
237262
'COMPLETION - Bash/ZSH completion for karma.\n\n' +
238263
'Installation:\n' +
239264
' $0 completion >> ~/.bashrc')
240-
.strictCommands(false)
241265
.version(false)
242266
}
243267

‎test/e2e/cli.feature

+79-11
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,11 @@ Feature: CLI
1515
karma <command>
1616
1717
Commands:
18-
karma init Initialize a config file.
19-
karma start Start the server / do a single run.
20-
karma run Trigger a test run.
21-
karma stop Stop the server.
22-
karma completion Shell completion for karma.
18+
karma init [configFile] Initialize a config file.
19+
karma start [configFile] Start the server / do a single run.
20+
karma run [configFile] Trigger a test run.
21+
karma stop [configFile] Stop the server.
22+
karma completion Shell completion for karma.
2323
2424
Options:
2525
--help Print usage and options. [boolean]
@@ -45,17 +45,62 @@ Feature: CLI
4545
karma <command>
4646
4747
Commands:
48-
karma init Initialize a config file.
49-
karma start Start the server / do a single run.
50-
karma run Trigger a test run.
51-
karma stop Stop the server.
52-
karma completion Shell completion for karma.
48+
karma init [configFile] Initialize a config file.
49+
karma start [configFile] Start the server / do a single run.
50+
karma run [configFile] Trigger a test run.
51+
karma stop [configFile] Stop the server.
52+
karma completion Shell completion for karma.
5353
5454
Options:
5555
--help Print usage and options. [boolean]
5656
--version Print current version. [boolean]
5757
58-
Unknown command: strat
58+
Unknown argument: strat
59+
"""
60+
61+
Scenario: Error when option is unknown
62+
When I execute Karma with arguments: "start --invalid-option"
63+
Then the stderr is exactly:
64+
"""
65+
Karma - Spectacular Test Runner for JavaScript.
66+
67+
START - Start the server / do a single run.
68+
69+
Usage:
70+
karma start [configFile]
71+
72+
Positionals:
73+
configFile Path to the Karma configuration file [string]
74+
75+
Options:
76+
--help Print usage and options. [boolean]
77+
--port <integer> Port where the server is running.
78+
--auto-watch Auto watch source files and run on change.
79+
--detached Detach the server.
80+
--no-auto-watch Do not watch source files.
81+
--log-level <disable | error | warn | info | debug> Level
82+
of logging.
83+
--colors Use colors when reporting and printing logs.
84+
--no-colors Do not use colors when reporting or printing
85+
logs.
86+
--reporters List of reporters (available: dots, progress,
87+
junit, growl, coverage).
88+
--browsers List of browsers to start (eg. --browsers
89+
Chrome,ChromeCanary,Firefox).
90+
--capture-timeout <integer> Kill browser if does not capture in
91+
given time [ms].
92+
--single-run Run the test when browsers captured and exit.
93+
--no-single-run Disable single-run.
94+
--report-slower-than <integer> Report tests that are slower than
95+
given time [ms].
96+
--fail-on-empty-test-suite Fail on empty test suite.
97+
--no-fail-on-empty-test-suite Do not fail on empty test suite.
98+
--fail-on-failing-test-suite Fail on failing test suite.
99+
--no-fail-on-failing-test-suite Do not fail on failing test suite.
100+
--format-error A path to a file that exports the format
101+
function. [string]
102+
103+
Unknown arguments: invalid-option, invalidOption
59104
"""
60105

61106
Scenario: Init command help
@@ -69,6 +114,9 @@ Feature: CLI
69114
Usage:
70115
karma init [configFile]
71116
117+
Positionals:
118+
configFile Name of the generated Karma configuration file [string]
119+
72120
Options:
73121
--help Print usage and options. [boolean]
74122
--log-level <disable | error | warn | info | debug> Level of logging.
@@ -87,6 +135,9 @@ Feature: CLI
87135
Usage:
88136
karma start [configFile]
89137
138+
Positionals:
139+
configFile Path to the Karma configuration file [string]
140+
90141
Options:
91142
--help Print usage and options. [boolean]
92143
--port <integer> Port where the server is running.
@@ -112,6 +163,8 @@ Feature: CLI
112163
--no-fail-on-empty-test-suite Do not fail on empty test suite.
113164
--fail-on-failing-test-suite Fail on failing test suite.
114165
--no-fail-on-failing-test-suite Do not fail on failing test suite.
166+
--format-error A path to a file that exports the format
167+
function. [string]
115168
"""
116169

117170
Scenario: Run command help
@@ -125,6 +178,9 @@ Feature: CLI
125178
Usage:
126179
karma run [configFile] [-- <clientArgs>]
127180
181+
Positionals:
182+
configFile Path to the Karma configuration file [string]
183+
128184
Options:
129185
--help Print usage and options. [boolean]
130186
--port <integer> Port where the server is listening.
@@ -136,6 +192,15 @@ Feature: CLI
136192
--colors Use colors when reporting and printing logs.
137193
--no-colors Do not use colors when reporting or printing
138194
logs.
195+
--removed-files Comma-separated paths to removed files. Useful
196+
when automatic file watching is disabled.
197+
[string]
198+
--changed-files Comma-separated paths to changed files. Useful
199+
when automatic file watching is disabled.
200+
[string]
201+
--added-files Comma-separated paths to added files. Useful
202+
when automatic file watching is disabled.
203+
[string]
139204
"""
140205

141206
Scenario: Stop command help
@@ -149,6 +214,9 @@ Feature: CLI
149214
Usage:
150215
karma stop [configFile]
151216
217+
Positionals:
218+
configFile Path to the Karma configuration file [string]
219+
152220
Options:
153221
--help Print usage and options. [boolean]
154222
--port <integer> Port where the server is listening.

‎test/unit/cli.spec.js

+23-36
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
'use strict'
22

3-
const yargs = require('yargs')
43
const path = require('path')
54
const mocks = require('mocks')
65

@@ -34,7 +33,7 @@ describe('cli', () => {
3433
}
3534

3635
const processArgs = (args, opts) => {
37-
const argv = yargs.parse(args)
36+
const argv = m.describeRoot().parse(args)
3837
return e.processArgs(argv, opts || {}, fsMock, pathMock)
3938
}
4039

@@ -63,14 +62,14 @@ describe('cli', () => {
6362
describe('processArgs', () => {
6463
it('should override if multiple options given', () => {
6564
// yargs parses --port 123 --port 456 as port = [123, 456] which makes no sense
66-
const options = processArgs(['some.conf', '--port', '12', '--log-level', 'info', '--port', '34', '--log-level', 'debug'])
65+
const options = processArgs(['start', 'some.conf', '--port', '12', '--log-level', 'info', '--port', '34', '--log-level', 'debug'])
6766

6867
expect(options.port).to.equal(34)
6968
expect(options.logLevel).to.equal('DEBUG')
7069
})
7170

7271
it('should return camelCased options', () => {
73-
const options = processArgs(['some.conf', '--port', '12', '--single-run'])
72+
const options = processArgs(['start', 'some.conf', '--port', '12', '--single-run'])
7473

7574
expect(options.configFile).to.exist
7675
expect(options.port).to.equal(12)
@@ -79,105 +78,105 @@ describe('cli', () => {
7978

8079
it('should parse options without configFile and set default', () => {
8180
setCWD('/cwd')
82-
const options = processArgs(['--auto-watch', '--auto-watch-interval', '10'])
81+
const options = processArgs(['start', '--auto-watch'])
8382
expect(path.resolve(options.configFile)).to.equal(path.resolve('/cwd/karma.conf.js'))
8483
expect(options.autoWatch).to.equal(true)
85-
expect(options.autoWatchInterval).to.equal(10)
8684
})
8785

8886
it('should set default karma.conf.coffee config file if exists', () => {
8987
setCWD('/cwd2')
90-
const options = processArgs(['--port', '10'])
88+
const options = processArgs(['start', '--port', '10'])
9189

9290
expect(path.resolve(options.configFile)).to.equal(path.resolve('/cwd2/karma.conf.coffee'))
9391
})
9492

9593
it('should set default karma.conf.ts config file if exists', () => {
9694
setCWD('/cwd3')
97-
const options = processArgs(['--port', '10'])
95+
const options = processArgs(['start', '--port', '10'])
9896

9997
expect(path.resolve(options.configFile)).to.equal(path.resolve('/cwd3/karma.conf.ts'))
10098
})
10199

102100
it('should not set default config if neither exists', () => {
103101
setCWD('/')
104-
const options = processArgs([])
102+
const options = processArgs(['start'])
105103

106104
expect(options.configFile).to.equal(null)
107105
})
108106

109107
it('should parse auto-watch, colors, singleRun to boolean', () => {
110-
let options = processArgs(['--auto-watch', 'false', '--colors', 'false', '--single-run', 'false'])
108+
let options = processArgs(['start', '--auto-watch', 'false', '--colors', 'false', '--single-run', 'false'])
111109

112110
expect(options.autoWatch).to.equal(false)
113111
expect(options.colors).to.equal(false)
114112
expect(options.singleRun).to.equal(false)
115113

116-
options = processArgs(['--auto-watch', 'true', '--colors', 'true', '--single-run', 'true'])
114+
options = processArgs(['start', '--auto-watch', 'true', '--colors', 'true', '--single-run', 'true'])
117115

118116
expect(options.autoWatch).to.equal(true)
119117
expect(options.colors).to.equal(true)
120118
expect(options.singleRun).to.equal(true)
121119
})
122120

123121
it('should replace log-level constants', () => {
124-
let options = processArgs(['--log-level', 'debug'])
122+
let options = processArgs(['start', '--log-level', 'debug'])
125123
expect(options.logLevel).to.equal(constant.LOG_DEBUG)
126124

127-
options = processArgs(['--log-level', 'error'])
125+
options = processArgs(['start', '--log-level', 'error'])
128126
expect(options.logLevel).to.equal(constant.LOG_ERROR)
129127

130-
options = processArgs(['--log-level', 'warn'])
128+
options = processArgs(['start', '--log-level', 'warn'])
131129
expect(options.logLevel).to.equal(constant.LOG_WARN)
132130

133-
options = processArgs(['--log-level', 'foo'])
131+
options = processArgs(['start', '--log-level', 'foo'])
134132
expect(mockery.process.exit).to.have.been.calledWith(1)
135133

136-
options = processArgs(['--log-level'])
134+
options = processArgs(['start', '--log-level'])
137135
expect(mockery.process.exit).to.have.been.calledWith(1)
138136
})
139137

140138
it('should parse format-error into a function', () => {
141139
// root export
142-
let options = processArgs(['--format-error', '../../test/unit/fixtures/format-error-root'])
140+
let options = processArgs(['start', '--format-error', '../../test/unit/fixtures/format-error-root'])
143141
const formatErrorRoot = require('../../test/unit/fixtures/format-error-root')
144142
expect(options.formatError).to.equal(formatErrorRoot)
145143

146144
// property export
147-
options = processArgs(['--format-error', '../../test/unit/fixtures/format-error-property'])
145+
options = processArgs(['start', '--format-error', '../../test/unit/fixtures/format-error-property'])
148146
const formatErrorProperty = require('../../test/unit/fixtures/format-error-property').formatError
149147
expect(options.formatError).to.equal(formatErrorProperty)
150148
})
151149

152150
it('should parse browsers into an array', () => {
153-
const options = processArgs(['--browsers', 'Chrome,ChromeCanary,Firefox'])
151+
const options = processArgs(['start', '--browsers', 'Chrome,ChromeCanary,Firefox'])
154152
expect(options.browsers).to.deep.equal(['Chrome', 'ChromeCanary', 'Firefox'])
155153
})
156154

157155
it('should resolve configFile to absolute path', () => {
158156
setCWD('/cwd')
159-
const options = processArgs(['some/config.js'])
157+
const options = processArgs(['start', 'some/config.js'])
160158
expect(path.resolve(options.configFile)).to.equal(path.resolve('/cwd/some/config.js'))
161159
})
162160

163161
it('should parse report-slower-than to a number', () => {
164-
let options = processArgs(['--report-slower-than', '2000'])
162+
let options = processArgs(['start', '--report-slower-than', '2000'])
165163
expect(options.reportSlowerThan).to.equal(2000)
166164

167-
options = processArgs(['--no-report-slower-than'])
165+
options = processArgs(['start', '--no-report-slower-than'])
168166
expect(options.reportSlowerThan).to.equal(0)
169167
})
170168

171169
it('should cast reporters to array', () => {
172-
let options = processArgs(['--reporters', 'dots,junit'])
170+
let options = processArgs(['start', '--reporters', 'dots,junit'])
173171
expect(options.reporters).to.deep.equal(['dots', 'junit'])
174172

175-
options = processArgs(['--reporters', 'dots'])
173+
options = processArgs(['start', '--reporters', 'dots'])
176174
expect(options.reporters).to.deep.equal(['dots'])
177175
})
178176

179177
it('should parse removed/added/changed files to array', () => {
180178
const options = processArgs([
179+
'run',
181180
'--removed-files', 'r1.js,r2.js',
182181
'--changed-files', 'ch1.js,ch2.js',
183182
'--added-files', 'a1.js,a2.js'
@@ -187,18 +186,6 @@ describe('cli', () => {
187186
expect(options.addedFiles).to.deep.equal(['a1.js', 'a2.js'])
188187
expect(options.changedFiles).to.deep.equal(['ch1.js', 'ch2.js'])
189188
})
190-
191-
it('should error on args with underscores', () => {
192-
let expectedException
193-
try {
194-
const options = processArgs(['--no_browsers'])
195-
expectedException = 'Should have thrown but got ' + options
196-
} catch (e) {
197-
expectedException = e
198-
} finally {
199-
expect(expectedException + '').to.includes('Bad argument: no_browsers did you mean no-browsers')
200-
}
201-
})
202189
})
203190

204191
describe('parseClientArgs', () => {

0 commit comments

Comments
 (0)
Please sign in to comment.