Process exits without exceptions or logs when using karma.config.parseConfig

[npetruzzelli]

I'm using karma.config.parseConfig to create a configuration using the public API:
https://karma-runner.github.io/6.0/dev/public-api.html

And my code has no way of being notified of errors. For example, the process exits silently if you pass a relative path to parseConfig. No exceptions are thrown and the logger doesn't output any messages to the console. Neither me (via the console) nor my application (via thrown exceptions) has any way of knowing that something went wrong.

I am aware that using the CLI, it would use path.resolve to create an absolute path, but that is not the issue. I want to be informed about problems with the input being provided so that I can then tell the user or take some kind of action based on the kind of error it is.

EXAMPLE

console.log('START OF FILE')
const karma = require('karma')

// When passing a relative path, the code attempts to locate the file relative to 
// `node_modules/karma/lib/conf.js`, not relative to your file or the current 
// working directory, but the error is caught and not re-thrown.
const myConfig = karma.config.parseConfig("./path/to/karma.conf.js")

console.log('END OF FILE') // The process exits before this statement is executed.

RELATED CODE

karma/lib/config.js

Lines 6 to 7 in e246461

	const logger = require('./logger')
	const log = logger.create('config')

karma/lib/config.js

Lines 354 to 424 in e246461

	function parseConfig (configFilePath, cliOptions) {
	let configModule
	if (configFilePath) {
	try {
	configModule = require(configFilePath)
	if (typeof configModule === 'object' && typeof configModule.default !== 'undefined') {
	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')
	} else if (extension === '.ls' && !LIVE_SCRIPT_AVAILABLE) {
	log.error('You need to install LiveScript.\n npm install LiveScript --save-dev')
	} 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)
	}
	if (!helper.isFunction(configModule)) {
	log.error('Config file must export a function!\n' + CONFIG_SYNTAX_HELP)
	return process.exit(1)
	}
	} else {
	configModule = () => {} // if no config file path is passed, we define a dummy config module.
	}
	const config = new Config()
	// save and reset hostname and listenAddress so we can detect if the user
	// changed them
	const defaultHostname = config.hostname
	config.hostname = null
	const defaultListenAddress = config.listenAddress
	config.listenAddress = null
	// add the user's configuration in
	config.set(cliOptions)
	try {
	configModule(config)
	} catch (e) {
	log.error('Error in config file!\n', e)
	return process.exit(1)
	}
	// merge the config from config file and cliOptions (precedence)
	config.set(cliOptions)
	// if the user changed listenAddress, but didn't set a hostname, warn them
	if (config.hostname === null && config.listenAddress !== null) {
	log.warn(`ListenAddress was set to ${config.listenAddress} but hostname was left as the default: ` +
	`${defaultHostname}. If your browsers fail to connect, consider changing the hostname option.`)
	}
	// restore values that weren't overwritten by the user
	if (config.hostname === null) {
	config.hostname = defaultHostname
	}
	if (config.listenAddress === null) {
	config.listenAddress = defaultListenAddress
	}
	// configure the logger as soon as we can
	logger.setup(config.logLevel, config.colors, config.loggers)
	log.debug(configFilePath ? `Loading config ${configFilePath}` : 'No config file specified.')
	return normalizeConfig(config, configFilePath)
	}

ROUGH START

Log4js isn't setup until the function is almost done, this might explain why logging doesn't seem to be enabled. I'm not familiar with that library, so I am not sure what a good solution would look like, but a possible work around for critical errors that normally exit the process might look like the following:

