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

Improve shallow rendering for use with higher-order components #539

Open
lencioni opened this issue Aug 12, 2016 · 39 comments
Open

Improve shallow rendering for use with higher-order components #539

lencioni opened this issue Aug 12, 2016 · 39 comments

Comments

@lencioni
Copy link
Contributor

When using shallow rendering with a component that is wrapped by a higher order component, you need to take some extra steps to make shallow rendering give you the structure you expect. One way is to shallow render the wrapped component, find the unwrapped component within this structure, and then shallow render that. e.g.

const wrapper = shallow(<Foo />).find('Foo').shallow();

Of course, you can also export the unwrapped component as a named export, and use that to find by reference as well.

import Foo, { Foo as UnwrappedFoo } from './Foo';

...

const wrapper = shallow(<Foo />).find(UnwrappedFoo).shallow();

This bums me out a little, because if you have a bunch of tests for a component, and then later decide to wrap that component in a HOC, you need to go back and update all of your tests in this way. Using the string approach feels less than ideal, and the reference approach feels a bit cumbersome when applied on a large scale. I'm wondering if we can come up with a solution that will work well for this scenario. Here are some thoughts.

  • In Mixed-depth shallow rendering mode #250, @lelandrichardson proposed "Mixed-depth shallow rendering mode", which would allow you to pass an expand option to shallow with references to components to "expand" the shallow render tree into. I think this would work if your component is only wrapped by a single HOC, but if you wrap it in multiple HOCs, you end up with the same problem again, because you have multiple levels that you need to expand, but only a reference to the top level.
  • We could add an until option to shallow that would take a reference to a component or a string reference. This would reduce some of the boilerplate of .find('Foo').shallow() just a little, but I think it has many of the same disadvantages that we currently have.
  • Another possibility is to provide a depth option to shallow, that would take a number of components to drill down into. I think that this would be more comfortable than what we currently do, but it still doesn't fully solve the problem--you still have to know to add this option to every place you want to use shallow rendering with wrapped components. And, if you wrap a component in another HOC, you need to increment all of the values.
  • We could add a static property on HOCs that signify that they are HOCs. Enzyme could then look for this static property when shallow rendering, and dig deeper. And, if you find yourself in a situation where you want to avoid this behavior, we could add an option to shallow to opt-out, but maybe that's YAGNI. The downside is that this would only work for HOCs that opt in to this behavior, so it likely wouldn't solve the problem when using third-party HOCs--unless it somehow caught on and spread to all corners.
  • What if we allowed people to register a list of regexes used to expand components when shallow rendering, matching on their names. Using this, folks could configure their testing environment to do what they want, and then not have to think about it much any more. At Airbnb, we use a naming convention of functionName(ComponentName) for HOCs which we could configure all be automatically expanded with a regex like /\([^\)]+\)/. I like this option because I think it would solve this issue for us with little configuration, but I dislike it because it makes shallow rendering somewhat magical and might cause unexpected results.

Do any of these sound good? Should we employ multiple strategies? Are there other possibilities?

@nfcampos
Copy link
Collaborator

You can also do

const wrapper = shallow(<Foo />).first().shallow();

I definitely feel this pain, but I'm not sure what the right solution is. But I do lean more towards defining the depth

@lencioni
Copy link
Contributor Author

That's true, but I think you have to tag on another .first().shallow() for each HOC.

@nfcampos
Copy link
Collaborator

yes if you have 3 HOCs it becomes the rather terrible

const wrapper = shallow(<Foo />).first().shallow().first().shallow().first().shallow();

@ljharb
Copy link
Member

ljharb commented Aug 12, 2016

The only option I like there is #250. Even with multiple HOCs, I think that would work transparently. You'd still always need to export the pure version, but I think that's just an inherent limitation in HOCs that enzyme can't gracefully overcome.

@lencioni
Copy link
Contributor Author

lencioni commented Aug 12, 2016

If I understand that proposal correctly, you would need to export each level of HOC that you wrap your component in, and then pass those all as references to the expand option. I think I would rather use .find('Foo').shallow().

Edit: I think the find approach might have a similar issue actually...

@aweary
Copy link
Collaborator

aweary commented Aug 12, 2016

