feat: Allow frameworks to inject middleware

jamestalmage · jamestalmage · commit d972f3d4cbf4 · 2016-06-23T14:32:30.000-04:00
The Server#_start function was injected with `webServer` and `socketServer`. This prevents frameworks from being able to manipulate `config.middleware` before those servers are created.

The solution is simply to defer injection until after all frameworks have loaded, and then fetch those values with `injector.get`.

There were no e2e tests involving middleware. I have added two, including one that verifies the new capabilities.
diff --git a/lib/server.js b/lib/server.js
@@ -129,8 +129,8 @@ Server.prototype.refreshFiles = function () {
 // Private Methods
 // ---------------
 
-Server.prototype._start = function (config, launcher, preprocess, fileList, webServer,
-                                    capturedBrowsers, socketServer, executor, done) {
+Server.prototype._start = function (config, launcher, preprocess, fileList,
+                                    capturedBrowsers, executor, done) {
   var self = this
   if (config.detached) {
     this._detach(config, done)
@@ -143,6 +143,9 @@ Server.prototype._start = function (config, launcher, preprocess, fileList, webS
     self._injector.get('framework:' + framework)
   })
 
+  var webServer = self._injector.get('webServer')
+  var socketServer = self._injector.get('socketServer')
+
   // A map of launched browsers.
   var singleRunDoneBrowsers = Object.create(null)
 
diff --git a/test/e2e/middleware.feature b/test/e2e/middleware.feature
@@ -0,0 +1,44 @@
+Feature: Middleware
+  In order to use Karma
+  As a person who wants to write great tests
+  I want to use custom middleware with Karma.
+
+  Scenario: Simple middleware
+    Given a configuration with:
+      """
+      files = ['middleware/test.js'];
+      browsers = ['PhantomJS'];
+      plugins = [
+        'karma-jasmine',
+        'karma-phantomjs-launcher',
+        _resolve('middleware/middleware')
+      ];
+      middleware = [
+        'foo'
+      ]
+      """
+    When I start Karma
+    Then it passes with:
+      """
+      .
+      PhantomJS
+      """
+
+  Scenario: Frameworks can add middleware
+    Given a configuration with:
+      """
+      files = ['middleware/test.js'];
+      browsers = ['PhantomJS'];
+      plugins = [
+        'karma-jasmine',
+        'karma-phantomjs-launcher',
+        _resolve('middleware/middleware')
+      ];
+      frameworks = ['jasmine', 'foo']
+      """
+    When I start Karma
+    Then it passes with:
+      """
+      .
+      PhantomJS
+      """
diff --git a/test/e2e/pass-opts.feature b/test/e2e/pass-opts.feature
@@ -19,5 +19,4 @@ Feature: Passing Options
     Then it passes with no debug:
       """
       .
-      PhantomJS
       """
diff --git a/test/e2e/support/middleware/middleware.js b/test/e2e/support/middleware/middleware.js
@@ -0,0 +1,21 @@
+function middleware (request, response, next) {
+  if (/\/foo\.js/.test(request.normalizedUrl)) {
+    response.setHeader('Content-Type', 'text/plain')
+    response.writeHead(200)
+    response.end('this is the middleware response')
+    return
+  }
+  next()
+}
+
+function framework (config) {
+  config.middleware = config.middleware || []
+  config.middleware.push('foo')
+}
+
+framework.$inject = ['config']
+
+module.exports = {
+  'framework:foo': ['factory', framework],
+  'middleware:foo': ['value', middleware]
+}
diff --git a/test/e2e/support/middleware/test.js b/test/e2e/support/middleware/test.js
@@ -0,0 +1,14 @@
+function httpGet (url) {
+  var xmlHttp = new XMLHttpRequest()
+
+  xmlHttp.open('GET', url, false)
+  xmlHttp.send(null)
+
+  return xmlHttp.responseText
+}
+
+describe('foo', function () {
+  it('should should serve /foo.js', function () {
+    expect(httpGet('/foo.js')).toBe('this is the middleware response')
+  })
+})
diff --git a/test/e2e/support/world.js b/test/e2e/support/world.js
@@ -14,7 +14,10 @@ exports.World = function World () {
     frameworks: ['jasmine'],
     basePath: __dirname,
     colors: false,
-    __dirname: __dirname
+    __dirname: __dirname,
+    _resolve: function (name) {
+      return path.resolve(__dirname, '..', 'support', name)
+    }
   }
 
   this.addConfigContent = (function (_this) {
diff --git a/test/unit/server.spec.js b/test/unit/server.spec.js
@@ -84,6 +84,10 @@ describe('server', () => {
       close: () => {}
     }
 
+    sinon.stub(server._injector, 'get')
+      .withArgs('webServer').returns(mockWebServer)
+      .withArgs('socketServer').returns(mockSocketServer)
+
     webServerOnError = null
   })
 
@@ -92,7 +96,7 @@ describe('server', () => {
   // ============================================================================
   describe('_start', () => {
     it('should start the web server after all files have been preprocessed successfully', () => {
-      server._start(mockConfig, mockLauncher, null, mockFileList, mockWebServer, browserCollection, mockSocketServer, mockExecutor, doneSpy)
+      server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy)
 
       expect(mockFileList.refresh).to.have.been.called
       expect(fileListOnResolve).not.to.be.null
@@ -106,7 +110,7 @@ describe('server', () => {
     })
 
     it('should start the web server after all files have been preprocessed with an error', () => {
-      server._start(mockConfig, mockLauncher, null, mockFileList, mockWebServer, browserCollection, mockSocketServer, mockExecutor, doneSpy)
+      server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy)
 
       expect(mockFileList.refresh).to.have.been.called
       expect(fileListOnReject).not.to.be.null
@@ -120,7 +124,7 @@ describe('server', () => {
     })
 
     it('should launch browsers after the web server has started', () => {
-      server._start(mockConfig, mockLauncher, null, mockFileList, mockWebServer, browserCollection, mockSocketServer, mockExecutor, doneSpy)
+      server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy)
 
       expect(mockWebServer.listen).not.to.have.been.called
       expect(server._injector.invoke).not.to.have.been.calledWith(mockLauncher.launch, mockLauncher)
@@ -132,7 +136,7 @@ describe('server', () => {
     })
 
     it('should try next port if already in use', () => {
-      server._start(mockConfig, mockLauncher, null, mockFileList, mockWebServer, browserCollection, mockSocketServer, mockExecutor, doneSpy)
+      server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy)
 
       expect(mockWebServer.listen).not.to.have.been.called
       expect(webServerOnError).not.to.be.null
@@ -150,7 +154,7 @@ describe('server', () => {
     })
 
     it('should emit a listening event once server begin accepting connections', () => {
-      server._start(mockConfig, mockLauncher, null, mockFileList, mockWebServer, browserCollection, mockSocketServer, mockExecutor, doneSpy)
+      server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy)
 
       var listening = sinon.spy()
       server.on('listening', listening)
@@ -163,7 +167,7 @@ describe('server', () => {
     })
 
     it('should emit a browsers_ready event once all the browsers are captured', () => {
-      server._start(mockConfig, mockLauncher, null, mockFileList, mockWebServer, browserCollection, mockSocketServer, mockExecutor, doneSpy)
+      server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy)
 
       var browsersReady = sinon.spy()
       server.on('browsers_ready', browsersReady)
@@ -178,7 +182,7 @@ describe('server', () => {
     })
 
     it('should emit a browser_register event for each browser added', () => {
-      server._start(mockConfig, mockLauncher, null, mockFileList, mockWebServer, browserCollection, mockSocketServer, mockExecutor, doneSpy)
+      server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy)
 
       var browsersReady = sinon.spy()
       server.on('browsers_ready', browsersReady)

Original file line number	Diff line number	Diff line change
`@@ -19,5 +19,4 @@ Feature: Passing Options`
`19`	`19`	`Then it passes with no debug:`
`20`	`20`	`"""`
`21`	`21`	`.`
`22`		`- PhantomJS`
`23`	`22`	`"""`