Skip to content
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

Potential issues with store as a prop and shallow test rendering in v6 #1161

Closed
markerikson opened this issue Jan 11, 2019 · 78 comments
Closed

Comments

@markerikson
Copy link
Contributor

I've seen a few different reports related to this topic, so I'm going to open up this issue for discussion.

In React-Redux v6, we removed the ability to pass the store as a prop named store directly to connected components. (See my post explaining why this change was made.)

Generally, the only meaningful use case I knew of for this capability was for unit testing, as some people passed a store directly as a prop instead of wrapping the component in a <Provider>.

I've seen some complaints that Enzyme's shallow() does not work well with putting a <Provider> around a connected component, but I'm not clear on the details of what exactly breaks.

Is it that Enzyme still does not support React's createContext correctly? That rendering shallow(<Provider store={store}><ConnectedComponent /></Provider>) does not actually generate any output from <ConnectedComponent />? Something else?

As a related question, what exactly does it mean to "shallow-render a connected component"? What do tests with this approach look like, particularly in terms of Enzyme API usage and assertions?

I'd appreciate some examples of tests that are having issues, explanations of what specifically is breaking (beyond just "I can't pass the store as a prop any more"), and any further details people can provide related to this issue.

@dschinkel
Copy link

dschinkel commented Jan 12, 2019

One of the biggest problems I have with React and Redux lately is that they are slowly both forcing everyone down the path of using the Context API without considering backward compatibility for isolated tests specifically with the most popular lib used still which is enzyme. All along React's and Redux's API was simple enough to where you could do simple things with it through shallow. That's no longer the case as this API evolves...and those who have happily and effectively written tests this way are having to come back and let you folks know about it when I think these uses cases should have already been found and part of React and Redux release testing.

By using the Context API that requires you to use mount() because you simply can no longer do easy things to components such as shallow. Redux 6 shut off the ability to pass in fake stores as props. All my past tests will break if we upgrade. Do I want my clients to sit on v5 for the next 5 years? I guess they could or we would have to rewrite the application (worse). Both situations are bad for the users of Redux and React in both scenarios; Wouldn't it be better would be if React and Redux would allow us to continue to write isolated tests easily as we've been able to do for the past 3-4 years by not restricting the API or changing it in a way that would prevent test simplicity?

I personally don't like using provider in my tests, it's huge bloat. It's been completely necessary the way I code react applications...unless you're writing integration tests. That's the thing, the React team seems to mainly just promote integration testing which is another beef I've had with that "framework". I cannot call this a lib anymore.

What if there are those who don't want to use contextAPI for persistence and actually are fine with passing state as props and find that route "simplistic" regardless of the "inconvenience".

The problem I see is that the way React and Redux are moving, they're continually changing the API in a way that undermines the ability to shallow and IMO they don't really care because they would have already noticed that enzyme shallow is breaking due to their major API changes. They're not backward compatible anymore, new react features are literally breaking things and they don't care about isolated tests IMO generally which is my general take on it because if they did they'd already have seen this issue before releasing such major changes to their API.

It's fundamental to be able to shallow components. That's what made react and redux so great in the beginning. VueJS still has that ability in fact it's built right into their testing lib.

On the point of TDD: I'm not saying you have to TDD. But those who do, we just want to be able to continue building these apps and test them in the same way we've always been able to. To shut off simple DI like that and to force people down paths that literally kill shallow to me is just not a very good move by React or Redux. The fact is if the React and Redux teams themselves wrote isolated tests more using enzyme shallow, they would have already seen this pain.

What if we're not using createContext() and we still want to shallow test components and DI other ways through our isolated tests? Then we don't use Redux anymore. Well that kinds sucks because if Redux 6 already allowed us to shallow easily and inject the store still, this wouldn't even be an issue for those who test using shallow without provider.

But even after that, lets say we don't use Redux anymore after that pain. React still is a barrier/concern because for example recently hooks, you can't shallow render hooks. Another TDD'ist I know ranted on not being able to shallow render hooks. Sure we can put in an issue but shouldn't the React team already figure that there are people who are going to want to shallow render still with "new stuff"?

I would like to think that at the least, the React and Redux team at least considers testing enzyme shallow and mount both, not just mount when they release stuff. They should find that hey, if we change the API this way, it might be good, but we might prevent the ability to shallow render with enzyme's existing lib, something they've been able to do all along. Or they should say hey, if we remove the ability to inject stuff simply as props, it's going to affect shallow tests in x way. The use cases are pretty simple on the enzyme side, you showed one already @markerikson

I don't have time to write a huge blog post on this at the moment @markerikso. There are tons of articles that have been out there for years on shallow rendering React or Redux and passing in the store as a prop. Those are plenty of use cases IMO.

@markerikson
Copy link
Contributor Author

markerikson commented Jan 12, 2019

So as I see it, there's a few general / potential issues here overall:

I think it's also important to note that both Enzyme and React-Redux are separate libraries and not controlled by the React team. Also, the React team does not, as far as I know, directly promote the use of Enzyme for testing.

@dschinkel , you've written a lot of words there. I appreciate that you actually more reasonably addressed this issue, but you're still primarily talking in generalities. It would help if you could specifically address the questions I've asked, provide some concrete examples of tests that you've written, and offer up some actual potential solutions (beyond just "bring back store-as-prop").

In other words: what things are you trying to assert in these tests? What capabilities do you need - just passing in a particular state to render the component's output?

As my mom said to me many times when I was growing up: "You've stated the problem. Now state the solution". :)

I'm also confused why "using Provider in my test is huge bloat".

@dschinkel
Copy link

dschinkel commented Jan 12, 2019

Here's the thing, I don't agree that this is all on the enzyme lib. You can evolve React or Redux in a way that's more backward compatible that supports testing components shallowly naturally so it makes any testing library now or in the future able to do that with simplicity. What if I use something other than enzyme? We're going to want to shallow test. But by closing of the API for certain DI routes, you automatically prevent that for any lib going forward.

I'm also confused why "using Provider in my test is huge bloat".

1) provider is really meant for cases where you want to persist state down not just the component under test but also its children. When I write isolated tests I don't care about rendering children or passing state down to their render
2) For test simplicity and readability. I've found that provider is completely necessary especially when our tests are very focused on testing certain behavior in the immediate component under test. I don't want to have to wrap things constantly in provider and have yet another object to deal with that's a black box like providers is. I send in fake stores to my components via a prop. It's not reliant on provider, and that's a huge plus to isolated testing.

@minznerjosh
Copy link

@dschinkel I hear you. Having worked with angular a lot before react, I missed the fact that testing experience was one of the cornerstones of the framework. I definitely got the impression that it wasn't the biggest priority in the react ecosystem. (Not complaining, there are always tradeoffs!)

Regarding hooks, that's still a pre-release feature. I don't remember where I read it, but I do remember reading that it won't become fully released until shallow rendering support is added.

And, with regards to the createContext() API, @ljharb and I (mostly him) have come up with an API in enzymejs/enzyme#1960 that will allow both mount()ed and shallow()ed components to be given the context they need to render. Obviously the PR is still open and in progress, but I'm confident that, even if that particular API doesn't make it in, another will.

I realize your frustrations are bigger than this particular issue, but support for these new features is coming to enzyme, and hopefully sooner rather than later.

@minznerjosh
Copy link

@markerikson FWIW, react actually does plug enzyme in their shallow render docs: https://reactjs.org/docs/shallow-renderer.html

@dschinkel
Copy link

dschinkel commented Jan 12, 2019

I'm sorry I sound like a jerk. It's just a big issue for use who have really come to enjoy TDD'ing React and Redux for all this to just change and prevent us from ultimately using shallow and writing isolated tests in very very simple ways. I guess I feel like this should have already been caught by both teams and I guess I have the feeling that with all the focus on integration tests i see with the React team, that they don't care. Prove me wrong, I'd love to be proven wrong on that last part.

To be fair, I promised mark a blog post but I just have not had the time.

@minznerjosh

FWIW, react actually does plug enzyme in their shallow render docs:

Right! Keep in mind a lot of us have used React for over 4 years now as well as Redux. In the beginning both were very much aware of the value of shallow testing early on. But feel they're losing that focus due to focus only on integration test support. And the lack of both not understanding these use cases already I guess bothers me to some degree. Just being honest.

And, with regards to the createContext() API, @ljharb and I (mostly him) have come up with an API in enzymejs/enzyme#1960 that will allow both mount()ed and shallow()ed components to be given the context they need to render. Obviously the PR is still open and in progress, but I'm confident that, even if that particular API doesn't make it in, another will.

And I appreciate it @minznerjosh

The fact is a lot of us are consultants. A lot of us practice TDD (or not) and quite a few of us write a lot of isolated tests and through React found awesome simple ways to do so with shallow. So if I go to a client, I want to be able to both take them where they're at AND show them a better way. If they're using mount all the time but we've found a great way to test via shallow and have always been able to, if a client is on React version x and Redux version y, I'd like to know that we can still count on at least React and Redux teams to try to keep the framework's API changing in a way that's easily tested using shallow and that we don't have to keep worrying about this going forward.

It would help if you could specifically address the questions I've asked, provide some concrete examples of tests that you've written, and offer up some actual potential solutions (beyond just "bring back store-as-prop")

Yea. Will when I find the time. Glad you started this thread. You're the only one who has really addressed it at this point.

@markerikson
Copy link
Contributor Author

markerikson commented Jan 12, 2019

@dschinkel : I know you have very strong opinions on these issues. I want to keep those out of this discussion as much as possible.

I'd like to focus specifically on technical aspects of the React-Redux API. Don't use the word "TDD". Don't complain about how React is changing, or how you feel like they "don't care" about the ecosystem.

Tell me what specific technical needs you have, and hopefully we can come up with some solutions that help everybody.

