Skip to content
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

Merged
merged 4 commits into from
Jun 3, 2013

Conversation

ericf
Copy link
Collaborator

@ericf ericf commented Jun 3, 2013

If more than one copy of express-state is loaded, because of dependency issues, it will not plugin into the same copy of express more than once.

ericf added 3 commits June 3, 2013 02:04
This removes top-level vars in favor of describe-scoped vars.
If more than one copy of express-state is loaded, because of dependency
issues, it will not plugin into the same copy of express more than once.
@ericf
Copy link
Collaborator Author

ericf commented Jun 3, 2013

/cc @caridy

@ghost ghost assigned ericf Jun 3, 2013
expect = require('chai').expect,
express = require('express'),
Copy link
Contributor

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 real express 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 uses mockery.

@ericf
Copy link
Collaborator Author

ericf commented Jun 3, 2013

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 uses mockery.

Good point, I'll do that instead.

This moves the express mock and dynamic `require()` loading via mockery
into its own test file.
@ericf
Copy link
Collaborator Author

ericf commented Jun 3, 2013

@caridy I updated this to move the multiple copies tests to their own file; this makes it much cleaner. I do want to use the real express module because of the way it sets up __proto__ and to make sure all the prototype plugging is working correctly.

@@ -49,6 +42,16 @@ describe('state', function () {
});

describe('global .local', function () {
var origLocal;

beforeEach(function () {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 mockery to require and create a new app on every test, there is not need to restore original values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

@caridy
Copy link
Contributor

caridy commented Jun 3, 2013

+1

ericf added a commit that referenced this pull request Jun 3, 2013
Prevent multiple copies of express-state from overwriting `expose()`
@ericf ericf merged commit 8de02e4 into master Jun 3, 2013
@ericf ericf deleted the multi-protect branch June 3, 2013 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants