New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicate actions in stream when used with redux-mock-store #389

Open
JalilArfaoui opened this Issue Dec 31, 2017 · 14 comments

Comments

Projects
None yet
6 participants
@JalilArfaoui

JalilArfaoui commented Dec 31, 2017

Do you want to request a feature or report a bug?

A bug I think, but not sure if it's redux-observable or redux-mock-store who has the wrong behavior. So I will post in both repos.

What is the current behavior?

When writing a single test with jest and redux-mock-store, everything works as expected.
But if I use the mockStore multiple times (in the same test or even in another), then the actions dispatched in any of the created stores are sent multiple times in the observable (but only once in the store, as store.getActions() states.

Here's a reproduction repo : https://framagit.org/jalil/redux-observable-mock-store-duplicate

tldr;

This works :

const store = mockStore();
store.dispatch({ type: 'FOO' }); // -> Observable get 1 FOO action

This doesn't :

const store = mockStore();
store2 = mockStore();
mockStore();
store.dispatch({ type: 'FOO' }); // => Observable get 3 FOO actions
const store = mockStore();
store2 = mockStore();
mockStore();
mockStore();
mockStore();
store.dispatch({ type: 'FOO' }); // -> Observable get 5 FOO actions

... and so on ...

What is the expected behavior?

I expect, as I use replaceEpic and mockStore, and I use different jest tests, that one test should not affect another, and one mockStore should not affect another.

So, I expect, even if I have multiple tests each one calling mockStore(), to have my epics receiving the right stream of actions.

Which versions of redux-observable, and which browser and OS are affected by this issue? Did this work in previous versions of redux-observable?

My above reproduction code uses :

"redux": "^3.7.2",
"redux-observable": "^0.17.0",
"rxjs": "^5.5.6"
"jest": "^22.0.4",
"redux-mock-store": "^1.4.0"

Thanks for your help.

@JalilArfaoui

This comment has been minimized.

JalilArfaoui commented Dec 31, 2017

Issue on redux-mock-store : dmitry-zaets/redux-mock-store#124

@jayphelps

This comment has been minimized.

Member

jayphelps commented Dec 31, 2017

Cross post from your StackOverflow question https://stackoverflow.com/questions/48045348/duplicate-actions-in-redux-observable-stream-when-used-with-redux-mock-store/48045589#48045589

redux-observable currently makes the assumption that the middleware function returned by createEpicMiddleware() won't be called multiple times. redux-mock-store appears to call it every time you call mockStore. While this behavior is probably not officially documented, my gut tells me redux-observable should not be making this assumption. Similar libraries like redux-saga used to as well, but they might have stopped.

It's not immediately clear why this has never been noticed before by anyone. I don't see any notable changes to either library that would have introduced this recently. My best guess is that it didn't have any perceivable side effect so no one noticed. e.g. your example doesn't filter any actions or emit any, instead just always logging with .do().

I'm on holiday right now, so I can't dig into this until later this week. Sorry! But you can work around it by creating a new epicMiddleware and calling configureMockStore for every test instead of reusing it. One example might be something like this:

const mockStore = (...args) => {
  const epicMiddleware = createEpicMiddleware(someEpic);
  const factory = configureMockStore([epicMiddleware]);
  return factory(...args);
};

Adjust to your needs; e.g. if you need to change the root epic.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Dec 31, 2017

For those who are interested, the internal implementation details of this bug: There is an epic$ Subject which is a stream of root epics. We create only one of these per middleware, when you call createEpicMiddleware(). Since redux-mock-store calls the epicMiddleware function multiple times, each time is calling epic$.next(rootEpic) and any previous internal subscriptions to epic$ receive that root epic and any actions you dispatch.

I have an idea on how to solve it, but it ain't pretty 😆

@JalilArfaoui

This comment has been minimized.

JalilArfaoui commented Dec 31, 2017

Thanks @jayphelps and sorry for cross-posting, I wasn't sure I wasn't doing something wrong ...

I can confirm that if I recreate configureMockStore and mockStore before each test, it works as expected, so there is no urge with this :-)
 
