From 1d15e53eb1ad468b97baff68f21d72d3f9d51b54 Mon Sep 17 00:00:00 2001 From: Nico Jansen Date: Thu, 21 Nov 2019 08:35:54 +0100 Subject: [PATCH] fix(middleware/runner): handle file list rejections --- lib/executor.js | 30 ++++++ lib/middleware/runner.js | 67 +++++++------ test/unit/executor.spec.js | 92 +++++++++++++---- test/unit/middleware/runner.spec.js | 147 +++++++++++++++++----------- 4 files changed, 230 insertions(+), 106 deletions(-) diff --git a/lib/executor.js b/lib/executor.js index 41f6a9ebd..7863df20b 100644 --- a/lib/executor.js +++ b/lib/executor.js @@ -9,6 +9,7 @@ class Executor { this.emitter = emitter this.executionScheduled = false + this.errorsScheduled = [] this.pendingCount = 0 this.runningBrowsers = null @@ -37,10 +38,39 @@ class Executor { } } + /** + * Schedule an error to be reported + * @param {string} errorMessage + * @returns {boolean} a boolean indicating whether or not the error was handled synchronously + */ + scheduleError (errorMessage) { + if (this.capturedBrowsers.areAllReady()) { + log.warn(errorMessage) + const errorResult = { + success: 0, + failed: 0, + skipped: 0, + error: errorMessage, + exitCode: 1 + } + this.emitter.emit('run_start', []) // A run cannot complete without being started + this.emitter.emit('run_complete', [] /* No browsers ran actual tests */, errorResult) + return true + } else { + this.errorsScheduled.push(errorMessage) + return false + } + } + onRunComplete () { if (this.executionScheduled) { this.schedule() } + if (this.errorsScheduled.length) { + const errorsToReport = this.errorsScheduled + this.errorsScheduled = [] + errorsToReport.forEach((error) => this.scheduleError(error)) + } } onBrowserComplete () { diff --git a/lib/middleware/runner.js b/lib/middleware/runner.js index 914d7417b..14c3dbb32 100644 --- a/lib/middleware/runner.js +++ b/lib/middleware/runner.js @@ -1,3 +1,4 @@ +// @ts-check /** * Runner middleware is responsible for communication with `karma run`. * @@ -35,19 +36,18 @@ function createRunnerMiddleware (emitter, fileList, capturedBrowsers, reporter, } const data = request.body - emitter.once('run_start', function () { - const responseWrite = response.write.bind(response) - responseWrite.colors = data.colors - reporter.addAdapter(responseWrite) - // clean up, close runner response - emitter.once('run_complete', function (browsers, results) { - reporter.removeAdapter(responseWrite) - const emptyTestSuite = (results.failed + results.success) === 0 ? 0 : 1 - response.end(constant.EXIT_CODE + emptyTestSuite + results.exitCode) - }) + updateClientArgs(data) + handleRun(data) + refreshFileList(data).then(() => { + executor.schedule() + }).catch((error) => { + const errorMessage = `Error during refresh file list. ${error.stack || error}` + executor.scheduleError(errorMessage) }) + }) + function updateClientArgs (data) { helper.restoreOriginalArgs(config) if (_.isEmpty(data.args)) { log.debug('Ignoring empty client.args from run command') @@ -59,43 +59,52 @@ function createRunnerMiddleware (emitter, fileList, capturedBrowsers, reporter, log.warn('Replacing client.args with ', data.args, ' as their types do not match.') config.client.args = data.args } + } + async function refreshFileList (data) { let fullRefresh = true if (helper.isArray(data.changedFiles)) { - data.changedFiles.forEach(function (filepath) { - fileList.changeFile(path.resolve(config.basePath, filepath)) + await Promise.all(data.changedFiles.map(async function (filepath) { + await fileList.changeFile(path.resolve(config.basePath, filepath)) fullRefresh = false - }) + })) } if (helper.isArray(data.addedFiles)) { - data.addedFiles.forEach(function (filepath) { - fileList.addFile(path.resolve(config.basePath, filepath)) + await Promise.all(data.addedFiles.map(async function (filepath) { + await fileList.addFile(path.resolve(config.basePath, filepath)) fullRefresh = false - }) + })) } if (helper.isArray(data.removedFiles)) { - data.removedFiles.forEach(function (filepath) { - fileList.removeFile(path.resolve(config.basePath, filepath)) + await Promise.all(data.removedFiles.map(async function (filepath) { + await fileList.removeFile(path.resolve(config.basePath, filepath)) fullRefresh = false - }) + })) } if (fullRefresh && data.refresh !== false) { log.debug('Refreshing all the files / patterns') - fileList.refresh().then(function () { - // Wait for the file list refresh to complete before starting test run, - // otherwise the context.html generation might not see new/updated files. - if (!config.autoWatch) { - executor.schedule() - } - }) - } else { - executor.schedule() + await fileList.refresh() } - }) + } + + function handleRun (data) { + emitter.once('run_start', function () { + const responseWrite = response.write.bind(response) + responseWrite.colors = data.colors + reporter.addAdapter(responseWrite) + + // clean up, close runner response + emitter.once('run_complete', function (_browsers, results) { + reporter.removeAdapter(responseWrite) + const emptyTestSuite = (results.failed + results.success) === 0 ? 0 : 1 + response.end(constant.EXIT_CODE + emptyTestSuite + results.exitCode) + }) + }) + } } } diff --git a/test/unit/executor.spec.js b/test/unit/executor.spec.js index 054259839..47cb5a896 100644 --- a/test/unit/executor.spec.js +++ b/test/unit/executor.spec.js @@ -1,10 +1,13 @@ 'use strict' +// @ts-check const Browser = require('../../lib/browser') const BrowserCollection = require('../../lib/browser_collection') const EventEmitter = require('../../lib/events').EventEmitter const Executor = require('../../lib/executor') +const log = require('../../lib/logger').create() + describe('executor', () => { let emitter let capturedBrowsers @@ -21,36 +24,85 @@ describe('executor', () => { executor.socketIoSockets = new EventEmitter() spy = { - onRunStart: () => null, - onSocketsExecute: () => null + onRunStart: sinon.stub(), + onSocketsExecute: sinon.stub(), + onRunComplete: sinon.stub() } - - sinon.spy(spy, 'onRunStart') - sinon.spy(spy, 'onSocketsExecute') + sinon.stub(log, 'warn') emitter.on('run_start', spy.onRunStart) + emitter.on('run_complete', spy.onRunComplete) executor.socketIoSockets.on('execute', spy.onSocketsExecute) }) - it('should start the run and pass client config', () => { - capturedBrowsers.areAllReady = () => true + describe('schedule', () => { + it('should start the run and pass client config', () => { + capturedBrowsers.areAllReady = () => true + + executor.schedule() + expect(spy.onRunStart).to.have.been.called + expect(spy.onSocketsExecute).to.have.been.calledWith(config.client) + }) + + it('should wait for all browsers to finish', () => { + capturedBrowsers.areAllReady = () => false - executor.schedule() - expect(spy.onRunStart).to.have.been.called - expect(spy.onSocketsExecute).to.have.been.calledWith(config.client) + // they are not ready yet + executor.schedule() + expect(spy.onRunStart).not.to.have.been.called + expect(spy.onSocketsExecute).not.to.have.been.called + + capturedBrowsers.areAllReady = () => true + emitter.emit('run_complete') + expect(spy.onRunStart).to.have.been.called + expect(spy.onSocketsExecute).to.have.been.called + }) }) - it('should wait for all browsers to finish', () => { - capturedBrowsers.areAllReady = () => false + describe('scheduleError', () => { + it('should return `true` if scheduled synchronously', () => { + const result = executor.scheduleError('expected error') + expect(result).to.be.true + }) + + it('should emit both "run_start" and "run_complete"', () => { + executor.scheduleError('expected error') + expect(spy.onRunStart).to.have.been.called + expect(spy.onRunComplete).to.have.been.called + expect(spy.onRunStart).to.have.been.calledBefore(spy.onRunComplete) + }) + + it('should report the error', () => { + const expectedError = 'expected error' + executor.scheduleError(expectedError) + expect(spy.onRunComplete).to.have.been.calledWith([], { + success: 0, + failed: 0, + skipped: 0, + error: expectedError, + exitCode: 1 + }) + }) + + it('should wait for scheduled runs to end before reporting the error', () => { + // Arrange + let browsersAreReady = true + const expectedError = 'expected error' + capturedBrowsers.areAllReady = () => browsersAreReady + executor.schedule() + browsersAreReady = false - // they are not ready yet - executor.schedule() - expect(spy.onRunStart).not.to.have.been.called - expect(spy.onSocketsExecute).not.to.have.been.called + // Act + const result = executor.scheduleError(expectedError) + browsersAreReady = true - capturedBrowsers.areAllReady = () => true - emitter.emit('run_complete') - expect(spy.onRunStart).to.have.been.called - expect(spy.onSocketsExecute).to.have.been.called + // Assert + expect(result).to.be.false + expect(spy.onRunComplete).to.not.have.been.called + emitter.emit('run_complete') + expect(spy.onRunComplete).to.have.been.calledWith([], sinon.match({ + error: expectedError + })) + }) }) }) diff --git a/test/unit/middleware/runner.spec.js b/test/unit/middleware/runner.spec.js index 0f831bf2a..ba601c07f 100644 --- a/test/unit/middleware/runner.spec.js +++ b/test/unit/middleware/runner.spec.js @@ -55,16 +55,23 @@ describe('middleware.runner', () => { } executor = { - schedule: () => emitter.emit('run_start') + scheduled: false, + schedule: () => { + executor.scheduled = true + emitter.emit('run_start') + if (executor.onSchedule) { + executor.onSchedule() + } + } } emitter = new EventEmitter() capturedBrowsers = new BrowserCollection(emitter) fileListMock = { - refresh: () => Promise.resolve(), - addFile: () => null, - removeFile: () => null, - changeFile: () => null + refresh: sinon.stub(), + addFile: sinon.stub(), + removeFile: sinon.stub(), + changeFile: sinon.stub() } nextSpy = sinon.spy() @@ -82,15 +89,21 @@ describe('middleware.runner', () => { sinon.stub(capturedBrowsers, 'areAllReady').callsFake(() => true) response.once('end', () => { - expect(nextSpy).to.not.have.been.called - expect(response).to.beServedAs(200, 'result\x1FEXIT10') - done() + try { + expect(nextSpy).to.not.have.been.called + expect(response).to.beServedAs(200, 'result\x1FEXIT10') + done() + } catch (err) { + done(err) + } }) handler(new HttpRequestMock('/__run__'), response, nextSpy) - mockReporter.write('result') - emitter.emit('run_complete', capturedBrowsers, { exitCode: 0 }) + executor.onSchedule = () => { + mockReporter.write('result') + emitter.emit('run_complete', capturedBrowsers, { exitCode: 0 }) + } }) it('should set the empty to 0 if empty results', (done) => { @@ -98,15 +111,21 @@ describe('middleware.runner', () => { sinon.stub(capturedBrowsers, 'areAllReady').callsFake(() => true) response.once('end', () => { - expect(nextSpy).to.not.have.been.called - expect(response).to.beServedAs(200, 'result\x1FEXIT00') - done() + try { + expect(nextSpy).to.not.have.been.called + expect(response).to.beServedAs(200, 'result\x1FEXIT00') + done() + } catch (err) { + done(err) + } }) handler(new HttpRequestMock('/__run__'), response, nextSpy) - mockReporter.write('result') - emitter.emit('run_complete', capturedBrowsers, { exitCode: 0, success: 0, failed: 0 }) + executor.onSchedule = () => { + mockReporter.write('result') + emitter.emit('run_complete', capturedBrowsers, { exitCode: 0, success: 0, failed: 0 }) + } }) it('should set the empty to 1 if successful tests', (done) => { @@ -114,15 +133,21 @@ describe('middleware.runner', () => { sinon.stub(capturedBrowsers, 'areAllReady').callsFake(() => true) response.once('end', () => { - expect(nextSpy).to.not.have.been.called - expect(response).to.beServedAs(200, 'result\x1FEXIT10') - done() + try { + expect(nextSpy).to.not.have.been.called + expect(response).to.beServedAs(200, 'result\x1FEXIT10') + done() + } catch (err) { + done(err) + } }) handler(new HttpRequestMock('/__run__'), response, nextSpy) - mockReporter.write('result') - emitter.emit('run_complete', capturedBrowsers, { exitCode: 0, success: 3, failed: 0 }) + executor.onSchedule = () => { + mockReporter.write('result') + emitter.emit('run_complete', capturedBrowsers, { exitCode: 0, success: 3, failed: 0 }) + } }) it('should set the empty to 1 if failed tests', (done) => { @@ -130,20 +155,24 @@ describe('middleware.runner', () => { sinon.stub(capturedBrowsers, 'areAllReady').callsFake(() => true) response.once('end', () => { - expect(nextSpy).to.not.have.been.called - expect(response).to.beServedAs(200, 'result\x1FEXIT10') - done() + try { + expect(nextSpy).to.not.have.been.called + expect(response).to.beServedAs(200, 'result\x1FEXIT10') + done() + } catch (err) { + done(err) + } }) handler(new HttpRequestMock('/__run__'), response, nextSpy) - mockReporter.write('result') - emitter.emit('run_complete', capturedBrowsers, { exitCode: 0, success: 0, failed: 6 }) + executor.onSchedule = () => { + mockReporter.write('result') + emitter.emit('run_complete', capturedBrowsers, { exitCode: 0, success: 0, failed: 6 }) + } }) it('should not run if there is no browser captured', (done) => { - sinon.stub(fileListMock, 'refresh') - response.once('end', () => { expect(nextSpy).to.not.have.been.called expect(response).to.beServedAs(200, 'No captured browser, open http://localhost:8877/\n') @@ -156,11 +185,7 @@ describe('middleware.runner', () => { it('should refresh explicit files if specified', (done) => { capturedBrowsers.add(new Browser()) - sinon.stub(capturedBrowsers, 'areAllReady').callsFake(() => true) - sinon.stub(fileListMock, 'refresh') - sinon.stub(fileListMock, 'addFile') - sinon.stub(fileListMock, 'changeFile') - sinon.stub(fileListMock, 'removeFile') + sinon.stub(capturedBrowsers, 'areAllReady').returns(true) const RAW_MESSAGE = JSON.stringify({ addedFiles: ['/new.js'], @@ -178,14 +203,14 @@ describe('middleware.runner', () => { request.emit('data', RAW_MESSAGE) request.emit('end') - process.nextTick(() => { + executor.onSchedule = () => { expect(fileListMock.refresh).not.to.have.been.called expect(fileListMock.addFile).to.have.been.calledWith(path.resolve('/new.js')) expect(fileListMock.removeFile).to.have.been.calledWith(path.resolve('/foo.js')) expect(fileListMock.removeFile).to.have.been.calledWith(path.resolve('/bar.js')) expect(fileListMock.changeFile).to.have.been.calledWith(path.resolve('/changed.js')) done() - }) + } }) it('should wait for refresh to finish if applicable before scheduling execution', (done) => { @@ -193,25 +218,21 @@ describe('middleware.runner', () => { sinon.stub(capturedBrowsers, 'areAllReady').callsFake(() => true) let res = null - const fileListPromise = new Promise((resolve, reject) => { + const fileListPromise = new Promise((resolve) => { res = resolve }) - sinon.stub(fileListMock, 'refresh').returns(fileListPromise) - sinon.stub(executor, 'schedule') + fileListMock.refresh.returns(fileListPromise) const request = new HttpRequestMock('/__run__') handler(request, response, nextSpy) process.nextTick(() => { expect(fileListMock.refresh).to.have.been.called - expect(executor.schedule).to.not.have.been.called + expect(executor.scheduled).to.be.false - // Now try resolving the promise + executor.onSchedule = done + // Now resolving the promise res() - setTimeout(() => { - expect(executor.schedule).to.have.been.called - done() - }, 2) }) }) @@ -219,9 +240,6 @@ describe('middleware.runner', () => { capturedBrowsers.add(new Browser()) sinon.stub(capturedBrowsers, 'areAllReady').callsFake(() => true) - sinon.spy(fileListMock, 'refresh') - sinon.stub(executor, 'schedule') - const RAW_MESSAGE = JSON.stringify({ refresh: false }) const request = new HttpRequestMock('/__run__', { @@ -234,11 +252,14 @@ describe('middleware.runner', () => { request.emit('data', RAW_MESSAGE) request.emit('end') - process.nextTick(() => { - expect(fileListMock.refresh).not.to.have.been.called - expect(executor.schedule).to.have.been.called - done() - }) + executor.onSchedule = () => { + try { + expect(fileListMock.refresh).not.to.have.been.called + done() + } catch (err) { + done(err) + } + } }) it('should not schedule execution if refreshing and autoWatch', (done) => { @@ -247,16 +268,12 @@ describe('middleware.runner', () => { capturedBrowsers.add(new Browser()) sinon.stub(capturedBrowsers, 'areAllReady').callsFake(() => true) - sinon.spy(fileListMock, 'refresh') - sinon.stub(executor, 'schedule') - handler(new HttpRequestMock('/__run__'), response, nextSpy) - process.nextTick(() => { + executor.onSchedule = () => { expect(fileListMock.refresh).to.have.been.called - expect(executor.schedule).not.to.have.been.called done() - }) + } }) it('should ignore other urls', (done) => { @@ -265,6 +282,22 @@ describe('middleware.runner', () => { done() }) }) + + it('should scheduleError when file list rejects', (done) => { + const error = new Error('expected error for testing') + capturedBrowsers.add(new Browser()) + sinon.stub(capturedBrowsers, 'areAllReady').returns(true) + fileListMock.refresh.rejects(error) + handler(new HttpRequestMock('/__run__'), response, nextSpy) + executor.scheduleError = (errorMessage) => { + try { + expect(errorMessage).eq(`Error during refresh file list. ${error.stack}`) + done() + } catch (err) { + done(err) + } + } + }) }) describe('', () => {