feat(config): improve karma.config.parseConfig error handling (#3635)

npetruzzelli · web-flow · commit 9dba1e20af48 · 2021-02-02T16:33:43.000-08:00
diff --git a/lib/config.js b/lib/config.js
@@ -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 {
@@ -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')
@@ -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.
@@ -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)
diff --git a/lib/server.js b/lib/server.js
@@ -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))
 
diff --git a/test/unit/config.spec.js b/test/unit/config.spec.js
@@ -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: ['https://some.com', 'https://more.org/file.js'] }),
       '/conf/both.js': wrapCfg({ files: ['one.js', 'two.js'], exclude: ['third.js'] }),
@@ -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]
@@ -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
@@ -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 })
 