Ideally, I think it would be nice to have an API would be able to find a component at any level in the potential render tree for a component, even if it was too deep to be rendered with a shallow render.

var deepWrapper = wrapper.shallowFromDeepChild('SomeDeepComponent');

Though I think that would be difficult to implement. I like #250 but if you have to export every level of HOC then it doesn't seem to bring any advantage over just testing the exported component directly. It's definitely a hard problem.

@lencioni
Copy link
Contributor Author

@aweary I think your suggestion is similar to my thought about adding an until option, which would keep going until it rendered something that matched the selector.

@CurtisHumphrey
Copy link
Contributor

CurtisHumphrey commented Aug 15, 2016

I suggest instead of a list to expand have a list of do not expand. For me and our tests, most of the time we use shallow because we have components inside of it we do not want to expand (big things maybe directly connect to redux etc). We know this list as we build the widget and tests. Then if we later wrap it with HOC that list doesn't change and nothing in the tests have to change, problem solved.

@ljharb
Copy link
Member

ljharb commented Aug 15, 2016

Ideally we'd want both options I think: "only expand through these" or "expand unless it's one of these".

@ljharb
Copy link
Member

ljharb commented Aug 22, 2016

re #539 (comment) - specifically, as a separate API from shallow - and I don't think we should allow any sort of "depth" number, since that tightly couples tests to JSX structure.

@CurtisHumphrey
Copy link
Contributor

CurtisHumphrey commented Sep 7, 2016

I second @ljharb on both points: both options and no depth number

@oliviertassinari
Copy link

oliviertassinari commented Nov 10, 2016

For anyone looking for a temporary solution, we are using an until operator at @doctolib as we use quite some HoC:

cc @matthieuprat for the credit.

enzyme/operator/until.js

export default function until(selector, {context} = this.options) {
  if (this.isEmptyRender() || typeof this.node.type === 'string')
    return this;

  const instance = this.instance();
  if (instance.getChildContext) {
    context = {
      ...context,
      ...instance.getChildContext(),
    };
  }

  return this.is(selector)
    ? this.shallow({context})
    : this.shallow({context})::until(selector, {context});
}

That we use it like this:

const AppBanner = ({banner, children}) => {
  return (
    <Banner>
      {children}
    </Banner>
  );
};

export default compose(
  pure,
  connect(({
    globalMessagesBanner$,
  }) => ({
    banner: globalMessagesBanner$.startWith('key: common.error'),
  })),
)(AppBanner);
const wrapper = shallow(<AppBanner />, {context});

assert.strictEqual(
  wrapper.until('[banner]').find(Banner).props().children,
  'key: common.error'
);

@matthieuprat
Copy link

Here is a Gist with a bunch of tests for this until operator to make it more "graspable": https://gist.github.com/matthieuprat/5fd37abbd4a4002e6cfe0c73ae54cda8

@elodszopos
Copy link

Here's a little helper that I use. Works for multiple nesting too (multiple decorators / HoC wrapping).

export function renderComponent(Component, defaultProps = {}) {
  return function innerRender(props = {}, nestLevel) {
    let rendered = shallow(<Component {...defaultProps} {...props} />);

    if (nestLevel) {
      let repeat = nestLevel;

      while (repeat--) {
        rendered = rendered.first().shallow();
      }
    }

    return rendered;
  };
}

And then:

const renderer = renderComponent(Foo); // defined in describe
const wrapper = renderer({ someProp: someValue }, 1); // @connected Foo, to be used in an it block

It's a temporary solution really. I'd like to fully support this effort and if there's anything I can help with, I will.

I'm also all for what @ljharb said in his comment above. The temporary solution that I have tightly couples to JSX and is volatile. Far from ideal.

@kumar303
Copy link

kumar303 commented Aug 10, 2017

EDIT: Here is a more detailed explanation of this approach: https://hacks.mozilla.org/2018/04/testing-strategies-for-react-and-redux/

This is a helper that is working for me so far (here is the current code and tests). It's nice because you can wrap as many HOCs around your component as you need to without having to update any test code. It's pretty similar to @oliviertassinari's approach but takes a component class instead of a selector.

import { oneLine } from 'common-tags';
import { shallow } from 'enzyme';

