From 650d7e3022cf6315b4bbdf5ea84c737350874511 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Thu, 6 Nov 2014 19:31:06 -0800 Subject: [PATCH] feat: Fail on launcher-, reporter-, plugin-, or preprocessor-load errors. Stop karma if a launcher, reporter, plugin, or preprocessor fails to load, after attempting to load each of them, and reporting errors for each load error. Closes #855 --- lib/launcher.js | 12 +++-- lib/plugin.js | 38 +++++++------ lib/preprocessor.js | 67 ++++++++++++----------- lib/reporter.js | 5 +- lib/server.js | 20 ++++++- test/e2e/load.feature | 106 +++++++++++++++++++++++++++++++++++++ test/unit/launcher.spec.js | 17 +++++- 7 files changed, 205 insertions(+), 60 deletions(-) create mode 100644 test/e2e/load.feature diff --git a/lib/launcher.js b/lib/launcher.js index 988133176..42b1a9a77 100644 --- a/lib/launcher.js +++ b/lib/launcher.js @@ -11,7 +11,7 @@ var processDecorator = require('./launchers/process').decoratorFactory // TODO(vojta): remove once nobody uses it var baseBrowserDecoratorFactory = function (baseLauncherDecorator, captureTimeoutLauncherDecorator, - retryLauncherDecorator, processLauncherDecorator) { + retryLauncherDecorator, processLauncherDecorator) { return function (launcher) { baseLauncherDecorator(launcher) captureTimeoutLauncherDecorator(launcher) @@ -20,7 +20,7 @@ var baseBrowserDecoratorFactory = function (baseLauncherDecorator, captureTimeou } } -var Launcher = function (emitter, injector) { +var Launcher = function (server, emitter, injector) { var browsers = [] var lastStartTime @@ -61,15 +61,17 @@ var Launcher = function (emitter, injector) { var browser = injector.createChild([locals], ['launcher:' + name]).get('launcher:' + name) } catch (e) { if (e.message.indexOf('No provider for "launcher:' + name + '"') !== -1) { - log.warn('Can not load "%s", it is not registered!\n ' + + log.error('Cannot load browser "%s": it is not registered! ' + 'Perhaps you are missing some plugin?', name) } else { - log.warn('Can not load "%s"!\n ' + e.stack, name) + log.error('Cannot load browser "%s"!\n ' + e.stack, name) } + emitter.emit('load_error', 'launcher', name) return } + if (server.loadErrors.length > 0) return [] // TODO(vojta): remove in v1.0 (BC for old launchers) if (!browser.forceKill) { browser.forceKill = function () { @@ -193,7 +195,7 @@ var Launcher = function (emitter, injector) { emitter.on('exit', this.killAll) } -Launcher.$inject = ['emitter', 'injector'] +Launcher.$inject = ['server', 'emitter', 'injector'] Launcher.generateId = function () { return '' + Math.floor(Math.random() * 100000000) diff --git a/lib/plugin.js b/lib/plugin.js index a68e6d62d..cfc60e35f 100644 --- a/lib/plugin.js +++ b/lib/plugin.js @@ -6,7 +6,7 @@ var log = require('./logger').create('plugin') var IGNORED_PACKAGES = ['karma-cli', 'karma-runner.github.com'] -exports.resolve = function (plugins) { +exports.resolve = function (plugins, emitter) { var modules = [] var requirePlugin = function (name) { @@ -15,35 +15,39 @@ exports.resolve = function (plugins) { modules.push(require(name)) } catch (e) { if (e.code === 'MODULE_NOT_FOUND' && e.message.indexOf(name) !== -1) { - log.warn('Cannot find plugin "%s".\n Did you forget to install it ?\n' + + log.error('Cannot find plugin "%s".\n Did you forget to install it?\n' + ' npm install %s --save-dev', name, name) } else { - log.warn('Error during loading "%s" plugin:\n %s', name, e.message) + log.error('Error during loading "%s" plugin:\n %s', name, e.message) } + emitter.emit('load_error', 'plug_in', name) } } plugins.forEach(function (plugin) { if (helper.isString(plugin)) { - if (plugin.indexOf('*') !== -1) { - var pluginDirectory = path.normalize(path.join(__dirname, '/../..')) - var regexp = new RegExp('^' + plugin.replace('*', '.*')) - - log.debug('Loading %s from %s', plugin, pluginDirectory) - fs.readdirSync(pluginDirectory).filter(function (pluginName) { - return IGNORED_PACKAGES.indexOf(pluginName) === -1 && regexp.test(pluginName) - }).forEach(function (pluginName) { - requirePlugin(pluginDirectory + '/' + pluginName) - }) - } else { + if (plugin.indexOf('*') === -1) { requirePlugin(plugin) + return } - } else if (helper.isObject(plugin)) { + var pluginDirectory = path.normalize(path.join(__dirname, '/../..')) + var regexp = new RegExp('^' + plugin.replace('*', '.*')) + + log.debug('Loading %s from %s', plugin, pluginDirectory) + fs.readdirSync(pluginDirectory).filter(function (pluginName) { + return IGNORED_PACKAGES.indexOf(pluginName) === -1 && regexp.test(pluginName) + }).forEach(function (pluginName) { + requirePlugin(pluginDirectory + '/' + pluginName) + }) + return + } + if (helper.isObject(plugin)) { log.debug('Loading inlined plugin (defining %s).', Object.keys(plugin).join(', ')) modules.push(plugin) - } else { - log.warn('Invalid plugin %s', plugin) + return } + log.error('Invalid plugin %s', plugin) + emitter.emit('load_error', 'plug_in', plugin) }) return modules diff --git a/lib/preprocessor.js b/lib/preprocessor.js index 8da50338e..298171500 100644 --- a/lib/preprocessor.js +++ b/lib/preprocessor.js @@ -36,13 +36,13 @@ var createNextProcessor = function (preprocessors, file, done) { } } -var createPreprocessor = function (config, basePath, injector) { - var alreadyDisplayedWarnings = {} +var createPreprocessor = function (config, basePath, injector, emitter) { + var alreadyDisplayedErrors = {} var instances = {} var patterns = Object.keys(config) var instantiatePreprocessor = function (name) { - if (alreadyDisplayedWarnings[name]) { + if (alreadyDisplayedErrors[name]) { return } @@ -52,13 +52,13 @@ var createPreprocessor = function (config, basePath, injector) { p = injector.get('preprocessor:' + name) } catch (e) { if (e.message.indexOf('No provider for "preprocessor:' + name + '"') !== -1) { - log.warn('Can not load "%s", it is not registered!\n ' + - 'Perhaps you are missing some plugin?', name) + log.error('Can not load "%s", it is not registered!\n ' + + 'Perhaps you are missing some plugin?', name) } else { - log.warn('Can not load "%s"!\n ' + e.stack, name) + log.error('Can not load "%s"!\n ' + e.stack, name) } - - alreadyDisplayedWarnings[name] = true + alreadyDisplayedErrors[name] = true + emitter.emit('load_error', 'preprocessor', name) } return p @@ -85,30 +85,33 @@ var createPreprocessor = function (config, basePath, injector) { var nextPreprocessor = createNextProcessor(preprocessors, file, done) for (var i = 0; i < patterns.length; i++) { - if (mm(file.originalPath, patterns[i], {dot: true})) { - if (thisFileIsBinary) { - log.warn('Ignoring preprocessing (%s) %s because it is a binary file.', - config[patterns[i]].join(', '), file.originalPath) - } else { - config[patterns[i]].forEach(function (name) { - var p = instances[name] - if (p == null) { - p = instantiatePreprocessor(name) - } - - if (p == null) { - if (!alreadyDisplayedWarnings[name]) { - alreadyDisplayedWarnings[name] = true - log.warn('Failed to instantiate preprocessor %s', name) - } - return - } - - instances[name] = p - preprocessors.push(p) - }) - } + if (!mm(file.originalPath, patterns[i], {dot: true})) continue + + if (thisFileIsBinary) { + log.warn('Ignoring preprocessing (%s) %s because it is a binary file.', + config[patterns[i]].join(', '), file.originalPath) + continue } + + config[patterns[i]].forEach(function (name) { + var p = instances[name] + + if (p == null) { + p = instantiatePreprocessor(name) + } + + if (p == null) { + if (!alreadyDisplayedErrors[name]) { + alreadyDisplayedErrors[name] = true + log.error('Failed to instantiate preprocessor %s', name) + emitter.emit('load_error', 'preprocessor', name) + } + return + } + + instances[name] = p + preprocessors.push(p) + }) } nextPreprocessor(null, thisFileIsBinary ? buffer : buffer.toString()) @@ -116,6 +119,6 @@ var createPreprocessor = function (config, basePath, injector) { }) } } -createPreprocessor.$inject = ['config.preprocessors', 'config.basePath', 'injector'] +createPreprocessor.$inject = ['config.preprocessors', 'config.basePath', 'injector', 'emitter'] exports.createPreprocessor = createPreprocessor diff --git a/lib/reporter.js b/lib/reporter.js index 22a21de88..cac9deea3 100644 --- a/lib/reporter.js +++ b/lib/reporter.js @@ -107,11 +107,12 @@ var createReporters = function (names, config, emitter, injector) { reporters.push(injector.createChild([locals], ['reporter:' + name]).get('reporter:' + name)) } catch (e) { if (e.message.indexOf('No provider for "reporter:' + name + '"') !== -1) { - log.warn('Can not load "%s", it is not registered!\n ' + + log.error('Can not load reporter "%s", it is not registered!\n ' + 'Perhaps you are missing some plugin?', name) } else { - log.warn('Can not load "%s"!\n ' + e.stack, name) + log.error('Can not load "%s"!\n ' + e.stack, name) } + emitter.emit('load_error', 'reporter', name) return } var color_name = name + '_color' diff --git a/lib/server.js b/lib/server.js index 739ac8617..189935778 100644 --- a/lib/server.js +++ b/lib/server.js @@ -51,6 +51,8 @@ var Server = function (cliOptions, done) { this.log = logger.create() + this.loadErrors = [] + var config = cfg.parseConfig(cliOptions.configFile, cliOptions) var modules = [{ @@ -58,6 +60,7 @@ var Server = function (cliOptions, done) { logger: ['value', logger], done: ['value', done || process.exit], emitter: ['value', this], + server: ['value', this], launcher: ['type', Launcher], config: ['value', config], preprocess: ['factory', preprocessor.createPreprocessor], @@ -82,8 +85,9 @@ var Server = function (cliOptions, done) { }] }] + this._setUpLoadErrorListener() // Load the plugins - modules = modules.concat(plugin.resolve(config.plugins)) + modules = modules.concat(plugin.resolve(config.plugins, this)) this._injector = new di.Injector(modules) } @@ -169,12 +173,16 @@ Server.prototype._start = function (config, launcher, preprocess, fileList, webS config.protocol, config.hostname, config.port, config.urlRoot) self.emit('listening', config.port) - if (config.browsers && config.browsers.length) { self._injector.invoke(launcher.launch, launcher).forEach(function (browserLauncher) { singleRunDoneBrowsers[browserLauncher.id] = false }) } + var noLoadErrors = self.loadErrors.length + if (noLoadErrors > 0) { + self.log.error('Found %d load error%s', noLoadErrors, noLoadErrors === 1 ? '' : 's') + process.kill(process.pid, 'SIGINT') + } }) } @@ -363,6 +371,14 @@ Server.prototype._start = function (config, launcher, preprocess, fileList, webS }) } +Server.prototype._setUpLoadErrorListener = function () { + var self = this + self.on('load_error', function (type, name) { + self.log.debug('Registered a load error of type %s with name %s', type, name) + self.loadErrors.push([type, name]) + }) +} + Server.prototype._detach = function (config, done) { var log = this.log var tmpFile = tmp.fileSync({keep: true}) diff --git a/test/e2e/load.feature b/test/e2e/load.feature new file mode 100644 index 000000000..fab9fdbcc --- /dev/null +++ b/test/e2e/load.feature @@ -0,0 +1,106 @@ +Feature: Basic Testrunner + In order to use Karma + As a person who wants to write great tests + I want Karma to terminate upon misconfiguration + + Scenario: Execute with missing browser + Given a configuration with: + """ + files = ['basic/plus.js', 'basic/test.js']; + browsers = ['NonExistingBrowser', 'PhantomJS']; + plugins = [ + 'karma-jasmine', + 'karma-phantomjs-launcher' + ]; + singleRun = false + """ + When I start Karma + Then it fails with like: + """ + Cannot load browser "NonExistingBrowser": it is not registered! Perhaps you are missing some plugin\? + """ + And it fails with like: + """ +Found 1 load error + """ + + Scenario: Execute with missing plugin + Given a configuration with: + """ + files = ['basic/plus.js', 'basic/test.js']; + browsers = ['PhantomJS']; + plugins = [ + 'karma-totally-non-existing-plugin', + 'karma-jasmine', + 'karma-phantomjs-launcher' + ]; + singleRun = false + """ + When I start Karma + Then it fails with like: + """ +Cannot find plugin "karma-totally-non-existing-plugin". +[\s]+Did you forget to install it\? +[\s]+npm install karma-totally-non-existing-plugin --save-dev + """ + And it fails with like: + """ +Found 1 load error + """ + + Scenario: Execute with missing reporter + Given a configuration with: + """ + files = ['basic/plus.js', 'basic/test.js']; + browsers = ['PhantomJS']; + reporters = ['unreal-reporter'] + plugins = [ + 'karma-jasmine', + 'karma-phantomjs-launcher' + ]; + singleRun = false + """ + When I start Karma + Then it fails with like: + """ +Can not load reporter "unreal-reporter", it is not registered! +[\s]+Perhaps you are missing some plugin\? + """ + And it fails with like: + """ +Found 1 load error + """ + + Scenario: Execute with missing reporter, plugin and browser + Given a configuration with: + """ + files = ['basic/plus.js', 'basic/test.js']; + browsers = ['NonExistingBrowser', 'PhantomJS']; + reporters = ['unreal-reporter'] + plugins = [ + 'karma-totally-non-existing-plugin', + 'karma-jasmine', + 'karma-phantomjs-launcher' + ]; + singleRun = false + """ + When I start Karma + Then it fails with like: + """ +Can not load reporter "unreal-reporter", it is not registered! +[\s]+Perhaps you are missing some plugin\? + """ + And it fails with like: + """ +Cannot find plugin "karma-totally-non-existing-plugin". +[\s]+Did you forget to install it\? +[\s]+npm install karma-totally-non-existing-plugin --save-dev + """ + And it fails with like: + """ + Cannot load browser "NonExistingBrowser": it is not registered! Perhaps you are missing some plugin\? + """ + And it fails with like: + """ +Found 3 load errors + """ diff --git a/test/unit/launcher.spec.js b/test/unit/launcher.spec.js index 109664d9c..dfbde5842 100644 --- a/test/unit/launcher.spec.js +++ b/test/unit/launcher.spec.js @@ -69,18 +69,22 @@ describe('launcher', () => { describe('Launcher', () => { var emitter - var l = emitter = null + var server + var l = emitter = server = null beforeEach(() => { emitter = new events.EventEmitter() + server = {'loadErrors': []} + var injector = new di.Injector([{ 'launcher:Fake': ['type', FakeBrowser], 'launcher:Script': ['type', ScriptBrowser], + 'server': ['value', server], 'emitter': ['value', emitter], 'config': ['value', {captureTimeout: 0}], 'timer': ['factory', createMockTimer] }]) - l = new launcher.Launcher(emitter, injector) + l = new launcher.Launcher(server, emitter, injector) }) describe('launch', () => { @@ -93,6 +97,15 @@ describe('launcher', () => { expect(browser.name).to.equal('Fake') }) + it('should not start when server has load errors', () => { + server.loadErrors = ['error'] + l.launch(['Fake'], 'http:', 'localhost', 1234, '/root/', 1) + var browser = FakeBrowser._instances.pop() + expect(browser.start).to.not.have.been.called + expect(browser.id).to.equal(lastGeneratedId) + expect(browser.name).to.equal('Fake') + }) + it('should allow launching a script', () => { l.launch(['/usr/local/bin/special-browser'], 'http:', 'localhost', 1234, '/', 1)