pburtchaell / redux-promise-middleware

Enables simple, yet robust handling of async action creators in Redux
https://pburtchaell.gitbook.io/redux-promise-middleware/
MIT License
1.98k stars 188 forks source link

Testing strategies #132

Closed andrewmclagan closed 7 years ago

andrewmclagan commented 7 years ago

Are there any standard testing strategies for this library?

I would be interested in writing a docs PR if people could outline their testing strategies vs our own.

pburtchaell commented 7 years ago

I don't have any standards to suggest right off the bat. I'd like to see what others have to suggest. Generally, I've written tests using the tests for the middleware as a model.

Do you have any suggestions, @andrewmclagan? I'd also be curious for your input here, @tomatau!

andrewmclagan commented 7 years ago

Firstly we are only just migrating to redux-promise-middleware from our custom thunk middleware.

REDUCERS:

I would not be testing the middleware as it has tests in the library. But rather testing how the reducer responds to *_REQUEST, *_SUCCESS, *_FAILURE actions. My only concern is keeping our test codebase DRY, as we don't actually have action creators for those three actions this becomes difficult. We currently test like this:

test('should handle FETCH_COLLECTION_REQUEST action correctly', () => {
  const action = {
    type: `${ActionTypes.FETCH_COLLECTION}_REQUEST`,
  };
  const actual = invoiceReducer(initialState, action);
  expect(Immutable.is(actual, expectedRequestState)).toBe(true);
});

export default function reducer(state = initialState, action = {}) {
  switch (action.type) {

    case `${InvoiceActions.FETCH_COLLECTION}_REQUEST`:
      return state
        .set('fetching', true)
        .set('error', false);

    default:
      return state;
  }
}

ACTION CREATORS

Easy. Simply do a deep equality test on the output of the action creator, again we wont be testing this middleware as it has tests:

export const fetchCollection = (params) => ({
  type: FETCH_COLLECTION,
  payload: Api.fetchInvoices(params),
});

test('fetchCollection creates correct action', () => {
  const params = { foo: 'bar', bar: 'foo' };
  expect(fetchCollection(params)).toEqual({
    type: FETCH_COLLECTION,
    payload: Api.fetchInvoices(params),
  });
});
andrewmclagan commented 7 years ago

The following is how we currently test reducers, my concern is manually reacting the three actions:

*_REQUEST, *_SUCCESS, *_FAILURE

We loose our win with reduced boilerplate when testing..?

siemiatj commented 7 years ago

Yeah I'd love to see examples of tests like in redux documentation : http://redux.js.org/docs/recipes/WritingTests.html#async-action-creators

tomatau commented 7 years ago

@andrewmclagan I test in a similar manner to what you've posted and find the *_REQUEST, *_SUCCESS, *_FAILURE explicitness useful in my tests. It's also more clear for newcomers joining my team who are still learning. Aiming for DRY here isn't such a big win?.. If you find the same pattern between multiple reducers then perhaps make an abstraction and test that once -- then test that the reducers use that abstraction instead of repeating the tests.

If the repeated template strings/concatenation of the promiseTypeSuffixes is a problem, perhaps make 3 functions that do that for you and reuse those functions instead. They could also be shared with your reducer functions themselves if you use the same suffixes throughout your project.

const requestType = (type) => `${type}_REQUEST`

test('should handle FETCH_COLLECTION_REQUEST action correctly', () => {
  const action = {
    type: requestType(ActionTypes.FETCH_COLLECTION),
  };
  const actual = invoiceReducer(initialState, action);
  expect(Immutable.is(actual, expectedRequestState)).toBe(true);
});

edit: It's also acceptable to make helpers in individual tests or shared between tests. If you find your tests to have repeated logic and code then you can just refactor them with functions and logic.

This issue feels more related to writing unit tests in general and writing tests for reducers in general - it's not really specific to this middleware.

tomatau commented 7 years ago

