From 62892fa558473db27409ed8db0bda3bd7251a301 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Sun, 28 Feb 2021 13:02:21 -0500 Subject: [PATCH 01/29] feat: initial promise support for `parseConfig` and `Server` --- lib/config.js | 63 +++++++++++++++++++++++++++++++++++---------------- lib/server.js | 32 ++++++++++++++++++-------- 2 files changed, 67 insertions(+), 28 deletions(-) diff --git a/lib/config.js b/lib/config.js index 1192afbac..4972587cd 100644 --- a/lib/config.js +++ b/lib/config.js @@ -411,34 +411,59 @@ function parseConfig (configFilePath, cliOptions, parseOptions) { // add the user's configuration in config.set(cliOptions) + let configModuleReturn try { - configModule(config) + configModuleReturn = configModule(config) } catch (e) { return fail('Error in config file!\n', e) } + function finalizeConfig (config) { + // merge the config from config file and cliOptions (precedence) + config.set(cliOptions) - // 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: ` + + // 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 - } + } + // 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) + // 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.') + log.debug(configFilePath ? `Loading config ${configFilePath}` : 'No config file specified.') - return normalizeConfig(config, configFilePath) + return normalizeConfig(config, configFilePath) + } + const returnIsThenable = configModuleReturn != null && typeof configModuleReturn === 'object' && typeof configModuleReturn.then === 'function' + const promiseConfig = parseOptions && parseOptions.promiseConfig === true + if (returnIsThenable) { + if (promiseConfig !== true) { + const errorMessage = + 'The `parseOptions.promiseConfig` option must be set to `true` to ' + + 'enable promise return values from configuration files.' + return fail(errorMessage) + } + return configModuleReturn.then(function onKarmaConfigModuleFulfilled (/* ignoredResolutionValue */) { + return finalizeConfig(config) + }) + } else { + if (promiseConfig) { + try { + return Promise.resolve(finalizeConfig(config)) + } catch (exception) { + return Promise.reject(exception) + } + } else { + return finalizeConfig(config) + } + } } // PUBLIC API diff --git a/lib/server.js b/lib/server.js index a6ae81dab..bace88622 100644 --- a/lib/server.js +++ b/lib/server.js @@ -55,22 +55,36 @@ function createSocketIoServer (webServer, executor, config) { } class Server extends KarmaEventEmitter { - constructor (cliOptions, done) { + constructor (cliOptionsOrConfig, done) { super() - logger.setupFromConfig(cliOptions) + logger.setupFromConfig({ + colors: cliOptionsOrConfig.colors, + logLevel: cliOptionsOrConfig.logLevel + }) this.log = logger.create('karma-server') this.loadErrors = [] 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 + if (cliOptionsOrConfig instanceof cfg.Config) { + config = cliOptionsOrConfig + } else { + try { + config = cfg.parseConfig( + cliOptionsOrConfig.configFile, + cliOptionsOrConfig, + { + promiseConfig: false, + 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)) From 39b9f80e71d69cb77e288794b5f587c08a355e03 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Sun, 28 Feb 2021 13:40:10 -0500 Subject: [PATCH 02/29] feat: add promise support to `parseConfig`'s fail function --- lib/config.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/config.js b/lib/config.js index 4972587cd..b8fe5879a 100644 --- a/lib/config.js +++ b/lib/config.js @@ -352,11 +352,16 @@ const CONFIG_SYNTAX_HELP = ' module.exports = function(config) {\n' + ' };\n' function parseConfig (configFilePath, cliOptions, parseOptions) { + const promiseConfig = parseOptions && parseOptions.promiseConfig === true function fail () { log.error(...arguments) if (parseOptions && parseOptions.throwErrors === true) { const errorMessage = Array.from(arguments).join(' ') - throw new Error(errorMessage) + const err = new Error(errorMessage) + if (promiseConfig) { + return Promise.reject(err) + } + throw err } else { const warningMessage = 'The `parseConfig()` function historically called `process.exit(1)`' + @@ -442,7 +447,6 @@ function parseConfig (configFilePath, cliOptions, parseOptions) { return normalizeConfig(config, configFilePath) } const returnIsThenable = configModuleReturn != null && typeof configModuleReturn === 'object' && typeof configModuleReturn.then === 'function' - const promiseConfig = parseOptions && parseOptions.promiseConfig === true if (returnIsThenable) { if (promiseConfig !== true) { const errorMessage = From c6c431487735b510c5e3724027337b4617489fab Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Tue, 2 Mar 2021 11:17:04 -0500 Subject: [PATCH 03/29] feat: add async config support to `cli`, `runner`, and `stopper` --- lib/cli.js | 51 ++++++++++++++++++++++++++++++-------------------- lib/config.js | 6 ++++-- lib/runner.js | 29 ++++++++++++++++++++++++---- lib/server.js | 6 +++--- lib/stopper.js | 29 +++++++++++++++++++++++++--- 5 files changed, 89 insertions(+), 32 deletions(-) diff --git a/lib/cli.js b/lib/cli.js index 6e7fdac4f..36710f327 100644 --- a/lib/cli.js +++ b/lib/cli.js @@ -7,6 +7,7 @@ const fs = require('graceful-fs') const Server = require('./server') const helper = require('./helper') const constant = require('./constants') +const cfg = require('./config') function processArgs (argv, options, fs, path) { Object.getOwnPropertyNames(argv).forEach(function (name) { @@ -278,26 +279,36 @@ exports.process = () => { } exports.run = () => { - const config = exports.process() - - switch (config.cmd) { - case 'start': - new Server(config).start() - break - case 'run': - require('./runner') - .run(config) - .on('progress', printRunnerProgress) - break - case 'stop': - require('./stopper').stop(config) - break - case 'init': - require('./init').init(config) - break - case 'completion': - require('./completion').completion(config) - break + const cliOptions = exports.process() + const cmd = cliOptions.cmd // prevent config from changing the command + const cmdSupportsConfigPromise = + cmd === 'start' || cmd === 'run' || cmd === 'stop' + if (!cmdSupportsConfigPromise) { + switch (cmd) { + case 'init': + require('./init').init(cliOptions) + break + case 'completion': + require('./completion').completion(cliOptions) + break + } + } else { + cfg.parseConfig(cliOptions.configFile, cliOptions, { promiseConfig: true }) + .then(function onKarmaConfigFulfilled (config) { + switch (cmd) { + case 'start': + new Server(config).start() + break + case 'run': + require('./runner') + .run(config) + .on('progress', printRunnerProgress) + break + case 'stop': + require('./stopper').stop(config) + break + } + }) } } diff --git a/lib/config.js b/lib/config.js index b8fe5879a..2cb954245 100644 --- a/lib/config.js +++ b/lib/config.js @@ -353,9 +353,10 @@ const CONFIG_SYNTAX_HELP = ' module.exports = function(config) {\n' + function parseConfig (configFilePath, cliOptions, parseOptions) { const promiseConfig = parseOptions && parseOptions.promiseConfig === true + const throwErrors = parseOptions && parseOptions.throwErrors === true function fail () { log.error(...arguments) - if (parseOptions && parseOptions.throwErrors === true) { + if (throwErrors === true || promiseConfig === true) { const errorMessage = Array.from(arguments).join(' ') const err = new Error(errorMessage) if (promiseConfig) { @@ -451,7 +452,8 @@ function parseConfig (configFilePath, cliOptions, parseOptions) { if (promiseConfig !== true) { const errorMessage = 'The `parseOptions.promiseConfig` option must be set to `true` to ' + - 'enable promise return values from configuration files.' + 'enable promise return values from configuration files. ' + + 'Example: `parseConfig(path, cliOptions, { promiseConfig: true })`' return fail(errorMessage) } return configModuleReturn.then(function onKarmaConfigModuleFulfilled (/* ignoredResolutionValue */) { diff --git a/lib/runner.js b/lib/runner.js index e98a39da1..9f5b31187 100644 --- a/lib/runner.js +++ b/lib/runner.js @@ -32,14 +32,35 @@ function parseExitCode (buffer, defaultExitCode, failOnEmptyTestSuite) { } // TODO(vojta): read config file (port, host, urlRoot) -function run (config, done) { - config = config || {} +function run (cliOptionsOrConfig, done) { + cliOptionsOrConfig = cliOptionsOrConfig || {} - logger.setupFromConfig(config) + logger.setupFromConfig({ + colors: cliOptionsOrConfig.colors, + logLevel: cliOptionsOrConfig.logLevel + }) done = helper.isFunction(done) ? done : process.exit - config = cfg.parseConfig(config.configFile, config) + let config + if (cliOptionsOrConfig instanceof cfg.Config) { + config = cliOptionsOrConfig + } else { + try { + config = cfg.parseConfig( + cliOptionsOrConfig.configFile, + cliOptionsOrConfig, + { + promiseConfig: false, + 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(1) + } + } let exitCode = 1 const emitter = new EventEmitter() const options = { diff --git a/lib/server.js b/lib/server.js index bace88622..7263e758d 100644 --- a/lib/server.js +++ b/lib/server.js @@ -57,13 +57,13 @@ function createSocketIoServer (webServer, executor, config) { class Server extends KarmaEventEmitter { constructor (cliOptionsOrConfig, done) { super() + cliOptionsOrConfig = cliOptionsOrConfig || {} logger.setupFromConfig({ colors: cliOptionsOrConfig.colors, logLevel: cliOptionsOrConfig.logLevel }) - this.log = logger.create('karma-server') - + done = helper.isFunction(done) ? done : process.exit this.loadErrors = [] let config @@ -82,7 +82,7 @@ class Server extends KarmaEventEmitter { } 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) + done(1) return } } diff --git a/lib/stopper.js b/lib/stopper.js index 5e87be595..6ea06dcde 100644 --- a/lib/stopper.js +++ b/lib/stopper.js @@ -3,11 +3,34 @@ const cfg = require('./config') const logger = require('./logger') const helper = require('./helper') -exports.stop = function (config, done) { - config = config || {} - logger.setupFromConfig(config) +exports.stop = function (cliOptionsOrConfig, done) { + cliOptionsOrConfig = cliOptionsOrConfig || {} + logger.setupFromConfig({ + colors: cliOptionsOrConfig.colors, + logLevel: cliOptionsOrConfig.logLevel + }) const log = logger.create('stopper') done = helper.isFunction(done) ? done : process.exit + + let config + if (cliOptionsOrConfig instanceof cfg.Config) { + config = cliOptionsOrConfig + } else { + try { + config = cfg.parseConfig( + cliOptionsOrConfig.configFile, + cliOptionsOrConfig, + { + promiseConfig: false, + 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(1) + } + } config = cfg.parseConfig(config.configFile, config) const request = http.request({ From 35c992eeec945bd86a9585e37866c0ac1d535609 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Fri, 5 Mar 2021 11:24:28 -0500 Subject: [PATCH 04/29] chore: rename `cmdSupportsConfigPromise` to `cmdNeedsConfig` --- lib/cli.js | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/lib/cli.js b/lib/cli.js index 36710f327..de4a49562 100644 --- a/lib/cli.js +++ b/lib/cli.js @@ -281,18 +281,8 @@ exports.process = () => { exports.run = () => { const cliOptions = exports.process() const cmd = cliOptions.cmd // prevent config from changing the command - const cmdSupportsConfigPromise = - cmd === 'start' || cmd === 'run' || cmd === 'stop' - if (!cmdSupportsConfigPromise) { - switch (cmd) { - case 'init': - require('./init').init(cliOptions) - break - case 'completion': - require('./completion').completion(cliOptions) - break - } - } else { + const cmdNeedsConfig = cmd === 'start' || cmd === 'run' || cmd === 'stop' + if (cmdNeedsConfig) { cfg.parseConfig(cliOptions.configFile, cliOptions, { promiseConfig: true }) .then(function onKarmaConfigFulfilled (config) { switch (cmd) { @@ -309,6 +299,15 @@ exports.run = () => { break } }) + } else { + switch (cmd) { + case 'init': + require('./init').init(cliOptions) + break + case 'completion': + require('./completion').completion(cliOptions) + break + } } } From 1a8ce5889118e321a1b6cfe6de563ab0305d58be Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Fri, 5 Mar 2021 11:26:22 -0500 Subject: [PATCH 05/29] fix: catch config errors and exit --- lib/cli.js | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/lib/cli.js b/lib/cli.js index de4a49562..d9854cfa5 100644 --- a/lib/cli.js +++ b/lib/cli.js @@ -283,22 +283,34 @@ exports.run = () => { const cmd = cliOptions.cmd // prevent config from changing the command const cmdNeedsConfig = cmd === 'start' || cmd === 'run' || cmd === 'stop' if (cmdNeedsConfig) { - cfg.parseConfig(cliOptions.configFile, cliOptions, { promiseConfig: true }) - .then(function onKarmaConfigFulfilled (config) { - switch (cmd) { - case 'start': - new Server(config).start() - break - case 'run': - require('./runner') - .run(config) - .on('progress', printRunnerProgress) - break - case 'stop': - require('./stopper').stop(config) - break - } - }) + cfg.parseConfig( + cliOptions.configFile, + cliOptions, + { + promiseConfig: true, + throwErrors: true + } + ).then(function onKarmaConfigFulfilled (config) { + switch (cmd) { + case 'start': + new Server(config).start() + break + case 'run': + require('./runner') + .run(config) + .on('progress', printRunnerProgress) + break + case 'stop': + require('./stopper').stop(config) + break + } + }, function onKarmaConfigRejected (reason) { + console.error(reason) // TODO: configure a CLI Logger? + + // The `run` function is a private application, not a public API. We don't + // need to worry about process.exit vs throw vs promise rejection here. + process.exit(1) + }) } else { switch (cmd) { case 'init': From 088aa7964a71632eb6378aacec565abda15d5fd5 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Fri, 5 Mar 2021 11:29:45 -0500 Subject: [PATCH 06/29] chore: remove throwErrors implication from promiseConfig option --- lib/config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/config.js b/lib/config.js index 2cb954245..b18c9d95b 100644 --- a/lib/config.js +++ b/lib/config.js @@ -356,7 +356,7 @@ function parseConfig (configFilePath, cliOptions, parseOptions) { const throwErrors = parseOptions && parseOptions.throwErrors === true function fail () { log.error(...arguments) - if (throwErrors === true || promiseConfig === true) { + if (throwErrors) { const errorMessage = Array.from(arguments).join(' ') const err = new Error(errorMessage) if (promiseConfig) { From 5d92865dd3d61c9570bd85102e56352d9dc0715c Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Fri, 5 Mar 2021 11:31:17 -0500 Subject: [PATCH 07/29] fix: add warning for deprecated use of CLI options --- lib/runner.js | 9 +++++++++ lib/server.js | 7 +++++++ lib/stopper.js | 6 ++++++ 3 files changed, 22 insertions(+) diff --git a/lib/runner.js b/lib/runner.js index 9f5b31187..81ce9e1ff 100644 --- a/lib/runner.js +++ b/lib/runner.js @@ -35,6 +35,9 @@ function parseExitCode (buffer, defaultExitCode, failOnEmptyTestSuite) { function run (cliOptionsOrConfig, done) { cliOptionsOrConfig = cliOptionsOrConfig || {} + // TODO: Should `const log = logger.create('runner')` be moved into the + // : function for consistency with server.js and stopper.js? or the + // : reverse (make server and stopper consistent with runner?) logger.setupFromConfig({ colors: cliOptionsOrConfig.colors, logLevel: cliOptionsOrConfig.logLevel @@ -46,6 +49,12 @@ function run (cliOptionsOrConfig, done) { if (cliOptionsOrConfig instanceof cfg.Config) { config = cliOptionsOrConfig } else { + const deprecatedCliOptionsMessage = + 'Passing raw CLI options to `runner(config, done)` is deprecated. Use ' + + '`parseConfig(configFilePath, cliOptions, {promiseConfig: true, throwErrors: true})` ' + + 'to prepare a processed `Config` instance and pass that as the ' + + '`config` argument instead.' + log.warn(deprecatedCliOptionsMessage) try { config = cfg.parseConfig( cliOptionsOrConfig.configFile, diff --git a/lib/server.js b/lib/server.js index 7263e758d..07fabc32f 100644 --- a/lib/server.js +++ b/lib/server.js @@ -70,6 +70,13 @@ class Server extends KarmaEventEmitter { if (cliOptionsOrConfig instanceof cfg.Config) { config = cliOptionsOrConfig } else { + const deprecatedCliOptionsMessage = + 'Passing raw CLI options to `new Server(config, done)` is ' + + 'deprecated. Use ' + + '`parseConfig(configFilePath, cliOptions, {promiseConfig: true, throwErrors: true})` ' + + 'to prepare a processed `Config` instance and pass that as the ' + + '`config` argument instead.' + this.log.warn(deprecatedCliOptionsMessage) try { config = cfg.parseConfig( cliOptionsOrConfig.configFile, diff --git a/lib/stopper.js b/lib/stopper.js index 6ea06dcde..b03fb046d 100644 --- a/lib/stopper.js +++ b/lib/stopper.js @@ -16,6 +16,12 @@ exports.stop = function (cliOptionsOrConfig, done) { if (cliOptionsOrConfig instanceof cfg.Config) { config = cliOptionsOrConfig } else { + const deprecatedCliOptionsMessage = + 'Passing raw CLI options to `stopper(config, done)` is deprecated. Use ' + + '`parseConfig(configFilePath, cliOptions, {promiseConfig: true, throwErrors: true})` ' + + 'to prepare a processed `Config` instance and pass that as the ' + + '`config` argument instead.' + log.warn(deprecatedCliOptionsMessage) try { config = cfg.parseConfig( cliOptionsOrConfig.configFile, From 8a52b2b72d6cad93d1a0d2331c6921177374e8e6 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Thu, 11 Mar 2021 09:56:34 -0500 Subject: [PATCH 08/29] fix: treat rejections the same as exceptions --- lib/config.js | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/config.js b/lib/config.js index b18c9d95b..2f710b3f7 100644 --- a/lib/config.js +++ b/lib/config.js @@ -456,9 +456,14 @@ function parseConfig (configFilePath, cliOptions, parseOptions) { 'Example: `parseConfig(path, cliOptions, { promiseConfig: true })`' return fail(errorMessage) } - return configModuleReturn.then(function onKarmaConfigModuleFulfilled (/* ignoredResolutionValue */) { - return finalizeConfig(config) - }) + return configModuleReturn.then( + function onKarmaConfigModuleFulfilled (/* ignoredResolutionValue */) { + return finalizeConfig(config) + }, + function onKarmaConfigModuleRejected (reason) { + return fail('Error in config file!\n', reason) + } + ) } else { if (promiseConfig) { try { From c4fd16e88ccdc79410ed579a32add267b6789e30 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Thu, 11 Mar 2021 12:40:38 -0500 Subject: [PATCH 09/29] chore: setup logger early when promises are used --- lib/config.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/config.js b/lib/config.js index 2f710b3f7..9384aed0b 100644 --- a/lib/config.js +++ b/lib/config.js @@ -354,6 +354,13 @@ const CONFIG_SYNTAX_HELP = ' module.exports = function(config) {\n' + function parseConfig (configFilePath, cliOptions, parseOptions) { const promiseConfig = parseOptions && parseOptions.promiseConfig === true const throwErrors = parseOptions && parseOptions.throwErrors === true + const shouldSetupLoggerEarly = promiseConfig + if (shouldSetupLoggerEarly) { + // `setupFromConfig` provides defaults for `colors` and `logLevel`. + // `setup` provides defaults for `appenders` + // The first argument MUST BE an object + logger.setupFromConfig({}) + } function fail () { log.error(...arguments) if (throwErrors) { From e4f48877b8af247e15f39e518696428316185770 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Thu, 11 Mar 2021 13:29:20 -0500 Subject: [PATCH 10/29] chore: remove obsolete `console.error` --- lib/cli.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/cli.js b/lib/cli.js index d9854cfa5..37ef2193c 100644 --- a/lib/cli.js +++ b/lib/cli.js @@ -304,8 +304,9 @@ exports.run = () => { require('./stopper').stop(config) break } - }, function onKarmaConfigRejected (reason) { - console.error(reason) // TODO: configure a CLI Logger? + }, function onKarmaConfigRejected (/* ignoredReason */) { + // The reject reason isn't used to log a message since parseConfig already + // calls a configured logger method with an almost identical message. // The `run` function is a private application, not a public API. We don't // need to worry about process.exit vs throw vs promise rejection here. From a43554bf3afc6fa9b0d34ce9444c2b83bcd7e5bd Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Thu, 11 Mar 2021 14:05:13 -0500 Subject: [PATCH 11/29] chore: guard `logger.setupFromConfig` calls --- lib/runner.js | 16 +++++++--------- lib/server.js | 8 ++++---- lib/stopper.js | 8 ++++---- 3 files changed, 15 insertions(+), 17 deletions(-) diff --git a/lib/runner.js b/lib/runner.js index 81ce9e1ff..58839727d 100644 --- a/lib/runner.js +++ b/lib/runner.js @@ -34,21 +34,19 @@ function parseExitCode (buffer, defaultExitCode, failOnEmptyTestSuite) { // TODO(vojta): read config file (port, host, urlRoot) function run (cliOptionsOrConfig, done) { cliOptionsOrConfig = cliOptionsOrConfig || {} - - // TODO: Should `const log = logger.create('runner')` be moved into the - // : function for consistency with server.js and stopper.js? or the - // : reverse (make server and stopper consistent with runner?) - logger.setupFromConfig({ - colors: cliOptionsOrConfig.colors, - logLevel: cliOptionsOrConfig.logLevel - }) - done = helper.isFunction(done) ? done : process.exit let config if (cliOptionsOrConfig instanceof cfg.Config) { config = cliOptionsOrConfig } else { + // TODO: Should `const log = logger.create('runner')` be moved into the + // : function for consistency with server.js and stopper.js? or the + // : reverse (make server and stopper consistent with runner?) + logger.setupFromConfig({ + colors: cliOptionsOrConfig.colors, + logLevel: cliOptionsOrConfig.logLevel + }) const deprecatedCliOptionsMessage = 'Passing raw CLI options to `runner(config, done)` is deprecated. Use ' + '`parseConfig(configFilePath, cliOptions, {promiseConfig: true, throwErrors: true})` ' + diff --git a/lib/server.js b/lib/server.js index 07fabc32f..cec1ecd0b 100644 --- a/lib/server.js +++ b/lib/server.js @@ -58,10 +58,6 @@ class Server extends KarmaEventEmitter { constructor (cliOptionsOrConfig, done) { super() cliOptionsOrConfig = cliOptionsOrConfig || {} - logger.setupFromConfig({ - colors: cliOptionsOrConfig.colors, - logLevel: cliOptionsOrConfig.logLevel - }) this.log = logger.create('karma-server') done = helper.isFunction(done) ? done : process.exit this.loadErrors = [] @@ -70,6 +66,10 @@ class Server extends KarmaEventEmitter { if (cliOptionsOrConfig instanceof cfg.Config) { config = cliOptionsOrConfig } else { + logger.setupFromConfig({ + colors: cliOptionsOrConfig.colors, + logLevel: cliOptionsOrConfig.logLevel + }) const deprecatedCliOptionsMessage = 'Passing raw CLI options to `new Server(config, done)` is ' + 'deprecated. Use ' + diff --git a/lib/stopper.js b/lib/stopper.js index b03fb046d..504190331 100644 --- a/lib/stopper.js +++ b/lib/stopper.js @@ -5,10 +5,6 @@ const helper = require('./helper') exports.stop = function (cliOptionsOrConfig, done) { cliOptionsOrConfig = cliOptionsOrConfig || {} - logger.setupFromConfig({ - colors: cliOptionsOrConfig.colors, - logLevel: cliOptionsOrConfig.logLevel - }) const log = logger.create('stopper') done = helper.isFunction(done) ? done : process.exit @@ -16,6 +12,10 @@ exports.stop = function (cliOptionsOrConfig, done) { if (cliOptionsOrConfig instanceof cfg.Config) { config = cliOptionsOrConfig } else { + logger.setupFromConfig({ + colors: cliOptionsOrConfig.colors, + logLevel: cliOptionsOrConfig.logLevel + }) const deprecatedCliOptionsMessage = 'Passing raw CLI options to `stopper(config, done)` is deprecated. Use ' + '`parseConfig(configFilePath, cliOptions, {promiseConfig: true, throwErrors: true})` ' + From 8490ed63ed22b8cd2396f64c3ae65de9293be470 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Thu, 11 Mar 2021 17:10:37 -0500 Subject: [PATCH 12/29] chore: update documentation for parseConfig --- docs/dev/04-public-api.md | 143 +++++++++++++++++++++++++++++++++++++- 1 file changed, 141 insertions(+), 2 deletions(-) diff --git a/docs/dev/04-public-api.md b/docs/dev/04-public-api.md index ff327a41a..15ba0e0e9 100644 --- a/docs/dev/04-public-api.md +++ b/docs/dev/04-public-api.md @@ -156,20 +156,159 @@ stopper.stop({port: 9876}, function(exitCode) { }) ``` -## karma.config.parseConfig([configFilePath], [cliOptions]) +## karma.config + +### config.parseConfig([configFilePath], [cliOptions], [parseOptions]) This function will load given config file and returns a filled config object. This can be useful if you want to integrate karma into another tool and want to load the karma config while honoring the karma defaults. For example, the [stryker-karma-runner](https://github.com/stryker-mutator/stryker-karma-runner) uses this to load your karma configuration and use that in the stryker configuration. +#### Usage + +##### Deprecated Behavior + +The following still works, but the way it behaves is deprecated and will be +changed in a future major version. + +```javascript +const cfg = require('karma').config; +const path = require('path'); +// Read karma.conf.js, but override port with 1337 +const karmaConfig = cfg.parseConfig( + path.resolve('./karma.conf.js'), + { port: 1337 } +); +``` + +The new behavior in the future will involve throwing exceptions instead of +exiting the process and aynchronous config files will be supported through the +use of promises. + +##### New Behavior + +```javascript +const cfg = require('karma').config; +const path = require('path'); +// Read karma.conf.js, but override port with 1337 +const karmaConfig = cfg.parseConfig( + path.resolve('./karma.conf.js'), + { port: 1337 }, + { throwErrors: true } +) +``` + +##### New Behavior with Promise Support + ```javascript const cfg = require('karma').config; const path = require('path'); // Read karma.conf.js, but override port with 1337 -const karmaConfig = cfg.parseConfig(path.resolve('./karma.conf.js'), { port: 1337 } ); +cfg.parseConfig( + path.resolve('./karma.conf.js'), + { port: 1337 }, + { promiseConfig: true, throwErrors: true } +).then( + (karmaConfig) => { /* use the config with the public API */ }, + (rejectReason) => { /* respond to the rejection reason error */ } +); ``` + +#### `configFilePath` argument + +- **Type:** String | `null` | `undefined` +- **Default Value:** `undefined` + +A string representing a file system path pointing to the config file whose +default export is a function that will be used to set Karma configuration +options. This function will be passed an instance of the `Config` class as its +first argument. If this option is not provided, then only the options provided +by the `cliOptions` argument will be set. + +- JavaScript must use CommonJS modules. +- ECMAScript modules are not currently supported by Karma when using + JavaScript. + - Other formats, such as TypeScript, may support ECMAScript modules. + + +#### `cliOptions` argument + +- **Type:** Object | `null` | `undefined` +- **Default Value:** `undefined` + +An object whose values will take priority over options set in the config file. +The config object passed to function exported by the config file will already +have these options applied. Any changes the config file makes to these options +will effectively be ignored in the final configuration. + +Supports all the same options as the config file and is applied using the same +`config.set()` method. + +The expected source of this argument is parsed command line options, but +programatic users may construct this object or leave it out entirely. + + +#### `parseOptions` argument + +- **Type:** Object | `null` | `undefined` +- **Default Value:** `undefined` + +`parseOptions` is an object whose properties are configuration options that +allow additional control over parsing and opt-in access to new behaviors or +features. + +These options are only related to parsing configuration files and object and are +not related to the configuration of Karma itself. + + +##### `parseOptions.promiseConfig` option + +- **Type:** Boolean +- **Default Value:** `false` + +When `parseOptions.promiseConfig === true`, then `parseConfig` will return a +promise instead of a configuration object. + +When this option is `true`, then the function exported by the config file may +return a promise. The resolution of that promise indicates that all asynchronous +activity has been completed. Internally, the resolved/fulfilled value is +ignored. As with synchronous usage, all changes to the config object must be +done with the `config.set()` method. + +If the function exported by the config file does not return a promise, then +parsing is completed and an immediately fulfilled promise is returned. + +Whether the function exported by the config file returns a promise or not, the +promise returned by `parseConfig()` will resolve with a parsed configuration +object, an instance of the `Config` class, as the value. + +_**In most cases, `parseOptions.throwErrors = true` should also be set. This +disables process exiting and allows errors to result in rejected promises.**_ + + +##### `parseOptions.throwErrors` option + +- **Type:** Boolean +- **Default Value:** `false` + +In the past, `parseConfig()` would call `process.exit(exitCode)` when it +encountered a critical failure. This meant that your own code had no way of +responding to failures before the Node.js process exited. + +By passing `parseOptions.throwErrors = true`, `parseConfig()` will disable +process exiting. + +For synchronous usage, it will throw an exception instead of exiting the +process. Your code can then catch the exception and respond how ever it needs +to. + +If the asynchronous API (`parseOptions.promiseConfig = true`) is being used, +then `parseOptions.throwErrors = true` allows the promise to be rejected +instead of exiting the process. + + ## karma.constants ### **constants.VERSION** From 63dc58f76a2d5b3703af692fb08d6b7f30be4d24 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Fri, 12 Mar 2021 11:57:23 -0500 Subject: [PATCH 13/29] fix: remove excess config assignment --- lib/stopper.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/stopper.js b/lib/stopper.js index 504190331..964eb5e37 100644 --- a/lib/stopper.js +++ b/lib/stopper.js @@ -37,7 +37,6 @@ exports.stop = function (cliOptionsOrConfig, done) { done(1) } } - config = cfg.parseConfig(config.configFile, config) const request = http.request({ hostname: config.hostname, From 4c09e033d1073e97492c661679502a6d7a5f8f16 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Fri, 12 Mar 2021 13:39:01 -0500 Subject: [PATCH 14/29] chore: document only new behavior with promises --- docs/dev/04-public-api.md | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/docs/dev/04-public-api.md b/docs/dev/04-public-api.md index 15ba0e0e9..7eb8262a4 100644 --- a/docs/dev/04-public-api.md +++ b/docs/dev/04-public-api.md @@ -188,19 +188,6 @@ use of promises. ##### New Behavior -```javascript -const cfg = require('karma').config; -const path = require('path'); -// Read karma.conf.js, but override port with 1337 -const karmaConfig = cfg.parseConfig( - path.resolve('./karma.conf.js'), - { port: 1337 }, - { throwErrors: true } -) -``` - -##### New Behavior with Promise Support - ```javascript const cfg = require('karma').config; const path = require('path'); From 65e583d9838136024b0210db508b31673d867ec5 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Fri, 12 Mar 2021 13:40:46 -0500 Subject: [PATCH 15/29] chore: remove reference to example that is no longer accurate --- docs/dev/04-public-api.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/dev/04-public-api.md b/docs/dev/04-public-api.md index 7eb8262a4..9a13ff6ff 100644 --- a/docs/dev/04-public-api.md +++ b/docs/dev/04-public-api.md @@ -162,8 +162,7 @@ stopper.stop({port: 9876}, function(exitCode) { This function will load given config file and returns a filled config object. This can be useful if you want to integrate karma into another tool and want to load -the karma config while honoring the karma defaults. For example, the [stryker-karma-runner](https://github.com/stryker-mutator/stryker-karma-runner) -uses this to load your karma configuration and use that in the stryker configuration. +the karma config while honoring the karma defaults. #### Usage From 765cd0eab3c7644fd2b0347551cab3c5657d7a23 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Fri, 12 Mar 2021 13:43:13 -0500 Subject: [PATCH 16/29] chore: update Server constructor docs --- docs/dev/04-public-api.md | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/docs/dev/04-public-api.md b/docs/dev/04-public-api.md index 9a13ff6ff..2d24d8c1e 100644 --- a/docs/dev/04-public-api.md +++ b/docs/dev/04-public-api.md @@ -6,6 +6,17 @@ You can, however, call Karma programmatically from your node module. Here is the ### Constructor +- **Returns:** `Server` instance. + +#### Usage + +Notice the capital 'S' on `require('karma').Server`. + +##### Deprecated Behavior + +The following still works, but the way it behaves is deprecated and will be +changed in a future major version. + ```javascript var Server = require('karma').Server var karmaConfig = {port: 9876} @@ -15,7 +26,27 @@ var server = new Server(karmaConfig, function(exitCode) { }) ``` -Notice the capital 'S' on `require('karma').Server`. +##### New Behavior + +```javascript +const karma = require('karma') +const parseConfig = karma.config.parseConfig +const Server = karma.Server + +parseConfig( + null, + { port: 9876 }, + { promiseConfig: true, throwErrors: true } +).then( + (karmaConfig) => { + const server = new Server(karmaConfig, function doneCallback(exitCode) { + console.log('Karma has exited with ' + exitCode) + process.exit(exitCode) + }) + }, + (rejectReason) => { /* respond to the rejection reason error */ } +); +``` ### **server.start()** From ae7dbf7e249be2982bf846dcd84025912ba2eed8 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Fri, 12 Mar 2021 13:50:59 -0500 Subject: [PATCH 17/29] chore: update runner and stopper docs --- docs/dev/04-public-api.md | 81 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 76 insertions(+), 5 deletions(-) diff --git a/docs/dev/04-public-api.md b/docs/dev/04-public-api.md index 2d24d8c1e..5c5c53788 100644 --- a/docs/dev/04-public-api.md +++ b/docs/dev/04-public-api.md @@ -150,8 +150,17 @@ This event gets triggered whenever all the browsers, which belong to a test run, ### **runner.run(options, [callback=process.exit])** +- **Returns:** `EventEmitter` + The equivalent of `karma run`. +#### Usage + +##### Deprecated Behavior + +The following still works, but the way it behaves is deprecated and will be +changed in a future major version. + ```javascript var runner = require('karma').runner runner.run({port: 9876}, function(exitCode) { @@ -160,6 +169,35 @@ runner.run({port: 9876}, function(exitCode) { }) ``` +##### New Behavior + +```javascript +const karma = require('karma') + +karma.config.parseConfig( + null, + { port: 9876 }, + { promiseConfig: true, throwErrors: true } +).then( + (karmaConfig) => { + karma.runner.run(karmaConfig, function doneCallback(exitCode, possibleErrorCode) { + console.log('Karma has exited with ' + exitCode) + process.exit(exitCode) + }) + }, + (rejectReason) => { /* respond to the rejection reason error */ } +); +``` + +#### `callback` argument + +The callback receives the exit code as the first argument. + +If there is an error, the error code will be provided as the second parameter to +the error callback. + +#### runner Events + `runner.run()` returns an `EventEmitter` which emits a `progress` event passing the reporter output as a `Buffer` object. @@ -175,7 +213,15 @@ runner.run({port: 9876}).on('progress', function(data) { ### **stopper.stop(options, [callback=process.exit])** -This function will signal a running server to stop. The equivalent of `karma stop`. +This function will signal a running server to stop. The equivalent of +`karma stop`. + +#### Usage + +##### Deprecated Behavior + +The following still works, but the way it behaves is deprecated and will be +changed in a future major version. ```javascript var stopper = require('karma').stopper @@ -187,6 +233,35 @@ stopper.stop({port: 9876}, function(exitCode) { }) ``` +##### New Behavior + +```javascript +const karma = require('karma') + +karma.config.parseConfig( + null, + { port: 9876 }, + { promiseConfig: true, throwErrors: true } +).then( + (karmaConfig) => { + karma.stopper.stop(karmaConfig, function doneCallback(exitCode, possibleErrorCode) { + if (exitCode === 0) { + console.log('Server stop as initiated') + } + process.exit(exitCode) + }) + }, + (rejectReason) => { /* respond to the rejection reason error */ } +); +``` + +#### `callback` argument + +The callback receives the exit code as the first argument. + +If there is an error, the error code will be provided as the second parameter to +the error callback. + ## karma.config ### config.parseConfig([configFilePath], [cliOptions], [parseOptions]) @@ -383,7 +458,3 @@ The default console appender ### **constants.EXIT_CODE** The exit code - -## Callback function notes - -- If there is an error, the error code will be provided as the second parameter to the error callback. From 91bf78a93bb7e80c769ec42259cd981dc28c82bd Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Fri, 12 Mar 2021 14:06:20 -0500 Subject: [PATCH 18/29] chore: replace redundant formatting in headers --- docs/dev/04-public-api.md | 44 +++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/docs/dev/04-public-api.md b/docs/dev/04-public-api.md index 5c5c53788..7115aca96 100644 --- a/docs/dev/04-public-api.md +++ b/docs/dev/04-public-api.md @@ -4,7 +4,7 @@ You can, however, call Karma programmatically from your node module. Here is the ## karma.Server(options, [callback=process.exit]) -### Constructor +### `constructor` - **Returns:** `Server` instance. @@ -48,7 +48,7 @@ parseConfig( ); ``` -### **server.start()** +### `server.start()` Equivalent of `karma start`. @@ -56,7 +56,7 @@ Equivalent of `karma start`. server.start() ``` -### **server.refreshFiles()** +### `server.refreshFiles()` Trigger a file list refresh. Returns a promise. @@ -64,7 +64,7 @@ Trigger a file list refresh. Returns a promise. server.refreshFiles() ``` -### **server.refreshFile(path)** +### `server.refreshFile(path)` Trigger a file refresh. Returns a promise. @@ -148,7 +148,7 @@ This event gets triggered whenever all the browsers, which belong to a test run, ## karma.runner -### **runner.run(options, [callback=process.exit])** +### `runner.run(options, [callback=process.exit])` - **Returns:** `EventEmitter` @@ -211,7 +211,7 @@ runner.run({port: 9876}).on('progress', function(data) { ## karma.stopper -### **stopper.stop(options, [callback=process.exit])** +### `stopper.stop(options, [callback=process.exit])` This function will signal a running server to stop. The equivalent of `karma stop`. @@ -264,7 +264,7 @@ the error callback. ## karma.config -### config.parseConfig([configFilePath], [cliOptions], [parseOptions]) +### `config.parseConfig([configFilePath], [cliOptions], [parseOptions])` This function will load given config file and returns a filled config object. This can be useful if you want to integrate karma into another tool and want to load @@ -401,60 +401,60 @@ then `parseOptions.throwErrors = true` allows the promise to be rejected instead of exiting the process. -## karma.constants +## `constants` -### **constants.VERSION** +### `constants.VERSION` The current version of karma -### **constants.DEFAULT_PORT** +### `constants.DEFAULT_PORT` The default port used for the karma server -### **constants.DEFAULT_HOSTNAME** +### `constants.DEFAULT_HOSTNAME` The default hostname used for the karma server -### **constants.DEFAULT_LISTEN_ADDR** +### `constants.DEFAULT_LISTEN_ADDR` The default address use for the karma server to listen on -### **constants.LOG_DISABLE** +### `constants.LOG_DISABLE` The value for disabling logs -### **constants.LOG_ERROR** +### `constants.LOG_ERROR` The value for the log `error` level -### **constants.LOG_WARN** +### `constants.LOG_WARN` The value for the log `warn` level -### **constants.LOG_INFO** +### `constants.LOG_INFO` The value for the log `info` level -### **constants.LOG_DEBUG** +### `constants.LOG_DEBUG` The value for the log `debug` level -### **constants.LOG_PRIORITIES** +### `constants.LOG_PRIORITIES` An array of log levels in descending order, i.e. `LOG_DISABLE`, `LOG_ERROR`, `LOG_WARN`, `LOG_INFO`, and `LOG_DEBUG` -### **constants.COLOR_PATTERN** +### `constants.COLOR_PATTERN` The default color pattern for log output -### **constants.NO_COLOR_PATTERN** +### `constants.NO_COLOR_PATTERN` The default pattern for log output without color -### **constants.CONSOLE_APPENDER** +### `constants.CONSOLE_APPENDER` The default console appender -### **constants.EXIT_CODE** +### `constants.EXIT_CODE` The exit code From e63c3e8539e902bd78ad014fed860794064344fd Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Fri, 12 Mar 2021 14:24:18 -0500 Subject: [PATCH 19/29] fix: replace header text that was accidentally removed --- docs/dev/04-public-api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/dev/04-public-api.md b/docs/dev/04-public-api.md index 7115aca96..717eb1855 100644 --- a/docs/dev/04-public-api.md +++ b/docs/dev/04-public-api.md @@ -401,7 +401,7 @@ then `parseOptions.throwErrors = true` allows the promise to be rejected instead of exiting the process. -## `constants` +## `karma.constants` ### `constants.VERSION` From 4d8e7ea4b1134ee226d02174d62a7ab12ebe7502 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Mon, 15 Mar 2021 10:58:38 -0400 Subject: [PATCH 20/29] chore: remove comment per code review --- lib/runner.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/runner.js b/lib/runner.js index 58839727d..7d07bb08b 100644 --- a/lib/runner.js +++ b/lib/runner.js @@ -40,9 +40,6 @@ function run (cliOptionsOrConfig, done) { if (cliOptionsOrConfig instanceof cfg.Config) { config = cliOptionsOrConfig } else { - // TODO: Should `const log = logger.create('runner')` be moved into the - // : function for consistency with server.js and stopper.js? or the - // : reverse (make server and stopper consistent with runner?) logger.setupFromConfig({ colors: cliOptionsOrConfig.colors, logLevel: cliOptionsOrConfig.logLevel From 3038717aea6266f6bdd1c7ffd949d49944ab5b13 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Mon, 15 Mar 2021 17:03:40 -0400 Subject: [PATCH 21/29] chore: make async CLI branches testable --- lib/cli.js | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/lib/cli.js b/lib/cli.js index 37ef2193c..95f42b491 100644 --- a/lib/cli.js +++ b/lib/cli.js @@ -282,8 +282,9 @@ exports.run = () => { const cliOptions = exports.process() const cmd = cliOptions.cmd // prevent config from changing the command const cmdNeedsConfig = cmd === 'start' || cmd === 'run' || cmd === 'stop' + let runPromise if (cmdNeedsConfig) { - cfg.parseConfig( + runPromise = cfg.parseConfig( cliOptions.configFile, cliOptions, { @@ -291,19 +292,23 @@ exports.run = () => { throwErrors: true } ).then(function onKarmaConfigFulfilled (config) { + let fulfillmentValue switch (cmd) { - case 'start': - new Server(config).start() + case 'start': { + const server = new Server(config) + fulfillmentValue = server.start().then(() => server) break + } case 'run': - require('./runner') + fulfillmentValue = require('./runner') .run(config) .on('progress', printRunnerProgress) break case 'stop': - require('./stopper').stop(config) + fulfillmentValue = require('./stopper').stop(config) break } + return fulfillmentValue }, function onKarmaConfigRejected (/* ignoredReason */) { // The reject reason isn't used to log a message since parseConfig already // calls a configured logger method with an almost identical message. @@ -313,15 +318,26 @@ exports.run = () => { process.exit(1) }) } else { - switch (cmd) { - case 'init': - require('./init').init(cliOptions) - break - case 'completion': - require('./completion').completion(cliOptions) - break + try { + let fulfillmentValue + switch (cmd) { + case 'init': + fulfillmentValue = require('./init').init(cliOptions) + break + case 'completion': + fulfillmentValue = require('./completion').completion(cliOptions) + break + } + runPromise = Promise.resolve(fulfillmentValue) + } catch (ex) { + runPromise = Promise.reject(ex) } } + + // Always return a promise for ease of testing. We want to know when it is + // safe to start making assertions. Without the promise, it would be difficult + // to test the asynchronous branches. + return runPromise } // just for testing From 8dd8d2c3371ceed01ee26bf8672427408bf89706 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Mon, 15 Mar 2021 17:04:59 -0400 Subject: [PATCH 22/29] chore: test CLI `run()` commands --- test/unit/cli.spec.js | 249 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 249 insertions(+) diff --git a/test/unit/cli.spec.js b/test/unit/cli.spec.js index 222e92c04..e59711288 100644 --- a/test/unit/cli.spec.js +++ b/test/unit/cli.spec.js @@ -2,6 +2,7 @@ const path = require('path') const mocks = require('mocks') +const proxyquire = require('proxyquire') const cli = require('../../lib/cli') const constant = require('../../lib/constants') @@ -206,4 +207,252 @@ describe('cli', () => { expect(args).to.deep.equal(['aa', '--bb', 'value']) }) }) + + describe('run', () => { + const COMMAND_COMPLETION = 'completion' + const COMMAND_INIT = 'init' + const COMMAND_RUN = 'run' + const COMMAND_START = 'start' + const COMMAND_STOP = 'stop' + const consoleErrorOriginal = console.error + const processExitOriginal = process.exit + let cliModule + let cliProcessFake = null + let completionFake = null + let initFake = null + let parseConfigFake = null + let runEmitterFake = null + let runFake = null + let ServerFake = null + let serverStartFake = null + let stopFake = null + let testCommand = null + let forceConfigFailure = false + + // `cliProcessFake` is used in multiple scopes, but not needed by the top + // scope. By using a factory, we can maintain one copy of the code in a + // single location while still having access to scopped variables that we + // need. + function createCliProcessFake () { + return sinon.fake(function cliProcessFake () { + const cliOptions = {} + if ( + testCommand === COMMAND_COMPLETION || + testCommand === COMMAND_INIT || + testCommand === COMMAND_RUN || + testCommand === COMMAND_START || + testCommand === COMMAND_STOP + ) { + cliOptions.cmd = testCommand + } else { + const errorMessage = + 'cli.spec.js: A valid command must be provided when testing the' + + 'exported `run()` method.' + throw new Error(errorMessage) + } + if (forceConfigFailure === true) { + cliOptions.forceConfigFailure = true + } + return cliOptions + }) + } + + before(() => { + proxyquire.noPreserveCache() + }) + + beforeEach(() => { + // Keep the test output clean + console.error = sinon.spy() + + // Keep the process from actually exiting + process.exit = sinon.spy() + + completionFake = sinon.fake() + initFake = sinon.fake() + parseConfigFake = sinon.fake(function parseConfigFake () { + const cliOptions = arguments[1] + + // Allow individual tests to test against success and failure without + // needing to manage multiple sinon fakes. + const forceConfigFailure = cliOptions && cliOptions.forceConfigFailure === true + if (forceConfigFailure) { + // No need to mock out the synchronous API, the CLI is not intended to + // use it + return Promise.reject(new Error('Intentional Failure For Testing')) + } + + // Most of our tests will ignore the actual config as the CLI passes it + // on to other methods that are tested elsewhere + const karmaConfig = { + ...cliOptions, + isFakeParsedConfig: true + } + return Promise.resolve(karmaConfig) + }) + runEmitterFake = {} + runEmitterFake.on = sinon.fake.returns(runEmitterFake) + runFake = sinon.fake.returns(runEmitterFake) + serverStartFake = sinon.fake.resolves() + ServerFake = sinon.fake.returns({ start: serverStartFake }) + stopFake = sinon.fake() + cliModule = proxyquire( + '../../lib/cli', + { + './completion': { + completion: completionFake + }, + './config': { + parseConfig: parseConfigFake + }, + './init': { + init: initFake + }, + './runner': { + run: runFake + }, + './server': ServerFake, + './stopper': { + stop: stopFake + } + } + ) + }) + + afterEach(() => { + // Restore globals, simultaneously removing references to the spies. + console.error = consoleErrorOriginal + process.exit = processExitOriginal + + // Reset the test command + testCommand = null + + // Most tests won't be testing what happens during a configuration failure + // Here we clean up after the ones that do. + forceConfigFailure = false + + // Restores all replaced properties set by sinon methods (`replace`, + // `spy`, and `stub`) + sinon.restore() + + // Remove references to Fakes that were not handled above. Avoids `before` + // and `beforeEach` aside effects and references not getting cleaned up + // after the last test. + cliModule = null + cliProcessFake = null + completionFake = null + initFake = null + parseConfigFake = null + runEmitterFake = null + runFake = null + ServerFake = null + serverStartFake = null + stopFake = null + }) + + after(() => { + proxyquire.preserveCache() + }) + + describe('commands', () => { + let cliProcessOriginal + beforeEach(() => { + cliProcessFake = createCliProcessFake() + cliProcessOriginal = cliModule.process + cliModule.process = cliProcessFake + }) + afterEach(() => { + if (cliModule) { + cliModule.process = cliProcessOriginal + } + }) + describe(COMMAND_COMPLETION, () => { + beforeEach(() => { + testCommand = COMMAND_COMPLETION + }) + it('should configure and call the completion method of the completion module', async () => { + await cliModule.run() + expect(completionFake.calledOnce).to.be.true + expect(completionFake.firstCall.args[0]).to.eql({ + cmd: COMMAND_COMPLETION + }) + }) + }) + describe(COMMAND_INIT, () => { + beforeEach(() => { + testCommand = COMMAND_INIT + }) + it('should configure and call the init method of the init module', async () => { + await cliModule.run() + expect(initFake.calledOnce).to.be.true + expect(initFake.firstCall.args[0]).to.eql({ + cmd: COMMAND_INIT + }) + }) + }) + describe(COMMAND_RUN, () => { + beforeEach(() => { + testCommand = COMMAND_RUN + }) + it('should configure and call the run method of the runner module', async () => { + await cliModule.run() + expect(runFake.calledOnce).to.be.true + expect(runFake.firstCall.args[0]).to.eql({ + cmd: COMMAND_RUN, + isFakeParsedConfig: true + }) + expect(runEmitterFake.on.calledOnce).to.be.true + expect(runEmitterFake.on.firstCall.args[0]).to.be.equal('progress') + }) + }) + describe(COMMAND_START, () => { + beforeEach(() => { + testCommand = COMMAND_START + }) + it('should configure and start the server', async () => { + await cliModule.run() + expect(ServerFake.calledOnce).to.be.true + expect(ServerFake.firstCall.args[0]).to.eql({ + cmd: COMMAND_START, + isFakeParsedConfig: true + }) + expect(serverStartFake.calledOnce).to.be.true + }) + }) + describe(COMMAND_STOP, () => { + beforeEach(() => { + testCommand = COMMAND_STOP + }) + it('should configure and call the stop method of the stopper module', async () => { + await cliModule.run() + expect(stopFake.calledOnce).to.be.true + expect(stopFake.firstCall.args[0]).to.eql({ + cmd: COMMAND_STOP, + isFakeParsedConfig: true + }) + }) + }) + }) + describe('configuration failure', () => { + let cliProcessOriginal + beforeEach(() => { + forceConfigFailure = true + testCommand = COMMAND_START + + cliProcessFake = createCliProcessFake() + cliProcessOriginal = cliModule.process + cliModule.process = cliProcessFake + }) + afterEach(() => { + if (cliModule) { + cliModule.process = cliProcessOriginal + } + }) + it('should exit the process with a non-zero exit code when configuration parsing fails', async () => { + await cliModule.run() + expect(process.exit.calledOnce).to.be.true + expect(process.exit.firstCall.args[0]).not.to.be.equal(0) + }) + }) + }) }) From 5a3ad45a6e3515b3014e6cbdaf978fe535a6b442 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Tue, 16 Mar 2021 20:27:01 -0400 Subject: [PATCH 23/29] chore: test that server logs deprecation warning --- test/unit/server.spec.js | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/test/unit/server.spec.js b/test/unit/server.spec.js index 6523a709d..fceaebe4c 100644 --- a/test/unit/server.spec.js +++ b/test/unit/server.spec.js @@ -2,6 +2,7 @@ const Server = require('../../lib/server') const NetUtils = require('../../lib/utils/net-utils') const BrowserCollection = require('../../lib/browser_collection') const Browser = require('../../lib/browser') +const cfg = require('../../lib/config') const logger = require('../../lib/logger') describe('server', () => { @@ -16,7 +17,9 @@ describe('server', () => { let mockBoundServer let mockExecutor let doneStub + let log let logErrorSpy + let logWarnStub let server = mockConfig = browserCollection = webServerOnError = null let fileListOnResolve = fileListOnReject = mockLauncher = null let mockFileList = mockWebServer = mockSocketServer = mockExecutor = doneStub = null @@ -28,7 +31,9 @@ describe('server', () => { this.timeout(4000) browserCollection = new BrowserCollection() doneStub = sinon.stub() - logErrorSpy = sinon.spy(logger.create('karma-server'), 'error') + log = logger.create('karma-server') + logErrorSpy = sinon.spy(log, 'error') + logWarnStub = sinon.stub(log, 'warn') fileListOnResolve = fileListOnReject = null @@ -46,7 +51,6 @@ describe('server', () => { browserDisconnectTolerance: 0, browserNoActivityTimeout: 0 } - server = new Server(mockConfig, doneStub) sinon.stub(server._injector, 'invoke').returns([]) @@ -126,6 +130,37 @@ describe('server', () => { webServerOnError = null }) + afterEach(() => { + logWarnStub.restore() + }) + + describe('constructor', () => { + it('should log a warning when the first argument is not an instance of Config', async () => { + // Reset the spy interface on the stub. It may have already been called by + // code in the `before` or `beforeEach` hooks. + logWarnStub.resetHistory() + + const rawConfig = { + karmaConfigForTest: true + } + return cfg.parseConfig( + null, + rawConfig, + { promiseConfig: true, throwErrors: true } + ).then((parsedConfig) => { + const messageSubstring = + 'Passing raw CLI options to `new Server(config, done)` is ' + + 'deprecated.' + + const serverWithParsed = new Server(parsedConfig, doneStub) // eslint-disable-line no-unused-vars + expect(logWarnStub).to.not.have.been.calledWith(sinon.match(messageSubstring)) + + const serverWithRaw = new Server(rawConfig, doneStub) // eslint-disable-line no-unused-vars + expect(logWarnStub).to.have.been.calledOnceWith(sinon.match(messageSubstring)) + }) + }) + }) + describe('start', () => { let config beforeEach(() => { From 212e791ff8061bc61e7a2115d6a36ff96f58f663 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Tue, 16 Mar 2021 20:37:08 -0400 Subject: [PATCH 24/29] chore: initial tests for async parseConfig --- test/unit/config.spec.js | 168 +++++++++++++++++++++++++++++++++++---- 1 file changed, 151 insertions(+), 17 deletions(-) diff --git a/test/unit/config.spec.js b/test/unit/config.spec.js index d115a2b6e..773813a9a 100644 --- a/test/unit/config.spec.js +++ b/test/unit/config.spec.js @@ -5,6 +5,24 @@ const loadFile = require('mocks').loadFile const helper = require('../../lib/helper') const logger = require('../../lib/logger.js') +/** + * If an object is "thenable", then it is considered to be a promise + * implementation. It does not have to be an instance of ECMAScript's own + * `Promise` class to be considered a promise. + * + * @param {*} value + * @returns {boolean} + * + * @see {@link https://promisesaplus.com/} + */ +function isPromiseLike (value) { + const valueType = typeof value + return ( + ((value != null && valueType === 'object') || valueType === 'function') && + typeof value.then === 'function' + ) +} + describe('config', () => { let m let e @@ -30,6 +48,21 @@ describe('config', () => { const wrapCfg = function (cfg) { return (config) => config.set(cfg) } + function wrapCfgResolvedPromise (cfg) { + return (config) => { + return new Promise((resolve, reject) => { + const delayMs = 50 + setTimeout(() => { + try { + config.set(cfg) + resolve() + } catch (configSetException) { + reject(configSetException) + } + }, delayMs) + }) + } + } beforeEach(() => { mocks = {} @@ -52,7 +85,12 @@ describe('config', () => { '/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'] }), '/conf/coffee.coffee': wrapCfg({ files: ['one.js', 'two.js'] }), - '/conf/default-export.js': { default: wrapCfg({ files: ['one.js', 'two.js'] }) } + '/conf/default-export.js': { default: wrapCfg({ files: ['one.js', 'two.js'] }) }, + '/conf/default-config': function noOperations () {}, + '/conf/returns-promise-that-resolves.js': wrapCfgResolvedPromise({ foo: 'bar' }), + '/conf/returns-promise-that-rejects.js': () => { + return Promise.reject(new Error('Unexpected Error')) + } } // load file under test @@ -75,10 +113,22 @@ describe('config', () => { }) describe('parseConfig', () => { - let logSpy + let logErrorStub + let logWarnStub + let processExitStub beforeEach(() => { - logSpy = sinon.spy(logger.create('config'), 'error') + const log = logger.create('config') + // Silence and monitor logged errors and warnings, regardless of the + // `logLevel` option. + logErrorStub = sinon.stub(log, 'error') + logWarnStub = sinon.stub(log, 'warn') + processExitStub = sinon.stub(process, 'exit') + }) + afterEach(() => { + logErrorStub.restore() + logWarnStub.restore() + processExitStub.restore() }) it('should resolve relative basePath to config directory', () => { @@ -116,24 +166,24 @@ describe('config', () => { expect(config.exclude).to.deep.equal(actual) }) - it('should log error and exit if file does not exist', () => { + it('should log an error and exit if file does not exist', () => { e.parseConfig('/conf/not-exist.js', {}) - expect(logSpy).to.have.been.called - const event = logSpy.lastCall.args + expect(logErrorStub).to.have.been.called + const event = logErrorStub.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).to.have.been.calledWith(1) }) - it('should log error and throw if file does not exist AND throwErrors is true', () => { + it('should log an 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(logErrorStub).to.have.been.called + const event = logErrorStub.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 @@ -142,8 +192,8 @@ describe('config', () => { it('should log an error and exit if invalid file', () => { e.parseConfig('/conf/invalid.js', {}) - expect(logSpy).to.have.been.called - const event = logSpy.lastCall.args + expect(logErrorStub).to.have.been.called + const event = logErrorStub.lastCall.args expect(event[0]).to.eql('Error in config file!\n') expect(event[1].message).to.eql('Unexpected token =') expect(mocks.process.exit).to.have.been.calledWith(1) @@ -155,8 +205,8 @@ describe('config', () => { } 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(logErrorStub).to.have.been.called + const event = logErrorStub.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 @@ -168,13 +218,97 @@ describe('config', () => { } expect(parseConfig).to.throw(Error, 'Config file must export a function!\n') - expect(logSpy).to.have.been.called - const event = logSpy.lastCall.args + expect(logErrorStub).to.have.been.called + const event = logErrorStub.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 fail when the config file\'s function returns a promise, but `parseOptions.promiseConfig` is not true', () => { + function parseConfig () { + e.parseConfig( + '/conf/returns-promise-that-resolves.js', {}, { throwErrors: true } + ) + } + const expectedErrorMessage = + 'The `parseOptions.promiseConfig` option must be set to `true` to ' + + 'enable promise return values from configuration files. ' + + 'Example: `parseConfig(path, cliOptions, { promiseConfig: true })`' + + expect(parseConfig).to.throw(Error, expectedErrorMessage) + expect(logErrorStub).to.have.been.called + const event = logErrorStub.lastCall.args + expect(event[0]).to.be.eql(expectedErrorMessage) + expect(mocks.process.exit).not.to.have.been.called + }) + + describe('when `parseOptions.promiseConfig` is true', () => { + it('should return a promise when promiseConfig is true', () => { + // Return value should always be a promise, regardless of whether or not + // the config file itself is synchronous or asynchronous and when no + // config file path is provided at all. + const noConfigFilePromise = e.parseConfig( + null, null, { promiseConfig: true } + ) + const syncConfigPromise = e.parseConfig( + '/conf/default-config', null, { promiseConfig: true } + ) + const asyncConfigPromise = e.parseConfig( + '/conf/returns-promise-that-resolves.js', + null, + { promiseConfig: true } + ) + + expect( + isPromiseLike(noConfigFilePromise), + 'Expected parseConfig to return a promise when no config file path is provided.' + ).to.be.true + expect( + isPromiseLike(syncConfigPromise), + 'Expected parseConfig to return a promise when the config file DOES NOT return a promise.' + ).to.be.true + expect( + isPromiseLike(asyncConfigPromise), + 'Expected parseConfig to return a promise when the config file returns a promise.' + ).to.be.true + }) + + it('should log an error and exit if invalid file', () => { + e.parseConfig('/conf/invalid.js', {}, { promiseConfig: true }) + + expect(logErrorStub).to.have.been.called + const event = logErrorStub.lastCall.args + expect(event[0]).to.eql('Error in config file!\n') + expect(event[1].message).to.eql('Unexpected token =') + expect(mocks.process.exit).to.have.been.calledWith(1) + }) + + it('should log an error and reject the promise if the config file rejects the promise returned by its function AND throwErrors is true', async () => { + const configThatRejects = e.parseConfig('/conf/returns-promise-that-rejects.js', {}, { promiseConfig: true, throwErrors: true }).catch((reason) => { + expect(logErrorStub).to.have.been.called + const event = logErrorStub.lastCall.args + expect(event[0]).to.eql('Error in config file!\n') + expect(event[1].message).to.eql('Unexpected Error') + expect(reason.message).to.eql('Error in config file!\n Error: Unexpected Error') + expect(reason).to.be.an.instanceof(Error) + }) + return configThatRejects + }) + + it('should log an error and reject the promise if invalid file AND throwErrors is true', async () => { + const configThatThrows = e.parseConfig('/conf/invalid.js', {}, { promiseConfig: true, throwErrors: true }).catch((reason) => { + expect(logErrorStub).to.have.been.called + const event = logErrorStub.lastCall.args + expect(event[0]).to.eql('Error in config file!\n') + expect(event[1].message).to.eql('Unexpected token =') + expect(reason.message).to.eql('Error in config file!\n SyntaxError: Unexpected token =') + expect(reason).to.be.an.instanceof(Error) + }) + return configThatThrows + }) + }) + it('should override config with given cli options', () => { const config = e.parseConfig('/home/config4.js', { port: 456, autoWatch: false }) @@ -288,7 +422,7 @@ describe('config', () => { it('should not read config file, when null', () => { const config = e.parseConfig(null, { basePath: '/some' }) - expect(logSpy).not.to.have.been.called + expect(logErrorStub).not.to.have.been.called expect(config.basePath).to.equal(resolveWinPath('/some')) // overridden by CLI expect(config.urlRoot).to.equal('/') }) // default value @@ -296,7 +430,7 @@ describe('config', () => { it('should not read config file, when null but still resolve cli basePath', () => { const config = e.parseConfig(null, { basePath: './some' }) - expect(logSpy).not.to.have.been.called + expect(logErrorStub).not.to.have.been.called expect(config.basePath).to.equal(resolveWinPath('./some')) expect(config.urlRoot).to.equal('/') }) // default value From a5477950db03b7b292b186f8e78df4a37ab044e9 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Tue, 16 Mar 2021 20:38:20 -0400 Subject: [PATCH 25/29] fix: isThenable criteria should be consistent with aPlus spec --- lib/config.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/config.js b/lib/config.js index 9384aed0b..4350ee05d 100644 --- a/lib/config.js +++ b/lib/config.js @@ -454,7 +454,19 @@ function parseConfig (configFilePath, cliOptions, parseOptions) { return normalizeConfig(config, configFilePath) } - const returnIsThenable = configModuleReturn != null && typeof configModuleReturn === 'object' && typeof configModuleReturn.then === 'function' + + /** + * Return value is a function or (non-null) object that has a `then` method. + * + * @type {boolean} + * @see {@link https://promisesaplus.com/} + */ + const returnIsThenable = ( + ( + (configModuleReturn != null && typeof configModuleReturn === 'object') || + typeof configModuleReturn === 'function' + ) && typeof configModuleReturn.then === 'function' + ) if (returnIsThenable) { if (promiseConfig !== true) { const errorMessage = From b5ddd20d27e358f43c88093f97c7f8300d7bef1d Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Tue, 16 Mar 2021 20:44:57 -0400 Subject: [PATCH 26/29] chore: minor IDE support via JSDoc block --- lib/config.js | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/lib/config.js b/lib/config.js index 4350ee05d..cd9510bfd 100644 --- a/lib/config.js +++ b/lib/config.js @@ -268,6 +268,9 @@ function normalizeConfig (config, configFilePath) { return config } +/** + * @class + */ class Config { constructor () { this.LOG_DISABLE = constant.LOG_DISABLE @@ -351,6 +354,42 @@ const CONFIG_SYNTAX_HELP = ' module.exports = function(config) {\n' + ' });\n' + ' };\n' +/** + * Retrieve a parsed and finalized Karma `Config` instance. This `karmaConfig` + * object may be used to configure public API methods such a `Server`, + * `runner.run`, and `stopper.stop`. + * + * @param {?string} [configFilePath=null] + * A string representing a file system path pointing to the config file + * whose default export is a function that will be used to set Karma + * configuration options. This function will be passed an instance of the + * `Config` class as its first argument. If this option is not provided, + * then only the options provided by the `cliOptions` argument will be + * set. + * @param {Object} cliOptions + * An object whose values will take priority over options set in the + * config file. The config object passed to function exported by the + * config file will already have these options applied. Any changes the + * config file makes to these options will effectively be ignored in the + * final configuration. + * + * `cliOptions` all the same options as the config file and is applied + * using the same `config.set()` method. + * @param {Object} parseOptions + * @param {boolean} [parseOptions.promiseConfig=false] + * When `true`, a promise that resolves to a `Config` object will be + * returned. This also allows the function exported by config files (if + * provided) to be asynchronous by returning a promise. Resolving this + * promise indicates that all async activity has completed. The resolution + * value itself is ignored, all configuration must be done with + * `config.set`. + * @param {boolean} [parseOptions.throwErrors=false] + * When `true`, process exiting on critical failures will be disabled. In + * The error will be thrown as an exception. If + * `parseOptions.promiseConfig` is also `true`, then the error will + * instead be used as the promise's reject reason. + * @returns {Config|Promise} + */ function parseConfig (configFilePath, cliOptions, parseOptions) { const promiseConfig = parseOptions && parseOptions.promiseConfig === true const throwErrors = parseOptions && parseOptions.throwErrors === true From 027e4b0cf61a4027f59a1a36e58754f60e5a913c Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Mon, 22 Mar 2021 13:24:16 -0400 Subject: [PATCH 27/29] chore: simplify async test wrapper --- test/unit/config.spec.js | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/test/unit/config.spec.js b/test/unit/config.spec.js index 773813a9a..a248e5db4 100644 --- a/test/unit/config.spec.js +++ b/test/unit/config.spec.js @@ -48,20 +48,8 @@ describe('config', () => { const wrapCfg = function (cfg) { return (config) => config.set(cfg) } - function wrapCfgResolvedPromise (cfg) { - return (config) => { - return new Promise((resolve, reject) => { - const delayMs = 50 - setTimeout(() => { - try { - config.set(cfg) - resolve() - } catch (configSetException) { - reject(configSetException) - } - }, delayMs) - }) - } + const wrapAsyncCfg = function (cfg) { + return async (config) => config.set(cfg) } beforeEach(() => { @@ -87,7 +75,7 @@ describe('config', () => { '/conf/coffee.coffee': wrapCfg({ files: ['one.js', 'two.js'] }), '/conf/default-export.js': { default: wrapCfg({ files: ['one.js', 'two.js'] }) }, '/conf/default-config': function noOperations () {}, - '/conf/returns-promise-that-resolves.js': wrapCfgResolvedPromise({ foo: 'bar' }), + '/conf/returns-promise-that-resolves.js': wrapAsyncCfg({ foo: 'bar' }), '/conf/returns-promise-that-rejects.js': () => { return Promise.reject(new Error('Unexpected Error')) } From e4b1918f7480e2fb0c92d3aa273fcc90ea33d598 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Mon, 22 Mar 2021 13:26:01 -0400 Subject: [PATCH 28/29] chore: reduce async noise in `run` --- lib/cli.js | 81 ++++++++++++++++++++++-------------------------------- 1 file changed, 33 insertions(+), 48 deletions(-) diff --git a/lib/cli.js b/lib/cli.js index 95f42b491..41950abd4 100644 --- a/lib/cli.js +++ b/lib/cli.js @@ -278,66 +278,51 @@ exports.process = () => { return processArgs(argv, { cmd: argv._.shift() }, fs, path) } -exports.run = () => { +exports.run = async () => { const cliOptions = exports.process() const cmd = cliOptions.cmd // prevent config from changing the command const cmdNeedsConfig = cmd === 'start' || cmd === 'run' || cmd === 'stop' - let runPromise if (cmdNeedsConfig) { - runPromise = cfg.parseConfig( - cliOptions.configFile, - cliOptions, - { - promiseConfig: true, - throwErrors: true - } - ).then(function onKarmaConfigFulfilled (config) { - let fulfillmentValue - switch (cmd) { - case 'start': { - const server = new Server(config) - fulfillmentValue = server.start().then(() => server) - break + let config + try { + config = await cfg.parseConfig( + cliOptions.configFile, + cliOptions, + { + promiseConfig: true, + throwErrors: true } - case 'run': - fulfillmentValue = require('./runner') - .run(config) - .on('progress', printRunnerProgress) - break - case 'stop': - fulfillmentValue = require('./stopper').stop(config) - break - } - return fulfillmentValue - }, function onKarmaConfigRejected (/* ignoredReason */) { - // The reject reason isn't used to log a message since parseConfig already - // calls a configured logger method with an almost identical message. + ) + } catch (karmaConfigException) { + // The reject reason/exception isn't used to log a message since + // parseConfig already calls a configured logger method with an almost + // identical message. // The `run` function is a private application, not a public API. We don't // need to worry about process.exit vs throw vs promise rejection here. process.exit(1) - }) - } else { - try { - let fulfillmentValue - switch (cmd) { - case 'init': - fulfillmentValue = require('./init').init(cliOptions) - break - case 'completion': - fulfillmentValue = require('./completion').completion(cliOptions) - break + } + switch (cmd) { + case 'start': { + const server = new Server(config) + await server.start() + return server } - runPromise = Promise.resolve(fulfillmentValue) - } catch (ex) { - runPromise = Promise.reject(ex) + case 'run': + return require('./runner') + .run(config) + .on('progress', printRunnerProgress) + case 'stop': + return require('./stopper').stop(config) + } + } else { + switch (cmd) { + case 'init': + return require('./init').init(cliOptions) + case 'completion': + return require('./completion').completion(cliOptions) } } - - // Always return a promise for ease of testing. We want to know when it is - // safe to start making assertions. Without the promise, it would be difficult - // to test the asynchronous branches. - return runPromise } // just for testing From d44b5df858c5d0c0fbe3ff71646f1c8a7d20edc2 Mon Sep 17 00:00:00 2001 From: Nicholas Petruzzelli Date: Mon, 22 Mar 2021 13:47:54 -0400 Subject: [PATCH 29/29] chore: check `instanceof` for Promise object in tests --- test/unit/config.spec.js | 37 ++++++++++--------------------------- 1 file changed, 10 insertions(+), 27 deletions(-) diff --git a/test/unit/config.spec.js b/test/unit/config.spec.js index a248e5db4..ddab67910 100644 --- a/test/unit/config.spec.js +++ b/test/unit/config.spec.js @@ -5,24 +5,6 @@ const loadFile = require('mocks').loadFile const helper = require('../../lib/helper') const logger = require('../../lib/logger.js') -/** - * If an object is "thenable", then it is considered to be a promise - * implementation. It does not have to be an instance of ECMAScript's own - * `Promise` class to be considered a promise. - * - * @param {*} value - * @returns {boolean} - * - * @see {@link https://promisesaplus.com/} - */ -function isPromiseLike (value) { - const valueType = typeof value - return ( - ((value != null && valueType === 'object') || valueType === 'function') && - typeof value.then === 'function' - ) -} - describe('config', () => { let m let e @@ -86,6 +68,7 @@ describe('config', () => { global: {}, process: mocks.process, Error: Error, // Without this, chai's `.throw()` assertion won't correctly check against constructors. + Promise: Promise, require (path) { if (mockConfigs[path]) { return mockConfigs[path] @@ -248,18 +231,18 @@ describe('config', () => { { promiseConfig: true } ) - expect( - isPromiseLike(noConfigFilePromise), + expect(noConfigFilePromise).to.be.an.instanceof( + Promise, 'Expected parseConfig to return a promise when no config file path is provided.' - ).to.be.true - expect( - isPromiseLike(syncConfigPromise), + ) + expect(syncConfigPromise).to.be.an.instanceof( + Promise, 'Expected parseConfig to return a promise when the config file DOES NOT return a promise.' - ).to.be.true - expect( - isPromiseLike(asyncConfigPromise), + ) + expect(asyncConfigPromise).to.be.an.instanceof( + Promise, 'Expected parseConfig to return a promise when the config file returns a promise.' - ).to.be.true + ) }) it('should log an error and exit if invalid file', () => {