Skip to content

Commit 80febfb

Browse files
nicojsjohnjbarton
nicojs
authored and
johnjbarton
committedNov 27, 2019
fix(middleware/runner): handle file list rejections (#3400)
Add error handling for rejections on the file list methods in the `runner` middleware. As discussed in #3396 it does this by handling an error the same way an error in a run is handled. Fixes #3396
·
v6.4.4v5.0.0
1 parent adc6a66 commit 80febfb

File tree

4 files changed

+231
-106
lines changed

4 files changed

+231
-106
lines changed
 

‎lib/executor.js

+33
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ class Executor {
99
this.emitter = emitter
1010

1111
this.executionScheduled = false
12+
this.errorsScheduled = []
1213
this.pendingCount = 0
1314
this.runningBrowsers = null
1415

@@ -37,10 +38,42 @@ class Executor {
3738
}
3839
}
3940

41+
/**
42+
* Schedule an error to be reported
43+
* @param {string} errorMessage
44+
* @returns {boolean} a boolean indicating whether or not the error was handled synchronously
45+
*/
46+
scheduleError (errorMessage) {
47+
// We don't want to interfere with any running test.
48+
// Verify that no test is running before reporting the error.
49+
if (this.capturedBrowsers.areAllReady()) {
50+
log.warn(errorMessage)
51+
const errorResult = {
52+
success: 0,
53+
failed: 0,
54+
skipped: 0,
55+
error: errorMessage,
56+
exitCode: 1
57+
}
58+
const noBrowsersStartedTests = []
59+
this.emitter.emit('run_start', noBrowsersStartedTests) // A run cannot complete without being started
60+
this.emitter.emit('run_complete', noBrowsersStartedTests, errorResult)
61+
return true
62+
} else {
63+
this.errorsScheduled.push(errorMessage)
64+
return false
65+
}
66+
}
67+
4068
onRunComplete () {
4169
if (this.executionScheduled) {
4270
this.schedule()
4371
}
72+
if (this.errorsScheduled.length) {
73+
const errorsToReport = this.errorsScheduled
74+
this.errorsScheduled = []
75+
errorsToReport.forEach((error) => this.scheduleError(error))
76+
}
4477
}
4578

4679
onBrowserComplete () {

‎lib/middleware/runner.js

+37-29
Original file line numberDiff line numberDiff line change
@@ -35,19 +35,18 @@ function createRunnerMiddleware (emitter, fileList, capturedBrowsers, reporter,
3535
}
3636

3737
const data = request.body
38-
emitter.once('run_start', function () {
39-
const responseWrite = response.write.bind(response)
40-
responseWrite.colors = data.colors
41-
reporter.addAdapter(responseWrite)
4238

43-
// clean up, close runner response
44-
emitter.once('run_complete', function (browsers, results) {
45-
reporter.removeAdapter(responseWrite)
46-
const emptyTestSuite = (results.failed + results.success) === 0 ? 0 : 1
47-
response.end(constant.EXIT_CODE + emptyTestSuite + results.exitCode)
48-
})
39+
updateClientArgs(data)
40+
handleRun(data)
41+
refreshFileList(data).then(() => {
42+
executor.schedule()
43+
}).catch((error) => {
44+
const errorMessage = `Error during refresh file list. ${error.stack || error}`
45+
executor.scheduleError(errorMessage)
4946
})
47+
})
5048

49+
function updateClientArgs (data) {
5150
helper.restoreOriginalArgs(config)
5251
if (_.isEmpty(data.args)) {
5352
log.debug('Ignoring empty client.args from run command')
@@ -59,43 +58,52 @@ function createRunnerMiddleware (emitter, fileList, capturedBrowsers, reporter,
5958
log.warn('Replacing client.args with ', data.args, ' as their types do not match.')
6059
config.client.args = data.args
6160
}
61+
}
6262

63+
async function refreshFileList (data) {
6364
let fullRefresh = true
6465

6566
if (helper.isArray(data.changedFiles)) {
66-
data.changedFiles.forEach(function (filepath) {
67-
fileList.changeFile(path.resolve(config.basePath, filepath))
67+
await Promise.all(data.changedFiles.map(async function (filepath) {
68+
await fileList.changeFile(path.resolve(config.basePath, filepath))
6869
fullRefresh = false
69-
})
70+
}))
7071
}
7172

7273
if (helper.isArray(data.addedFiles)) {
73-
data.addedFiles.forEach(function (filepath) {
74-
fileList.addFile(path.resolve(config.basePath, filepath))
74+
await Promise.all(data.addedFiles.map(async function (filepath) {
75+
await fileList.addFile(path.resolve(config.basePath, filepath))
7576
fullRefresh = false
76-
})
77+
}))
7778
}
7879

7980
if (helper.isArray(data.removedFiles)) {
80-
data.removedFiles.forEach(function (filepath) {
81-
fileList.removeFile(path.resolve(config.basePath, filepath))
81+
await Promise.all(data.removedFiles.map(async function (filepath) {
82+
await fileList.removeFile(path.resolve(config.basePath, filepath))
8283
fullRefresh = false
83-
})
84+
}))
8485
}
8586

8687
if (fullRefresh && data.refresh !== false) {
8788
log.debug('Refreshing all the files / patterns')
88-
fileList.refresh().then(function () {
89-
// Wait for the file list refresh to complete before starting test run,
90-
// otherwise the context.html generation might not see new/updated files.
91-
if (!config.autoWatch) {
92-
executor.schedule()
93-
}
94-
})
95-
} else {
96-
executor.schedule()
89+
await fileList.refresh()
9790
}
98-
})
91+
}
92+
93+
function handleRun (data) {
94+
emitter.once('run_start', function () {
95+
const responseWrite = response.write.bind(response)
96+
responseWrite.colors = data.colors
97+
reporter.addAdapter(responseWrite)
98+
99+
// clean up, close runner response
100+
emitter.once('run_complete', function (_browsers, results) {
101+
reporter.removeAdapter(responseWrite)
102+
const emptyTestSuite = (results.failed + results.success) === 0 ? 0 : 1
103+
response.end(constant.EXIT_CODE + emptyTestSuite + results.exitCode)
104+
})
105+
})
106+
}
99107
}
100108
}
101109

