-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add docs for endpoint testing with mongoose #1420
Conversation
Signed-off-by: Zell Liew <zellwk@gmail.com>
Signed-off-by: Zell Liew <zellwk@gmail.com>
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.
Since all other recipes use semicolons at the end of lines this should too.
**Next, start your MongoDB instance with Mongomem** | ||
|
||
```js | ||
test.before('start server', async t => await MongoDBServer.start()) |
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.
The await is redundant here since it's in a return statement.
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.
👍
const res = await request(app) | ||
.get('/test1') | ||
.send({ | ||
email: 'someemail@gmail.com' |
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.
Might want to switch this to example@example.com
just to be safe that you're not using a real email in an example.
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.
👍
Hopefully this is just a copying error but while testing this in my own project all I manage to get is an error thrown and my tests all fail.
|
@zellwk we already have a recipe that uses |
@OmgImAlexis Hmm. Would you mind trying again? Recipe looks right to me. Whats in your @novemberborn I can edit the file directly, would that work better? Regarding that recipe, I tested it and it doesn't work directly with my app because the Mongoose instances are different, which is why I decided to make this recipe. |
Here's my import test from 'ava';
import request from 'supertest';
import {MongoDBServer} from 'mongomem';
import {setupFixtures, removeFixtures, setupMongoose} from './utils';
test.before('start server', async () => {
await MongoDBServer.start();
});
test.beforeEach(async t => {
const db = await setupMongoose();
const app = require('../main');
// Setup any fixtures you need here. This is a placeholder code
await setupFixtures();
// Pass app and mongoose into your tests
t.context.app = app;
t.context.db = db;
});
test.afterEach.always(async t => {
const {db} = t.context;
// Note: removeFixtures is a placeholder. Write your own
await removeFixtures();
await db.connection.close();
});
// Note the serial tests
test.serial('create', async t => {
const {app} = t.context;
const res = await request(app).post('/user').send({
username: 'ava',
password: 'avarocks'
});
t.is(res.status, 201);
t.is(res.body.username, 'ava');
});
test.after.always('cleanup', () => MongoDBServer.tearDown());
import mongoose from 'mongoose';
import {MongoDBServer} from 'mongomem';
const setupFixtures = () => {};
const removeFixtures = () => {};
const setupMongoose = async () => {
await mongoose.connect(await MongoDBServer.getConnectionString());
return mongoose;
};
export {
setupFixtures,
removeFixtures,
setupMongoose
}; |
Looks right. Can you tell me how you handled |
In
import {Router} from 'express';
import HTTPError from 'http-errors';
import log from '../log';
import config from '../config';
import {User} from '../models';
import {isValidObjectId} from '../middleware';
const router = new Router();
router.get(['/', '/:id'], isValidObjectId, async (req, res, next) => {
if (req.params.id) {
const user = await User.find({
_id: req.params.id
}).exec().catch(next);
return res.send(user);
}
const users = await User.find({}).sort({
date: -1
}).exec().catch(next);
res.send(users);
});
router.post('/', (req, res, next) => {
if (!config.get('signups.enabled')) {
// @TODO: This may not be the best status code for this.
return next(new HTTPError.MethodNotAllowed('Signups are currently disabled.'));
}
const user = new User({
username: req.body.username || '',
email: req.body.email || '',
password: req.body.password
});
log.debug(user);
user.save().then(() => {
log.debug('User saved.');
return res.send({
user
});
}).catch(err => {
log.debug('User error.');
// Duplicate field
if (err.code === 11000) {
return next(new HTTPError.Conflict('User already exists.'));
}
return next(err);
});
});
export default router; |
Do you mind trying out a handler like this?
|
Okay this is an issue with mongomem, looks like it's trying to bind to Ref: CImrie/mongomem#2 |
Ah yes. I had the same problem too. Just to confirm. This recipe works fine for you. Yeah? |
Yep after releasing the port from my docker container it's now fine. |
As per CImrie/mongomem#2 (comment) maybe this should use |
Both recipes are very similar to each other. Ideally we have one. If there's some additional use case or nuance we can discuss it in the recipe. |
@OmgImAlexis I have tried using mondob-memory-server with the following code, but was unable to get tests to run in parallel still.
The error I get when attempting to run tests in parallel is this:
Here, it seems like, without using |
@novemberborn Sure. We can merge it into one recipe eventually. Can we iron out the issues mentioned above before doing so? That way, we can see if there's really an additional use case or nuance. |
@nodkz since you're the dev for |
} | ||
|
||
test.beforeEach(async t => { | ||
const db = await setupMongoose() |
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.
Would be better to inline the setupMongoose
function here.
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.
Sure
|
||
test.beforeEach(async t => { | ||
const db = await setupMongoose() | ||
const app = require('./server') |
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.
This isn't reloaded for every test. Better to require it outside the test hook.
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.
The 'require' statement configures Mongoose according to the current test (I believe). May not work properly if Mongoose is not set up per test.
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.
Modules are cached after the first time they're loaded, so requiring ./server
for each test shouldn't make a difference.
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're right about this. Let me make the requested changes after confirming if there's a better implementation with @nodkz, as commented below.
const app = require('./server') | ||
|
||
// Setup any fixtures you need here. This is a placeholder code | ||
await setupFixtures() |
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.
This seems pretty critical actually. How does app
use the db
instance?
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.
Will expand on this!
const { db } = t.context | ||
// Note: removeFixtures is a placeholder. Write your own | ||
await removeFixtures() | ||
await db.connection.close() |
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.
https://github.com/avajs/ava/blob/master/docs/recipes/isolated-mongodb-integration-tests.md doesn't do this. I wonder if it should.
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.
Another way is to disconnect mongoose. Works the same, as far as I can tell. Awaiting @nodkz's input on mongodb-memory-server
right now. Otherwise, tests can't run in parallel. Should we wait? Or should I edit according to your comments first?
@OmgImAlexis @zellwk You may test mongoose in several ways:
This tests works in parallel. Each test file has own mongodb server. Also it should work for your case, where you use a dedicated database for every test case in one test file. If not, please provide test repo i try to figure out what is the problem. I see following disadvantages in your example:
|
@nodkz Thanks for chipping in! I've tried your suggestions, but I still couldn't get ava tests to run in parallel. By parallel, I mean tests within a test file. So far, my test file looks like this:
Is this the best we can do? If it helps with debugging, you can find a demo repo at https://github.com/zellwk/ava-mdb-test. |
@nodkz, @novemberborn, @OmgImAlexis any suggestions on the demo repo I've sent? If not, I'll go ahead and edit this docs again. |
@zellwk miss your prev message. Now I'm looking your repo. Write about results today later. |
Tests do not work on node 7.2.1Got following error:
On 8.2.0 works perfectly. I think that NOTE is required in docs about minimal node version. Fix mongoose deprecationWarning:
|
In mongodb-memory-server@1.4.1 ship some changes with babel-polyfill. Now your tests may work without babel-polyfill and regenerator. |
That's a good idea. Could you add that to this PR? I can fix it up afterwards to if you'd like. |
Thanks @nodkz! @zellwk could you check that's all good with this recipe, so we don't need the polyfill and stuff? |
Signed-off-by: Zell Liew <zellwk@gmail.com>
Signed-off-by: Zell Liew <zellwk@gmail.com>
Done!
Done too! Removed the Mongoose parts and pointed them to the mongoose docs instead. I'm not sure whether we should change Mongmem to Mongodb-memory server on that recipe. Thoughts, @nodkz? @OmgImAlexis and @novemberborn, I also added an explanation on why the user should use |
@@ -156,3 +139,13 @@ And you're done! | |||
You may choose to abstract code for `test.before`, `test.beforeEach`, `test.afterEach.always` and `test.after.always` into a separate file. | |||
|
|||
To see a demo of this configuration file, look at https://github.com/zellwk/ava-mdb-test | |||
|
|||
## Why `test.serial` instead of `test` |
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.
Why not just use a new database for each test? That's how we run tests with agenda and we don't have any issues.
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.
When you use mongoose, you interact with the database through Mongoose. Since there can only be one Mongoose instance (and that Mongoose instance must always point to the same database), we cant access multiple databases even if we create them.
Do I make sense?
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.
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.
Ah! I understood @nodkz's explanation (above) wrongly. Although you can create multiple connections with Mongoose, you need to create multiple models. Let me switch the explanation as soon as I find some time to.
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.
See User
and UserOnServer2
models constructed on one schema with different connections in the following example:
https://github.com/nodkz/mongodb-memory-server#several-mongoose-connections-simultaneously
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.
@OmgImAlexis updated with a slightly better explanation. Do I make sense in this new one?
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.
That does make sense but since ava is meant to be run in parallel it'd be better to explain how to set it up by default instead of expecting tests to run in serial.
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.
I fully understand that AVA runs tests in parallel, and your desire to set it up by default. I would love to do that too.
However, based on @nodkz's explanation, and my personal research over the past month, I strongly believe endpoint testing with Mongoose and AVA is an exception where serial tests would be preferred.
The setup for parallel tests changes how a person would create their schemas, and thus their app, too much for the additional benefit of running tests in parallel within each file. Besides, if you create multiple AVA test files, each test file still runs in parallel to other files, which shouldn't slow down testing too much.
I'll be glad to change the docs if you could show me a good way to run endpoint testing in parallel with Mongoose and Supertest.
Firstly I'm using
Full list of options for const mongod = new MongodbMemoryServer({
instance?: {
port?: ?number, // by default choose any free port
dbPath?: string, // by default create in temp directory
storageEngine?: string, // by default `ephemeralForTest`
debug?: boolean, // by default false
},
binary?: {
version?: string, // by default '3.4.4'
downloadDir?: string, // by default %HOME/.mongodb-prebuilt
platform?: string, // by default os.platform()
arch?: string, // by default os.arch()
http?: any, // see mongodb-download package
debug?: boolean, // by default false
},
debug?: boolean, // by default false
autoStart?: boolean, // by default true
}); |
In that case, shall we continue with Mongomem with the mongodb recipe? |
Signed-off-by: Zell Liew <zellwk@gmail.com>
* Consistent spelling of Express, Mongoose and SuperTest * Move prerequisites into their own section, so the test file section reads better * Reformat code examples * Remove code comments from examples where they repeat the preceding paragraph * Reword paragraphs so they read better (in my humble opinion, that is) I've also changed the section on using `test.serial`: I'm pretty sure Mongoose already uses the same connection in the tests and the app. Notably, the connection is made in a `test.before()` hook, not a `test.beforeEach()` hook. Thus the real concern is tests interleaving and the database being in an unknown or conflicting state.
Thanks @zellwk et al. I think this is pretty much there! I did push some edits, so please let me know if I've butchered anything 😇 I've also changed the section on using Please let me know if I got this wrong.
We can modify that recipe to use |
|
||
```js | ||
// Disconnect MongoDB and Mongoose after all tests are done | ||
test.after.always(async t => { |
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.
This can just be () => {
since t isn't used and there's nothing using async.
|
||
```js | ||
// Cleans up database after every test | ||
test.afterEach.always(async t => await User.remove()) | ||
test.afterEach.always(async t => await User.remove()); |
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.
No need for the t
argument here.
I'd change it to test.afterEach.always(() => User.remove());
When you run your first test, MongoDB downloads the latest MongoDB Binaries. It may take a minute. (The download is ~70mb). | ||
|
||
**Add fixtures for each test** | ||
When you run your first test, MongoDB downloads the latest MongoDB binaries. The download is ~70MB so this may take a minute. | ||
|
||
You'll want to populate your database with dummy data. Here's an example: | ||
|
||
```js | ||
test.beforeEach(async t => { |
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.
No need for the t
argument.
// Create connection to mongoose before all tests | ||
test.before(async t => mongoose.connect(await mongod.getConnectionString(), { useMongoClient: true })) | ||
// Create connection to Mongoose before tests are run | ||
test.before(async t => { |
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.
No need for the t
argument.
3. [Mongoose](http://mongoosejs.com) | ||
1. [`mongod-memory-server`](https://github.com/nodkz/mongodb-memory-server) (A MongoDB in-memory Server) | ||
2. [SuperTest](https://github.com/visionmedia/supertest) (An endpoint testing library) | ||
3. [Mongoose ODM](http://mongoosejs.com) |
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.
Why was this changed when "Mongoose" is know as "Mongoose" not "Mongoose ODM"?
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.
Ah I took that from the document title, but they don't refer to it that way elsewhere. Will change back, thanks!
@OmgImAlexis updated. |
@@ -76,7 +76,7 @@ test.beforeEach(async t => { | |||
Dummy data should be cleared after each test: | |||
|
|||
```js | |||
test.afterEach.always(async t => await User.remove()); | |||
test.afterEach.always(async () => await User.remove()); |
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.
async
and await
are redundant here since you're returning a Promise
from User.remove()
.
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.
The explicitness doesn't hurt I think.
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.
There's no need for it and anyone using xo
will need to remove it if they copy+paste. Better to just remove it and set an example by only shipping code that's linted even if it's only in the docs.
@novemberborn LGTM 👍 |
Thank you so much for your efforts @zellwk, @OmgImAlexis and @nodkz! |
Hi all. I've written a module to handle parallel testing with Mongo. It's in early test but please check it out. https://github.com/cyberwombat/mongoprime |
Docs for endpoint tests. #849.
Let me know if I'm opening the PR correctly?