/*
 * Repeatedly render a component tree using enzyme.shallow() until
 * finding and rendering TargetComponent.
 *
 * The `componentInstance` parameter is a React component instance.
 * Example: <MyComponent {...props} />
 *
 * The `TargetComponent` parameter is the React class (or function) that
 * you want to retrieve from the component tree.
 */
export function shallowUntilTarget(componentInstance, TargetComponent, {
  maxTries = 10,
  shallowOptions,
  _shallow = shallow,
} = {}) {
  if (!componentInstance) {
    throw new Error('componentInstance parameter is required');
  }
  if (!TargetComponent) {
    throw new Error('TargetComponent parameter is required');
  }

  let root = _shallow(componentInstance, shallowOptions);

  if (typeof root.type() === 'string') {
    // If type() is a string then it's a DOM Node.
    // If it were wrapped, it would be a React component.
    throw new Error(
      'Cannot unwrap this component because it is not wrapped');
  }

  for (let tries = 1; tries <= maxTries; tries++) {
    if (root.is(TargetComponent)) {
      // Now that we found the target component, render it.
      return root.shallow(shallowOptions);
    }
    // Unwrap the next component in the hierarchy.
    root = root.dive();
  }

  throw new Error(oneLine`Could not find ${TargetComponent} in rendered
    instance: ${componentInstance}; gave up after ${maxTries} tries`
  );
}

Here is an example of how I use it. In SomeComponent.js:

import React from 'react';
import { compose } from 'redux';
import { connect } from 'react-redux';

export function SomeComponentBase() {
  return <div>This is the unwrapped component that we want to test</div>;
}

// Pretend that this is a typical Redux mapper.
const mapStateToProps = (state) => ({});

export default compose(
  connect(mapStateToProps),
)(SomeComponentBase);

Example of rendering it for a test:

import SomeComponent, { SomeComponentBase } from './SomeComponent';
import { shallowUntilTarget } from './utils';

// This unwraps SomeComponent until it finds SomeComponentBase:
const root = shallowUntilTarget(<SomeComponent />, SomeComponentBase);

@ljharb
Copy link
Member

ljharb commented Sep 26, 2017

Linking to #250.

@seanconnollydev
Copy link

I wasn't able to get the "shallow until render" approaches to work, so went with an approach that mocks the specific component I want to avoid rendering. Jest mocks are a little quirky, but they did the job. Especially helpful when testing Apollo's Query/graphql components.

https://medium.com/@sconnolly17/testing-graphql-container-components-with-react-apollo-and-jest-9706a00b4aeb

@dschinkel
Copy link

dschinkel commented Nov 29, 2018

My opinion on this:

This bums me out a little, because if you have a bunch of tests for a component, and then later decide to wrap that component in a HOC, you need to go back and update all of your tests in this way

Honestly you've changed your design, your Component Under test is now doing more. It's no longer as simple. So you should expect your tests to break in this case. It's par for the course. When you change your design by introducing an HOC, there's no magic that's going to allow your tests from breaking. Your tests are designed to give you feedback. This is not too much fragility as it is you've changed your contract in a big way inside that component. So it's totally expected if you introduce an HOC like that.

When I TDD I get that kind of feedback early. And it forces me to think a little more about my design up front in baby steps as I go. I expect this kind of feedback from shallow.

Instead of looking for a solution that prevents your tests from breaking and more magic to get around it, think more about how your'e designing your components and when it's natural to expect your tests to break when you completely redesign the contract like that. There's no such thing as "all fragile or all not". You can have a mix of fragility, with expected contracts breaking your tests, both are different cases...so know when changing your design, you're actually getting the feedback you should be getting. If you test after well, it's kinda too late, now you have to try to go and break apart your components to try and make them simpler. That's exactly why I TDD. I don't end up in that situation that much...because I move things around and break things apart early.

I know this is a late reply, but I'm just looking through old issues in here. Thought to comment.

@kumar303
Copy link

Honestly you've changed your design, your Component Under test is now doing more.

Yes but the new HOC is just an implementation detail. The interface of your component did not change so the unit tests for it should not have to change.

@dschinkel
Copy link

dschinkel commented Nov 29, 2018