@dschinkel
Copy link

dschinkel commented Jan 12, 2019

TDD is part of context here and largely why I feel both teams are unaware of these use cases TBH.

Isolated tests is part of context independent of TDD but understanding use cases for either case, and that there are quite a few devs that test via shallow because of TDD's approach to design feedback and evolving a feature is why I mentioned it. I feel there's a lack of realizing that there is a pretty large group out there who write shallow tests because of that.

We can take out TDD from my rants above. Just replace it with "shallow testing". Simple.

@markerikson
Copy link
Contributor Author

I know, and I'm saying that phrase is irrelevant to the discussion. I don't care what conceptual approach you're using to write your tests. I want to know what technical problems you're encountering because of "store-as-prop" being removed, and the other changes in v6, besides the obvious literal "this breaking change broke my code" aspect.

@ljharb
Copy link

ljharb commented Jan 12, 2019

I hope for enzyme to have createContext support soon, but I’d urge you not to drop < 16.3 compat lightly. yes, enzyme is basically the officially endorsed testing solution at the moment, although that could always change if we don’t catch up to React’s feature set quickly enough :-)

I do also hope to have a general solution soon in enzyme that works ergonomically for provider-like components (both createContext Providers, and the incredibly common Provider component pattern, as well as components with lifecycle methods to set up state). Once that exists, it might not be as big of an issue for libraries to make this sort of change.

In other words, ofc do what you like with your library, but here are some thoughts i hope are helpful:

  1. enzyme will continue to strive to make it easy and ergonomic to use every React feature, and also/separately, every common community pattern, unrelated to my personal opinion on it (eg: i abhor render props as a bastardization of jsx, but enzyme now has a .renderProp API to make working with them easier, and supporting this workflow is a priority whether it’s my favorite or not) This includes the Provider-like support i alluded to above.
  2. it can be very user-hostile to aggressively drop support for older platforms (node/browser engines), frameworks (React versions), or patterns (props over context). It would be really pleasant for users of redux if they could continue to use up to date versions of the library without being forced to upgrade node, drop browsers from support, upgrade react, or rewrite their code. It’s also important for redux maintainers to have a pleasant experience maintaining the library, and these concerns often collide. Of course, make the decision you feel is best for your library, but hopefully also for your users :-)

If there’s something enzyme can do that isn’t already being worked on, please do file an issue and I’ll do my best to help find a good solution!

@markerikson
Copy link
Contributor Author

React-Redux v6 already dropped compat for <= 16.3. We have a minimum peer dependency of 16.4. 16.3 is the minimum for use of createContext, and 16.4 brought in the fixes for getDerivedStateFromProps. (We wound up not needing to use gDSFP ourselves, but if you're on 16.3, there's no reason not to be on 16.4.)

In general, if you need to stay on React <= 16.3, v5 still works just fine.

I will say that some of Sebastian's comments in facebook/react#14110 suggest that perhaps the new context API isn't as ideal as it has been described, particularly with the way React-Redux uses it. Having said that, we still had plenty of good reasons to switch to using it this way in v6, as I described in my blog post, and the removal of "store-as-prop" is a direct technical result of that change.

@markerikson
Copy link
Contributor Author

@dschinkel : I've got free time over the next couple days and can put some attention into this. What I need from you is several actual tests that run under v5, but not under v6, including components. I don't care how "real" the components are, I just need all the actual code for the tests to really run.

I'm particularly interested in how you are setting up the store, and what you are asserting.

Ideally, I'd love to have a link to an actual repo that I can just clone , install, and run the tests. Failing that, I'd settle for a complete tests file in a gist.

I do want to help, especially since there seem to be others dealing with this issue as well, but I need you to give me the resources to investigate so I can figure out options.

@dschinkel
Copy link

dschinkel commented Jan 12, 2019

Ok thanks Mark, I'll put something up by end of the day or early Sunday. Today I'm busy until the afternoon then I'll find some time tonight.

@markerikson
Copy link
Contributor Author

Playing around a bit with a sandbox provided by @BTMPL .

Unfortunately, as best as I can tell, shallow() simply does not call a Context.Consumer's render callback. Dan pointed out that React's shallow renderer is incredibly simplistic. It seems like the first blocking issue here is that the shallow renderer at least needs to have some specific support for handling context consumers.

@minznerjosh
Copy link

@markerikson I opened enzymejs/enzyme#1966 to handle that.

@minznerjosh
Copy link

Though perhaps support should come from react-test-renderer itself in some capacity?

@markerikson
Copy link
Contributor Author

How much of the issue is Enzyme, and how much is the underlying React shallow renderer?

@minznerjosh
Copy link

It’s sometimes hard to tell. Enzyme does a lot to make shallow rendering behave more like actual react. (Like implementing shouldComponentUpdate, for example.)

The way I’d like to see context supported in enzyme is that you can render a context provider, and then if you dive() the consumer, the render prop will be called with the correct value. The underlying react shallow renderer has no concept of diving, so support for that would have to come from enzyme.

