From 21a3e4679d7d5d5ef605cc905413e05285e75b66 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 | 23 +++---- lib/reporter.js | 5 +- lib/server.js | 20 ++++++- test/e2e/load.feature | 106 +++++++++++++++++++++++++++++++++ test/unit/launcher.spec.js | 17 +++++- test/unit/preprocessor.spec.js | 29 ++++----- 8 files changed, 198 insertions(+), 52 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 2efc4a314..860bcdbdc 100644 --- a/lib/preprocessor.js +++ b/lib/preprocessor.js @@ -38,12 +38,14 @@ var createNextProcessor = function (preprocessors, file, done) { } var createPreprocessor = function (config, basePath, injector) { - var alreadyDisplayedWarnings = {} + var alreadyDisplayedErrors = {} var instances = {} var patterns = Object.keys(config) + var emitter = injector.get('emitter') + var instantiatePreprocessor = function (name) { - if (alreadyDisplayedWarnings[name]) { + if (alreadyDisplayedErrors[name]) { return } @@ -53,13 +55,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 @@ -105,9 +107,10 @@ var createPreprocessor = function (config, basePath, injector) { } if (p == null) { - if (!alreadyDisplayedWarnings[name]) { - alreadyDisplayedWarnings[name] = true - log.warn('Failed to instantiate preprocessor %s', name) + if (!alreadyDisplayedErrors[name]) { + alreadyDisplayedErrors[name] = true + log.error('Failed to instantiate preprocessor %s', name) + emitter.emit('load_error', 'preprocessor', name) } return } 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) diff --git a/test/unit/preprocessor.spec.js b/test/unit/preprocessor.spec.js index c995bdc4c..c9fcd4111 100644 --- a/test/unit/preprocessor.spec.js +++ b/test/unit/preprocessor.spec.js @@ -1,12 +1,13 @@ import mocks from 'mocks' import di from 'di' import path from 'path' +import EventEmitter from 'events' describe('preprocessor', () => { var pp var m var mockFs - + var emitterSetting // mimic first few bytes of a pdf file var binarydata = new Buffer([0x25, 0x50, 0x44, 0x66, 0x46, 0x00]) @@ -28,7 +29,7 @@ describe('preprocessor', () => { 'graceful-fs': mockFs, minimatch: require('minimatch') } - + emitterSetting = {'emitter': ['value', new EventEmitter()]} m = mocks.loadFile(path.join(__dirname, '/../../lib/preprocessor.js'), mocks_) }) @@ -38,7 +39,7 @@ describe('preprocessor', () => { done(null, 'new-content') }) - var injector = new di.Injector([{'preprocessor:fake': ['factory', () => fakePreprocessor]}]) + var injector = new di.Injector([{'preprocessor:fake': ['factory', () => fakePreprocessor]}, emitterSetting]) pp = m.createPreprocessor({'**/*.js': ['fake']}, null, injector) var file = {originalPath: '/some/a.js', path: 'path'} @@ -57,7 +58,7 @@ describe('preprocessor', () => { done(null, 'new-content') }) - var injector = new di.Injector([{'preprocessor:fake': ['factory', () => fakePreprocessor]}]) + var injector = new di.Injector([{'preprocessor:fake': ['factory', () => fakePreprocessor]}, emitterSetting]) pp = m.createPreprocessor({'**/*.js': ['fake']}, null, injector) var file = {originalPath: '/some/.dir/a.js', path: 'path'} @@ -76,7 +77,7 @@ describe('preprocessor', () => { done(null, 'new-content') }) - var injector = new di.Injector([{'preprocessor:fake': ['factory', () => fakePreprocessor]}]) + var injector = new di.Injector([{'preprocessor:fake': ['factory', () => fakePreprocessor]}, emitterSetting]) var config = {'**/*.txt': ['fake']} pp = m.createPreprocessor(config, null, injector) @@ -97,7 +98,7 @@ describe('preprocessor', () => { done(null, '') }) - var injector = new di.Injector([{'preprocessor:fake': ['factory', () => fakePreprocessor]}]) + var injector = new di.Injector([{'preprocessor:fake': ['factory', () => fakePreprocessor]}, emitterSetting]) pp = m.createPreprocessor({'**/*.js': ['fake']}, null, injector) var file = {originalPath: '/some/a.txt', path: 'path'} @@ -122,7 +123,7 @@ describe('preprocessor', () => { var injector = new di.Injector([{ 'preprocessor:fake1': ['factory', () => fakePreprocessor1], 'preprocessor:fake2': ['factory', () => fakePreprocessor2] - }]) + }, emitterSetting]) pp = m.createPreprocessor({'**/*.js': ['fake1', 'fake2']}, null, injector) @@ -138,7 +139,7 @@ describe('preprocessor', () => { }) it('should compute SHA', (done) => { - pp = m.createPreprocessor({}, null, new di.Injector([])) + pp = m.createPreprocessor({}, null, new di.Injector([emitterSetting])) var file = {originalPath: '/some/a.js', path: 'path'} pp(file, () => { @@ -166,7 +167,7 @@ describe('preprocessor', () => { var injector = new di.Injector([{ 'preprocessor:fake': ['factory', () => fakePreprocessor] - }]) + }, emitterSetting]) pp = m.createPreprocessor({'**/a.js': ['fake']}, null, injector) @@ -192,7 +193,7 @@ describe('preprocessor', () => { var injector = new di.Injector([{ 'preprocessor:failing': ['factory', () => failingPreprocessor] - }]) + }, emitterSetting]) pp = m.createPreprocessor({'**/*.js': ['failing']}, null, injector) @@ -216,7 +217,7 @@ describe('preprocessor', () => { var injector = new di.Injector([{ 'preprocessor:failing': ['factory', () => failingPreprocessor], 'preprocessor:fake': ['factory', () => fakePreprocessor] - }]) + }, emitterSetting]) pp = m.createPreprocessor({'**/*.js': ['failing', 'fake']}, null, injector) @@ -235,7 +236,7 @@ describe('preprocessor', () => { var injector = new di.Injector([{ 'preprocessor:fake': ['factory', () => fakePreprocessor] - }]) + }, emitterSetting]) pp = m.createPreprocessor({'**/*': ['fake']}, null, injector) @@ -257,7 +258,7 @@ describe('preprocessor', () => { var injector = new di.Injector([{ 'preprocessor:fake': ['factory', () => fakePreprocessor] - }]) + }, emitterSetting]) pp = m.createPreprocessor({'**/*': ['fake']}, null, injector) @@ -296,7 +297,7 @@ describe('preprocessor', () => { 'preprocessor:fakeB': ['factory', () => fakePreprocessorB], 'preprocessor:fakeC': ['factory', () => fakePreprocessorC], 'preprocessor:fakeD': ['factory', () => fakePreprocessorD] - }]) + }, emitterSetting]) pp = m.createPreprocessor({ '/*/a.js': ['fakeA', 'fakeB'],