Skip to content

Commit

Permalink
feat(config): improve karma.config.parseConfig error handling (#3635)
Browse files Browse the repository at this point in the history
  • Loading branch information
npetruzzelli authored Feb 3, 2021
1 parent a14a24e commit 9dba1e2
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 10 deletions.
31 changes: 23 additions & 8 deletions lib/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,26 @@ const CONFIG_SYNTAX_HELP = ' module.exports = function(config) {\n' +
' });\n' +
' };\n'

function parseConfig (configFilePath, cliOptions) {
function parseConfig (configFilePath, cliOptions, parseOptions) {
function fail () {
log.error(...arguments)
if (parseOptions && parseOptions.throwErrors === true) {
const errorMessage = Array.from(arguments).join(' ')
throw new Error(errorMessage)
} else {
const warningMessage =
'The `parseConfig()` function historically called `process.exit(1)`' +
' when it failed. This behavior is now deprecated and function will' +
' throw an error in the next major release. To suppress this warning' +
' pass `throwErrors: true` as a third argument to opt-in into the new' +
' behavior and adjust your code to respond to the exception' +
' accordingly.' +
' Example: `parseConfig(path, cliOptions, { throwErrors: true })`'
log.warn(warningMessage)
process.exit(1)
}
}

let configModule
if (configFilePath) {
try {
Expand All @@ -360,8 +379,6 @@ function parseConfig (configFilePath, cliOptions) {
configModule = configModule.default
}
} catch (e) {
log.error('Error in config file!\n ' + e.stack || e)

const extension = path.extname(configFilePath)
if (extension === '.coffee' && !COFFEE_SCRIPT_AVAILABLE) {
log.error('You need to install CoffeeScript.\n npm install coffeescript --save-dev')
Expand All @@ -370,11 +387,10 @@ function parseConfig (configFilePath, cliOptions) {
} else if (extension === '.ts' && !TYPE_SCRIPT_AVAILABLE) {
log.error('You need to install TypeScript.\n npm install typescript ts-node --save-dev')
}
return process.exit(1)
return fail('Error in config file!\n ' + e.stack || e)
}
if (!helper.isFunction(configModule)) {
log.error('Config file must export a function!\n' + CONFIG_SYNTAX_HELP)
return process.exit(1)
return fail('Config file must export a function!\n' + CONFIG_SYNTAX_HELP)
}
} else {
configModule = () => {} // if no config file path is passed, we define a dummy config module.
Expand All @@ -395,8 +411,7 @@ function parseConfig (configFilePath, cliOptions) {
try {
configModule(config)
} catch (e) {
log.error('Error in config file!\n', e)
return process.exit(1)
return fail('Error in config file!\n', e)
}

// merge the config from config file and cliOptions (precedence)
Expand Down
10 changes: 9 additions & 1 deletion lib/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,15 @@ class Server extends KarmaEventEmitter {

this.loadErrors = []

const config = cfg.parseConfig(cliOptions.configFile, cliOptions)
let config
try {
config = cfg.parseConfig(cliOptions.configFile, cliOptions, { throwErrors: true })
} catch (parseConfigError) {
// TODO: change how `done` falls back to exit in next major version
// SEE: https://github.com/karma-runner/karma/pull/3635#discussion_r565399378
(done || process.exit)(1)
return
}

this.log.debug('Final config', util.inspect(config, false, /** depth **/ null))

Expand Down
44 changes: 43 additions & 1 deletion test/unit/config.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ describe('config', () => {
'/conf/invalid.js': () => {
throw new SyntaxError('Unexpected token =')
},
'/conf/export-not-function.js': 'not-a-function',
// '/conf/export-null.js': null, // Same as `/conf/not-exist.js`?
'/conf/exclude.js': wrapCfg({ exclude: ['one.js', 'sub/two.js'] }),
'/conf/absolute.js': wrapCfg({ files: ['http://some.com', 'https://more.org/file.js'] }),
'/conf/both.js': wrapCfg({ files: ['one.js', 'two.js'], exclude: ['third.js'] }),
Expand All @@ -57,6 +59,7 @@ describe('config', () => {
m = loadFile(path.join(__dirname, '/../../lib/config.js'), mocks, {
global: {},
process: mocks.process,
Error: Error, // Without this, chai's `.throw()` assertion won't correctly check against constructors.
require (path) {
if (mockConfigs[path]) {
return mockConfigs[path]
Expand Down Expand Up @@ -123,7 +126,20 @@ describe('config', () => {
expect(mocks.process.exit).to.have.been.calledWith(1)
})

it('should throw and log error if invalid file', () => {
it('should log error and throw if file does not exist AND throwErrors is true', () => {
function parseConfig () {
e.parseConfig('/conf/not-exist.js', {}, { throwErrors: true })
}

expect(parseConfig).to.throw(Error, 'Error in config file!\n Error: Cannot find module \'/conf/not-exist.js\'')
expect(logSpy).to.have.been.called
const event = logSpy.lastCall.args
expect(event.toString().split('\n').slice(0, 2)).to.be.deep.equal(
['Error in config file!', ' Error: Cannot find module \'/conf/not-exist.js\''])
expect(mocks.process.exit).not.to.have.been.called
})

it('should log an error and exit if invalid file', () => {
e.parseConfig('/conf/invalid.js', {})

expect(logSpy).to.have.been.called
Expand All @@ -133,6 +149,32 @@ describe('config', () => {
expect(mocks.process.exit).to.have.been.calledWith(1)
})

it('should log an error and throw if invalid file AND throwErrors is true', () => {
function parseConfig () {
e.parseConfig('/conf/invalid.js', {}, { throwErrors: true })
}

expect(parseConfig).to.throw(Error, 'Error in config file!\n SyntaxError: Unexpected token =')
expect(logSpy).to.have.been.called
const event = logSpy.lastCall.args
expect(event[0]).to.eql('Error in config file!\n')
expect(event[1].message).to.eql('Unexpected token =')
expect(mocks.process.exit).not.to.have.been.called
})

it('should log error and throw if file does not export a function AND throwErrors is true', () => {
function parseConfig () {
e.parseConfig('/conf/export-not-function.js', {}, { throwErrors: true })
}

expect(parseConfig).to.throw(Error, 'Config file must export a function!\n')
expect(logSpy).to.have.been.called
const event = logSpy.lastCall.args
expect(event.toString().split('\n').slice(0, 1)).to.be.deep.equal(
['Config file must export a function!'])
expect(mocks.process.exit).not.to.have.been.called
})

it('should override config with given cli options', () => {
const config = e.parseConfig('/home/config4.js', { port: 456, autoWatch: false })

Expand Down

0 comments on commit 9dba1e2

Please sign in to comment.