Skip to content

Commit

Permalink
feat: Allow frameworks to inject middleware
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jamestalmage committed Jun 23, 2016
1 parent eb94739 commit d972f3d
Show file tree
Hide file tree
Showing 7 changed files with 99 additions and 11 deletions.
7 changes: 5 additions & 2 deletions lib/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)

Expand Down
44 changes: 44 additions & 0 deletions test/e2e/middleware.feature
Original file line number Diff line number Diff line change
@@ -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
"""
1 change: 0 additions & 1 deletion test/e2e/pass-opts.feature
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,4 @@ Feature: Passing Options
Then it passes with no debug:
"""
.
PhantomJS
"""
21 changes: 21 additions & 0 deletions test/e2e/support/middleware/middleware.js
Original file line number Diff line number Diff line change
@@ -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]
}
14 changes: 14 additions & 0 deletions test/e2e/support/middleware/test.js
Original file line number Diff line number Diff line change
@@ -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')
})
})
5 changes: 4 additions & 1 deletion test/e2e/support/world.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
18 changes: 11 additions & 7 deletions test/unit/server.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ describe('server', () => {
close: () => {}
}

sinon.stub(server._injector, 'get')
.withArgs('webServer').returns(mockWebServer)
.withArgs('socketServer').returns(mockSocketServer)

webServerOnError = null
})

Expand All @@ -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
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down

0 comments on commit d972f3d

Please sign in to comment.