Just to respond to your best guess, I don't think it's why no one noticed ... I wanted my reproduction example to be simple so it does nothing but I had this on real project with real world epic. I guess no one saw it because no one tests epics :-) Or maybe only with one test case each ... Or maybe with a single mocked store ... Or maybe redefining createEpicMiddleware each time ...

@jayphelps

This comment has been minimized.

Member

jayphelps commented Dec 31, 2017

I guess no one saw it because no one tests epics

Dunno if this was a joke, but this is very very not true lol 😄 I've seen a large number of projects testing epics this way or similar.

Separately, a growing number of people test individual epics directly by just calling them with their own ActionsObservable, not using redux or any middleware. e.g. https://redux-observable.js.org/docs/recipes/InjectingDependenciesIntoEpics.html This is how I recommend people test and what the docs will suggest when I have time to adjust the them to reflect it. There's work being done with the rxjs TestScheduler as well in the next month or so.

@JalilArfaoui

This comment has been minimized.

JalilArfaoui commented Dec 31, 2017

Yes, I was joking :-)

Thank you for pointing out the dependency injection solution for testing ... I though it was better to test epics with a "real store with dispatched actions" ... but maybe I should contentrate on testing only my epics and not the whole redux-observable ... I'll try to follow your "Injecting Dependencies" example and see !

Can I help with the docs or anything ?

@JalilArfaoui

This comment has been minimized.

JalilArfaoui commented Dec 31, 2017

🎉 Happy New Year from France by the way :-) 🎉

@JalilArfaoui

This comment has been minimized.

JalilArfaoui commented Dec 31, 2017

I can see that the docs are in the repo, so I can try to make a PR if you like, to see if I got you correctly :-)

@jayphelps

This comment has been minimized.

Member

jayphelps commented Dec 31, 2017

Yes, I was joking :-)

😆 I figured hehe

Can I help with the docs or anything ?

In this particular case I'm planning to have them center around the rxjs TestScheduler changes that we're working on still. You're still welcome to submit a PR of course! Just be cautious not to spend too much time on them as they'll almost certainly be changing again. Happy new year! (signing off for the day)

@Nimelrian

This comment has been minimized.

Nimelrian commented Jan 24, 2018

Just ran into this issue myself and thought I was going crazy. Good to see that this is being tracked and that a workaround exists until the actual cause is fixed. 👍

@kralcifer-ms

This comment has been minimized.

kralcifer-ms commented Jan 26, 2018

@jayphelps I'm interested in trying out the rxjs TestScheduler stuff. Looks like you were posting about marble diagram type tests a couple of years ago. Should I jump in with what's available or is there something coming soon that I should wait for that you just hinted "working on still"?

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jan 26, 2018

The example I included is valid. There’s also this project which we’ve used to experiment with proposed changes to the TestScheduler https://www.npmjs.com/package/rx-sandbox

@besrabasant

This comment has been minimized.

besrabasant commented Feb 19, 2018

const mockStore = (...args) => {
      const epicMiddleware = createEpicMiddleware(someEpic);
      const factory = configureMockStore([epicMiddleware]);
      return factory(...args);
};

This saved my day. I have been stuck whole day trying to figure out what was wrong with my redux store configuration. Thanks @jayphelps

maxkostow added a commit to maxkostow/redux-observable that referenced this issue Mar 2, 2018

@maxkostow maxkostow referenced this issue Mar 2, 2018

Merged

Add a warning for #389 #441

2 of 2 tasks complete

jayphelps added a commit that referenced this issue Mar 2, 2018

@callumlocke

This comment has been minimized.

callumlocke commented Apr 17, 2018

Hi, just found this thread after a long debugging session. The custom mockStore function above works. Just so I understand, is this a suggested temporary workaround, and if so, what will the long-term solution look like?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment