From 2812c6f9e901d65f79f5599d10102507dfdbbd08 Mon Sep 17 00:00:00 2001 From: johnjbarton Date: Tue, 30 Apr 2019 14:41:35 -0700 Subject: [PATCH] feat(preprocessor): preprocessor_priority execution order. Between 3.x and 4.x we moved from combineLists to underscore.union in preprocessor. Apparently combineLists was incorrect: the order of preprocessing changed in some cases. Conceptually the order ought to depend upon the preprocessor, not the file. Implement config.preprocessor_priority['preprocessor-name'] = priority, higher means run earlier. Default priority is 0. Add back compat API for karma-browserify. Update docs. --- docs/config/04-preprocessors.md | 5 +- lib/config.js | 1 + lib/preprocessor.js | 20 ++++++-- lib/server.js | 2 +- test/unit/preprocessor.spec.js | 88 +++++++++++++++++++++++++-------- 5 files changed, 89 insertions(+), 27 deletions(-) diff --git a/docs/config/04-preprocessors.md b/docs/config/04-preprocessors.md index 72bd9c3f7..7854dfdb8 100644 --- a/docs/config/04-preprocessors.md +++ b/docs/config/04-preprocessors.md @@ -108,8 +108,9 @@ preprocessors: { Then karma will execute `'a'` before executing `'b'`. -If a file matches multiple keys, karma will do its best to execute the -preprocessors in a reasonable order. So if you have: +If a file matches multiple keys, karma will use the `config.preprocessor_priority` +map to set the order. If this config option is not set, karma do its best to +execute the preprocessors in a reasonable order. So if you have: ```js preprocessors: { diff --git a/lib/config.js b/lib/config.js index b69c73a41..adf9840e2 100644 --- a/lib/config.js +++ b/lib/config.js @@ -302,6 +302,7 @@ class Config { this.proxies = {} this.proxyValidateSSL = true this.preprocessors = {} + this.preprocessor_priority = {} this.urlRoot = '/' this.upstreamProxy = undefined this.reportSlowerThan = 0 diff --git a/lib/preprocessor.js b/lib/preprocessor.js index 61c34e065..443dd3970 100644 --- a/lib/preprocessor.js +++ b/lib/preprocessor.js @@ -33,7 +33,7 @@ function createNextProcessor (preprocessors, file, done) { } } -function createPreprocessor (config, basePath, injector) { +function createPriorityPreprocessor (config, preprocessorPriority, basePath, injector) { const emitter = injector.get('emitter') const alreadyDisplayedErrors = {} const instances = {} @@ -96,9 +96,15 @@ function createPreprocessor (config, basePath, injector) { } }) + // Apply preprocessor priority. + let sortedPreprocessorNames = preprocessorNames + .map((name) => [name, preprocessorPriority[name] || 0]) + .sort((a, b) => b[1] - a[1]) + .map((duo) => duo[0]) + let preprocessors = [] const nextPreprocessor = createNextProcessor(preprocessors, file, done) - preprocessorNames.forEach((name) => { + sortedPreprocessorNames.forEach((name) => { const p = instances[name] || instantiatePreprocessor(name) if (p == null) { @@ -125,6 +131,14 @@ function createPreprocessor (config, basePath, injector) { } } +// Deprecated API +function createPreprocessor (preprocessors, basePath, injector) { + console.log('Deprecated private createPreprocessor() API') + const preprocessorPriority = injector.get('config.preprocessor_priority') + return createPriorityPreprocessor(preprocessors, preprocessorPriority, basePath, injector) +} createPreprocessor.$inject = ['config.preprocessors', 'config.basePath', 'injector'] - exports.createPreprocessor = createPreprocessor + +createPriorityPreprocessor.$inject = ['config.preprocessors', 'config.preprocessor_priority', 'config.basePath', 'injector'] +exports.createPriorityPreprocessor = createPriorityPreprocessor diff --git a/lib/server.js b/lib/server.js index ed7662384..fdbb6c19e 100644 --- a/lib/server.js +++ b/lib/server.js @@ -75,7 +75,7 @@ class Server extends KarmaEventEmitter { watcher: ['value', watcher], launcher: ['type', Launcher], config: ['value', config], - preprocess: ['factory', preprocessor.createPreprocessor], + preprocess: ['factory', preprocessor.createPriorityPreprocessor], fileList: ['factory', FileList.factory], webServer: ['factory', createWebServer], serveFile: ['factory', createServeFile], diff --git a/test/unit/preprocessor.spec.js b/test/unit/preprocessor.spec.js index a02274447..15d6dc850 100644 --- a/test/unit/preprocessor.spec.js +++ b/test/unit/preprocessor.spec.js @@ -47,7 +47,7 @@ describe('preprocessor', () => { 'factory', function () { return fakePreprocessor } ] }, emitterSetting]) - pp = m.createPreprocessor({ '**/*.js': ['fake'] }, null, injector) + pp = m.createPriorityPreprocessor({ '**/*.js': ['fake'] }, {}, null, injector) const file = { originalPath: '/some/a.js', path: 'path' } @@ -68,7 +68,7 @@ describe('preprocessor', () => { const injector = new di.Injector([{ 'preprocessor:fake': ['factory', function () { return fakePreprocessor }] }, emitterSetting]) - pp = m.createPreprocessor({ '**/*.js': ['fake'] }, null, injector) + pp = m.createPriorityPreprocessor({ '**/*.js': ['fake'] }, {}, null, injector) const file = { originalPath: '/some/.dir/a.js', path: 'path' } @@ -90,7 +90,7 @@ describe('preprocessor', () => { 'preprocessor:fake': ['factory', function () { return fakePreprocessor }] }, emitterSetting]) const config = { '**/*.txt': ['fake'] } - pp = m.createPreprocessor(config, null, injector) + pp = m.createPriorityPreprocessor(config, {}, null, injector) const file = { originalPath: '/some/a.js', path: 'path' } @@ -112,7 +112,7 @@ describe('preprocessor', () => { const injector = new di.Injector([{ 'preprocessor:fake': ['factory', function () { return fakePreprocessor }] }, emitterSetting]) - pp = m.createPreprocessor({ '**/*.js': ['fake'] }, null, injector) + pp = m.createPriorityPreprocessor({ '**/*.js': ['fake'] }, {}, null, injector) const file = { originalPath: '/some/a.txt', path: 'path' } @@ -138,7 +138,7 @@ describe('preprocessor', () => { 'preprocessor:fake2': ['factory', function () { return fakePreprocessor2 }] }, emitterSetting]) - pp = m.createPreprocessor({ '**/*.js': ['fake1', 'fake2'] }, null, injector) + pp = m.createPriorityPreprocessor({ '**/*.js': ['fake1', 'fake2'] }, {}, null, injector) const file = { originalPath: '/some/a.js', path: 'path' } @@ -152,7 +152,7 @@ describe('preprocessor', () => { }) it('should compute SHA', (done) => { - pp = m.createPreprocessor({}, null, new di.Injector([emitterSetting])) + pp = m.createPriorityPreprocessor({}, {}, null, new di.Injector([emitterSetting])) const file = { originalPath: '/some/a.js', path: 'path' } pp(file, () => { @@ -182,7 +182,7 @@ describe('preprocessor', () => { 'preprocessor:fake': ['factory', function () { return fakePreprocessor }] }, emitterSetting]) - pp = m.createPreprocessor({ '**/a.js': ['fake'] }, null, injector) + pp = m.createPriorityPreprocessor({ '**/a.js': ['fake'] }, {}, null, injector) const fileProcess = { originalPath: '/some/a.js', path: 'path' } const fileSkip = { originalPath: '/some/b.js', path: 'path' } @@ -208,7 +208,7 @@ describe('preprocessor', () => { 'preprocessor:failing': ['factory', function () { return failingPreprocessor }] }, emitterSetting]) - pp = m.createPreprocessor({ '**/*.js': ['failing'] }, null, injector) + pp = m.createPriorityPreprocessor({ '**/*.js': ['failing'] }, {}, null, injector) const file = { originalPath: '/some/a.js', path: 'path' } @@ -232,7 +232,7 @@ describe('preprocessor', () => { 'preprocessor:fake': ['factory', function () { return fakePreprocessor }] }, emitterSetting]) - pp = m.createPreprocessor({ '**/*.js': ['failing', 'fake'] }, null, injector) + pp = m.createPriorityPreprocessor({ '**/*.js': ['failing', 'fake'] }, {}, null, injector) const file = { originalPath: '/some/a.js', path: 'path' } @@ -261,7 +261,7 @@ describe('preprocessor', () => { 'preprocessor:fake': ['factory', function () { return fakePreprocessor }] }, emitterSetting]) - const pp = m.createPreprocessor({ '**/*.js': ['fake'] }, null, injector) + const pp = m.createPriorityPreprocessor({ '**/*.js': ['fake'] }, {}, null, injector) pp(file, () => { expect(fakePreprocessor).to.have.been.called @@ -277,7 +277,7 @@ describe('preprocessor', () => { it('should throw after 3 retries', (done) => { const injector = new di.Injector([{}, emitterSetting]) - const pp = m.createPreprocessor({ '**/*.js': [] }, null, injector) + const pp = m.createPriorityPreprocessor({ '**/*.js': [] }, {}, null, injector) pp(file, () => { }) @@ -299,7 +299,7 @@ describe('preprocessor', () => { 'preprocessor:fake': ['factory', function () { return fakePreprocessor }] }, emitterSetting]) - pp = m.createPreprocessor({ '**/*': ['fake'] }, null, injector) + pp = m.createPriorityPreprocessor({ '**/*': ['fake'] }, {}, null, injector) const file = { originalPath: '/some/photo.png', path: 'path' } @@ -322,7 +322,7 @@ describe('preprocessor', () => { 'preprocessor:fake': ['factory', function () { return fakePreprocessor }] }, emitterSetting]) - pp = m.createPreprocessor({ '**/*': ['fake'] }, null, injector) + pp = m.createPriorityPreprocessor({ '**/*': ['fake'] }, {}, null, injector) const file = { originalPath: '/some/photo.png', path: 'path' } @@ -344,7 +344,7 @@ describe('preprocessor', () => { 'preprocessor:fake': ['factory', function () { fakePreprocessor }] }, emitterSetting]) - pp = m.createPreprocessor({ '**/*': ['fake'] }, null, injector) + pp = m.createPriorityPreprocessor({ '**/*': ['fake'] }, {}, null, injector) const file = { originalPath: '/some/CAM_PHOTO.JPG', path: 'path' } @@ -357,7 +357,7 @@ describe('preprocessor', () => { }) }) - it('should merge lists of preprocessors', (done) => { + it('should merge lists of preprocessors using default priority', (done) => { const callOrder = [] const fakePreprocessorA = sinon.spy((content, file, done) => { callOrder.push('a') @@ -383,11 +383,11 @@ describe('preprocessor', () => { 'preprocessor:fakeD': ['factory', function () { return fakePreprocessorD }] }, emitterSetting]) - pp = m.createPreprocessor({ + pp = m.createPriorityPreprocessor({ '/*/a.js': ['fakeA', 'fakeB'], '/some/*': ['fakeB', 'fakeC'], '/some/a.js': ['fakeD'] - }, null, injector) + }, {}, null, injector) const file = { originalPath: '/some/a.js', path: 'path' } @@ -399,10 +399,56 @@ describe('preprocessor', () => { expect(fakePreprocessorC).to.have.been.called expect(fakePreprocessorD).to.have.been.called - expect(callOrder.indexOf('d')).not.to.equal(-1) - expect(callOrder.filter((letter) => { - return letter !== 'd' - })).to.eql(['a', 'b', 'c']) + expect(callOrder).to.eql(['a', 'b', 'c', 'd']) + done() + }) + }) + + it('should merge lists of preprocessors obeying priority', (done) => { + const callOrder = [] + const fakePreprocessorA = sinon.spy((content, file, done) => { + callOrder.push('a') + done(null, content) + }) + const fakePreprocessorB = sinon.spy((content, file, done) => { + callOrder.push('b') + done(null, content) + }) + const fakePreprocessorC = sinon.spy((content, file, done) => { + callOrder.push('c') + done(null, content) + }) + const fakePreprocessorD = sinon.spy((content, file, done) => { + callOrder.push('d') + done(null, content) + }) + + const injector = new di.Injector([{ + 'preprocessor:fakeA': ['factory', function () { return fakePreprocessorA }], + 'preprocessor:fakeB': ['factory', function () { return fakePreprocessorB }], + 'preprocessor:fakeC': ['factory', function () { return fakePreprocessorC }], + 'preprocessor:fakeD': ['factory', function () { return fakePreprocessorD }] + }, emitterSetting]) + + const priority = { 'fakeA': -1, 'fakeB': 1, 'fakeD': 100 } + + pp = m.createPriorityPreprocessor({ + '/*/a.js': ['fakeA', 'fakeB'], + '/some/*': ['fakeB', 'fakeC'], + '/some/a.js': ['fakeD'] + }, priority, null, injector) + + const file = { originalPath: '/some/a.js', path: 'path' } + + pp(file, (err) => { + if (err) throw err + + expect(fakePreprocessorA).to.have.been.called + expect(fakePreprocessorB).to.have.been.called + expect(fakePreprocessorC).to.have.been.called + expect(fakePreprocessorD).to.have.been.called + + expect(callOrder).to.eql(['d', 'b', 'c', 'a']) done() }) })