Here's a test file from my colleague and I think it works just fine!

import accountReducers from './accountCheck.reducers'
import { ACCOUNT_CHECK } from './accountCheck.actions'

describe('Account Check Reducers', function() {
  const initialState = {
    isPending: false,
    error: false,
    data: {},
  }
  const irrelevantAction = { type: 'IRRELEVANT_ACTION' }

  it(`returns the initialState when no state`, () => {
    expect(accountReducers(undefined, irrelevantAction)).to.eql(initialState)
  })

  describe(`ACCOUNT_CHECK`, () => {
    beforeEach(() => {
      this.previousState = {
        some: 'test random state',
      }
    })

    describe(`REQUEST`, () => {
      const accountRequestAction = {
        type: `${ACCOUNT_CHECK}_REQUEST`,
      }

      it(`sets initialState with isPending:true`, () => {
        const newState = accountReducers(this.previousState, accountRequestAction)
        expect(newState).to.eql({
          ...initialState,
          isPending: true,
        })
      })
    })

    describe(`ERROR`, () => {
      const accountErrorAction = {
        type: `${ACCOUNT_CHECK}_ERROR`,
        error: true,
        payload: new Error('some error'),
      }

      it(`sets initialState with error from action`, () => {
        const newState = accountReducers(this.previousState, accountErrorAction)
        expect(newState).to.eql({
          ...initialState,
          error: accountErrorAction.payload,
        })
      })
    })

    describe(`RECEIVE`, () => {
      const accountRecieveAction = {
        type: `${ACCOUNT_CHECK}_RECEIVE`,
        payload: {
          test: 'test payload',
        },
      }

      it(`sets initialState with data as action's payload`, () => {
        const newState = accountReducers(this.previousState, accountRecieveAction)
        expect(newState).to.eql({
          ...initialState,
          data: accountRecieveAction.payload,
        })
      })
    })
  })
})
pburtchaell commented 7 years ago

This issue feels more related to writing unit tests in general and writing tests for reducers in general. It's not really specific to this middleware.

I agree with @tomatau that this isn't directly related to the library, but to general testing of reducers and async action creators. That being said, it would be useful to have more extensive documentation—as well as a complete example—on this subject. I think testing often gets less attention when community docs are written.

Perhaps we could add a guide to the middleware docs?

Would you be interested in taking an initial stab at that, @andrewmclagan?

siemiatj commented 7 years ago

Well I was actually thinking about how to write tests for action creators that perform some XHR requests. I'm struggling to get this working after 2 days now and reading through a lot of source code (including packages and node itself). So I have a simple action creator like this one :

const createProject = (subCategoryId, title) => (dispatch) => (
  dispatch({
    type: CREATE,
    payload: axios.post('/api/v1/projects/', {
      sub_category: subCategoryId,
      title,
    })
  })
);

And now I'm trying to write a test case for it. So I used the standard approach here :

it('Creates a project if one does not exists yet', (done) => {
  nock('http://django:8000')
    .post('/api/v1/projects')
    .reply(200, { body: { data: { ...projectData } } });

  const expectedActions = [
    {
      type: commonImports.CREATE_PENDING,
      sub_category: projectData.sub_category,
      title: projectData.title,
    }, {
      type: commonImports.CREATE_FULFILLED,
      payload: { ...projectData },
    }
  ];
  const store = mockStore({
    pap: { project: { data: null } },
  });

  store.dispatch(createProject(projectData.sub_category, projectData.title));
    .then((res) => {
      expect(store.getActions()).toEqual(expectedActions);
    })
    .then(done)
    .catch(done);
});

but I'm getting (node:48825) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): TypeError: Cannot read property 'protocol' of undefined

I've tried using sinon and moxios but the same happens. Not sure if it's caused by my setup (wabpack + mocha + jsdom) or node or I should write the test differently.

UPDATE

