3

I've been reading some articles, and posts here on Stack Overflow about when I should mock a function and when I shouldn't, but I have a case where I'm not sure about what to do.

I have a UserService class which uses dependency injection concept to receive dependencies through its constructor.

class UserService {

 constructor(userRepository) {
    this.userRepository = userRepository;
 }

 async getUserByEmail(userEmail) {
    // would perform some validations to check if the value is an e-mail

    const user = await this.userRepository.findByEmail(email);

    return user;
 }

 async createUser(userData) {
    const isEmailInUse = await this.getUserByEmail(userData.email);

    if(isEmailInUse) {
        return "error";
    } 

    const user = await this.userRepository.create(userData);

    return user;
 }

} 

I want to test if the createUser method works properly, and for my tests, I created a fake userRepository which is basically a object with mocked methods that I will use while instantiating UserService Class

const UserService = require('./UserService.js');

describe("User Service tests", () => {

let userService;
let userRepository;

beforeEach(() => {
    userRepository = {
        findOne: jest.fn(),
        create: jest.fn(),
    }

    userService = new UserService(userRepository);
});

afterEach(() => {
    resetAllMocks();
});

describe("createUser", () => {

    it("should be able to create a new user", async () => {
        const newUserData = { name: 'User', email: 'user@test.com.br' }
        const user = { id: 1, name: 'User', email: 'user@test.com.br' }

        userRepository.create.mockResolvedValue(user);

        const result = await userService.createUser();

        expect(result).toStrictEqual(user);
    })
})

})

Note that in the createUser method, there is a call to the getUserByEmail method which is also a method of UserService class, and that is where I got confused.

Should I mock the getUserByEmail method even it is a method of the class I'm testing? If it is not the correct approach, what should I do?

jonrsharpe
  • 115,751
  • 26
  • 228
  • 437
Eduardo
  • 407
  • 1
  • 4
  • 12
  • This question is rather opinion-based, but I'd say that only mocking the repository will make your whole class easier to test. Only one thing to mock, and that way you check that your class methods work together to give you the expected result (returns `"error"` when you try to create an existing user) – blex Mar 21 '21 at 23:19
  • @blex My concern is that if the getUserByEmail method has some bug, the createUser test may fail even the problem is not in the function itself – Eduardo Mar 21 '21 at 23:35
  • 1
    It always depends on the case. You can consider them different units and test separately, or consider them one and test together. Since you mock a dependency, this reduces the number of moving parts so there will be less risks for *fail even the problem is not in the function itself*. Regardless of that, you can spy on a method to make sure a call meets your expectations. – Estus Flask Mar 22 '21 at 07:00

2 Answers2

3

You should almost always prefer not to mock parts of the thing you're supposed to be testing, in this case UserService. To illustrate why, consider these two tests:

  1. Provides a test double implementation for findByEmail on the repo object:

    it("throws an error if the user already exists", async () => {
        const email = "foo@bar.baz";
        const user = { email, name: "Foo Barrington" };
        const service = new UserService({
            findByEmail: (_email) => Promise.resolve(_email === email ? user : null),
        });
    
        await expect(service.createUser(user)).rejects.toThrow("User already exists");
    });
    
  2. Stubs out the service's own getUserByEmail method:

    it("throws an error if the user already exists", async () => {
        const email = "foo@bar.baz";
        const user = { email, name: "Foo Barrington" };
        const service = new UserService({});
        service.getUserByEmail = (_email) => Promise.resolve(_email === email ? user : null);
    
        await expect(service.createUser(user)).rejects.toThrow("User already exists");
    });
    

For your current implementation, both pass just fine. But let's think about how things might change.


Imagine we need to enrich the user model getUserByEmail provides at some point:

async getUserByEmail(userEmail) {
    const user = await this.userRepository.findByEmail(userEmail);
    user.moreStuff = await this.userRepository.getSomething(user.id);
    return user;
}

Obviously we don't need this extra data just to know whether or not the user exists, so we factor out the basic user object retrieval:

async getUserByEmail(userEmail) {
    const user = await this._getUser(userEmail);
    user.moreStuff = await.this.userRepository.getSomething(user.id);
    return user;
}

async createUser(userData) {
    if (await this._getUser(userData.email)) {
        throw new Error("User already exists");
    }
    return this.userRepository.create(userData);
}

async _getUser(userEmail) {
    return this.userRepository.findByEmail(userEmail);
}

If we were using test 1, we wouldn't have to change it at all - we're still consuming findByEmail on the repo, the fact that the internal implementation has changed is opaque to our test. But with test 2, that's now failing even though the code still does the same things. This is a false positive; the functionality works but the test fails.

In fact you could have applied that refactor, extracting _getUser, prior to a new feature making the need so clear; the fact that createUser uses getUserByEmail directly reflects accidental duplication of this.userRepository.findByEmail(email) - they have different reasons to change.


Or imagine we make some change that breaks getUserByEmail. Let's simulate a problem with the enrichment, for example:

async getUserByEmail(userEmail) {
    const user = await this.userRepository.findByEmail(userEmail);
    throw new Error("lol whoops!");
    return user;
}

If we're using test 1, our test for createUser fails too, but that's the correct outcome! The implementation is broken, a user cannot be created. With test 2 we have a false negative; the test passes but the functionality doesn't work.

In this case, you could say that it's better to see that only getUserByEmail is failing because that's where the problem is, but I'd contend that would be pretty confusing when you looked at the code: "createUser calls that method too, but the tests say it's fine...".

jonrsharpe
  • 115,751
  • 26
  • 228
  • 437
0

You shouldn't mock any of these functions since its creating users and reading data from the database. If you mock them then what's the point of the test. In other words, you wouldn't know if your app is working correctly with the database or not. Anyway, I would mock functions such as the functions that send emails and so on. Don't mock the functions that are the heart of the application. You should have a database for testing and another one for production.

  • 3
    That's what system tests are for (or integration tests). Here, OP is talking about unit testing – blex Mar 21 '21 at 23:20