From 3d5daf9da37ad0ca62a61c4a115c644a96a22517 Mon Sep 17 00:00:00 2001 From: Nick Petruzzelli Date: Tue, 23 Mar 2021 16:25:46 -0400 Subject: [PATCH] feat: support asynchronous `config.set()` call in karma.conf.js (#3660) The existing sync behavior co-exists with the new async behavior. * add promise support to `parseConfig` * add async config support to `cli`, `runner`, and `stopper` * Additional API for `Server()` accepting parsed config. Older API is deprecated. * update documentation for parseConfig * add warning for deprecated use of CLI options * update Server constructor, runner, and stopper docs --- docs/dev/04-public-api.md | 289 ++++++++++++++++++++++++++++++++++---- lib/cli.js | 66 ++++++--- lib/config.js | 140 +++++++++++++++--- lib/runner.js | 37 ++++- lib/server.js | 43 ++++-- lib/stopper.js | 36 ++++- test/unit/cli.spec.js | 249 ++++++++++++++++++++++++++++++++ test/unit/config.spec.js | 139 +++++++++++++++--- test/unit/server.spec.js | 39 ++++- 9 files changed, 923 insertions(+), 115 deletions(-) diff --git a/docs/dev/04-public-api.md b/docs/dev/04-public-api.md index ff327a41a..717eb1855 100644 --- a/docs/dev/04-public-api.md +++ b/docs/dev/04-public-api.md @@ -4,7 +4,18 @@ You can, however, call Karma programmatically from your node module. Here is the ## karma.Server(options, [callback=process.exit]) -### Constructor +### `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 @@ -15,9 +26,29 @@ 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()** +### `server.start()` Equivalent of `karma start`. @@ -25,7 +56,7 @@ Equivalent of `karma start`. server.start() ``` -### **server.refreshFiles()** +### `server.refreshFiles()` Trigger a file list refresh. Returns a promise. @@ -33,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. @@ -117,10 +148,19 @@ 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` 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) { @@ -129,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. @@ -142,9 +211,17 @@ 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`. + +#### Usage + +##### Deprecated Behavior -This function will signal a running server to stop. The equivalent of `karma stop`. +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 @@ -156,78 +233,228 @@ stopper.stop({port: 9876}, function(exitCode) { }) ``` -## karma.config.parseConfig([configFilePath], [cliOptions]) +##### 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])` 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 + +##### 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 } ); +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 */ } +); ``` -## karma.constants -### **constants.VERSION** +#### `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` 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 - -## Callback function notes - -- If there is an error, the error code will be provided as the second parameter to the error callback. diff --git a/lib/cli.js b/lib/cli.js index 6e7fdac4f..41950abd4 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) { @@ -277,27 +278,50 @@ exports.process = () => { return processArgs(argv, { cmd: argv._.shift() }, fs, path) } -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 +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' + if (cmdNeedsConfig) { + let config + try { + config = await cfg.parseConfig( + cliOptions.configFile, + cliOptions, + { + promiseConfig: true, + throwErrors: true + } + ) + } 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) + } + switch (cmd) { + case 'start': { + const server = new Server(config) + await server.start() + return server + } + 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) + } } } diff --git a/lib/config.js b/lib/config.js index 1192afbac..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,12 +354,61 @@ 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 + 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 (parseOptions && parseOptions.throwErrors === true) { + if (throwErrors) { 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)`' + @@ -411,34 +463,76 @@ 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 - } - - // 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.') + } + // restore values that weren't overwritten by the user + if (config.hostname === null) { + config.hostname = defaultHostname + } + if (config.listenAddress === null) { + config.listenAddress = defaultListenAddress + } - return normalizeConfig(config, configFilePath) + // configure the logger as soon as we can + logger.setup(config.logLevel, config.colors, config.loggers) + + log.debug(configFilePath ? `Loading config ${configFilePath}` : 'No config file specified.') + + return normalizeConfig(config, configFilePath) + } + + /** + * Return 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 = + 'The `parseOptions.promiseConfig` option must be set to `true` to ' + + 'enable promise return values from configuration files. ' + + 'Example: `parseConfig(path, cliOptions, { promiseConfig: true })`' + return fail(errorMessage) + } + return configModuleReturn.then( + function onKarmaConfigModuleFulfilled (/* ignoredResolutionValue */) { + return finalizeConfig(config) + }, + function onKarmaConfigModuleRejected (reason) { + return fail('Error in config file!\n', reason) + } + ) + } 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/runner.js b/lib/runner.js index e98a39da1..7d07bb08b 100644 --- a/lib/runner.js +++ b/lib/runner.js @@ -32,14 +32,39 @@ function parseExitCode (buffer, defaultExitCode, failOnEmptyTestSuite) { } // TODO(vojta): read config file (port, host, urlRoot) -function run (config, done) { - config = config || {} - - logger.setupFromConfig(config) - +function run (cliOptionsOrConfig, done) { + cliOptionsOrConfig = cliOptionsOrConfig || {} done = helper.isFunction(done) ? done : process.exit - config = cfg.parseConfig(config.configFile, config) + let config + if (cliOptionsOrConfig instanceof cfg.Config) { + config = cliOptionsOrConfig + } else { + 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})` ' + + 'to prepare a processed `Config` instance and pass that as the ' + + '`config` argument instead.' + log.warn(deprecatedCliOptionsMessage) + 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 a6ae81dab..cec1ecd0b 100644 --- a/lib/server.js +++ b/lib/server.js @@ -55,22 +55,43 @@ function createSocketIoServer (webServer, executor, config) { } class Server extends KarmaEventEmitter { - constructor (cliOptions, done) { + constructor (cliOptionsOrConfig, done) { super() - logger.setupFromConfig(cliOptions) - + cliOptionsOrConfig = cliOptionsOrConfig || {} this.log = logger.create('karma-server') - + done = helper.isFunction(done) ? done : process.exit 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 { + logger.setupFromConfig({ + colors: cliOptionsOrConfig.colors, + logLevel: cliOptionsOrConfig.logLevel + }) + 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, + 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) + return + } } this.log.debug('Final config', util.inspect(config, false, /** depth **/ null)) diff --git a/lib/stopper.js b/lib/stopper.js index 5e87be595..964eb5e37 100644 --- a/lib/stopper.js +++ b/lib/stopper.js @@ -3,12 +3,40 @@ 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 || {} const log = logger.create('stopper') done = helper.isFunction(done) ? done : process.exit - config = cfg.parseConfig(config.configFile, config) + + let config + 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})` ' + + 'to prepare a processed `Config` instance and pass that as the ' + + '`config` argument instead.' + log.warn(deprecatedCliOptionsMessage) + 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) + } + } const request = http.request({ hostname: config.hostname, 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) + }) + }) + }) }) diff --git a/test/unit/config.spec.js b/test/unit/config.spec.js index d115a2b6e..ddab67910 100644 --- a/test/unit/config.spec.js +++ b/test/unit/config.spec.js @@ -30,6 +30,9 @@ describe('config', () => { const wrapCfg = function (cfg) { return (config) => config.set(cfg) } + const wrapAsyncCfg = function (cfg) { + return async (config) => config.set(cfg) + } beforeEach(() => { mocks = {} @@ -52,7 +55,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': wrapAsyncCfg({ foo: 'bar' }), + '/conf/returns-promise-that-rejects.js': () => { + return Promise.reject(new Error('Unexpected Error')) + } } // load file under test @@ -60,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] @@ -75,10 +84,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 +137,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 +163,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 +176,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 +189,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(noConfigFilePromise).to.be.an.instanceof( + Promise, + 'Expected parseConfig to return a promise when no config file path is provided.' + ) + expect(syncConfigPromise).to.be.an.instanceof( + Promise, + 'Expected parseConfig to return a promise when the config file DOES NOT return a promise.' + ) + expect(asyncConfigPromise).to.be.an.instanceof( + Promise, + 'Expected parseConfig to return a promise when the config file returns a promise.' + ) + }) + + 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 +393,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 +401,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 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(() => {