Ok, looks that I've finally figured it out :

  it('Creates a project if one does not exists yet ', done => {
    const expectedActions = [
      {
        type: projectImports.CREATE_PENDING,
      },
      {
        type: projectImports.CREATE_RESOLVED,
        payload: { ...projectData },
        status: 200,
      },
    ];

    sandbox.stub(axios, 'post').returns(
      new Promise((resolve, reject) => {
        resolve({ status: 200, data: { ...projectData } });
      })
    );

    const store = mockStore({
      pap: { project: {} },
    });
    store.dispatch(projectImports.createProjectIfNeeded(projectData.sub_category, projectData.title))
      .then(() => {
        expect(store.getActions()).toEqual(expectedActions);
      }).then(done)
      .catch(done);
  });
tomatau commented 7 years ago

@siemiatj - Most of that looks fine!

I'd suggest stubbing your dependency (axios) rather than stubbing the endpoint through nock. It will make your test run quicker and you won't actually need to open any requests.

It will also solve your problem as axios when run in node context (not the browser) uses http which has different behaviour to XHR - it requires a full uri with protocol.

I really don't understand how nock has become so popular in this situation tbh, sinon.stub is even less verbose! Nock really just doesn't make sense as a "unit" test library - it's for "functional", acceptance or some people's take on "integration" tests.

tomatau commented 7 years ago

Here's a file and it's tests from one of my colleagues

import * as Thing from './services/Thing';
import config from 'config/app.config';

const actions = {
  GET_THINGS: 'GET_THINGS',

  getThings: (token) => ({
    type: actions.GET_THINGS,
    payload: {
      promise: Thing.getThings(token)
    }
  })
};

export default actions;

And the tests (sorry if they're broken I've had to delete some stuff in githubs comment editor)

import { expect } from 'chai';
import actions from './';
import configureStore from 'redux-mock-store';
import thunk from 'redux-thunk';
import promiseMiddleware from 'app/store/promise-middleware';
import * as Thing from './services/Thing';
import sinon from 'sinon';
import config from 'config/app.config';

describe('Thing Actions', () => {
  let store;
  beforeEach(() => {
    sinon.stub(Thing, 'getThings');
    const mockStore = configureStore([thunk, promiseMiddleware()]);
    store = mockStore({});
  });

  afterEach(() => {
    Thing.getServiceChecks.restore();
  });

  describe('Action types are types', () => {
    expect(actions.GET_THINGS).to.eql('GET_THINGS');
  });

  describe('serviceChecks()', () => {
    context('Given Thing.getThings() resolves', () => {
      const resolvedObject = { value: 'whatever' };
      beforeEach(() => {
        Thing.getThings.returns(Promise.resolve(resolvedObject));
      });

      it('should return a GET_THINGS action', () => {
        expect(actions.getThings("1234557")).to.have.property(
          'type', actions.GET_THINGS
        );
      });

      it('should resolve the resolvedObject from Things', () => {
        expect(
          actions.getThings().payload.promise
        ).to.become(resolvedObject);
      });

      it('dispatches GET_THINGS_PENDING and GET_THINGS_FULFILLED actions', () => {
        const testData = { test: '1234' };
        const expectedActions = [{
          type: `${actions.GET_THINGS}_PENDING`
        }, {
          type: `${actions.GET_THINGS}_FULFILLED`,
          payload: testData
        }];

        Thing.getThings.returns(Promise.resolve(testData));
        return store.dispatch(actions.getThings('1234'))
          .then(() => expect(store.getActions()).to.eql(expectedActions));
      });
    });

    context('Given Thing.getThings() rejects', () => {
      const rejectedObject = new Error('404');
      beforeEach(() => {
        Thing.getThings.returns(Promise.reject(rejectedObject));
      });

      it('should reject the rejectedObject', () => {
        expect(actions.getThings('12345').payload.promise).to.be.rejectedWith(rejectedObject);
      });

      it('dispatches PENDING and REJECTED actions', () => {
        const testError = new Error('testing');
        const expectedActions = [{
          type: `${actions.GET_THINGS}_PENDING`
        }, {
          type: `${actions.GET_THINGS}_REJECTED`,
          payload: testError,
          error: true
        }];

        Thing.getThings.returns(Promise.reject(testError));
        return store.dispatch(actions.serviceChecks('1234'))
          .then(
            () => expect(true).to.eql(false, 'getThings should have thrown'),
            () => expect(store.getActions()).to.eql(expectedActions)
          );
      });
    });
  });
});
andrewmclagan commented 7 years ago

