From 4d669bf36b091e8808c9a280900fe19c8b2a72cc Mon Sep 17 00:00:00 2001 From: Vojta Jina Date: Wed, 25 Dec 2013 17:07:23 -0800 Subject: [PATCH] feat(preprocessor): allow preprocessor to cancel test run Now a preprocessor can return an error, which - stops preprocessing (does not trigger other preprocessors), - cancels the test run. This changes the preprocessor API - preprocessor's `done` function accepts two arguments: - an error (`null` if no error), - content. The change is done in a backwards compatible way - if a preprocessor calls `done` with a single string argument, it is treated as no error. Closes #550 --- lib/file_list.js | 48 +++++++++- lib/middleware/karma.js | 8 ++ lib/preprocessor.js | 14 ++- lib/server.js | 2 + test/unit/file_list.spec.coffee | 149 +++++++++++++++++++++++++++++ test/unit/mocha-globals.coffee | 17 +++- test/unit/preprocessor.spec.coffee | 45 ++++++++- 7 files changed, 274 insertions(+), 9 deletions(-) diff --git a/lib/file_list.js b/lib/file_list.js index 954b5c52c..64bed149a 100644 --- a/lib/file_list.js +++ b/lib/file_list.js @@ -68,6 +68,22 @@ var List = function(patterns, excludes, emitter, preprocess, batchInterval) { var pendingDeferred; var pendingTimeout; + var errors = []; + + var addError = function(path) { + if (errors.indexOf(path) === -1) { + errors.push(path); + } + }; + + var removeError = function(path) { + var idx = errors.indexOf(path); + + if (idx !== -1) { + errors.splice(idx, 1); + } + }; + var resolveFiles = function(buckets) { var uniqueMap = {}; var files = { @@ -99,7 +115,12 @@ var List = function(patterns, excludes, emitter, preprocess, batchInterval) { clearTimeout(pendingTimeout); } - pendingDeferred.resolve(files || resolveFiles(self.buckets)); + if (!errors.length) { + pendingDeferred.resolve(files || resolveFiles(self.buckets)); + } else { + pendingDeferred.reject(errors.slice()); + } + pendingDeferred = pendingTimeout = null; }; @@ -143,6 +164,8 @@ var List = function(patterns, excludes, emitter, preprocess, batchInterval) { } }; + errors = []; + if (!pendingDeferred) { pendingDeferred = q.defer(); emitter.emit('file_list_modified', pendingDeferred.promise); @@ -193,8 +216,13 @@ var List = function(patterns, excludes, emitter, preprocess, batchInterval) { // TODO(vojta): reuse file objects var file = new File(path, stat.mtime); - preprocess(file, function() { + preprocess(file, function(err) { buckets[i].push(file); + + if (err) { + addError(path); + } + finish(); }); } else { @@ -279,9 +307,14 @@ var List = function(patterns, excludes, emitter, preprocess, batchInterval) { if (self.buckets === buckets) { addedFile.mtime = stat.mtime; - return preprocess(addedFile, function() { + return preprocess(addedFile, function(err) { // TODO(vojta): ignore if refresh/reload happens log.info('Added file "%s".', path); + + if (err) { + addError(path); + } + fireEventAndDefer(); done(); }); @@ -336,7 +369,13 @@ var List = function(patterns, excludes, emitter, preprocess, batchInterval) { // 1/ the preprocessor should not change the object in place, but create a copy that would // be eventually merged into the original file, here in the callback, synchronously. // 2/ delay the promise resolution - wait for any changeFile operations to finish - return preprocess(changedFile, function() { + return preprocess(changedFile, function(err) { + if (err) { + addError(path); + } else { + removeError(path); + } + // TODO(vojta): ignore if refresh/reload happens fireEventAndDefer(); done(); @@ -364,6 +403,7 @@ var List = function(patterns, excludes, emitter, preprocess, batchInterval) { if (buckets[i][j].originalPath === path) { buckets[i].splice(j, 1); log.info('Removed file "%s".', path); + removeError(path); fireEventAndDefer(); return done(); } diff --git a/lib/middleware/karma.js b/lib/middleware/karma.js index 31c5509c2..150a89d8e 100644 --- a/lib/middleware/karma.js +++ b/lib/middleware/karma.js @@ -110,6 +110,14 @@ var createKarmaMiddleware = function(filesPromise, serveStaticFile, return data.replace('%SCRIPTS%', scriptTags.join('\n')).replace('%MAPPINGS%', mappings); }); + }, function(errorFiles) { + serveStaticFile(requestUrl, response, function(data) { + common.setNoCacheHeaders(response); + return data.replace('%SCRIPTS%', '').replace('%MAPPINGS%', + 'window.__karma__.error("TEST RUN WAS CANCELLED because ' + + (errorFiles.length > 1 ? 'these files contain' : 'this file contains') + + ' some errors:\\n ' + errorFiles.join('\\n ') + '");'); + }); }); } diff --git a/lib/preprocessor.js b/lib/preprocessor.js index 76caaa02f..d1d1425c2 100644 --- a/lib/preprocessor.js +++ b/lib/preprocessor.js @@ -17,7 +17,19 @@ var createPreprocessor = function(config, basePath, injector) { return function(file, done) { var preprocessors = []; - var nextPreprocessor = function(content) { + var nextPreprocessor = function(error, content) { + // normalize B-C + if (arguments.length === 1 && typeof error === 'string') { + content = error; + error = null; + } + + if (error) { + file.content = null; + file.contentPath = null; + return done(error); + } + if (!preprocessors.length) { file.contentPath = null; file.content = content; diff --git a/lib/server.js b/lib/server.js index 1ca460979..0230c2e53 100644 --- a/lib/server.js +++ b/lib/server.js @@ -34,6 +34,8 @@ var start = function(injector, config, launcher, globalEmitter, preprocess, file if (config.autoWatch) { filesPromise.then(function() { injector.invoke(watcher.watch); + }, function() { + injector.invoke(watcher.watch); }); } diff --git a/test/unit/file_list.spec.coffee b/test/unit/file_list.spec.coffee index 427260daa..f2e228aad 100644 --- a/test/unit/file_list.spec.coffee +++ b/test/unit/file_list.spec.coffee @@ -19,6 +19,7 @@ describe 'file_list', -> '**': ['/a.txt', '/b.txt', '/c.txt', '/a.txt', '/c.txt', '/b.txt', '/a.txt', '/c.txt', '/a.txt', '/a.txt', '/c.txt'] + # TODO(vojta): create new fs, as we mutate the file stats now mockFs = mocks.fs.create some: '0.js': mocks.fs.file '2012-04-04' @@ -153,6 +154,20 @@ describe 'file_list', -> done() + it 'should reject the promise if a preprocessor fails', (done) -> + preprocessMock = (file, next) -> + next new Error('Prerocess failure.'), null + + # MATCH /some/a.js, /some/b.js + list = new m.List patterns('/some/*.js'), [], emitter, preprocessMock + spyResolve = sinon.spy() + spyReject = sinon.spy() + + list.refresh().then(spyResolve, spyReject).done -> + expect(spyResolve).not.to.have.been.called + expect(spyReject).to.have.been.called + done() + #============================================================================ # List.reload() @@ -436,6 +451,140 @@ describe 'file_list', -> expect(onFileListModifiedSpy).not.to.have.been.called + describe 'preprocess failure', -> + spyResolve = spyReject = null + + preprocessMock2 = (file, next) -> + matchFile = (pattern) -> + pattern.test and pattern.test(file.path) or pattern is file.path + + if preprocessMock2._fail.some matchFile + next new Error('Failed to preprocess.'), null + else + next() + + preprocessMock2.fail = (pattern) -> preprocessMock2._fail.push(pattern) + preprocessMock2.fix = (pattern) -> + preprocessMock2._fail = preprocessMock2._fail.filter((p) -> pattern is not p) + + + beforeEach -> + spyResolve = sinon.spy() + spyReject = sinon.spy() + preprocessMock2._fail = [] + + mockFs._touchFile '/some/a.js', '2012-04-04' + mockFs._touchFile '/some/b.js', '2012-05-05' + + + it 'should reject when an incorrect file added', (done) -> + # MATCH: /some/a.js, /some/b.js, /a.txt + list = new m.List patterns('/some/*.js', '/a.*'), [], emitter, preprocessMock2 + + # once files are resolved, execute next item in the queue + emitter.on 'file_list_modified', (files) -> files.then(spyResolve, spyReject).done next + + # refresh the list and kick off the queue + list.refresh() + + scheduleNext -> + spyResolve.reset() + spyReject.reset() + preprocessMock2.fail '/some/0.js' + list.addFile '/some/0.js' + + scheduleNext -> + expect(spyResolve).not.to.have.been.called + expect(spyReject).to.have.been.called + done() + + + it 'should resolve once all the files are fixed', (done) -> + # MATCH: /some/a.js, /some/b.js, /a.txt + list = new m.List patterns('/some/*.js', '/a.*'), [], emitter, preprocessMock2 + + # once files are resolved, execute next item in the queue + emitter.on 'file_list_modified', (files) -> files.then(spyResolve, spyReject).done next + + # refresh the list and kick off the queue + list.refresh() + + scheduleNext -> + preprocessMock2.fail '/some/a.js' + mockFs._touchFile '/some/a.js', '2020-01-01' + list.changeFile '/some/a.js' + + scheduleNext -> + spyResolve.reset() + spyReject.reset() + preprocessMock2.fix '/some/a.js' + mockFs._touchFile '/some/a.js', '2020-01-02' + list.changeFile '/some/a.js' + + scheduleNext -> + expect(spyResolve).to.have.been.called + expect(spyReject).not.to.have.been.called + done() + + + it 'should reject if only some files are fixed', (done) -> + # MATCH: /some/a.js, /some/b.js, /a.txt + list = new m.List patterns('/some/*.js', '/a.*'), [], emitter, preprocessMock2 + + # once files are resolved, execute next item in the queue + emitter.on 'file_list_modified', (files) -> files.then(spyResolve, spyReject).done next + + # refresh the list and kick off the queue + list.refresh() + + scheduleNext -> + preprocessMock2.fail '/some/a.js' + preprocessMock2.fail '/some/b.js' + mockFs._touchFile '/some/a.js', '2020-01-01' + list.changeFile '/some/a.js' + mockFs._touchFile '/some/b.js', '2020-01-01' + list.changeFile '/some/b.js' + + scheduleNext -> + spyResolve.reset() + spyReject.reset() + preprocessMock2.fix '/some/a.js' + mockFs._touchFile '/some/a.js', '2020-01-02' + list.changeFile '/some/a.js' + + scheduleNext -> + # /some/b.js still contains error + expect(spyResolve).not.to.have.been.called + expect(spyReject).to.have.been.called + done() + + + it 'should resolve if incorrect file is removed', (done) -> + # MATCH: /some/a.js, /some/b.js, /a.txt + list = new m.List patterns('/some/*.js', '/a.*'), [], emitter, preprocessMock2 + + # once files are resolved, execute next item in the queue + emitter.on 'file_list_modified', (files) -> files.then(spyResolve, spyReject).done next + + # refresh the list and kick off the queue + list.refresh() + + scheduleNext -> + preprocessMock2.fail '/some/a.js' + mockFs._touchFile '/some/a.js', '2020-01-01' + list.changeFile '/some/a.js' + + scheduleNext -> + spyResolve.reset() + spyReject.reset() + list.removeFile '/some/a.js' + + scheduleNext -> + expect(spyResolve).to.have.been.called + expect(spyReject).not.to.have.been.called + done() + + #============================================================================ # Batch Interval processing #============================================================================ diff --git a/test/unit/mocha-globals.coffee b/test/unit/mocha-globals.coffee index d1f05a8c2..672c50e28 100644 --- a/test/unit/mocha-globals.coffee +++ b/test/unit/mocha-globals.coffee @@ -47,13 +47,28 @@ chai.use (chai, utils) -> # TODO(vojta): move it somewhere ;-) nextTickQueue = [] nextTickCallback = -> + if not nextTickQueue.length then throw new Error 'Nothing scheduled!' nextTickQueue.shift()() - if nextTickQueue.length then process.nextTick nextTickCallback global.scheduleNextTick = (action) -> nextTickQueue.push action if nextTickQueue.length is 1 then process.nextTick nextTickCallback +nextQueue = [] +nextCallback = -> + if not nextQueue.length then throw new Error 'Nothing scheduled!' + nextQueue.shift()() + +global.scheduleNextTick = (action) -> + nextTickQueue.push action + if nextTickQueue.length is 1 then process.nextTick nextTickCallback + +global.scheduleNext = (action) -> + nextQueue.push action + +global.next = nextCallback + beforeEach -> nextTickQueue.length = 0 + nextQueue.length = 0 diff --git a/test/unit/preprocessor.spec.coffee b/test/unit/preprocessor.spec.coffee index f9d040afc..ed77b6cc0 100644 --- a/test/unit/preprocessor.spec.coffee +++ b/test/unit/preprocessor.spec.coffee @@ -23,7 +23,7 @@ describe 'preprocessor', -> it 'should preprocess matching file', (done) -> fakePreprocessor = sinon.spy (content, file, done) -> file.path = file.path + '-preprocessed' - done 'new-content' + done null, 'new-content' injector = new di.Injector [{'preprocessor:fake': ['factory', -> fakePreprocessor]}] pp = m.createPreprocessor {'**/*.js': ['fake']}, null, injector @@ -39,7 +39,7 @@ describe 'preprocessor', -> it 'should ignore not matching file', (done) -> fakePreprocessor = sinon.spy (content, file, done) -> - done '' + done null, '' injector = new di.Injector [{'preprocessor:fake': ['factory', -> fakePreprocessor]}] pp = m.createPreprocessor {'**/*.js': ['fake']}, null, injector @@ -54,7 +54,7 @@ describe 'preprocessor', -> it 'should apply all preprocessors', (done) -> fakePreprocessor1 = sinon.spy (content, file, done) -> file.path = file.path + '-p1' - done content + '-c1' + done null, content + '-c1' fakePreprocessor2 = sinon.spy (content, file, done) -> file.path = file.path + '-p2' @@ -94,3 +94,42 @@ describe 'preprocessor', -> expect(file.sha.length).to.equal 40 expect(file.sha).not.to.equal previousSHA done() + + + it 'should return error if any preprocessor fails', (done) -> + failingPreprocessor = sinon.spy (content, file, done) -> + done new Error('Some error'), null + + injector = new di.Injector [{ + 'preprocessor:failing': ['factory', -> failingPreprocessor] + }] + + pp = m.createPreprocessor {'**/*.js': ['failing']}, null, injector + + file = {originalPath: '/some/a.js', path: 'path'} + + pp file, (err) -> + expect(err).to.exist + done() + + + it 'should stop preprocessing after an error', (done) -> + failingPreprocessor = sinon.spy (content, file, done) -> + done new Error('Some error'), null + + fakePreprocessor = sinon.spy (content, file, done) -> + done null, content + + + injector = new di.Injector [{ + 'preprocessor:failing': ['factory', -> failingPreprocessor] + 'preprocessor:fake': ['factory', -> fakePreprocessor] + }] + + pp = m.createPreprocessor {'**/*.js': ['failing', 'fake']}, null, injector + + file = {originalPath: '/some/a.js', path: 'path'} + + pp file, (err) -> + expect(fakePreprocessor).not.to.have.been.called + done()