jfairbank / redux-saga-test-plan

Test Redux Saga with an easy plan.
http://redux-saga-test-plan.jeremyfairbank.com
MIT License
1.25k stars 127 forks source link

expectSaga test failure on missing assertions #99

Open adamterlson opened 7 years ago

adamterlson commented 7 years ago

Feature request: it'd be wonderful to be able to assert that a given saga yields exactly the specified effects--no more, and (critically for this ticket) no less.

The following example demonstrates the issue:

function* mainSaga(x, y) {
  yield take('HELLO');
  yield put({ type: 'ADD', payload: x + y });
  yield take('WORLD');
  yield put({ type: 'DONE' });
}

it('PARTIALLY handles dispatching actions', () => {
  return expectSaga(mainSaga, 40, 2)
    .put({ type: 'DONE' })
    // Put assertion on "ADD" is missing so the test does not break
    .run();
});

So perhaps a new helper which ensures that all side effects have an assertion would be nice to help developers be sure that their saga is appropriately covered:

it('EXPLICITLY handles dispatching actions', () => {
  return expectSaga(mainSaga, 40, 2)
    .put({ type: 'DONE' })
    .runExactly();
   // Put assertion on "ADD" is missing, test breaks!
});

I'm just making up a crappy API that probably won't work. Welcome your thoughts!

jfairbank commented 7 years ago

Hi, @adamterlson!

I like the spirit behind this idea, but requiring exact effects sounds like it couples the test to the implementation too much. If we required exact effects, then you'd also have to assert that the example saga yields take('HELLO') and take('WORLD').

If the desire is to require exact effects of a certain type, then we have a couple options. We could add a method like .put.exactly() as shown below:

it('EXPLICITLY handles dispatching actions', () => {
  return expectSaga(mainSaga, 40, 2)
    .put.exactly([
      { type: 'DONE' },
      { type: 'ADD', payload: 42 },
    ])
    .run();
});

Or we could potentially expose all yielded effects in the fulfilled value of the promise, as you and I discussed in #72.

it('EXPLICITLY handles dispatching actions', async () => {
  const effects = await expectSaga(mainSaga, 40, 2)
    .put({ type: 'ADD', payload: 42 })
    .put({ type: 'DONE' })
    .run();

  expect(effects.put.length).toBe(2);
});

What are your thoughts?

adamterlson commented 7 years ago

I like the spirit behind this idea, but requiring exact effects sounds like it couples the test to the implementation too much. If we required exact effects, then you'd also have to assert that the example saga yields take('HELLO') and take('WORLD').

You're right, I would indeed need to assert that the saga yields those takes if my runExactly were implemented. I copied from one of the doc samples and forgot to remove that bit from the saga as I did from the test! Whoops!

As for implementation coupling, I assume you don't feel that way toward the put.exactly example you gave? I certainly see your concern applying to .runExactly, but not put.exactly as it's just a more powerful assertion.

My thought right now is: why not both? Having put.exactly as shown would be quite nice (does it also give the ability to make assertions on effect order?), while having all effects available would make possible some very powerful checks that are not otherwise possible with the put.exactly helper.

I would also want a way to get all effects, of all types, in the order they were yielded. That is, expect(effects.length).toBe(2) and expect(effects[1].type).toBe('PUT')

Thanks for the discussion!