‎test/unit/executor.spec.js

+71-20
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ const BrowserCollection = require('../../lib/browser_collection')
55
const EventEmitter = require('../../lib/events').EventEmitter
66
const Executor = require('../../lib/executor')
77

8+
const log = require('../../lib/logger').create()
9+
810
describe('executor', () => {
911
let emitter
1012
let capturedBrowsers
@@ -21,36 +23,85 @@ describe('executor', () => {
2123
executor.socketIoSockets = new EventEmitter()
2224

2325
spy = {
24-
onRunStart: () => null,
25-
onSocketsExecute: () => null
26+
onRunStart: sinon.stub(),
27+
onSocketsExecute: sinon.stub(),
28+
onRunComplete: sinon.stub()
2629
}
27-
28-
sinon.spy(spy, 'onRunStart')
29-
sinon.spy(spy, 'onSocketsExecute')
30+
sinon.stub(log, 'warn')
3031

3132
emitter.on('run_start', spy.onRunStart)
33+
emitter.on('run_complete', spy.onRunComplete)
3234
executor.socketIoSockets.on('execute', spy.onSocketsExecute)
3335
})
3436

35-
it('should start the run and pass client config', () => {
36-
capturedBrowsers.areAllReady = () => true
37+
describe('schedule', () => {
38+
it('should start the run and pass client config', () => {
39+
capturedBrowsers.areAllReady = () => true
40+
41+
executor.schedule()
42+
expect(spy.onRunStart).to.have.been.called
43+
expect(spy.onSocketsExecute).to.have.been.calledWith(config.client)
44+
})
45+
46+
it('should wait for all browsers to finish', () => {
47+
capturedBrowsers.areAllReady = () => false
3748

38-
executor.schedule()
39-
expect(spy.onRunStart).to.have.been.called
40-
expect(spy.onSocketsExecute).to.have.been.calledWith(config.client)
49+
// they are not ready yet
50+
executor.schedule()
51+
expect(spy.onRunStart).not.to.have.been.called
52+
expect(spy.onSocketsExecute).not.to.have.been.called
53+
54+
capturedBrowsers.areAllReady = () => true
55+
emitter.emit('run_complete')
56+
expect(spy.onRunStart).to.have.been.called
57+
expect(spy.onSocketsExecute).to.have.been.called
58+
})
4159
})
4260

43-
it('should wait for all browsers to finish', () => {
44-
capturedBrowsers.areAllReady = () => false
61+
describe('scheduleError', () => {
62+
it('should return `true` if scheduled synchronously', () => {
63+
const result = executor.scheduleError('expected error')
64+
expect(result).to.be.true
65+
})
66+
67+
it('should emit both "run_start" and "run_complete"', () => {
68+
executor.scheduleError('expected error')
69+
expect(spy.onRunStart).to.have.been.called
70+
expect(spy.onRunComplete).to.have.been.called
71+
expect(spy.onRunStart).to.have.been.calledBefore(spy.onRunComplete)
72+
})
73+
74+
it('should report the error', () => {
75+
const expectedError = 'expected error'
76+
executor.scheduleError(expectedError)
77+
expect(spy.onRunComplete).to.have.been.calledWith([], {
78+
success: 0,
79+
failed: 0,
80+
skipped: 0,
81+
error: expectedError,
82+
exitCode: 1
83+
})
84+
})
85+
86+
it('should wait for scheduled runs to end before reporting the error', () => {
87+
// Arrange
88+
let browsersAreReady = true
89+
const expectedError = 'expected error'
90+
capturedBrowsers.areAllReady = () => browsersAreReady
91+
executor.schedule()
92+
browsersAreReady = false
4593

46-
// they are not ready yet
47-
executor.schedule()
48-
expect(spy.onRunStart).not.to.have.been.called
49-
expect(spy.onSocketsExecute).not.to.have.been.called
94+
// Act
95+
const result = executor.scheduleError(expectedError)
96+
browsersAreReady = true
5097

51-
capturedBrowsers.areAllReady = () => true
52-
emitter.emit('run_complete')
53-
expect(spy.onRunStart).to.have.been.called
54-
expect(spy.onSocketsExecute).to.have.been.called
98+
// Assert
99+
expect(result).to.be.false
100+
expect(spy.onRunComplete).to.not.have.been.called
101+
emitter.emit('run_complete')
102+
expect(spy.onRunComplete).to.have.been.calledWith([], sinon.match({
103+
error: expectedError
104+
}))
105+
})
55106
})
56107
})

‎test/unit/middleware/runner.spec.js

+90-57
Original file line numberDiff line numberDiff line change
@@ -55,16 +55,23 @@ describe('middleware.runner', () => {
5555
}
5656

5757
executor = {
58-
schedule: () => emitter.emit('run_start')
58+
scheduled: false,
59+
schedule: () => {
60+
executor.scheduled = true
61+
emitter.emit('run_start')
62+
if (executor.onSchedule) {
63+
executor.onSchedule()
64+
}
65+
}
5966
}
6067

6168
emitter = new EventEmitter()
6269
capturedBrowsers = new BrowserCollection(emitter)
6370
fileListMock = {
64-
refresh: () => Promise.resolve(),
65-
addFile: () => null,
66-
removeFile: () => null,
67-
changeFile: () => null
71+
refresh: sinon.stub(),
72+
addFile: sinon.stub(),
73+
removeFile: sinon.stub(),
74+
changeFile: sinon.stub()
6875
}
6976

7077
nextSpy = sinon.spy()
@@ -82,68 +89,90 @@ describe('middleware.runner', () => {
8289
sinon.stub(capturedBrowsers, 'areAllReady').callsFake(() => true)
8390

8491
response.once('end', () => {
85-
expect(nextSpy).to.not.have.been.called
86-
expect(response).to.beServedAs(200, 'result\x1FEXIT10')
87-
done()
92+
try {
93+
expect(nextSpy).to.not.have.been.called
94+
expect(response).to.beServedAs(200, 'result\x1FEXIT10')
95+
done()
96+
} catch (err) {
97+
done(err)
98+
}
8899
})
89100

90101
handler(new HttpRequestMock('/__run__'), response, nextSpy)
91102

92-
mockReporter.write('result')
93-
emitter.emit('run_complete', capturedBrowsers, { exitCode: 0 })
103+
executor.onSchedule = () => {
104+
mockReporter.write('result')
105+
emitter.emit('run_complete', capturedBrowsers, { exitCode: 0 })
106+
}
94107
})
95108

96109
it('should set the empty to 0 if empty results', (done) => {
97110
capturedBrowsers.add(new Browser())
98111
sinon.stub(capturedBrowsers, 'areAllReady').callsFake(() => true)
99112

100113
response.once('end', () => {
101-
expect(nextSpy).to.not.have.been.called
102-
expect(response).to.beServedAs(200, 'result\x1FEXIT00')
103-
done()
114+
try {
115+
expect(nextSpy).to.not.have.been.called
116+
expect(response).to.beServedAs(200, 'result\x1FEXIT00')
117+
done()
118+
} catch (err) {
119+
done(err)
120+
}
104121
})
105122

106123
handler(new HttpRequestMock('/__run__'), response, nextSpy)
107124

108-
mockReporter.write('result')
109-
emitter.emit('run_complete', capturedBrowsers, { exitCode: 0, success: 0, failed: 0 })
125+
executor.onSchedule = () => {
126+
mockReporter.write('result')
127+
emitter.emit('run_complete', capturedBrowsers, { exitCode: 0, success: 0, failed: 0 })
128+
}
110129
})
111130

112131
it('should set the empty to 1 if successful tests', (done) => {
113132
capturedBrowsers.add(new Browser())
114133
sinon.stub(capturedBrowsers, 'areAllReady').callsFake(() => true)
115134

116135
response.once('end', () => {
117-
expect(nextSpy).to.not.have.been.called
118-
expect(response).to.beServedAs(200, 'result\x1FEXIT10')
119-
done()
136+
try {
137+
expect(nextSpy).to.not.have.been.called
138+
expect(response).to.beServedAs(200, 'result\x1FEXIT10')
139+
done()
140+
} catch (err) {
141+
done(err)
142+
}
120143
})
121144

122145
handler(new HttpRequestMock('/__run__'), response, nextSpy)
123146

124-
mockReporter.write('result')
125-
emitter.emit('run_complete', capturedBrowsers, { exitCode: 0, success: 3, failed: 0 })
147+
executor.onSchedule = () => {
148+
mockReporter.write('result')
149+
emitter.emit('run_complete', capturedBrowsers, { exitCode: 0, success: 3, failed: 0 })
150+
}
126151
})
127152

128153
it('should set the empty to 1 if failed tests', (done) => {
129154
capturedBrowsers.add(new Browser())
130155
sinon.stub(capturedBrowsers, 'areAllReady').callsFake(() => true)
131156

132157
response.once('end', () => {
133-
expect(nextSpy).to.not.have.been.called
134-
expect(response).to.beServedAs(200, 'result\x1FEXIT10')
135-
done()
158+
try {
159+
expect(nextSpy).to.not.have.been.called
160+
expect(response).to.beServedAs(200, 'result\x1FEXIT10')
161+
done()
162+
} catch (err) {
163+
done(err)
164+
}
136165
})
137166

138167
handler(new HttpRequestMock('/__run__'), response, nextSpy)
139168

140-
mockReporter.write('result')
141-
emitter.emit('run_complete', capturedBrowsers, { exitCode: 0, success: 0, failed: 6 })
169+
executor.onSchedule = () => {
170+
mockReporter.write('result')
171+
emitter.emit('run_complete', capturedBrowsers, { exitCode: 0, success: 0, failed: 6 })
172+
}
142173
})
143174

144175
it('should not run if there is no browser captured', (done) => {
145-
sinon.stub(fileListMock, 'refresh')
146-
147176
response.once('end', () => {
148177
expect(nextSpy).to.not.have.been.called
149178
expect(response).to.beServedAs(200, 'No captured browser, open http://localhost:8877/\n')
@@ -156,11 +185,7 @@ describe('middleware.runner', () => {
156185

157186
it('should refresh explicit files if specified', (done) => {
158187
capturedBrowsers.add(new Browser())
159-
sinon.stub(capturedBrowsers, 'areAllReady').callsFake(() => true)
160-
sinon.stub(fileListMock, 'refresh')
161-
sinon.stub(fileListMock, 'addFile')
162-
sinon.stub(fileListMock, 'changeFile')
163-
sinon.stub(fileListMock, 'removeFile')
188+
sinon.stub(capturedBrowsers, 'areAllReady').returns(true)
164189

165190
const RAW_MESSAGE = JSON.stringify({
166191
addedFiles: ['/new.js'],
@@ -178,50 +203,43 @@ describe('middleware.runner', () => {
178203
request.emit('data', RAW_MESSAGE)
179204
request.emit('end')
180205

181-
process.nextTick(() => {
206+
executor.onSchedule = () => {
182207
expect(fileListMock.refresh).not.to.have.been.called
183208
expect(fileListMock.addFile).to.have.been.calledWith(path.resolve('/new.js'))
184209
expect(fileListMock.removeFile).to.have.been.calledWith(path.resolve('/foo.js'))
185210
expect(fileListMock.removeFile).to.have.been.calledWith(path.resolve('/bar.js'))
186211
expect(fileListMock.changeFile).to.have.been.calledWith(path.resolve('/changed.js'))
187212
done()
188-
})
213+
}
189214
})
190215

191216
it('should wait for refresh to finish if applicable before scheduling execution', (done) => {
192217
capturedBrowsers.add(new Browser())
193218
sinon.stub(capturedBrowsers, 'areAllReady').callsFake(() => true)
194219

195220
let res = null
196-
const fileListPromise = new Promise((resolve, reject) => {
221+
const fileListPromise = new Promise((resolve) => {
197222
res = resolve
198223
})
199-
sinon.stub(fileListMock, 'refresh').returns(fileListPromise)
200-
sinon.stub(executor, 'schedule')
224+
fileListMock.refresh.returns(fileListPromise)
201225

202226
const request = new HttpRequestMock('/__run__')
203227
handler(request, response, nextSpy)
204228

205229
process.nextTick(() => {
206230
expect(fileListMock.refresh).to.have.been.called
207-
expect(executor.schedule).to.not.have.been.called
231+
expect(executor.scheduled).to.be.false
208232

209-
// Now try resolving the promise
233+
executor.onSchedule = done
234+
// Now resolving the promise
210235
res()
211-
setTimeout(() => {
212-
expect(executor.schedule).to.have.been.called
213-
done()
214-
}, 2)
215236
})
216237
})
217238

218239
it('should schedule execution if no refresh', (done) => {
219240
capturedBrowsers.add(new Browser())
220241
sinon.stub(capturedBrowsers, 'areAllReady').callsFake(() => true)
221242

222-
sinon.spy(fileListMock, 'refresh')
223-
sinon.stub(executor, 'schedule')
224-
225243
const RAW_MESSAGE = JSON.stringify({ refresh: false })
226244

227245
const request = new HttpRequestMock('/__run__', {
@@ -234,11 +252,14 @@ describe('middleware.runner', () => {
234252
request.emit('data', RAW_MESSAGE)
235253
request.emit('end')
236254

237-
process.nextTick(() => {
238-
expect(fileListMock.refresh).not.to.have.been.called
239-
expect(executor.schedule).to.have.been.called
240-
done()
241-
})
255+
executor.onSchedule = () => {
256+
try {
257+
expect(fileListMock.refresh).not.to.have.been.called
258+
done()
259+
} catch (err) {
260+
done(err)
261+
}
262+
}
242263
})
243264

244265
it('should not schedule execution if refreshing and autoWatch', (done) => {
@@ -247,16 +268,12 @@ describe('middleware.runner', () => {
247268
capturedBrowsers.add(new Browser())
248269
sinon.stub(capturedBrowsers, 'areAllReady').callsFake(() => true)
249270

250-
sinon.spy(fileListMock, 'refresh')
251-
sinon.stub(executor, 'schedule')
252-
253271
handler(new HttpRequestMock('/__run__'), response, nextSpy)
254272

255-
process.nextTick(() => {
273+
executor.onSchedule = () => {
256274
expect(fileListMock.refresh).to.have.been.called
257-
expect(executor.schedule).not.to.have.been.called
258275
done()
259-
})
276+
}
260277
})
261278

262279
it('should ignore other urls', (done) => {
@@ -265,6 +282,22 @@ describe('middleware.runner', () => {
265282
done()
266283
})
267284
})
285+
286+
it('should scheduleError when file list rejects', (done) => {
287+
const error = new Error('expected error for testing')
288+
capturedBrowsers.add(new Browser())
289+
sinon.stub(capturedBrowsers, 'areAllReady').returns(true)
290+
fileListMock.refresh.rejects(error)
291+
handler(new HttpRequestMock('/__run__'), response, nextSpy)
292+
executor.scheduleError = (errorMessage) => {
293+
try {
294+
expect(errorMessage).eq(`Error during refresh file list. ${error.stack}`)
295+
done()
296+
} catch (err) {
297+
done(err)
298+
}
299+
}
300+
})
268301
})
269302

270303
describe('', () => {

0 commit comments

Comments
 (0)
Please sign in to comment.