-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent multiple copies of express-state from overwriting expose()
#4
Changes from all commits
b251838
3de2dc7
719ed75
14b81db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,7 @@ | |
"express3-handlebars": "*", | ||
"istanbul" : "*", | ||
"mocha" : "*", | ||
"mockery" : "*", | ||
"xunit-file" : "*" | ||
}, | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
/* global describe, it, beforeEach, afterEach */ | ||
'use strict'; | ||
|
||
var expect = require('chai').expect, | ||
mockery = require('mockery'); | ||
|
||
describe('multiple', function () { | ||
beforeEach(function () { | ||
mockery.enable({ | ||
useCleanCache : true, | ||
warnOnReplace : false, | ||
warnOnUnregistered: false | ||
}); | ||
|
||
mockery.registerMock('express', { | ||
application: {}, | ||
response : {} | ||
}); | ||
}); | ||
|
||
afterEach(function () { | ||
mockery.disable(); | ||
}); | ||
|
||
it('should not override `expose()` if it exists', function () { | ||
var expose = function () {}, | ||
express, state; | ||
|
||
express = require('express'); | ||
express.application.expose = express.response.expose = expose; | ||
state = require('../'); | ||
|
||
expect(express.application.expose).to.equal(expose); | ||
expect(express.response.expose).to.equal(expose); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,11 @@ | ||
/* global describe, it, beforeEach, afterEach */ | ||
'use strict'; | ||
|
||
var state = require('../'), | ||
express = require('express'), | ||
expressUtils = require('express/lib/utils'), | ||
var expressUtils = require('express/lib/utils'), | ||
expect = require('chai').expect, | ||
express = require('express'), | ||
|
||
origLocal = state.local, | ||
origNamespace = state.namespace; | ||
state = require('../'); | ||
|
||
describe('state', function () { | ||
var app; | ||
|
@@ -16,11 +14,6 @@ describe('state', function () { | |
app = express(); | ||
}); | ||
|
||
afterEach(function () { | ||
state.local = origLocal; | ||
state.namespace = origNamespace; | ||
}); | ||
|
||
describe('expose()', function () { | ||
var expose, locals; | ||
|
||
|
@@ -49,6 +42,16 @@ describe('state', function () { | |
}); | ||
|
||
describe('global .local', function () { | ||
var origLocal; | ||
|
||
beforeEach(function () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is ok to have all this on every test for now, but if you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I could switch this to use Mockery to re-load the express-state module every time. But I don't want to mock it, since this is explicitly trying to test that functionality. |
||
origLocal = state.local; | ||
}); | ||
|
||
afterEach(function () { | ||
state.local = origLocal; | ||
}); | ||
|
||
it('should create the exposed object at the specified `local`', function () { | ||
state.local = 'javascript'; | ||
expose(); | ||
|
@@ -63,6 +66,16 @@ describe('state', function () { | |
}); | ||
|
||
describe('global .namespace', function () { | ||
var origNamespace; | ||
|
||
beforeEach(function () { | ||
origNamespace = state.namespace; | ||
}); | ||
|
||
afterEach(function () { | ||
state.namespace = origNamespace; | ||
}); | ||
|
||
it('should be used when no namespace is provided', function () { | ||
state.namespace = 'App'; | ||
expose('foo'); | ||
|
@@ -98,6 +111,16 @@ describe('state', function () { | |
}); | ||
|
||
describe('setting: "state local"', function () { | ||
var origLocal; | ||
|
||
beforeEach(function () { | ||
origLocal = state.local; | ||
}); | ||
|
||
afterEach(function () { | ||
state.local = origLocal; | ||
}); | ||
|
||
it('should use app setting', function () { | ||
app.set('state local', 'javascript'); | ||
app.expose(); | ||
|
@@ -119,12 +142,29 @@ describe('state', function () { | |
}); | ||
|
||
describe('setting: "state namespace"', function () { | ||
var origNamespace; | ||
|
||
beforeEach(function () { | ||
origNamespace = state.namespace; | ||
}); | ||
|
||
afterEach(function () { | ||
state.namespace = origNamespace; | ||
}); | ||
|
||
it('should use app setting', function () { | ||
app.set('state namespace', 'App'); | ||
app.expose('foo'); | ||
expect(app.locals.state.App).to.equal('foo'); | ||
}); | ||
|
||
it('should be preferred over the global .namespace', function () { | ||
state.namespace = 'App'; | ||
app.set('state namespace', 'Data'); | ||
app.expose('foo'); | ||
expect(app.locals.state.Data).to.equal('foo'); | ||
}); | ||
|
||
it('should be used as a prefix to the `namespace` provided', function () { | ||
app.set('state namespace', 'App'); | ||
app.expose({foo: 'foo'}, 'data'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You use the original
express
modules for some tests, while mocking it for some other tests, that seems a little tricky. Can we guarantee that those tests are going to be executed in order? Is it absolutely necessary to use the realexpress
instead of a mock for all tests?If there is no way, or it is too difficult to accomplish, then I will say that the
multiple copies
test should be moved out of this file into its own test file that usesmockery
.