Yea I just don't think it's a big deal in those cases honestly personally plus you can use a recursive dive() helper method to solve this. But regardless I think if you're doing dive() lot, you have to wonder if maybe you're also actually over using HOCs (yes you can over use it, there are other ways to decouple) and I'm glad hooks are coming out with React as well now. Adding HOCs is actually making your component do more, and more complex. There are ways to break things apart and decouple without having to nest a bunch of HOCs or continuously keep adding HOCs often IMO. I agree with FB that HOCs and render props are really kind of a bad design pattern which is what they admitted in their hooks docs. HOCs are a form of injection but you end up making your component more complex, instead of adhereing to the 4 rules of simple design. It's a design feedback actually IMO with dive/shallow.

you especially don't want to end up with a Christmas tree:
render_props-hoc-christmas-tree

@dschinkel
Copy link

dschinkel commented Dec 5, 2018

We could add an until option to shallow that would take a reference to a component or a string reference. This would reduce some of the boilerplate of .find('Foo').shallow() just a little, but I think it has many of the same disadvantages that we currently have

@lencioni can you site what you mean by the following for a shallow until helper:

I think it has many of the same disadvantages that we currently have

plus you'd want that to dive() not shallow for a shallowUntil

@lencioni
Copy link
Contributor Author

lencioni commented Dec 5, 2018

Oh boy I wrote that a long time ago and before we added dive. I don't remember exactly what I meant, but maybe I was thinking about the possibility that Enzyme could be configured to do this automatically so when you introduce or remove an HOC, you don't need to change a bunch of tests. Although, that certainly has its own set of tradeoffs and that probably would be worse in a number of other ways.

@dalvallana
Copy link

dalvallana commented Feb 6, 2019

I'm into something similar here: MyComponent calls a class method under certain conditions in the constructor, so I want to spy on this method before shallowing MyComponent, in order to test it is called when it should. Normally I would do something like:

sinon.spy(MyComponent.prototype, 'myMethod');

But when MyComponent is wrapped in a HOC, I end up doing:

sinon.spy(MyComponent.prototype.constructor.WrappedComponent.prototype, 'myMethod');

which is frankly awful...

I honestly don't know if enzyme provides a nicer way of doing this... Maybe I'm missing something.

@ljharb
Copy link
Member

ljharb commented Feb 6, 2019

@dalvallana you either have to do that, or to export the wrapped component directly - neither of which is nice. In that specific case, however, you should be able to shallow render the HOC, and before calling .dive(), do spyOn(wrapper.childAt(0).type().prototype, 'myMethod') (i believe that will work).

@slikts
Copy link

slikts commented Apr 12, 2019

I wish this was solved in Enzyme instead of there being a potpourri of snippets, gists and unmaintained one-off packages to choose from. Not having official support suggests that there's something unsound about diving based on a selector, but this thread doesn't really explicate what the issues would be.

@ljharb
Copy link
Member

ljharb commented Apr 12, 2019

@slikts typically you'd use one .dive() per HOC, and not require any selectors.

@slikts
Copy link

slikts commented Apr 12, 2019

It's not required in the sense that there is a workaround (chaining .dive()), but the depth of nesting is an implementation detail. For example, I might refactor withABC() into withA(), withB(), etc., and the test failing would be a false negative.

@seanconnollydev
Copy link

seanconnollydev commented Apr 12, 2019

@slikts I think the root of the problem is that Enzyme provides too many utilities that are dependent on the structure of the component tree rather than something closer to what the user is seeing. This is the reason why I am moving away from Enzyme and towards react-testing-library. With RTL you would solve this problem with jest mocks, similar to what I wrote about in this post:

https://blog.usejournal.com/testing-graphql-container-components-with-react-apollo-and-jest-9706a00b4aeb

@ljharb
Copy link
Member

ljharb commented Apr 13, 2019

@slikts yes, that is true, but your tests for the component are testing some of the implementation details, and that's ok.

@goldenshun good testing is a balance of surface testing (like what RTL and mount provides) and unit testing (like what shallow provides). Only using one technique is always a mistake. Separately, using mocks and stubs makes tests more brittle, and should be avoided.

@goldensunliu

This comment has been minimized.

@seanconnollydev
Copy link

seanconnollydev commented Apr 13, 2019 via email

@ljharb
Copy link
Member

ljharb commented Apr 13, 2019

@goldenshun i suppose that’s subjective; i don’t find that to be the case.

