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

Improve API naming #204

Open Wollantine opened 6 years ago

Wollantine commented 6 years ago

I'm a big fan of how BDD strives for readability in tests. For example:

expect(favouriteLibrariesOf(self)).to.contain('redux-saga-test-plan');

I have the impression that this library wants to follow this pattern, hence the name expectSaga for its main entry point, however, I think the API naming could be improved towards BDD.

I think this improvement would benefit the lib's learning curve, its adoption, and its usefulness. That's why I wanted to start this discussion.

The usual flow for tests in redux-saga-test-plan is as follows:

expectSaga(mySaga)
  .assertion()
  .assertion()
  ...
  .setupMethod(...)
  .setupMethod(...)
  ...
  .run();

Take the great example at http://redux-saga-test-plan.jeremyfairbank.com/integration-testing/dispatching.html as a demonstration.

However, when looking at the code, two things happen:

How could those be solved?

Separate assertions from setup

As we want to keep the API surface minimal, I find it great that both types of methods can be chained in the same chain. So, one thing that could help in telling apart one from the other is some sort of convention among the groups.

For example, all setup methods could start with with, like withStateand withReducers already do:

expectSaga(mySaga)
  .withReducers(...)
  .withProviders(...)
  .withDispatch(...)
  .withRace(...)

You could also argue that there are two types of setup methods: Environment setup helpers, and effect creators. That's fine too, if the lib helps telling them apart.

Or all assertion methods could be prefixed with to and be infinitives:

expectSaga(mySaga)
  ...
  .toPut(...)
  .toCall(...)

Note that you don't need a convention for all the groups, but for N-1 of them. We people are great recognising patterns, but also the lack of them. So if all setup methods are prefixed with with, those that are not must be something else -> assertions.

Tests readable as a natural sentence

What matters here is, mainly, the conjunctions and prepositions and the time of the verbs. For example, when writing (kind of) natural language, we would write:

Expect saga mySaga, with state X, with providers Y, with dispatch actionA, to put actionA, to call functionB. Run!

Or maybe

Expect saga mySaga, with state X, on dispatching actionA, on selecting pieceOfStateB, to put actionC. Run!

Chai.js does this very well by providing language connectors that don't add any semantics to the code, e.g.

expect(object).to.have.a.property('a').that.equals(4).but.does.not.equal(object.b);

Maybe a similar approach could be used. This way, breaking changes could be avoided, although I want to leave the issue on whether to make it a breaking change or not to the current maintainers of the library.

Whatever decision on naming is taken, I would be happy to make a PR of it.

Thoughts? Ideas?

hktonylee commented 5 years ago

I mostly agree what is written in this article. The current naming is very confusing. I cannot tell if this method expectSaga(...).dispatch(someAction) is to expect an action, or dispatch an action. This is even more confusing when I see expectSaga(...).put(someAction) which is basically doing the another thing.

GabrielfLuchtenberg commented 5 years ago

+1 It would be much better if we have toPut, toCall @jfairbank if we create a PR with those changes, does it have any chances to be accepted?