6
\$\begingroup\$

I've wrapped an imported function (a promise) and attempted to write unit tests that bring my codebase to 100% coverage.

import { Auth } from 'aws-amplify';

import signIn from './shared';

describe('signIn', () => {
  afterEach(() => {
    jest.restoreAllMocks();
  });

  it('invokes the success callback on success', (done) => {
    const mockUser = { username: 'jon', password: 'snow' };

    jest
      .spyOn(Auth, 'signIn')
      .mockImplementation(() => Promise.resolve(mockUser));

    const mockSuccessCb = (user) => {
      expect(user).toBe(mockUser);
      done();
    };

    const mockErrorCb = (err) => {
      done(err);
    };

    signIn('jon', 'snow', mockSuccessCb, mockErrorCb);
  });

  it('invokes the error callback on err', (done) => {
    const mockUser = { username: 'jon', password: 'snow' };

    jest
      .spyOn(Auth, 'signIn')
      .mockImplementation(() => Promise.reject(mockUser));

    const mockSuccessCb = (user) => {
      expect(Auth.signIn).toHaveBeenCalledWith('jon', 'snow');
      expect(user).toBe(mockUser);
      done();
    };

    const mockErrorCb = () => {
      expect(Auth.signIn).toHaveBeenCalledWith('bob', 'tom');
      done();
    };

    signIn('bob', 'tom', mockSuccessCb, mockErrorCb);
  });
});

Auth is the imported object that is getting wrapped. In my first test, I attempt verify a successful login by returning a resolved promise. In the second test, I reject the promise. I got my 100% coverage, but since I tell my promise how to resolve itself, I can't help but feel like I'm setting myself up for false positives. Is this a good test? How can it be improved?

Edit: Added the original wrapper function for detail:

import { Auth } from 'aws-amplify';

const signIn = (
  username,
  password,
  successCallback,
  errorCallback,
) => {
  Auth.signIn(username, password)
    .then(user => successCallback(user))
    .catch(err => errorCallback(err));
};

export default signIn;
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

I think the test is ok. Remember what you're testing is this

const signIn = (
  username,
  password,
  successCallback,
  errorCallback,
) => {
  Auth.signIn(username, password)
    .then(user => successCallback(user))
    .catch(err => errorCallback(err));
};

and NOT Auth. Here Auth is just a dependency so you will stub its functionality. Considering that its interface is Promise-based, you need to test both cases - therefore having a test that verifies your expected (promise solved) and the one with the error is correct.

There are some changes I would add, though. For instance, rather than creating my own successCallback and errorCallback I would use mock functions, that would allow me to have further information

const successCallback = jest.fn();
const rejectCallback = jest.fn();

and I can check there if they were called or not and verify their arguments.

expect(successCallback).toHaveBeenCalledWith(mockUser);
expect(rejectCallback).not.toHaveBeenCalled();

and the other way around for the other test.

Then, I would take advantage of async/await for the tests, so we don't need to ensure that ugly done function is called. Basically, we declare the arrow function of the test as async, we await the promise calls and just by having the arrow function to be finished ensures our tests have passed.

An example of these changes would be something like this

it('invokes the success callback on success', async () => {
    jest
      .spyOn(Auth, 'signIn')
      .mockImplementation(() => Promise.resolve(mockUser));
    const successCallback = jest.fn();
    const rejectCallback = jest.fn();
    await signIn('jon', 'snow', successCallback, rejectCallback)
    expect(Auth.signIn).toHaveBeenCalledWith('jon', 'snow');
    expect(successCallback).toHaveBeenCalledWith(mockUser);
    expect(rejectCallback).not.toHaveBeenCalled();
  });

Another step would be that we have strings all over our tests, such as "jon" and "snow". If we already have a mocked user we could reference them from there by using mockUser.username, mockUser.password

That leaves us with the tests looking like this

it('invokes the success callback on success', async () => {
    jest
      .spyOn(Auth, 'signIn')
      .mockImplementation(() => Promise.resolve(mockUser));
    const successCallback = jest.fn();
    const rejectCallback = jest.fn();
    await signIn(mockUser.username, mockUser.password, successCallback, rejectCallback)
    expect(Auth.signIn).toHaveBeenCalledWith(mockUser.username, mockUser.password);
    expect(successCallback).toHaveBeenCalledWith(mockUser);
    expect(rejectCallback).not.toHaveBeenCalled();
  });

  it('invokes the error callback on err', async () => {
    jest
      .spyOn(Auth, 'signIn')
      .mockImplementation(() => Promise.reject(mockUser));

    const successCallback = jest.fn();
    const rejectCallback = jest.fn();
    await signIn(mockUser.username, mockUser.password, successCallback, rejectCallback);
    expect(Auth.signIn).toHaveBeenCalledWith(mockUser.username, mockUser.password);
    expect(rejectCallback).toHaveBeenCalledWith(mockUser);
    expect(successCallback).not.toHaveBeenCalled();
  });

As you can see they are pretty similar, so we could somewhat extract that functionality to a method with the common assertions.

We could create a function like this

const assertSignIn = async (mockedResponse, shouldBeCalledCallback, shouldNotBeCalledCallback) => {
        jest
            .spyOn(Auth, 'signIn')
            .mockImplementation(() => mockedResponse);
        const successCallback = jest.fn();
        const rejectCallback = jest.fn();
        await signIn(mockUser.username, mockUser.password, successCallback, rejectCallback)
        expect(Auth.signIn).toHaveBeenCalledWith(mockUser.username, mockUser.password);
        expect(shouldBeCalledCallback).toHaveBeenCalledWith(mockUser);
        expect(shouldNotBeCalledCallback).not.toHaveBeenCalled();
  };

and then call it properly from each test

it('invokes the success callback on success', async () => {
    const successCallback = jest.fn();
    const rejectCallback = jest.fn();
    await assertSignIn(Promise.resolve(mockUser), successCallback, rejectCallback);
  });

  it('invokes the error callback on err', async () => {
    const successCallback = jest.fn();
    const rejectCallback = jest.fn();
    await assertSignIn(Promise.reject(mockUser), rejectCallback, successCallback);
  });

Although both last parameters are mock function, the difference is conceptual - by declaring them outside you can ensure by reading the tests in which the successCallback should be executed and in which should not.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Welcome to Code Review! Good job on your first answer. Keep it coming! :-) \$\endgroup\$
    – AlexV
    May 15, 2019 at 22:09
  • \$\begingroup\$ thank you! I am looking forward to reviewing more js snippets :) \$\endgroup\$
    – Gonzalo.-
    May 16, 2019 at 1:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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