2
\$\begingroup\$

So I am trying to learn TDD/BDD with Javascript. I am creating a simple RESTful web app that uses GET, POST, PUT, and DELETE API calls to update data in a MongoDB database, and wanted to see if there was anything I can do to improve on the unit test I wrote. Both to make them more organized in the testing output, and to include more specific details that might be important from a testing standpoint.

The web app is built using the MEAN stack.

var superagent = require("superagent");
var chai = require("chai");
var mocha = require("mocha");
var should = require("should");
var expect = chai.expect;
var chaiHttp = require('chai-http');
var request = require("request");

var app = require('../app');

var mongoose = require("mongoose");
var campaign = require("../app_api/models/campaigns");




chai.use(chaiHttp);

describe("Campaign API Calls", function() {

    var test_id = 0;
    var test_campaign_name = "New_Tester_Campaign";

    describe("/POST/api/campaigns", function() {
        it("should create new campaign with valid input", (done) => {
            var campaign = {
                campaignName: test_campaign_name,
                campaignStatus: "active"
            }
            chai.request(app)
                .post('/api/campaigns')
                .send(campaign)
                .end((err, res) => {
                    res.status.should.be.equal(201);
                    res.body.should.be.type('object');
                    res.body.should.have.property('campaignName').eql(test_campaign_name);
                    res.body.should.have.property('campaignStatus');
                    res.body.should.have.property('_id');
                    test_id = res.body._id;  // Store ID of campaign for use by remaining tests
                done();
                });
        });
        it("should not create a new campaign with invalid input", (done) => {
            var campaign = {
                campaignName: "",
                campaignStatus: "active"
            }
            chai.request(app)
                .post('/api/campaigns')
                .send(campaign)
                .end((err, res) => {
                    res.status.should.be.equal(400);
                    res.body.should.have.property('message').eql('Campaign Name is Required');
                done();
                });
        });
    });

    describe("/GET/api/campaigns", function() {
        it("it should returns a list of campaigns", (done) => {
            chai.request(app)
                .get('/api/campaigns')
                .end((err, res) => {
                    res.status.should.be.equal(200);
                    res.body.should.be.type('object');
                done();
                });
        });
    });

    describe("/GET/api/campaigns/:campaignid", function () {
        it("it should GET the details of one campaign", (done) => {
            chai.request(app)
                .get('/api/campaigns/' + test_id)
                .end((err, res) => {
                    res.status.should.be.equal(200);
                    res.body.should.have.property('campaignName').eql(test_campaign_name);
                    res.body.should.have.property('_id').eql(test_id);
                done();
                });
        });
    });

    describe("/PUT/api/campaigns/:campaignid", function () {
        it("it should update a single campaign", (done) => {
            chai.request(app)
                .put('/api/campaign/' + test_id)
                .field('campaignName', 'Tester_Campaign')
                .end((err, res) => {
                    res.status.should.be.equal(200);
                    res.body.should.have.property('campaignName')
                        .eql('Tester_Campaign');
                don();
                });
        });
    });

    describe("/DELETE/api/campaigns/:campaignid", function() {
        it("it should delete a single campaign", function() {
            chai.request(app)
                .delete('/api/campaign/' + test_id)
                .end((err, res) => {
                    res.status.should.be.equal(200);
                    res.body.should.have.property('message')
                        .eql('Campaign successfully deleted');
                done();
                });
        });
    });
});
\$\endgroup\$
  • 1
    \$\begingroup\$ In the title please tell us what the code is doing and not your concerns about it. Also please add a more detailed description. I am creating a simple RESTful web app is not a good description. A RESTful app for what? \$\endgroup\$ – t3chb0t Apr 22 '17 at 8:48
  • \$\begingroup\$ updated the title and added some details about the RESTful web app \$\endgroup\$ – ebellefontaine Apr 22 '17 at 14:10
  • \$\begingroup\$ Tiny bit of the answer I'm working on, but wanted to mention early: it("it ...") -- The method name didn't tip you off that it it shouldn't start with "it"? :) \$\endgroup\$ – Fund Monica's Lawsuit Apr 22 '17 at 15:01
2
\$\begingroup\$

RESTful CRUD application API Unit Tests

REST and unit tests shouldn't even belong in the same sentence. When unit tests do a network request, that's not a unit test.

If you want to unit test your API logic, you test the server-side code starting from the part where the router hands off the control to your logic and up to the ORM layer. Your tests should then supply inputs as if the router supplied them, and the ORM should be faked to supply data as if they came from a DB.

    only test this portion
          v-----v
router <-> logic <-> orm <-> db
                    ^---^
             you fake this portion

// In the end, you get this:
test input  -> logic -> faked orm ----------.
test output <-       <- faked db responses <'

Now if what you really meant integration testing...

Tests should be isolated from side-effects, particularly from previous tests. That way, tests are independent in terms of when they're run and what ran before them. If side-effects are left to happen, the database state becomes unpredictable, tests become dependent on order, making them brittle - a cause of frustration when writing tests. So ideally, before each test, you reset your database to a known state.

For instance, you have a test that POSTs a campaign. The next test GETs a campaign which depends on the data POST put in. If for some reason management wanted the POST feature removed, of course you remove the test - which then breaks the GET test. You end up rewriting the GET test, an unnecessary effort. If you had more tests depending on the previous tests, well... you get the idea.

If you're using Node 4+, consider using const instead of var, particularly in your dependencies. That way, you don't accidentally replace the reference with another. Usually happens when you import the path module, and you have stuff named path all over the place. A missing var on an assignment to a local path could spell disaster for the entire code.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I've gone through the pain of unit testing NodeJs. I'm not sure it's worth the effort to go around the routing/server when it starts up that fast and is local. I would definitely recommend mocking the db though. \$\endgroup\$ – RubberDuck Apr 24 '17 at 16:17
  • \$\begingroup\$ Would you be able to provide an example please, or point me to one that I can look at? For mocking the DB side. \$\endgroup\$ – ebellefontaine Apr 24 '17 at 18:45
  • \$\begingroup\$ I was looking at Mockgoose, but wanted to see if you had any recommendations \$\endgroup\$ – ebellefontaine Apr 24 '17 at 19:01
  • \$\begingroup\$ @ebellefontaine I have no recommendations, the above being general advice. Select the tool that fits your needs. \$\endgroup\$ – Joseph Apr 24 '17 at 19:16
1
\$\begingroup\$
  • "use strict";. It'll be useful because it'll report errors like, say, the one I mention down below. No more spending days chasing down a bug caused by calling something foboar instead of foobar! (I did that once. It was awful.)
  • it("it should ...") should probably just be it("should ..."). Better yet, just say what you're making sure it does/has -- instead of it("should GET ..."), it should (heh) be it("returns ... to a GET")
  • Don't test responses, test results. If you successfully return 200 OK and the right body, but something messed up when doing the actual deletion, you're reporting success for something that failed. You need to do a second test to make sure that after a 200 OK, your content is actually deleted.
    • Obviously, the exception is when the result is "returned a response"
  • CTRL+F don() -- you made a typo. To quote someone with more experience than me when I asked them about it, "done is called after an async opertation completes. If you don’t call it, the test will time out after 2 seconds and fail." You should probably change that.
  • Why do you have so many blank lines before your code starts? "At most two" is a good rule of thumb to follow. More just seems unnecessary. It's not really hurting anything, though.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback, I'll definitely incorporate it into my code \$\endgroup\$ – ebellefontaine Apr 24 '17 at 19:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.