However, I could also imagine a world where the underlying react shallow renderer allows you to render a consumer, and pass in the context value as an option, and it will call the render prop. The aforementioned enzyme feature could be built on top of a shallow renderer feature like this. But it isn’t necessary, as my PR demonstrates.

@timdorr
Copy link
Member

timdorr commented Jan 12, 2019

Potentially dumb idea: what if we added an unsafe_store prop to connect? Would it be sufficient for that to only read the state from the store once, or are folks expecting it to subscribe as well? That's what I would expect with shallow rendering, but I don't know all of the edge cases.

@markerikson
Copy link
Contributor Author

That's roughly where my thoughts were headed - either that, or something like a overrideMockStoreState prop. Something that clearly is not intended for use in an actual app, but would hopefully be sufficient for a test.

@dschinkel
Copy link

dschinkel commented Jan 12, 2019

This is why giving you an example takes more than just code, it takes context even explaining how that ties into TDD so it's quite a bit of work on my end that I am willing to do tonight. I was spent and didn't want to but I will.

When I say context meaning testing approach. When I test drive my code, I do not initially care about testing lifecycle methods or "wiring", to me those are integration tests.

When enzyme first started out, shallow as far as I know didn't even support lifecycle methods. If it did I didn't care because I don't test drive starting with testing that way. As developers who wrote more integration tests with shallow, they started to demand shallow to behave more like mount() which to me is borderline a bad decision by enzyme because shallow to me is meant to test the rendered output, not to do full out integration testing like mount is meant for. If you're designing components, you should strive to decouple logic from your containers anyway and test those independent of your container.

So what I'm saying is, the focus I personally care about is:
a) My existing tests that I wrote for the past 3 years don't break. I've worked for multiple clients.
b) The approach I take to TDD (this matters I'm not the only one who does this) React goes like this:

  • Shallow render a container

  • If that container is wrapped in an HOC, dive() past each HOC because I care about testing the underlying container's output

  • Test redux behavior indirectly. By indirectly this means I send in a fake store which is simply an anonymous method that is a dummy. I then do not export stuff like mapStateToProps. I test that after my reducer is hit (lower level tests also test reducers, actions, and other business logic), that state updated the props with expected stub data. That way if someone screws up mapStateToProps my tests will tell me. This does not mean I'm testing redux itself, a common misconception that people tend to resort to who don't undrestand testing well just assume when they first glance at example isolated tests. I'll explain that in my example repo I'll setup, there's more to this.

  • Once I'm doing TDD'ing the underlying core behavior (not wiring..I don't care about wiring till after the main business logic has been TDD'd for a feature), I actually don't write a lot of integration tests. Integration tests is a loaded term. When I say integration tests I mean that I don't test a lot of "wiring" such as "hey did we wire up this button to this function". You can but I value lower level isolated tests first which is the main issue at stake here

  • I don't care about renderProps personally. Would I ever use them? I have but they tend to actually cause a lot of design smells. So...we need to consider that many of us don't use it or every new shiny thing that React comes up with. Not everyone follows the bandwagon because some of us see drawbacks when you use a lot of nested things together, especially when people nest a bunch of HOCs and renderProps together and get components that break Single Responsibility so badly, you can't even shallow anymore due to poor design. My point is, I don't really care about renderProps personally here

  • My code over the past years and even into the future may or may not use the context API with React. Either way Redux and React need to be flexible and allow us to inject context or a store

Because the TDD approach is entirely different then test after, this matters to me and fellow TDD'ists, and gives a big part of context here as in "why it's a problem for a lot of devs who TDD" or even "A lot of devs who don't TDD and also don't care about testing the wiring or integration tests first". I do not follow Dodds recommendation "Mostly integration tests" so keep that in mind as we work through this together.

Stay tuned I'll hit the example repo creation tonight and with more context and details around the approach.

@markerikson
Copy link
Contributor Author

I really don't care about the justifications for why you write tests a certain way.

I just want to see how you are writing the tests.

@dschinkel
Copy link

dschinkel commented Jan 13, 2019

Ugh Mark I get that. And I'm going to give that to you. I'm just responding more and giving context because I see people talking about lifecycle methods, etc.

But you also can't just ignore what I'm trying to explain about context. Testing isn't about just adding tests to some of us. It's part of an approach. Context goes with testing and for use cases you're going to have to listen if you want to work together on this. Stop hating TDD and understand that TDD means there's a lot of context in terms of the approach to using shallow that goes with just showing examples. I think that most devs don't know that this context exists which is precisely why much of these use cases might have been ignored or not even been aware of when React and/or Redux starts pushing out grand new changes to their API.

Part of the issue IMO with the JS community is exactly that, they immediately don't want to hear about TDD. I'm sorry but it's part of my approach and you need to understand it as part of understanding why and how I write isolated tests with shallow.

@markerikson
Copy link
Contributor Author

Honestly, no, I don't need to know why right now.

I'm not hating on TDD. I'm saying you're spending a lot of time writing long comments about things that don't give me the info I need to help address this issue.

@dschinkel
Copy link

dschinkel commented Jan 13, 2019

If you end up doing a new prop, I suggest keeping the name simple. overrideMockStoreState is too verbose. We know that it's dealing with state and mocking naturally override real implementations by nature which is implied by the term.

Just name it mockStore or fakeStore. So examples: mockStore={fakeStore()}, mockStore={dummyStore()} or fakeStore={mockStore()}, fakeStore={dummyStore()}.

The new prop would need to of course to work in the example code I'll share and that code is not using the context api or renderProps.

I worked on the example repo a little tonight, I'll have it done tomorrow morning / early afternoon.

@dschinkel
Copy link

It'll have to be by tomorrow, got a couple things I want to do before I post this.

@davesteinberg
Copy link

@markerikson Thanks for taking up my comment on your blog post and opening this issue. Here's a really simple repo that shows how I write unit tests for connected components, using enzyme's shallow and a mock store passed as a prop:

https://github.com/davesteinberg/react-redux-counter

In particular, you'll see these tests in the second describe block of Counter.test.js. Effectively, the first test validates mapStateToProps and the latter two validate mapDispatchToProps, though by actually unit testing the API of the resulting component. This is the simplest and cleanest way that I've found to do this type of testing, and so I've used it pretty extensively.

You can break them by updating to react-redux 6.0.0. The way I've fixed them so far is by wrapping the component under test in a Provider and switching from shallow() to mount(), e.g.:

const wrapper = mount(
  <Provider store={store}>
    <ConnectedCounter />
  </Provider>
);

Not hugely different here, but it means I have to worry about the whole render tree under the connected component, which can complicate tests in real projects.

@dschinkel
Copy link

dschinkel commented Jan 14, 2019

I test my mapStateToProps in a similar way, by setting a slice of the reducer to some stub data via a mock store OR I inject a mock API helper function (that returns stub data) that my action creator calls vs. the real API helper method, then assert the output of my container...and use shallow the entire time without a provider. I'll have mine up soon.

The way I've fixed them so far is by wrapping the component under test in a Provider and switching from shallow() to mount()

That's exactly what I don't want to do :)..use mount for everything past and going forward, and have to refactor and redesign existing tests already using shallow and sending in a store.