Certainly there is an improvement to be made here, but the brittleness you’re referring to is “you have to match the count of HOCs with the count of dives”. In practice, this is only mildly annoying - the brittleness of mocks is that your code will fail in production but your tests won’t show that, because the mocks aren’t up to date and mask the errors.

@seanconnollydev
Copy link

seanconnollydev commented Apr 13, 2019 via email

@ljharb
Copy link
Member

ljharb commented Apr 13, 2019

In practice, I’ve found the opposite to be true - and while changing the component structure shouldn’t change tests for other things, the tests for the component being changed shouldn’t necessarily be completely resilient to changes in structure.

@dschinkel
Copy link

dschinkel commented Apr 13, 2019

This thread drives me ape shit.

@dschinkel
Copy link

dschinkel commented Apr 13, 2019

https://blog.usejournal.com/testing-graphql-container-components-with-react-apollo-and-jest-9706a00b4aeb

@goldensunliu this is exactly why I hate Apollo, and is a typical example of what I've ranted about...a
ton of nested HOCs in a component or in an existing HOC. It's bad design period...and is why you're having to integration test it and having such a hard time with it period. Don't ask shallow to fix bad design for you...you're stuck with this kind of prod code when you use Apollo, so you'll have to mount it, regardless of the setup if you only integration test it. But what you should be doing is mostly unit tests (not mostly integration tests) in the first place. That's a whole other discussion.

Your premise that Enzyme doesn't help here is wrong, it's that Apollo's HOC nesting is shit. Essentially Apollo code is the imperative shell, and you shouldn't mix functional core logic inside all those nested HOCs.

Boundaries by Gary Bernhardt (this says Ruby but we use the concept in ANY code, any lang so understand what he's saying here and apply that to your React Design)

So in the case of Apollo test the imperative shell is the Apollo Nested HOCs garbage that you think is "making your life easier" but really it's causing you hell because it's poor design and therefore not easily testable. Decouple your business behavior (user handler logic, container logic) out of there and move them into their own layers, keep the methods you move out small, components small, and test them with unit tests with shallow or if they are just pure functions without React stuff, test them with simple unit tests using Jest or Mocha.

@hmmChase
Copy link

I have a class component called IdeaCard.js that using styled-components and react-apollo that looks like:

render() {
    return (
      <sc.li>
        <Mutation
          mutation={query.DELETE_IDEA_MUTATION}
          variables={{ id: this.props.id }}
        >
          {deleteIdea => (
            <sc.deleteBtn
              type="button"
              onClick={e => this.handleClickDeleteBtn(e, deleteIdea)}
            />
          )}
        </Mutation>

        <Mutation
          mutation={query.UPDATE_IDEA_MUTATION}
          variables={{ id: this.props.id, content: this.state.content }}
        >
          {updateIdea => (
            <sc.ideaInput
              type="text"
              value={this.state.content}
              onChange={e => this.handleChangeideaInput(e, updateIdea)}
            />
          )}
        </Mutation>
      </sc.li>
    );
  }

If I do:

wrapper = shallow(
    <MockedProvider mocks={mockQueries} addTypename={false}>
        <IdeaCard {...mockProps} />
    </MockedProvider>
);

And then:

console.log(wrapper.dive().dive().dive().debug());

I get:

<IdeaCardstyle__li>
    <Mutation mutation={{...}} variables={{...}}>
        [function]
    </Mutation>
    <Mutation mutation={{...}} variables={{...}}>
        [function]
    </Mutation>
</IdeaCardstyle__li>

Is there something I can do test this, or do I need to use mount instead of shallow?

Thanks for any help.

@ljharb
Copy link
Member

ljharb commented Jul 14, 2019

@hmmChase use the new wrappingComponent option to pass MockedProvider, which will save you a dive; but otherwise, what do you want to test? From that debug, you can .find Mutation and use .renderProp to test what the function returns, you can assert on the props passed to Mutation (and delegate that testing responsibility to Mutation's tests)… please file a new issue if you still have questions.

@sktguha
Copy link

sktguha commented Jul 16, 2020

Hi, any update on this ? are there plans to implement something like shallowuntil or like list of nodes which are to be expanded or not expanded, as discussed ?

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

No branches or pull requests