createStore tests review

[splendido]

I had a chance to look at some tests while working at #1626 and noticed there could be room to make them cleaner and more organized (at least from my point of view...).

This is a first pass on the createStore.spec.js file.
Let me know if this kind of review could be of interest and I'll try to complete it also on the other files.

P.S. it could be easier to follow the changes commit by commit, rather than looking at the final result directly.

[timdorr]

Hmm, I'm not sure we need this. I think my biggest issue is the meta-programming that puts things related to a test in a different part of the file. Thank you for your contributions, though!

[splendido]

I think my biggest issue is the meta-programming that puts things related to a test in a different part of the file.

Could you please point out which lines of meta-programming are you referring to?

I think there were a number of good points among the proposed commits to make tests better and clearer to be read.
Really, no one single commit looking interesting to you?

[timdorr]

Could you please point out which lines of meta-programming are you referring to?

https://github.com/splendido/redux/blob/335b12b46e85b2ef46c1576572e55c48457c72e3/test/createStore.spec.js#L368-L373

Really, no one single commit looking interesting to you?

They are mainly style changes. Nothing affecting coverage or comprehension.

[splendido]

Ok, I now see what you mean with "meta-programming".
Definitions for particular list of inputs were put at the top to avoid mistakes and simplify tests.
Take for example this (old) and this (new), or this (old) and this (new): they both have the same purpose but it's difficult to follow the tests along the lines.
What more, if you compare the two lists of objects used to test the non-function input they're not the same!
With the list of nonFunctions defined once for all, in an attempt to simulate a fixture, you get:

• cleaner code
• less duplicated code
• less risk to miss some important test cases (the two series of tests are not alike right now...)

You could also compare the list of tests used here to confirm there is no coherence among all these tests trying to ensure exceptions are thrown in case non-functions are used for parameters requiring function.

I think this could be seen as an improvement on tests' coverage, as well as a way to improve tests readability.

In my opinion also changing from:

const listenerA = expect.createSpy(() => {})

to:

const listenerA = createSpy()

(see also docs for expect.createSpy which takes no parameters...)

or from:

const listenerB = expect.createSpy(() => {})
const unSubB = store.subscribe(() => {
  listenerB()
  unSubB()
})

to:

const listenerB = createSpy().andCall(() => unSubB())
const unSubB = store.subscribe(listenerB)

makes the code leaner cleaner, and more correct.
Not useful? Perhaps not necessary, I agree...