This thread is becoming kinda fun seeing everyone's way of testing.

@AdrienLemaire
Copy link

Enzyme has started working on the ability to dive() consumers 10 days ago. Our react-redux v6 upgrade will be completed once they merge this feature and we can update all our tests with store sent as context instead of props

@ljharb
Copy link

ljharb commented Feb 14, 2019

@jamesg1 i'm suggesting that you shouldn't want to test something in isolation that's never possible to use in production in isolation. If you decide to move away from redux, you'd have to make lots of changes, so it's unreasonable to expect that tests for components utilizing it won't also require changes.

@venikx
Copy link

venikx commented Feb 14, 2019

That’s why I think components should do one thing and one thing great. Components fetching data/managing state shouldn’t care rendering elements.
So you’d have your state wrapping component, which gets it’s values from local state, hooks, redux whatever. Exporting that component without the way it gets data is useless, because the only thing it’s responsible for it gathering the data. Exporting unconnected components kinda feel the same, just doesn’t makes sense.

But that’s not at all relevant to the discussion I think?

@markerikson
Copy link
Contributor Author

Nope, not at all.

The point of the original issue is:

  • We had an API in version <=5 (store as prop) that folks relied on for two specific use cases (testing and isolating components in a larger tree)
  • We offered a suggested approach for the "isolated component" use case, but there were testing-related usages we weren't aware would become problematic
  • Figuring out what approach, if any, would resolve those concerns

Per my earlier comment, this will likely be resolved by an internal architectural change that will allow us to re-add the store as prop API, and thus solve those use cases.

Discussion of why some people prefer to test one way or another, or what the "right" approach is for testing things, is irrelevant to this issue.

@NunoCardoso
Copy link

Nope, not at all.

The point of the original issue is:

  • We had an API in version <=5 (store as prop) that folks relied on for two specific use cases (testing and isolating components in a larger tree)
  • We offered a suggested approach for the "isolated component" use case, but there were testing-related usages we weren't aware would become problematic
  • Figuring out what approach, if any, would resolve those concerns

Per my earlier comment, this will likely be resolved by an internal architectural change that will allow us to re-add the store as prop API, and thus solve those use cases.

Discussion of why some people prefer to test one way or another, or what the "right" approach is for testing things, is irrelevant to this issue.

Thank you for this clear comment. I was puzzled on how all the examples of store mocking with jest I found around were not working properly. Downgraded to react-redux 5.1.1 and all works fine.

I hope anyone who is looking for the same answers will find your post. I am a little skeptic over adding a functionality on prod code only for the sake of testing, but in this case, I think the pros beat the cons.

@hjhimanshu01
Copy link

