Are these good unit tests? I've been lazy about doing good unit testing, but I'm trying to change that.

users.js

'use strict';
var db = require('mongoose').connection;
var credential = require('credential');
var User = require('./models/user');  // the mongoose model

exports.createUser = function createUser(userAttrs, callback) {
	db.once('open', function () {
		User.findOne({ username: userAttrs.username }, 'username', function (err, person) {
			if (err) return callback(err);
			if (person) {
				return callback(new Error('User "' + userAttrs.username + '" already exists'));
			} else {
				credential.hash(userAttrs.password, function (err, hash) {
					if (err) return callback(err);
					userAttrs.password = hash;

					var user = new User(userAttrs);
					return user.save(function (err, user) {
						if (err) return callback(err);
						return callback(null, user);
					});
				});
			}
		});
	});
};

usersTest.js

'use strict';
var test = require('tape');
var sinon = require('sinon');
var rewire = require('rewire');

var users = rewire('../lib/users');

users.__set__('db', { once: sinon.stub().yields() });

test('Creating a user throws error if username is already taken', function (t) {
	users.__set__('User', { findOne: sinon.stub().yields(null, {})});

	users.createUser({ username: 'andrew', password: 'password'}, function (err, user) {
		t.equal(err.message, 'User "andrew" already exists');
		t.end();
	});
});

test('Creating a user returns the user if it does not exist', function (t) {
	function UserStub(attrs) {
		this.save = sinon.stub().yields(null, attrs);
	}
	UserStub.findOne = sinon.stub().yields(null, undefined);

	users.__set__('User', UserStub);
	users.__set__('credential', { hash: sinon.stub().yields(null, { hash: 'password' }) });

	users.createUser({ username: 'andrew', password: 'password' }, function (err, user) {
		t.error(err);
		t.equal(user.username, 'andrew');
		t.deepEqual(user.password.hash, 'password');
		t.end();
	});
});

Are these good unit tests? I've been lazy about doing good unit testing, but I want to change that. If you have any comments about the way I've done this, I'd really appreciate them.

What I'm wondering:

• I know that these tests don't every branch of the createUser function, but do they adequately test the branches they do cover?
• My understanding of unit testing is that I should stub the APIs that the code I want to unit test is using, so I'm stubbing db, User, credentials. Is this right? Am I doing it acceptably?
• Any other bad practices I'm using?

They look good to me and I would have done the same. Outside things are being mocked so this module is being tested in isolation. Another way I think you could have mocked User.findOne, and that I've done in similar situations, is instead of injecting a mock User into users using rewire, you can simply spy on User.findOne before calling users.createUser. Depends on the situation but sometimes both work. I'd be curious to hear which approach is better.

@skaterdav85 Thanks! So, something like this?

test('Creating a user calls User.findOne', function (t) {
    var User = require('../lib/models/user');
    sinon.spy(User, 'findOne');

    users.createUser({ username: 'andrew', password: 'password' });

    t.equal(User.findOne.getCall(0).args[0].username, 'andrew');
    t.end();
});

Would it be fair to say that this test verifies that I am using an outside module correctly, while my other tests verify that my logic (surrounding the module usage) is acting as I want?

I was thinking something like this:

'use strict';
var test = require('tape');
var sinon = require('sinon');
var rewire = require('rewire');
var User = require('./some-path/user'); // importing User into the test

var users = rewire('../lib/users');

users.__set__('db', { once: sinon.stub().yields() });

test('Creating a user throws error if username is already taken', function (t) {
       // stubbing it out here before calling users.createUser()
        sinon.stub(User, 'findOne').yields(null, {});

    users.createUser({ username: 'andrew', password: 'password'}, function (err, user) {
        t.equal(err.message, 'User "andrew" already exists');
        t.end();
    });
});

// ....

Rather than injecting a mock into the users module using rewire, you can import User into the test and stub out findOne directly before the module being tested (users in this case) gets called. I'd be curious to hear what others do though.

@skaterdav85 Ah, I see; thanks!

Strive not to mix conditionals with asynchronous callback nesting, as that's very prone to cause confusion to your future self, and maybe even your present self.

One way to do that is to use more guard clause style. E.g

if (foo) {
  done(new Error('foo!')); return;
}

Instead of (in your code example)

if (foo) {
  done(new Error('foo!')); return;
} else {
  bar();
}

Other than that, your tests seem cool! Keep it up. Writing tests mostly comes down to practice!