From 0a736116015f504f6991b28ab6f32da45a78088e Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Tue, 26 Jan 2021 09:44:30 -0500 Subject: [PATCH 01/12] feat: add initial logMessageFactory utility --- lib/utils/log-utils.js | 170 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 170 insertions(+) create mode 100644 lib/utils/log-utils.js diff --git a/lib/utils/log-utils.js b/lib/utils/log-utils.js new file mode 100644 index 000000000..97ea7b475 --- /dev/null +++ b/lib/utils/log-utils.js @@ -0,0 +1,170 @@ + +const constants = require('../constants') // other files assign this to the singular "constant". + +const { LOG_DEBUG, LOG_DISABLE, LOG_ERROR, LOG_INFO, LOG_WARN } = constants +const validLogLevels = [ + LOG_DEBUG, + LOG_DISABLE, + LOG_ERROR, + LOG_INFO, + LOG_WARN +] + +// Mutually Exclusive Actions +const ACTION_LOG = 'log' +const ACTION_LOG_AND_EXIT = 'exit' +const ACTION_THROW = 'throw' +const validActions = [ + ACTION_LOG, + ACTION_LOG_AND_EXIT, + ACTION_THROW +] +const actions = { + ACTION_LOG, + ACTION_LOG_AND_EXIT, + ACTION_THROW +} + +/** + * Create functions that may be used to handle and log messages. + * + * The factory changes the default behavior of the created message function + * so that the function may be consumed with minimal arguments and options. + * + * @param {Object} categoryLogger + * One of the internal log4js loggers created by Karma. This logger is used + * together with log levels as the default means of logging messages. + * @param {"exit"|"log"|"throw"} [defaultAction="log"] + * If this option is a valid action, then it will be used as the returned + * function's default action. Invalid values will result in the default + * log level being set to `"log"`. + * @param {"DEBUG"|"ERROR"|"INFO"|"OFF"|"WARN"} [defaultLevel="INFO"] + * If this option is a valid log level, then it will be used as the returned + * function's default log level. Invalid values will result in the default + * log level being set to `"INFO"`. + * @param {boolean} [mayExit = true] + * If this option is `false`, then process exiting is disabled. All other + * values enable process exiting. + * @param {boolean} [mayThrow = true] + * If this option is `false`, then throwing exceptions is disabled. All + * other values enable exception throwing. + * @returns {function} + * A function that may be used for handling and logging messages. + */ +function logMessageFactory ( + categoryLogger, + defaultAction = ACTION_LOG, + defaultLevel = LOG_INFO, + mayExit = true, + mayThrow = true +) { + const CAN_EXIT = mayExit !== false + const CAN_THROW = mayThrow !== false + const DEFAULT_ACTION = validActions.includes(defaultAction) + ? defaultAction + : ACTION_LOG + const DEFAULT_LEVEL = validLogLevels.includes(defaultLevel) + ? defaultLevel + : LOG_INFO + + /** + * A function that may be used to handle and log messages. + * + * @param {string} message + * @param {Object} [options] + * @param {"exit"|"log"|"throw"} [options.action="log"] + * If this option is `"throw"`, then `message` will be used to create an + * error object which will then be thrown. If this option is `"log"`, then + * the message will be logged using the appropriate function. If this + * option is `"exit"`, then the process will exit with `exitCode` after + * the message is logged. The default value for this option depends upon + * what was passed to the factory. If the factory does not configure it, + * then the default value will be `"log"`. If an invalid value is passed, + * the default value will be used. The factory can disable throwing and + * exiting, which are both enabled by default. If other actions are + * disabled, then this function will always be able to log the message. + * @param {function} [options.errorConstructor] + * When throwing an exception an `options.errorInstance` object is not + * provided, then this function is used to construct a new error object. + * The constructor will be passed `message` as its first argument. + * If this option is not a function, the code will then fall back to using + * the global `Error` constructor. + * @param {Object} [options.errorInstance] + * When throwing an exception, this error object will have its `message` + * property value set to the `message` argument before throwing the + * object. If the this option is not an object, the code will then attempt + * to use `options.errorConstructor`. + * @param {number} [options.exitCode=1] + * An integer that the process will exit with. By default this will be + * `1`. Any exit code that is not `0` will indicate an error. `0` + * indicates a success. + * @param {function} [options.logFunction="INFO"] + * If this option is a function, it will be used to log the message + * instead of Karma's internal log4js logger. It will be passed `message` + * as the first argument. An object with `action`, `error`, and `logLevel` + * as properties will be passed as the second argument. `error` will be + * the object created by `errorInstance` or `errorConstructor`, regardless + * of whether or not an exception is being thrown. + * @param {"DEBUG"|"ERROR"|"INFO"|"OFF"|"WARN"} [options.logLevel] + * When logging a message and `options.logFunction` is not provided, then + * this will be used to determine the appropriate log4js method that will + * be used to log the message. Valid options include all log constants + * that are available on a `Config` object constructed by Karma. The + * default value for this option depends upon what was passed to the + * factory. If the factory does not configure it, then the default value + * will be `"INFO"`. + */ + function logMessage (message, options) { + const _options = typeof options === 'object' && options !== null ? options : {} + const action = validActions.includes(_options.action) + ? _options.action + : DEFAULT_ACTION + const exitCode = _options.exitCode || 1 + const logLevel = validLogLevels.includes(_options.logLevel) + ? _options.logLevel + : DEFAULT_LEVEL + const { errorConstructor, errorInstance, logFunction } = _options + + let error = null + if (typeof errorInstance === 'object' && errorInstance !== null) { + error = errorInstance + if (error.message !== message) { + error.originalMessage = error.message + error.message = message + } + } else if (typeof errorConstructor === 'function') { + const ErrorConstructor = errorConstructor + error = new ErrorConstructor(message) + } else { + error = new Error(message) + } + + const shouldThrow = CAN_THROW && action === ACTION_THROW + if (shouldThrow) { + throw error + // TODO: Should the `log4js` function always be called? Do appenders emit + // : messages to other systems that care about messages? + } + + // const shouldLog = action === ACTION_LOG || action === ACTION_LOG_AND_EXIT + // if (shouldLog) { + if (typeof logFunction === 'function') { + logFunction(message, { action, error, logLevel }) + // TODO: Should the `log4js` function always be called? Do appenders emit + // : messages to other systems that care about messages? + } else { + categoryLogger.log(logLevel, message) + } + // } + + const shouldExit = CAN_EXIT && action === ACTION_LOG_AND_EXIT + if (shouldExit) { + process.exit(exitCode) + } + } + return logMessage +} + +logMessageFactory.actions = actions // Move these to `constants.js`? + +module.exports = exports = { logMessageFactory } From 58aa5bfe7e92ff8804453a53c1fae0fc608f97ec Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Tue, 26 Jan 2021 10:18:30 -0500 Subject: [PATCH 02/12] feat: initial implementation of `logMessageFactory` in `config.js` --- lib/config.js | 48 +++++++++++++++++++++++++++++++----------- lib/utils/log-utils.js | 8 +++---- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/lib/config.js b/lib/config.js index 1ea49b85c..4e2904a21 100644 --- a/lib/config.js +++ b/lib/config.js @@ -10,6 +10,16 @@ const constant = require('./constants') const _ = require('lodash') +const { logActions, logMessageFactory } = require('./utils/log-utils') +const { ACTION_LOG, ACTION_LOG_AND_EXIT } = logActions +const karmaLog = { + debug: logMessageFactory(log, ACTION_LOG, constant.LOG_DEBUG, false, false), + error: logMessageFactory(log, ACTION_LOG_AND_EXIT, constant.LOG_ERROR, true, true), + info: logMessageFactory(log, ACTION_LOG, constant.LOG_INFO, false, false), + off: logMessageFactory(log, ACTION_LOG, constant.LOG_DISABLE, false, false), + warn: logMessageFactory(log, ACTION_LOG, constant.LOG_WARN, true, false) +} + let COFFEE_SCRIPT_AVAILABLE = false let LIVE_SCRIPT_AVAILABLE = false let TYPE_SCRIPT_AVAILABLE = false @@ -351,7 +361,9 @@ const CONFIG_SYNTAX_HELP = ' module.exports = function(config) {\n' + ' });\n' + ' };\n' -function parseConfig (configFilePath, cliOptions) { +function parseConfig (configFilePath, options) { + const _options = typeof options === 'object' && options !== null ? options : {} + const { failureAction = 'exit', ...cliOptions } = _options let configModule if (configFilePath) { try { @@ -360,21 +372,30 @@ function parseConfig (configFilePath, cliOptions) { configModule = configModule.default } } catch (e) { - log.error('Error in config file!\n ' + e.stack || e) + const errorMessageSegments = [] + errorMessageSegments.push('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') + errorMessageSegments.push('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') + errorMessageSegments.push('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') + errorMessageSegments.push('You need to install TypeScript.\n npm install typescript ts-node --save-dev') } - return process.exit(1) + const configModuleImportErrorMessage = errorMessageSegments.join('\n') + return karmaLog.error( + configModuleImportErrorMessage, + { action: failureAction, errorInstance: e, exitCode: 1 } + ) } if (!helper.isFunction(configModule)) { - log.error('Config file must export a function!\n' + CONFIG_SYNTAX_HELP) - return process.exit(1) + const configModuleExportTypeErrorMessage = + ('Config file must export a function!\n' + CONFIG_SYNTAX_HELP) + return karmaLog.error( + configModuleExportTypeErrorMessage, + { action: failureAction, exitCode: 1 } + ) } } else { configModule = () => {} // if no config file path is passed, we define a dummy config module. @@ -395,8 +416,11 @@ function parseConfig (configFilePath, cliOptions) { try { configModule(config) } catch (e) { - log.error('Error in config file!\n', e) - return process.exit(1) + const configModuleExecutionErrorMessage = ('Error in config file!\n', e) + return karmaLog.error( + configModuleExecutionErrorMessage, + { action: failureAction, errorInstance: e, exitCode: 1 } + ) } // merge the config from config file and cliOptions (precedence) @@ -404,7 +428,7 @@ function parseConfig (configFilePath, 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: ` + + karmaLog.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 @@ -418,7 +442,7 @@ function parseConfig (configFilePath, cliOptions) { // 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.') + karmaLog.debug(configFilePath ? `Loading config ${configFilePath}` : 'No config file specified.') return normalizeConfig(config, configFilePath) } diff --git a/lib/utils/log-utils.js b/lib/utils/log-utils.js index 97ea7b475..ed2436338 100644 --- a/lib/utils/log-utils.js +++ b/lib/utils/log-utils.js @@ -11,6 +11,7 @@ const validLogLevels = [ ] // Mutually Exclusive Actions +// QUESTION: Move these to `constants.js`? const ACTION_LOG = 'log' const ACTION_LOG_AND_EXIT = 'exit' const ACTION_THROW = 'throw' @@ -48,7 +49,7 @@ const actions = { * @param {boolean} [mayThrow = true] * If this option is `false`, then throwing exceptions is disabled. All * other values enable exception throwing. - * @returns {function} + * @returns {logMessage} * A function that may be used for handling and logging messages. */ function logMessageFactory ( @@ -68,6 +69,7 @@ function logMessageFactory ( : LOG_INFO /** + * * A function that may be used to handle and log messages. * * @param {string} message @@ -165,6 +167,4 @@ function logMessageFactory ( return logMessage } -logMessageFactory.actions = actions // Move these to `constants.js`? - -module.exports = exports = { logMessageFactory } +module.exports = exports = { logActions: actions, logMessageFactory } From fc5adaa84a594ed4f3b6917c8256591a03ceab4b Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Tue, 26 Jan 2021 12:21:34 -0500 Subject: [PATCH 03/12] fix: address unit test failures --- lib/config.js | 2 +- lib/utils/log-utils.js | 48 +++++++++++++++++++++++++++++++++++----- test/unit/config.spec.js | 12 ++++++++++ 3 files changed, 55 insertions(+), 7 deletions(-) diff --git a/lib/config.js b/lib/config.js index 4e2904a21..853121f28 100644 --- a/lib/config.js +++ b/lib/config.js @@ -416,7 +416,7 @@ function parseConfig (configFilePath, options) { try { configModule(config) } catch (e) { - const configModuleExecutionErrorMessage = ('Error in config file!\n', e) + const configModuleExecutionErrorMessage = ['Error in config file!\n', e] return karmaLog.error( configModuleExecutionErrorMessage, { action: failureAction, errorInstance: e, exitCode: 1 } diff --git a/lib/utils/log-utils.js b/lib/utils/log-utils.js index ed2436338..d99f30506 100644 --- a/lib/utils/log-utils.js +++ b/lib/utils/log-utils.js @@ -26,6 +26,27 @@ const actions = { ACTION_THROW } +/** + * @param {string} levelName + * @returns {string} + * @throws {TypeError} + * + * @see [levelStr is always uppercase]{@link https://github.com/log4js-node/log4js-node/blob/v6.3.0/lib/levels.js#L49-L54} + * @see [builtin/default levels]{@link https://github.com/log4js-node/log4js-node/blob/v6.3.0/lib/levels.js#L90-L100} + * @see [static getLevel(sArg, defaultLevel)]{@link https://github.com/log4js-node/log4js-node/blob/v6.3.0/lib/logger.js#L113-L116} + * @see [level method name creation]{@link https://github.com/log4js-node/log4js-node/blob/v6.3.0/lib/logger.js#L113-L116} + */ +function getLevelMethodName (levelName) { + if (typeof levelName !== 'string') { + throw new TypeError('getLevelMethodName: `levelName` argument must be a string.') + } + const levelStrLower = levelName.toLowerCase() + const levelMethod = levelStrLower.replace(/_([a-z])/g, (g) => + g[1].toUpperCase() + ) + return levelMethod +} + /** * Create functions that may be used to handle and log messages. * @@ -116,7 +137,7 @@ function logMessageFactory ( * factory. If the factory does not configure it, then the default value * will be `"INFO"`. */ - function logMessage (message, options) { + function logMessage (messageArgs, options) { const _options = typeof options === 'object' && options !== null ? options : {} const action = validActions.includes(_options.action) ? _options.action @@ -126,14 +147,28 @@ function logMessageFactory ( ? _options.logLevel : DEFAULT_LEVEL const { errorConstructor, errorInstance, logFunction } = _options + const _messageArgs = Array.isArray(messageArgs) ? messageArgs : [messageArgs] + const [message, ...otherMessageArgs] = _messageArgs let error = null if (typeof errorInstance === 'object' && errorInstance !== null) { - error = errorInstance - if (error.message !== message) { - error.originalMessage = error.message - error.message = message + // Copy the original to avoid mutating by reference. We are especially + // interested in the name, message, and stack. + const ErrorConstructor = Object.getPrototypeOf(errorInstance).constructor + const newError = Object.create(ErrorConstructor) + + // Copy all own enumerable, non-enumerable, and Symbol key properties. + // Don't copy the descriptors directly, just in case it references + // something not exposed by the object. + const instanceDescriptors = Object.getOwnPropertyDescriptors(errorInstance) + for (const propName of Object.keys(instanceDescriptors)) { + newError[propName] = errorInstance[propName] + } + if (newError.message !== message) { + newError.originalMessage = newError.message + newError.message = message } + error = newError } else if (typeof errorConstructor === 'function') { const ErrorConstructor = errorConstructor error = new ErrorConstructor(message) @@ -155,7 +190,8 @@ function logMessageFactory ( // TODO: Should the `log4js` function always be called? Do appenders emit // : messages to other systems that care about messages? } else { - categoryLogger.log(logLevel, message) + const levelMethodName = getLevelMethodName(logLevel) + categoryLogger[levelMethodName](message, ...otherMessageArgs) } // } diff --git a/test/unit/config.spec.js b/test/unit/config.spec.js index 169c418af..00e58882a 100644 --- a/test/unit/config.spec.js +++ b/test/unit/config.spec.js @@ -9,6 +9,7 @@ describe('config', () => { let m let e let mocks + let logUtilsModule const resolveWinPath = (p) => helper.normalizeWinPath(path.resolve(p)) @@ -54,6 +55,14 @@ describe('config', () => { } // load file under test + logUtilsModule = loadFile( + path.join(__dirname, '/../../lib/utils/log-utils.js'), + {}, + { + process: mocks.process + } + ) + m = loadFile(path.join(__dirname, '/../../lib/config.js'), mocks, { global: {}, process: mocks.process, @@ -62,6 +71,9 @@ describe('config', () => { return mockConfigs[path] } if (path.indexOf('./') === 0) { + if (path.endsWith('utils/log-utils')) { + return logUtilsModule.exports + } return require('../../lib/' + path) } else { return require(path) From 2324c12ab1facbf5d00914f7b5cfbaa67a6ec0a8 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Tue, 26 Jan 2021 13:21:55 -0500 Subject: [PATCH 04/12] chore: reverse first pass changes --- lib/config.js | 48 +++------ lib/utils/log-utils.js | 206 --------------------------------------- test/unit/config.spec.js | 12 --- 3 files changed, 12 insertions(+), 254 deletions(-) delete mode 100644 lib/utils/log-utils.js diff --git a/lib/config.js b/lib/config.js index 853121f28..1ea49b85c 100644 --- a/lib/config.js +++ b/lib/config.js @@ -10,16 +10,6 @@ const constant = require('./constants') const _ = require('lodash') -const { logActions, logMessageFactory } = require('./utils/log-utils') -const { ACTION_LOG, ACTION_LOG_AND_EXIT } = logActions -const karmaLog = { - debug: logMessageFactory(log, ACTION_LOG, constant.LOG_DEBUG, false, false), - error: logMessageFactory(log, ACTION_LOG_AND_EXIT, constant.LOG_ERROR, true, true), - info: logMessageFactory(log, ACTION_LOG, constant.LOG_INFO, false, false), - off: logMessageFactory(log, ACTION_LOG, constant.LOG_DISABLE, false, false), - warn: logMessageFactory(log, ACTION_LOG, constant.LOG_WARN, true, false) -} - let COFFEE_SCRIPT_AVAILABLE = false let LIVE_SCRIPT_AVAILABLE = false let TYPE_SCRIPT_AVAILABLE = false @@ -361,9 +351,7 @@ const CONFIG_SYNTAX_HELP = ' module.exports = function(config) {\n' + ' });\n' + ' };\n' -function parseConfig (configFilePath, options) { - const _options = typeof options === 'object' && options !== null ? options : {} - const { failureAction = 'exit', ...cliOptions } = _options +function parseConfig (configFilePath, cliOptions) { let configModule if (configFilePath) { try { @@ -372,30 +360,21 @@ function parseConfig (configFilePath, options) { configModule = configModule.default } } catch (e) { - const errorMessageSegments = [] - errorMessageSegments.push('Error in config file!\n ' + e.stack || e) + log.error('Error in config file!\n ' + e.stack || e) const extension = path.extname(configFilePath) if (extension === '.coffee' && !COFFEE_SCRIPT_AVAILABLE) { - errorMessageSegments.push('You need to install CoffeeScript.\n npm install coffeescript --save-dev') + log.error('You need to install CoffeeScript.\n npm install coffeescript --save-dev') } else if (extension === '.ls' && !LIVE_SCRIPT_AVAILABLE) { - errorMessageSegments.push('You need to install LiveScript.\n npm install LiveScript --save-dev') + log.error('You need to install LiveScript.\n npm install LiveScript --save-dev') } else if (extension === '.ts' && !TYPE_SCRIPT_AVAILABLE) { - errorMessageSegments.push('You need to install TypeScript.\n npm install typescript ts-node --save-dev') + log.error('You need to install TypeScript.\n npm install typescript ts-node --save-dev') } - const configModuleImportErrorMessage = errorMessageSegments.join('\n') - return karmaLog.error( - configModuleImportErrorMessage, - { action: failureAction, errorInstance: e, exitCode: 1 } - ) + return process.exit(1) } if (!helper.isFunction(configModule)) { - const configModuleExportTypeErrorMessage = - ('Config file must export a function!\n' + CONFIG_SYNTAX_HELP) - return karmaLog.error( - configModuleExportTypeErrorMessage, - { action: failureAction, exitCode: 1 } - ) + 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. @@ -416,11 +395,8 @@ function parseConfig (configFilePath, options) { try { configModule(config) } catch (e) { - const configModuleExecutionErrorMessage = ['Error in config file!\n', e] - return karmaLog.error( - configModuleExecutionErrorMessage, - { action: failureAction, errorInstance: e, exitCode: 1 } - ) + log.error('Error in config file!\n', e) + return process.exit(1) } // merge the config from config file and cliOptions (precedence) @@ -428,7 +404,7 @@ function parseConfig (configFilePath, options) { // if the user changed listenAddress, but didn't set a hostname, warn them if (config.hostname === null && config.listenAddress !== null) { - karmaLog.warn(`ListenAddress was set to ${config.listenAddress} but hostname was left as the default: ` + + 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 @@ -442,7 +418,7 @@ function parseConfig (configFilePath, options) { // configure the logger as soon as we can logger.setup(config.logLevel, config.colors, config.loggers) - karmaLog.debug(configFilePath ? `Loading config ${configFilePath}` : 'No config file specified.') + log.debug(configFilePath ? `Loading config ${configFilePath}` : 'No config file specified.') return normalizeConfig(config, configFilePath) } diff --git a/lib/utils/log-utils.js b/lib/utils/log-utils.js deleted file mode 100644 index d99f30506..000000000 --- a/lib/utils/log-utils.js +++ /dev/null @@ -1,206 +0,0 @@ - -const constants = require('../constants') // other files assign this to the singular "constant". - -const { LOG_DEBUG, LOG_DISABLE, LOG_ERROR, LOG_INFO, LOG_WARN } = constants -const validLogLevels = [ - LOG_DEBUG, - LOG_DISABLE, - LOG_ERROR, - LOG_INFO, - LOG_WARN -] - -// Mutually Exclusive Actions -// QUESTION: Move these to `constants.js`? -const ACTION_LOG = 'log' -const ACTION_LOG_AND_EXIT = 'exit' -const ACTION_THROW = 'throw' -const validActions = [ - ACTION_LOG, - ACTION_LOG_AND_EXIT, - ACTION_THROW -] -const actions = { - ACTION_LOG, - ACTION_LOG_AND_EXIT, - ACTION_THROW -} - -/** - * @param {string} levelName - * @returns {string} - * @throws {TypeError} - * - * @see [levelStr is always uppercase]{@link https://github.com/log4js-node/log4js-node/blob/v6.3.0/lib/levels.js#L49-L54} - * @see [builtin/default levels]{@link https://github.com/log4js-node/log4js-node/blob/v6.3.0/lib/levels.js#L90-L100} - * @see [static getLevel(sArg, defaultLevel)]{@link https://github.com/log4js-node/log4js-node/blob/v6.3.0/lib/logger.js#L113-L116} - * @see [level method name creation]{@link https://github.com/log4js-node/log4js-node/blob/v6.3.0/lib/logger.js#L113-L116} - */ -function getLevelMethodName (levelName) { - if (typeof levelName !== 'string') { - throw new TypeError('getLevelMethodName: `levelName` argument must be a string.') - } - const levelStrLower = levelName.toLowerCase() - const levelMethod = levelStrLower.replace(/_([a-z])/g, (g) => - g[1].toUpperCase() - ) - return levelMethod -} - -/** - * Create functions that may be used to handle and log messages. - * - * The factory changes the default behavior of the created message function - * so that the function may be consumed with minimal arguments and options. - * - * @param {Object} categoryLogger - * One of the internal log4js loggers created by Karma. This logger is used - * together with log levels as the default means of logging messages. - * @param {"exit"|"log"|"throw"} [defaultAction="log"] - * If this option is a valid action, then it will be used as the returned - * function's default action. Invalid values will result in the default - * log level being set to `"log"`. - * @param {"DEBUG"|"ERROR"|"INFO"|"OFF"|"WARN"} [defaultLevel="INFO"] - * If this option is a valid log level, then it will be used as the returned - * function's default log level. Invalid values will result in the default - * log level being set to `"INFO"`. - * @param {boolean} [mayExit = true] - * If this option is `false`, then process exiting is disabled. All other - * values enable process exiting. - * @param {boolean} [mayThrow = true] - * If this option is `false`, then throwing exceptions is disabled. All - * other values enable exception throwing. - * @returns {logMessage} - * A function that may be used for handling and logging messages. - */ -function logMessageFactory ( - categoryLogger, - defaultAction = ACTION_LOG, - defaultLevel = LOG_INFO, - mayExit = true, - mayThrow = true -) { - const CAN_EXIT = mayExit !== false - const CAN_THROW = mayThrow !== false - const DEFAULT_ACTION = validActions.includes(defaultAction) - ? defaultAction - : ACTION_LOG - const DEFAULT_LEVEL = validLogLevels.includes(defaultLevel) - ? defaultLevel - : LOG_INFO - - /** - * - * A function that may be used to handle and log messages. - * - * @param {string} message - * @param {Object} [options] - * @param {"exit"|"log"|"throw"} [options.action="log"] - * If this option is `"throw"`, then `message` will be used to create an - * error object which will then be thrown. If this option is `"log"`, then - * the message will be logged using the appropriate function. If this - * option is `"exit"`, then the process will exit with `exitCode` after - * the message is logged. The default value for this option depends upon - * what was passed to the factory. If the factory does not configure it, - * then the default value will be `"log"`. If an invalid value is passed, - * the default value will be used. The factory can disable throwing and - * exiting, which are both enabled by default. If other actions are - * disabled, then this function will always be able to log the message. - * @param {function} [options.errorConstructor] - * When throwing an exception an `options.errorInstance` object is not - * provided, then this function is used to construct a new error object. - * The constructor will be passed `message` as its first argument. - * If this option is not a function, the code will then fall back to using - * the global `Error` constructor. - * @param {Object} [options.errorInstance] - * When throwing an exception, this error object will have its `message` - * property value set to the `message` argument before throwing the - * object. If the this option is not an object, the code will then attempt - * to use `options.errorConstructor`. - * @param {number} [options.exitCode=1] - * An integer that the process will exit with. By default this will be - * `1`. Any exit code that is not `0` will indicate an error. `0` - * indicates a success. - * @param {function} [options.logFunction="INFO"] - * If this option is a function, it will be used to log the message - * instead of Karma's internal log4js logger. It will be passed `message` - * as the first argument. An object with `action`, `error`, and `logLevel` - * as properties will be passed as the second argument. `error` will be - * the object created by `errorInstance` or `errorConstructor`, regardless - * of whether or not an exception is being thrown. - * @param {"DEBUG"|"ERROR"|"INFO"|"OFF"|"WARN"} [options.logLevel] - * When logging a message and `options.logFunction` is not provided, then - * this will be used to determine the appropriate log4js method that will - * be used to log the message. Valid options include all log constants - * that are available on a `Config` object constructed by Karma. The - * default value for this option depends upon what was passed to the - * factory. If the factory does not configure it, then the default value - * will be `"INFO"`. - */ - function logMessage (messageArgs, options) { - const _options = typeof options === 'object' && options !== null ? options : {} - const action = validActions.includes(_options.action) - ? _options.action - : DEFAULT_ACTION - const exitCode = _options.exitCode || 1 - const logLevel = validLogLevels.includes(_options.logLevel) - ? _options.logLevel - : DEFAULT_LEVEL - const { errorConstructor, errorInstance, logFunction } = _options - const _messageArgs = Array.isArray(messageArgs) ? messageArgs : [messageArgs] - const [message, ...otherMessageArgs] = _messageArgs - - let error = null - if (typeof errorInstance === 'object' && errorInstance !== null) { - // Copy the original to avoid mutating by reference. We are especially - // interested in the name, message, and stack. - const ErrorConstructor = Object.getPrototypeOf(errorInstance).constructor - const newError = Object.create(ErrorConstructor) - - // Copy all own enumerable, non-enumerable, and Symbol key properties. - // Don't copy the descriptors directly, just in case it references - // something not exposed by the object. - const instanceDescriptors = Object.getOwnPropertyDescriptors(errorInstance) - for (const propName of Object.keys(instanceDescriptors)) { - newError[propName] = errorInstance[propName] - } - if (newError.message !== message) { - newError.originalMessage = newError.message - newError.message = message - } - error = newError - } else if (typeof errorConstructor === 'function') { - const ErrorConstructor = errorConstructor - error = new ErrorConstructor(message) - } else { - error = new Error(message) - } - - const shouldThrow = CAN_THROW && action === ACTION_THROW - if (shouldThrow) { - throw error - // TODO: Should the `log4js` function always be called? Do appenders emit - // : messages to other systems that care about messages? - } - - // const shouldLog = action === ACTION_LOG || action === ACTION_LOG_AND_EXIT - // if (shouldLog) { - if (typeof logFunction === 'function') { - logFunction(message, { action, error, logLevel }) - // TODO: Should the `log4js` function always be called? Do appenders emit - // : messages to other systems that care about messages? - } else { - const levelMethodName = getLevelMethodName(logLevel) - categoryLogger[levelMethodName](message, ...otherMessageArgs) - } - // } - - const shouldExit = CAN_EXIT && action === ACTION_LOG_AND_EXIT - if (shouldExit) { - process.exit(exitCode) - } - } - return logMessage -} - -module.exports = exports = { logActions: actions, logMessageFactory } diff --git a/test/unit/config.spec.js b/test/unit/config.spec.js index 00e58882a..169c418af 100644 --- a/test/unit/config.spec.js +++ b/test/unit/config.spec.js @@ -9,7 +9,6 @@ describe('config', () => { let m let e let mocks - let logUtilsModule const resolveWinPath = (p) => helper.normalizeWinPath(path.resolve(p)) @@ -55,14 +54,6 @@ describe('config', () => { } // load file under test - logUtilsModule = loadFile( - path.join(__dirname, '/../../lib/utils/log-utils.js'), - {}, - { - process: mocks.process - } - ) - m = loadFile(path.join(__dirname, '/../../lib/config.js'), mocks, { global: {}, process: mocks.process, @@ -71,9 +62,6 @@ describe('config', () => { return mockConfigs[path] } if (path.indexOf('./') === 0) { - if (path.endsWith('utils/log-utils')) { - return logUtilsModule.exports - } return require('../../lib/' + path) } else { return require(path) From 645bf8b21e86ba61cbabf35b400d7bc8541bcd86 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Tue, 26 Jan 2021 13:49:19 -0500 Subject: [PATCH 05/12] feat: simplify parseConfig failure implementation --- lib/config.js | 41 +++++++++++++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/lib/config.js b/lib/config.js index 1ea49b85c..39ac73109 100644 --- a/lib/config.js +++ b/lib/config.js @@ -351,7 +351,28 @@ 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?.throwErrors === true) { + // Some calls to `log.config` pass multiple arguments + // Should the be joined by a space or a new line character? + 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,21 +381,22 @@ function parseConfig (configFilePath, cliOptions) { configModule = configModule.default } } catch (e) { - log.error('Error in config file!\n ' + e.stack || e) + const errorMessageSegments = [] + errorMessageSegments.push('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') + errorMessageSegments.push('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') + errorMessageSegments.push('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') + errorMessageSegments.push('You need to install TypeScript.\n npm install typescript ts-node --save-dev') } - return process.exit(1) + const configModuleImportErrorMessage = errorMessageSegments.join('\n') + return fail(configModuleImportErrorMessage) } 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 +417,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) From 8cb9dbb4698dcf2e688d8dffb0c09ad22f5cddca Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Tue, 26 Jan 2021 13:53:43 -0500 Subject: [PATCH 06/12] fix: replace use of optional chaining, node 10 does not support it --- lib/config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/config.js b/lib/config.js index 39ac73109..305478c34 100644 --- a/lib/config.js +++ b/lib/config.js @@ -354,7 +354,7 @@ const CONFIG_SYNTAX_HELP = ' module.exports = function(config) {\n' + function parseConfig (configFilePath, cliOptions, parseOptions) { function fail () { log.error(...arguments) - if (parseOptions?.throwErrors === true) { + if (parseOptions && parseOptions.throwErrors === true) { // Some calls to `log.config` pass multiple arguments // Should the be joined by a space or a new line character? const errorMessage = Array.from(arguments).join(' ') From 09716d76abea4333b75b2c9dfb6655dc5023f6fe Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Tue, 26 Jan 2021 14:02:35 -0500 Subject: [PATCH 07/12] chore: clean comments --- lib/config.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/config.js b/lib/config.js index 305478c34..1e228eb0f 100644 --- a/lib/config.js +++ b/lib/config.js @@ -355,8 +355,6 @@ function parseConfig (configFilePath, cliOptions, parseOptions) { function fail () { log.error(...arguments) if (parseOptions && parseOptions.throwErrors === true) { - // Some calls to `log.config` pass multiple arguments - // Should the be joined by a space or a new line character? const errorMessage = Array.from(arguments).join(' ') throw new Error(errorMessage) } else { From 3cb3326fc4ae14c6dc7b64d5f86fec0f6dbf106b Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Tue, 26 Jan 2021 17:14:55 -0500 Subject: [PATCH 08/12] chore: revert segments and move failure after install messages --- lib/config.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/config.js b/lib/config.js index 1e228eb0f..b08bbde7d 100644 --- a/lib/config.js +++ b/lib/config.js @@ -379,19 +379,15 @@ function parseConfig (configFilePath, cliOptions, parseOptions) { configModule = configModule.default } } catch (e) { - const errorMessageSegments = [] - errorMessageSegments.push('Error in config file!\n ' + e.stack || e) - const extension = path.extname(configFilePath) if (extension === '.coffee' && !COFFEE_SCRIPT_AVAILABLE) { - errorMessageSegments.push('You need to install CoffeeScript.\n npm install coffeescript --save-dev') + log.error('You need to install CoffeeScript.\n npm install coffeescript --save-dev') } else if (extension === '.ls' && !LIVE_SCRIPT_AVAILABLE) { - errorMessageSegments.push('You need to install LiveScript.\n npm install LiveScript --save-dev') + log.error('You need to install LiveScript.\n npm install LiveScript --save-dev') } else if (extension === '.ts' && !TYPE_SCRIPT_AVAILABLE) { - errorMessageSegments.push('You need to install TypeScript.\n npm install typescript ts-node --save-dev') + log.error('You need to install TypeScript.\n npm install typescript ts-node --save-dev') } - const configModuleImportErrorMessage = errorMessageSegments.join('\n') - return fail(configModuleImportErrorMessage) + return fail('Error in config file!\n ' + e.stack || e) } if (!helper.isFunction(configModule)) { return fail('Config file must export a function!\n' + CONFIG_SYNTAX_HELP) From bc22df3c0b58a1eaa3e0c8c090fa310ee596a95a Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Tue, 26 Jan 2021 17:17:36 -0500 Subject: [PATCH 09/12] chore: add tests for the 3 failures when throwErrors is enabled --- test/unit/config.spec.js | 43 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/test/unit/config.spec.js b/test/unit/config.spec.js index 169c418af..25db6e16a 100644 --- 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: ['http://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,31 @@ describe('config', () => { expect(mocks.process.exit).to.have.been.calledWith(1) }) + 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 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 override config with given cli options', () => { const config = e.parseConfig('/home/config4.js', { port: 456, autoWatch: false }) From 967994599d73e23ebf3e17d708f66f8b95340434 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Tue, 26 Jan 2021 17:19:33 -0500 Subject: [PATCH 10/12] chore: remove server dependency on legacy exit behavior --- lib/server.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/server.js b/lib/server.js index 0cf94ce97..9b4bbedef 100644 --- a/lib/server.js +++ b/lib/server.js @@ -55,7 +55,7 @@ function createSocketIoServer (webServer, executor, config) { } class Server extends KarmaEventEmitter { - constructor (cliOptions, done) { + constructor (cliOptions, done = process.exit) { super() logger.setupFromConfig(cliOptions) @@ -63,14 +63,20 @@ 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) { + done(1) + return + } this.log.debug('Final config', util.inspect(config, false, /** depth **/ null)) let modules = [{ helper: ['value', helper], logger: ['value', logger], - done: ['value', done || process.exit], + done: ['value', done], emitter: ['value', this], server: ['value', this], watcher: ['value', watcher], From 52ba4aeff89a8984586e85964938327b937d2dde Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Wed, 27 Jan 2021 11:28:19 -0500 Subject: [PATCH 11/12] chore: keep related tests next to each other --- test/unit/config.spec.js | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/test/unit/config.spec.js b/test/unit/config.spec.js index 25db6e16a..d115a2b6e 100644 --- a/test/unit/config.spec.js +++ b/test/unit/config.spec.js @@ -149,28 +149,29 @@ describe('config', () => { expect(mocks.process.exit).to.have.been.calledWith(1) }) - it('should log error and throw if file does not export a function AND throwErrors is true', () => { + it('should log an error and throw if invalid file AND throwErrors is true', () => { function parseConfig () { - e.parseConfig('/conf/export-not-function.js', {}, { throwErrors: true }) + e.parseConfig('/conf/invalid.js', {}, { throwErrors: true }) } - expect(parseConfig).to.throw(Error, 'Config file must export a function!\n') + 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.toString().split('\n').slice(0, 1)).to.be.deep.equal( - ['Config file must export a function!']) + 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 an error and throw if invalid file AND throwErrors is true', () => { + it('should log error and throw if file does not export a function AND throwErrors is true', () => { function parseConfig () { - e.parseConfig('/conf/invalid.js', {}, { throwErrors: true }) + e.parseConfig('/conf/export-not-function.js', {}, { throwErrors: true }) } - expect(parseConfig).to.throw(Error, 'Error in config file!\n SyntaxError: Unexpected token =') + + 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[0]).to.eql('Error in config file!\n') - expect(event[1].message).to.eql('Unexpected token =') + 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 }) From 12211dd7764507dfeafcef1e780cf8f69954dce5 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Wed, 27 Jan 2021 11:33:12 -0500 Subject: [PATCH 12/12] revert: server done OR exit behavior --- lib/server.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/server.js b/lib/server.js index 9b4bbedef..458db4923 100644 --- a/lib/server.js +++ b/lib/server.js @@ -55,7 +55,7 @@ function createSocketIoServer (webServer, executor, config) { } class Server extends KarmaEventEmitter { - constructor (cliOptions, done = process.exit) { + constructor (cliOptions, done) { super() logger.setupFromConfig(cliOptions) @@ -67,7 +67,9 @@ class Server extends KarmaEventEmitter { try { config = cfg.parseConfig(cliOptions.configFile, cliOptions, { throwErrors: true }) } catch (parseConfigError) { - done(1) + // 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 } @@ -76,7 +78,7 @@ class Server extends KarmaEventEmitter { let modules = [{ helper: ['value', helper], logger: ['value', logger], - done: ['value', done], + done: ['value', done || process.exit], emitter: ['value', this], server: ['value', this], watcher: ['value', watcher],