hjhimanshu01 commented Mar 16, 2019

@markerikson , any updates on how to pass store to components while using shallow for testing ?

Sample issue :

wrapper = shallow(
      <Provider store={store}>
      <Component />
      </Provider>
    );

store configured via 'redux-mock-store'.

@markerikson
Copy link
Contributor Author

@hjhimanshu01 : this should be fixed in the v7 alpha. Try adding it as an alias:

yarn add react-redux@npm:@acemarke/react-redux@next

Also note that you need react@^16.8.2 for this.

@hjhimanshu01
Copy link

@markerikson , I'm sorry , I didn't got what you explained , please could you elaborate a little?

@dschinkel
Copy link

dschinkel commented Mar 17, 2019

@dschinkel may I suggest using named exports to expose connected components to mitigate testing connected components, below is an example pattern.

@ezrasingh to answer your question (which I still think is relevant to this thread personally and valuable to have in here nonetheless), I'm in agreement with @ljharb.

Let me try to provide you a little more detail. By exporting it, you're not testing the redux wiring indirectly anymore, nor are you testing it in concert with the lower level behavior that directly impacts that container, and testing that together matters. I test redux functionality like mapStateToProps / mapDispatchToProps (the custom prop wiring within mapStateToProps or mapDispatchToProps where I specify the behavior/actions to call) that live in my containers in an indirect way and it matters that my internals all the way from my container down for redux work without exporting those internals or being coupled to them. If I do not test connected, then I'm risking that my component still does not work, and my lower level tests not telling me why when my component hits production and problems are found after the fact. By testing through the connected component indirectly (not coupled to redux), I guarantee my output is what I expected through the redux API.

I distinguish these as what are called "Isolated Component" tests as opposed to what I label "Integration Tests" which can be anything from a mounted test that test children and entire tree of the parent under test, testing through forms via button clicks and checkin handler wiring, cypress tests, lifecycle method tests, and other higher level UI tests. Both component and integration tests can be headless so it depends on where you are drawing the line and seams in your application whether I call it a component vs integration personally. I tend not to test through button clicks, inputting stuff into forms, those are integration tests to me. I do those after I write isolated tests lower down.

The urge to export a component from a connected container or a component that's heavily nested in other things just to test it is an indicator of lack of understanding how how to test well (a lack of practice which TDD'ing for many years has helped me understand how to test well for example)...it's a hack, as well as an indicator of code/design smell. It's usually you've neglected good production/component design which is why people resort to trying to export the bare component because they have not other options when design is not simple. And to test a container (and that your reduxy or non-reduxy code below a container works), you don't have to export your bare component.

And the way I test is I test connected containers as black boxes. I shouldn't "have" to export anything. I only allow myself to dive once (most the time I don't have to dive at all), but if I find myself having to dive even once or more than once, I question my design. It's usually an indicator that my prod design is not simple enough; the result of having many collaborators (breaking SRP) OR the way in which I'm trying to inject stuff, or that I'm testing at the wrong level and should move some business logic out of my container and test that logic outside the container in isolation. Often it's a combo of all that that impacts my tests. And I see often this as I design out a feature, not after the fact because I'm getting that feedback from my Test Driven Design (TDD) cycle and fix those issues often during the small refactoring I due during the blue step in TDD. I don't end up in such a mess because that cycle won't let me.

And I see a lot of teams doing this which trying is trying to design everything up front frantically rushing to hit a deadline, then attempting to test after or never because they find components or methods that are overly complicated (results in design that is not broken apart, not simple).

If I find myself saying "I need to export this component because it's wrapped by too many collaborators and I can't get to the part I wanna test in my container or I have to mock a ton, or enzyme dive a ton to get to that"; ....well that's honestly an indication my design sucks and my component knows too much (most likely breaking Single Responsibility) which means I probably need to break my container out into smaller containers OR decouple logic away from my component in other ways besides the famous renderProps or nested HOCs. And resorting to only/mostly Integration tests are not a "fix/hack" for bad design / code smells.

How about not using renderProps, not using so many nested HOCs, try doing that some other way instead..inject via regular props!!! yay! I know people think that's a pain but honestly all this shit we do to try to get around that has to me made React a mess and caused React not to be simple anymore). If you inject via regular props and not wrap and end up with nested levels of shit, you can easily decouple and break stuff apart. Trust me, React is not better for us dodging just sending things in like state and use case logic via props. React was just fine like it was before all this magic came into play. The key here is to break stuff apart, keep things SMALL. Then things become much easier and you find you don't need all these crazy patterns.

There's something Uncle Bob Martin, Fowler, many more keep trying to get developers to understand which is to continually and often break things down (functions, modules, etc) to as small as they can possible go, then you don't have to worry about the complexity of testing these things or managing them and you have more flexibility and levels/seams at which you can decide to test at. Kent Beck also says this in his 4 Rules of Simple Design. React components and containers should be SUPER small. If not, you're not breaking them down enough, and the way you might be injecting might not be the best way.