- function parseConfig (configFilePath, cliOptions) {
+ function parseConfig (configFilePath, cliOptions, functionOptions) {
+   // TODO: Can optional chaining be used in this repository?
+   const throwErrors = functionOptions && functionOptions.throwErrors === true
   let configModule
   if (configFilePath) {
     try {
       configModule = require(configFilePath)
       if (typeof configModule === 'object' && typeof configModule.default !== 'undefined') {
         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')
       } else if (extension === '.ls' && !LIVE_SCRIPT_AVAILABLE) {
         log.error('You need to install LiveScript.\n  npm install LiveScript --save-dev')
       } else if (extension === '.ts' && !TYPE_SCRIPT_AVAILABLE) {
         log.error('You need to install TypeScript.\n  npm install typescript ts-node --save-dev')
       }
+      if(throwErrors) {
+        const errMessage = "" // some relevant message built from the above logs
+        throw new Error(errMessage)
+      } else {
-      return process.exit(1)
+        return process.exit(1)
+      }
     }
     if (!helper.isFunction(configModule)) {
       log.error('Config file must export a function!\n' + CONFIG_SYNTAX_HELP)
       return process.exit(1)
     }
   } else {
     configModule = () => {} // if no config file path is passed, we define a dummy config module.
   }
 
   const config = new Config()
 
   // save and reset hostname and listenAddress so we can detect if the user
   // changed them
   const defaultHostname = config.hostname
   config.hostname = null
   const defaultListenAddress = config.listenAddress
   config.listenAddress = null
 
   // add the user's configuration in
   config.set(cliOptions)
 
   try {
     configModule(config)
   } catch (e) {
     log.error('Error in config file!\n', e)
+    if(throwErrors) {
+      const errMessage = "" // some relevant message built from the above logs
+      throw new Error(errMessage)
+    } else {
-    return process.exit(1)
+      return process.exit(1)
+    }
   }
 
   // merge the config from config file and cliOptions (precedence)
   config.set(cliOptions)
 
   // if the user changed listenAddress, but didn't set a hostname, warn them
   if (config.hostname === null && config.listenAddress !== null) {
     log.warn(`ListenAddress was set to ${config.listenAddress} but hostname was left as the default: ` +
       `${defaultHostname}. If your browsers fail to connect, consider changing the hostname option.`)
   }
   // restore values that weren't overwritten by the user
   if (config.hostname === null) {
     config.hostname = defaultHostname
   }
   if (config.listenAddress === null) {
     config.listenAddress = defaultListenAddress
   }
 
   // configure the logger as soon as we can
   logger.setup(config.logLevel, config.colors, config.loggers)
 
   log.debug(configFilePath ? `Loading config ${configFilePath}` : 'No config file specified.')
 
-  return normalizeConfig(config, configFilePath)
+  return normalizeConfig(config, configFilePath, { throwErrors })
 }

Any functions that parseConfig calls, such as normalizeConfig, would need similar accomadations.

CLOSING

I feel like I may be missing something obvious and I just haven't seen the documentation or code for it yet. Could someone help point me in the correct direction if this is something that already works?

[npetruzzelli]

This appears to be similar to, but different from: #3612

[devoto13]

I agree that current behavior is inconvenient. We should continue to clean up these process.exit() calls from code to make programmatic usage more friendly, so PR is very much welcome.

I would suggest introducing a helper function that can be used instead of log.error() + process.exit() pairs. Something like:

const fail = (message) => {
  log.error(message);
  if (throwErrors) {
    throw new Error(message);
  } else {
    log.warn('`parseConfig()` function used to call `process.exit(1)` when config parsing 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 usage accordingly. Example: `parseConfig(path, options, { throwErrors: true })`');
    // ^^^ or something along these lines to convey the idea to the consumers
    process.exit(1);
  }
}

Then update

karma/lib/server.js

Line 66 in e246461

const config = cfg.parseConfig(cliOptions.configFile, cliOptions)

to go over a new path, catch an error and run (done || process.exit)(1) callback directly.

[npetruzzelli]

I can try taking a swing at it. Let's see how far I can get.

Does this project maintain a develop branch and I'm not seeing it for some reason? Should I create one on my fork?

I don't see anything in the contributing docs/webpages about what kind of work flow this repository uses. (I'm used to working with some variation of GitFlow)

[johnjbarton]

We just use the simple github solution: create a fork on your github account and branches you push to your repo can be converted to pull requests in the github UI. We squash PRs so you can have multiple commits.

[npetruzzelli]

Understood. Thanks!

[devoto13]

Fixed by #3635