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.

Sign up for GitHub

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(cli): Better CLI args validation #1234

Merged
merged 1 commit into from
Jul 16, 2015
+38 −4
Merged

feat(cli): Better CLI args validation #1234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion lib/cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,16 @@ var processArgs = function (argv, options, fs, path) {
}

if (helper.isString(options.logLevel)) {
options.logLevel = constant['LOG_' + options.logLevel.toUpperCase()] || constant.LOG_DISABLE
var logConstant = constant['LOG_' + options.logLevel.toUpperCase()]
if (helper.isDefined(logConstant)) {
options.logLevel = logConstant
} else {
console.error('Log level must be one of disable, error, warn, info, or debug.')
process.exit(1)
}
} else if (helper.isDefined(options.logLevel)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@srawlins can you also fail if the value is a string but not one of the option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vojtajina It looks like the current behavior, on line 35, is to default unknown options to LOG_DISABLE so that if users currently specify NO or OFF or FALSE or NONE, etc. then it will just disable. You think we should instead fail if it is unknown?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes.

I think it might be better to just show a warning when the user misconfigures --log-level. It won't affect Karma. This is different from failing to instantiate a preprocessor or launcher (we should fail in that case, because it might pass the tests - a browser that would fail didn't start).

What do you think?

console.error('Log level must be one of disable, error, warn, info, or debug.')
process.exit(1)
}

if (helper.isString(options.singleRun)) {
Expand Down
31 changes: 28 additions & 3 deletions test/unit/cli.spec.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ describe 'cli', ->
constant = require '../../lib/constants'
path = require 'path'
mocks = require 'mocks'
loadFile = mocks.loadFile
mockery = m = e = null

fsMock = mocks.fs.create
cwd:
Expand All @@ -26,9 +28,26 @@ describe 'cli', ->

processArgs = (args, opts) ->
argv = optimist.parse(args)
cli.processArgs argv, opts || {}, fsMock, pathMock

beforeEach -> setCWD '/'
e.processArgs argv, opts || {}, fsMock, pathMock

beforeEach ->
setCWD '/'
mockery = {}
mockery.process = exit: sinon.spy()
mockery.console = error: sinon.spy()

# load file under test
m = loadFile __dirname + '/../../lib/cli.js', mockery, {
global: {},
console: mockery.console,
process: mockery.process,
require: (path) ->
if path.indexOf('./') is 0
require '../../lib/' + path
else
require path
}
e = m.exports

describe 'processArgs', ->

Expand Down Expand Up @@ -95,6 +114,12 @@ describe 'cli', ->
options = processArgs ['--log-level', 'warn']
expect(options.logLevel).to.equal constant.LOG_WARN

options = processArgs ['--log-level', 'foo']
expect(mockery.process.exit).to.have.been.calledWith 1

options = processArgs ['--log-level']
expect(mockery.process.exit).to.have.been.calledWith 1


it 'should parse browsers into an array', ->
options = processArgs ['--browsers', 'Chrome,ChromeCanary,Firefox']
Expand Down