My tests at the container for the most part already don't know anything about redux or connect. The way I test is I send in a mock or dummy store which are two kinds of test doubles depending on how I'm testing, (not using a mocking framework) and I test the output of the render and that's it. I don't test internals. My very outside test, again when I TDD does this and it only passes if my lower-level tests pass and lower level constructs are implemented correctly (my container test gives me feedback as well as related lower level tests related to the code my container calls or uses). I only have a very few outside tests at the container via shallow and pass in a store as a prop. They're not that aware of redux. Most of my other isolated tests live lower, around use case logic (user action logic, helpers, etc independent of framework stuff if I can help it). I even abstract out lifecycle logic from containers and test that logic in isolation as much as possible.

Thank you for this clear comment. I was puzzled on how all the examples of store mocking with jest

And this is why I don't use/need heavy mocking frameworks :). They introduce pain of having to figure out how to use them, too much magic, unnecessary setup, etc. I don't need a mocking framework to mock the store...not even redux-mock-store...totally unnecessary. I just pass dummies to plug holes for the store as a prop. Hence why this still needs to work in v7 and hence is my style of testing and does matter.

As to this:

wrapper = shallow(
      <Provider store={store}>
      <Component />
      </Provider>
    );

@hjhimanshu01 why would you wanna do that? the whole point of shallow is you don't need a provider because the point is you're testing one component in the tree. Provider is used when you mount and want to test the entire tree together and pass state to all components in the tree and test together, which I personally label an integration test.

@hjhimanshu01
Just wondered why you've relied on redux-mock-store, can you explain what that gives you? I haven't found a use case where redux-mock-store gave me anything I needed outside simply creating my own custom mocks which are very simple to make.

@markerikson
I must have missed this, where was this?

We offered a suggested approach for the "isolated component" use case, but there were testing-related usages we weren't aware would become problematic

I've been wrapped up in a new job, so trying to resurface here.

@venikx
Copy link

venikx commented Mar 17, 2019

@dschinkel
I agree with having to use elaborate mocking framework is a sign, mock should be straightforward and used in very particular cases.

I don’t agree with the statements that when you test your containers, that they are not coupled to redux. They are 100% coupled to redux, that’s just nature of using connect.

I also don’t agree that your container tests don’t know anything about redux in test, because they are aware, since you need to pass down a store prop. Do implicitly you have a dependency on Redux.

But again, I think this is just the nature of the redux connected components/tests. They are coupled and it’s ok they are aware, since it’s small integration test.

I see containers as entry points just like when using express in node. These entry points call other functions who are a bit easier to test in isolation.

@markerikson
Copy link
Contributor Author

sigh

Folks, as I already said: please refrain from continuing discussions about various testing methodologies in this thread, or I'm going to have to lock it.

The version 7 alpha re-adds the ability to pass store as a prop to connected components. Please try that out and see if it resolves your issues. The latest version is @acemarke/react-redux@7.0.0-alpha.5, which is tagged as next.

The easiest way to try it is to add it as an alias for react-redux when installing it:

yarn add react-redux@npm:@acemarke/react-redux@next

Note that since v7 uses hooks internally, you'll need to have at least React 16.8.2, and you'll probably need to use React's new act() test API to wrap any bits of your test that might update the components.

@hjhimanshu01
Copy link

@dschinkel , actually , that code snippet was written by someone else .

Here's the snippet

const container = wrapper.dive({ context: { store } }).dive();

    expect(
      container.find('AnotherComponent').first().props().propValue
    ).to.eql(expectedPropValue);

I do think it is integration testing usedd here , but can't say exactly.

@markerikson
Copy link
Contributor Author

This should be resolved now that v7 is out and you can pass store as a prop again.

@shiraze
Copy link

shiraze commented May 3, 2019

I'm still seeing this issue in v7.
I'm using React 16.8.6, Enzyme 3.8.0.
Issue didn't exist with react-redux 5.1.0, but updating to 7.0.3 causes unit tests that rely on store prop to now fail.

Example unit test that fails:

import React from "react";

import MainPage from "./mainPage";

import Enzyme from "enzyme";
import Adapter from "enzyme-adapter-react-16";
import { createShallow } from "@material-ui/core/test-utils";

import configureMockStore from "redux-mock-store";
import thunk from "redux-thunk";
const middlewares = [thunk];
const mockStore = configureMockStore(middlewares);

Enzyme.configure({ adapter: new Adapter() });

jest.mock("./dataFormContainer", () => "MockedDataFormContainer");
jest.mock("./resultsList", () => "MockedResultsList");
jest.mock("../style/mainPage.css", () => ({
  withStyles: component => component,
}));

