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

fix(test:server): ensure db seeding completes before test run #352

Closed
mvhenderson opened this issue Jul 15, 2014 · 3 comments
Closed

fix(test:server): ensure db seeding completes before test run #352

mvhenderson opened this issue Jul 15, 2014 · 3 comments

Comments

@mvhenderson
Copy link

The db operations in server/config/seed.js are async, which may allow the express app to startup prior to the database seeding completes. This causes an interment failure of the server unit tests. Found while researching Travis build error, details here

Creating an issue so we don't loose track of this. I'll have time to fix later this week. Thinking of using caolan/async as a devDependency of the generated app should do the trick.

CC: @JaKXz @chester1000 @apercu

@JaKXz
Copy link
Collaborator

JaKXz commented Jul 15, 2014

👍 sounds good to me.

@mvhenderson
Copy link
Author

I'm not convinced that server/app.js is the right place to do database seeding. I also feel that seedDB should not be an all-or-nothing setting by default - for a dev server it makes sense, it doesn't for production (and could be dangerous), and for test it depends on how the tests are written.

There are two ways to write server tests, both represented in the default generation. One way is to assume a seeded database like the Thing tests, the other is to hydrate the database in the test logic like the User tests. I personally prefer the latter as unit tests should not assume any application state.

The primary goal here is to ensure there is no async database operations in process prior to running tests. A secondary goal is the ability to configure in which environment(s) seeding takes place. I believe there are two options:

  1. Move seeding from app.js to grunt
    This assumes that the production server (node server/app.js) would never seed the db. The test and serve grunt tasks would get updated with a seed step prior to starting mocha/express. If seeding isn't desired, this step can be commended out.

    The seed step could be a multitask that doesn't assume any environment variable (as above) or a simple task that uses the NODE_ENV to determine which database to connect to.

    This option requires no changes to existing tests. Configuring which environments are seeded would be done in the Gruntfile instead of the server config file.

  2. Update tests to wait for seeding to complete
    This would keep seeding in app.js, but to guarantee that seeding has completed prior to running test, we would need to add a custom property to the app to hold a Promise which each test would need to check in the mocha before() function, something like

    describe('User Tests', function () {
       before(function (done) {
           app.mongoReadyPromise.then(done);
       });
    });

    Configuration would remain in the server config file, but as a sane default the production.js should be updated with seedDB = false.

I prefer option 1.

Finally, we should cleanup some of the redundancy in the seeding process. For example, the same list of things is in the thing.controller.js and seed(mongoose).js templates. Perhaps a better path is to have a node module for each model that can be reused by the db/no-db generated code, something like server/api/thing/thing.data.js which simply exports an array of objects.

@mvhenderson
Copy link
Author

Thought of a third option that should take less effort - just change the tests to be independent of seeding by default.

  • change seedDB to false by default and set it to true in development.js
  • change the Thing test to seed it's own data
  • change the User test to move the beforeEach delete to before since the afterEach does the cleanup for us
  • (optional) refactor the seeding code to keep it DRY

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

No branches or pull requests

3 participants