@pburtchaell I'd be happy to write up some testing docs. Wont be for a few weeks as we are pressed at present.

@tomatau Yes we also created functions to abstract the request cycle boiler plate :)

/**
 * Appends REQUEST asyc action type
 *
 * @author Andrew McLagan <andrew@ethicaljobs.com.au>
 */

export function REQUEST(actionType) {
  return `${actionType}_REQUEST`;
}

/**
 * Appends SUCCESS asyc action type
 *
 * @author Andrew McLagan <andrew@ethicaljobs.com.au>
 */

export function SUCCESS(actionType) {
  return `${actionType}_SUCCESS`;
}

/**
 * Appends FAILURE asyc action type
 *
 * @author Andrew McLagan <andrew@ethicaljobs.com.au>
 */

export function FAILURE(actionType) {
  return `${actionType}_FAILURE`;
}
pburtchaell commented 7 years ago

I recently added tests for to the complex example for the global error middleware and the async action creators. There is nothing new here in terms of testing strategies, but it could be referenced in the docs.

andrewmclagan commented 7 years ago

As a follow on from this.

What are peoples strategies for testing the promise payload of an async action creator?

For example:

// Action creator
function fetchPosts(params) {
  return {
    type: 'FETCH_POSTS',
    payload: api.get('/posts', params),
  };
}

// Testing the action creator
test('fetchPosts creates correct action', () => {
  const action = fetchPosts({ limit: 10, category: 'blog' });
  expect(action).toEqual({
    type: 'FETCH_POSTS',
    payload: api.get('/posts', { limit: 10, category: 'blog' }),
  });
});

This will assert a correct action { type } and fail when it is incorrect.

Although the problem is its not asserting that api.get(...) is being called with the correct arguments.

Would I be correct to assume that this is covered by testing how the reducer reacts to the async action types { REQUEST, SUCCESS, FAILURE } the middleware creates?

Perhaps not because the result of api.get(...) is an external influence and is only testable through fixtures? Im a little lost :-) It seems there is room for any arguments to the api helper to be incorrect as we cant only assert that a promise has been set as the payload.

IMO mocking is really smelly code, when we are doing functional programming. We followed this example in the redux docs, although do people think its allot of work to test a simple payload? Are there strategies to DRY this up?

http://redux.js.org/docs/recipes/WritingTests.html

nicolechung commented 7 years ago

(Using Jest) I wrote a basic response payload:

const expectedAction = {
    type: ACTION_NAME,
    payload: {
      status: 200,
      data: {
       // what your response data is
      }
    }
  }

And then used "Match Object"...it doesn't have to be exactly like the response as long as it contains the stuff you care about (status code, data)

return store.dispatch(fetchLyric())
  .then(() => {
    let actions = store.getActions() // this is an array but my promise just returns one action
    expect(actions[0]).toMatchObject(expectedAction)
  })
andrewmclagan commented 7 years ago

@nicolechung ah cool, so create a mock store with the middleware?

nicolechung commented 7 years ago

I think so. It's probably a big motivator for people to switch to redux-saga, because with sagas you don't have to mock api calls.

Api calls - one thing that has tripped me up is that you need different mock libraries (is that the word?) depending on what you are using to make your AJAX / api calls.

If you are using axios I would recommend using moxios to mock your requests and responses.

If you are using fetch, nock seems to work okay.