describe("MainPage renders correct subcomponent", () => {
  let wrapper, store, shallow;

  beforeEach(() => {
    shallow = createShallow({ dive: true });
  });

  it("renders a DataFormContainer when activeForm is set", () => {
    store = mockStore({
      mainPage: {
        activeForm: {
          form: "myform",
          desc: "form title",
        },
        connection: "href/entity",
        query: "searchItem",
      },
      logon: {},
    });

    wrapper = shallow(<MainPage store={store} />).shallow();

    expect(wrapper.find("MockedDataFormContainer")).toHaveLength(1);
    expect(wrapper.find("MockedResultsList")).toHaveLength(0);

    expect(wrapper.find("MockedDataFormContainer").prop("title")).toBe(
      "form title"
    );
  });
});

I get this error thrown when attempting to run test:

    TypeError: ShallowWrapper::dive() can only be called on components

      40 |     });
      41 |
    > 42 |     wrapper = shallow(<MainPage store={store} />).shallow();
         |               ^
      43 |
      44 |     expect(wrapper.find("MockedDataFormContainer")).toHaveLength(1);
      45 |     expect(wrapper.find("MockedResultsList")).toHaveLength(0);

      at ShallowWrapper.<anonymous> (node_modules/enzyme/build/ShallowWrapper.js:2057:19)
      at ShallowWrapper.single (node_modules/enzyme/build/ShallowWrapper.js:1960:25)
      at ShallowWrapper.dive (node_modules/enzyme/build/ShallowWrapper.js:2051:21)
      at shallowWithContext (node_modules/@material-ui/core/test-utils/createShallow.js:37:22)`

I'm using an enhanced shallow provided by the Material-UI team (https://material-ui.com/guides/testing/#createshallow-options-shallow), but in effect I'm just doing:
wrapper = shallow(<MainPage store={store} />).dive(); if I were to use shallow from enzyme.

The problem appears to be that the result of shallow() is now <ConnectFunction store={{...}} classes={{...}} /> whereas before it used to <Connect(MainPage) store={{...}} classes={{...}} />

@markerikson
Copy link
Contributor Author

@shiraze : two possible guesses for what's happening:

  • connect now uses React.memo() to wrap our own component, so there's technically two levels of wrapping going on. That may mean you might have to dive() twice or something
  • it's also possible that Enzyme doesn't know how to deal well with React.memo()-wrapped components

@ljharb
Copy link

ljharb commented May 3, 2019

This should improve in the next release of enzyme.

@shiraze
Copy link

shiraze commented May 3, 2019

Thanks @markerikson and @ljharb . I'm actually thinking of changing our coding style so all our components are tested independently of HOCs like connect(), as described here: https://redux.js.org/recipes/writing-tests#connected-components
I'll then separately test their wrapped behaviour by using <Provider>

This should make tests easier, and also lead to component development decoupled from HOCs

jcoyne added a commit to LD4P/sinopia_editor that referenced this issue May 30, 2019
… tested

One option for testing mapStateToProps directly would have required exporting private code as public, just for the purpose of testing.  That's a bit of a code smell.

Another option would be to pass a mock store into the component.  That was something we could do in react-redux v5 and which was restored in v7, but it was removed in v6, which is what we are on. See reduxjs/react-redux#1161

This seems like the best way of testing this mapping without upgrading react-redux or unnecessarily breaking encapsulation.
jcoyne added a commit to LD4P/sinopia_editor that referenced this issue May 30, 2019
… tested

One option for testing mapStateToProps directly would have required exporting private code as public, just for the purpose of testing.  That's a bit of a code smell.

Another option would be to pass a mock store into the component.  That was something we could do in react-redux v5 and which was restored in v7, but it was removed in v6, which is what we are on. See reduxjs/react-redux#1161

This seems like the best way of testing this mapping without upgrading react-redux or unnecessarily breaking encapsulation.
@bruno-edo
Copy link

@ljharb Currently I'm using Enzyme's v 3.10.0 and am experiencing the issue @shiraze described. Any updates on when Enzyme will be able to shallow render components connected to Redux v7?

@timdorr
Copy link
Member

timdorr commented Jun 11, 2019

It may be dependant on enzymejs/enzyme#2011

@ljharb
Copy link

ljharb commented Jun 11, 2019

@bruno-edo please file an issue on enzyme itself for that; it should work with latest enzyme, the latest react 16 adapter, and the latest react-test-renderer.

@bruno-edo
Copy link

@ljharb I was using both the latest enzyme and adapter versions, but I was unaware that react-test-renderer was also required. Would you perhaps be able to post a link with an example on the correct way to use Enzyme to shallow render a connected component (I want to avoid posting an unnecessary issue report)?

Also, the project I'm trying to use Enzyme on is a React Native project, so maybe that is part of the issue?

@ljharb
Copy link

ljharb commented Jun 12, 2019

@bruno-edo it's a direct dependency of the react 16 adapter, but if you have a lockfile you'll need to ensure that's updated also to match your react version.

Certainly there'll be issues with RN, since the current adapters are for react-dom, not react-native. Track enzymejs/enzyme#1436 for that.

Let's please, though, all move enzyme discussion to the enzyme repo and stop bothering the redux maintainers with off topic discussion :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests