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

ReactWrapper::setProps() can only be called on the root #1925

Open
2 of 13 tasks
LandonSchropp opened this issue Dec 4, 2018 · 35 comments
Open
2 of 13 tasks

ReactWrapper::setProps() can only be called on the root #1925

LandonSchropp opened this issue Dec 4, 2018 · 35 comments

Comments

@LandonSchropp
Copy link

Current behavior

My team is trying to test a component that consumes a MobX wrapper like this:

let component = mount(
  <Provider store={ store }>
    <Awesome name="Landon" />
  </Provider>
);

In this simplified example, Awesome's implementation would look something like this:

@inject('store')
@observer
class Awesome extends React.Component {

  componentDidMount() {
    this.props.store.awesomePerson = this.props.name;
  }

  componentDidUpdate() {
    this.props.store.awesomePerson = this.props.name;
  }

  componentDidUnmount() {
    delete this.props.store.awesomePerson;
  }
  
  render() {
    return <p>You're awesome { this.prop.name }</p>;
  }
}

We're trying to test the componentDidUpdate method. Because we need to use a Provider, we can't call setProps on the root element. Instead, we tried to do this:

component.find('Awesome').setProps({ name: "Nicole" });

Unfortunately, this produces this error in Enzyme:

ReactWrapper::setProps() can only be called on the root

It looks like this issue has been opened before with setState, but doesn't seem to be working for setProps.

Expected behavior

We should be able to set the props of a child component.

Your environment

API

  • shallow
  • mount
  • render

Version

library version
enzyme 3.4.2
react 16.4.2
react-dom 16.4.2
react-test-renderer N/A
adapter (below) 1.2.0

Adapter

  • enzyme-adapter-react-16
  • enzyme-adapter-react-16.3
  • enzyme-adapter-react-16.2
  • enzyme-adapter-react-16.1
  • enzyme-adapter-react-15
  • enzyme-adapter-react-15.4
  • enzyme-adapter-react-14
  • enzyme-adapter-react-13
  • enzyme-adapter-react-helper
  • others ( )
@ljharb
Copy link
Member

ljharb commented Dec 4, 2018

Conceptually, the enzyme wrapper is rendered, from the root, as a pure function of props and state. It makes sense to be able to set state on any component, to be able to simulate certain inputs.

However, any props that you want to pass to your Awesome component are determined by its parent.

This is a bit of a special case, where you're the one determining the props passed to Awesome, but it's wrapped in a Provider - but in this case, <Awesome> is passed as a children prop to Provider.

So, you can wrapper.setProps('children', <Awesome differentProps />), and I'd expect that to trigger the componentDidUpdate.

@LandonSchropp
Copy link
Author

Thanks for the quick reply! We'll give this workaround a try.

In the meantime, should this behavior eventually be changed? I've run into a similar use case before where I had a component that renders a table cell, and I had to create the rest of the table context.

let component = mount(
  <table>
    <tbody>
      <tr>
        <AwesomeTableCell name="Landon" />
      </tr>
    </tbody>
  </table>
)

@ljharb
Copy link
Member

ljharb commented Dec 4, 2018

I'd expect the same thing - component.setProps('children', replacementTbody).

It's worth thinking about if this is a common enough use case to warrant special API support for it.

@LandonSchropp
Copy link
Author

In that case above, I think this would be a little more convenient for me:

component.find('AwesomeTableCell').setProps({ name: "Jordan" });

@ljharb
Copy link
Member

ljharb commented Dec 7, 2018

@LandonSchropp the challenge is that the only things you should be able to set props on is things in the children you passed in - not things that, say, AwesomeTableCell renders.

@LandonSchropp
Copy link
Author

@ljharb Maybe I'm missing something here, but isn't it impossible to test AwesomeTableCell without first sticking it in the other table elements? I think react requires <td> to be inside of <tr>.

Even those the child elements require a certain context, my tests are still only concerned with AwesomeTableCell. How would you test AwesomeTableCell#componentDidUpdate?

@ljharb
Copy link
Member

ljharb commented Dec 8, 2018

@LandonSchropp sorry if i wasn't clear. Here's what I'm suggesting from your OP:

const wrapper = mount(
  <Provider store={ store }>
    <Awesome name="Landon" />
  </Provider>
);

expect(store.awesomePerson).toBe('Landon');

wrapper.setProps({ children: <Awesome name="Bob" /> });
wrapper.update(); // this may or may not be needed, you'll have to try it and see

expect(store.awesomePerson).toBe('Bob');

(Note that you're mutating a prop, which is a very very very bad and un-Reacty thing to do)

With your other snippet:

const wrapper = mount(
  <table>
    <tbody>
      <tr>
        <AwesomeTableCell name="Landon" />
      </tr>
    </tbody>
  </table>
);

// some assertion about Landon

wrapper.setProps({
  children: (
    <tbody>
      <tr>
        <AwesomeTableCell name="Bob" />
      </tr>
    </tbody>
  ),
});
wrapper.update(); // maybe
// some assertion about Bob

@smoleniuch
Copy link

I have a similar problem.

I would like to test my component which is dependant on the provider.

    it('Renders valid no data text', () => {

        let wrapper = mount(
         <ThemeProvider>
           <ResponsiveTable
            noDataText="no data"
            loadingError={false}
            isLoading={false}  data={[]}/>
         </ThemeProvider>
       )
        expect(wrapper.contains(<div>no data</div>)).toBeTruthy()
        wrapper.find('ResponsiveTable').instance().setProps({isLoading:'true'}) // this doesnt work due to lack of setProps method
        expect(wrapper.contains(<div>no data</div>)).toBeFalsy()

     })

If the setProps method would be available on the instance i could test such component.

@ljharb
Copy link
Member

ljharb commented Dec 8, 2018

@smoleniuch

wrapper.setProps({
  children: (
    <ResponsiveTable
      noDataText="no data"
      loadingError={false}
      isLoading={true} 
      data={[]}
    />
  )
});

@LandonSchropp
Copy link
Author

@ljharb Thanks again for the detailed replies. I'm still getting used to testing React with Enzyme, and it seems to me I still have some learning to do to properly build components that correctly fit into the correct model. Based on your response, I think I have enough information to work around my current issue. I appreciate your help in getting there. 😀

If it's okay with you, I would like to leave this issue open as a feature request. I do think there are a few use cases where it would be more convenient to be able to call setProps directly on a child component based on the necessary usage of that component.

@ljharb
Copy link
Member

ljharb commented Dec 9, 2018

I think leaving this open is a great idea; so far all the use cases have been about setting props on one of the elements initially passed in to the wrapper - not about setting props on a rendered child component - and there might be something to explore there.

@smoleniuch
Copy link

smoleniuch commented Dec 9, 2018

@ljharb

Thanks for the response.But the idea behind updating the props in the child component is that i would like to test componentDidUpdate life cycle method.

componentDidUpdate(prevProps) {
    const { showNotification, hideNotification } = this.props.notificationBlanket
    if (this.props.isLoading && !prevProps.isLoading) {
        showNotification({
            type: 'pending',
            name: 'loadingIndicator'
        })
    }

    if (!this.props.isLoading && prevProps.isLoading) {
        hideNotification()
    }
}

Maybe this code is not perfect but i have build NotificationBlanket component which shows notifications for wrapped component.I would like to wrap more components with this blanket.

Ommiting the use case, how to test componentDidUpdate in that scenario?when the Component is wrapped by Provider

I know that you can pass the context directly to the mounted component.Is it the only solution for this?
According to this it is not encouraged.

@ljharb
Copy link
Member

ljharb commented Dec 9, 2018

Generally i shallow-render the component, and setProps directly on it. Instead of wrapping it in the Provider, I’d manually provide the context option to shallow.

@mmoss
Copy link

mmoss commented Dec 13, 2018

@ljharb how do you handle, for instance, when you need to force focus for accessibility purposes so you use a ref... It doesn't look as though refs are compatible with shallow, so how does one test both setting props of something that requires a context, that also requires a ref?

@ljharb
Copy link
Member

ljharb commented Dec 13, 2018

@mmoss since string refs are deprecated, you can use a ref callback, explicitly invoke it, and trust that it gets a DOM node. You can also use mount to verify that you do in fact get the DOM node you want. (shallow takes a context option)

@mmoss
Copy link

mmoss commented Dec 17, 2018

@ljharb I'm not sure I'm following how that would let me test this...

The issue I've got is that I'm using React.createRef(); (not using string refs) and also need to set the context... If I wrap a Component in the context provider, I then can't uset setProps on the component that has the ref...

export class MyButton extends React.Component {

  buttonRef = React.createRef();

  componentDidUpdate(prevProps, prevState) {
    if (prevProps.someProp !== this.props.someProp) {
      this.buttonRef.current.focus();
    }
  }

  render() {
    return (
      <React.Fragment>
        <MyContext.Consumer />
        <button
          ref={this.buttonRef}
          // ...

Update: I worked around this by just stubbing the ref and using shallow rendering. I needed to assert that the focus method was invoked anyhoo.

@ljharb
Copy link
Member

ljharb commented Dec 18, 2018

(enzyme doesn't yet support Context; not sure if that's relevant to your example or not)

I think stubbing the ref is probably the simplest approach, since shallow can't truly support refs (only mount can).

@EyalPerry
Copy link

EyalPerry commented Jan 21, 2019

@ljharb
The use case for setProps on non-root wrappers is to greatly help the community implement integration testing with various libraries- something which is not quite possible at this moment.
I, for instance, am forced to implement e2e tests to ensure some scenarios function properly, one example would be:

I am using react-router's Router and Link components.

I want to write a test suite which tests the following:
a) an anchor tag is rendered as a result of rendering a Link component
b) the component reacts to prop changes (post rendering) and changes the href, className and style attributes on the anchor tag.

For test a to pass, I can not use shallow, since the Link component is abstracted by my own component so that react-router imports do not pollute the code base.

Unfortunately, once mounted, the react-router Link component throws an error unless a Router component is present somewhere above it in the element tree.

Due to this, the mount root becomes the Router and not the Link component.
Thus, setProps now refers to the Router.
Which means I can not test scenario b.

@ljharb
Copy link
Member

ljharb commented Jan 21, 2019

@EyalPerry it can still be done by re-setting the “children” prop on the root, albeit not very ergonomically. This is definitely a use case i think that can be well addressed without needing to setProps on a child; it’s being worked on.

@EyalPerry

This comment has been minimized.

@EyalPerry
Copy link

@ljharb it's being worked on === you are working on setProps for non root wrappers?

@EyalPerry
Copy link

@ljharb The solution you propose does indeed work, but it is not as I would like. I wrap enzyme in a thin layer of sugar and use that layer. resetting children whenever I want to set props sounds like it could break tests, my spider senses are tingling.

@ljharb
Copy link
Member

ljharb commented Jan 21, 2019

@EyalPerry no, i don't think it ever makes sense to set props anywhere but the top of the tree, just like in react. However, I think the use case of "i need to test X, but in order to do that, I have to wrap it in 1 or more "provider"-like components, and i'll need to set props on X" can be addressed with a separate API when creating the initial wrapper. This includes your Router use case.

iow, instead of:

const wrapper = mount(<Router><Link /></Router>);

you'd do something similar to:

const wrapper = mount(<Link />, { wrappedWith(root) { return <Router>{root}</Router>; } });

@EyalPerry
Copy link

@ljharb awesome. Thank you for the update!

@flq
Copy link

flq commented Feb 13, 2019

We had a similar situation for a component that does require a TranslationContext to be tested correctly (so, root is translationcontext and the tested component is inside).

What we did is to wrap the whole thing into another ad-hoc component and pass-through the properties like such:

mount(
  React.createElement(
    props => (
      <I18nextProvider i18n={i18n}>
        <ComponentUnderTest {...props} />
      </I18nextProvider>
    ),
    properties)
);

That way, you can set props on the root and they trickle down to the component under test

@adarrra
Copy link

adarrra commented Apr 5, 2019

But what if I export component wrapped in HOC? e.g in component I have
export default withBreakpoints(Navigation)); this withBreakpoints pass props e.g. sm, md, lg
so then I can't setProps (change breakpoints props) on Navigation component directly

@ljharb
Copy link
Member

ljharb commented Apr 5, 2019

@adarrra i'm not sure why you'd need to. In your case, the HOC-wrapped component is in charge of what props Navigation receives, so if you set props and/or state on the wrapped component, it will affect what Navigation gets.

@jaydlawrence
Copy link

We had a similar situation for a component that does require a TranslationContext to be tested correctly (so, root is translationcontext and the tested component is inside).

What we did is to wrap the whole thing into another ad-hoc component and pass-through the properties like such:

mount(
  React.createElement(
    props => (
      <I18nextProvider i18n={i18n}>
        <ComponentUnderTest {...props} />
      </I18nextProvider>
    ),
    properties)
);

That way, you can set props on the root and they trickle down to the component under test

Similar to this, I solved this by making a proxy component:

const Proxy = ({ options }) => (
  <Provider>
    <MyComponent
      options={options}
    />
  </Provider>
);

const wrapper = mount(
  <Proxy options='initial value' />
);

wrapper.setProps(
  {
    options: 'new value'
  }
);

@worldsayshi
Copy link

If the goal is to provide the component with a particular context there is an alternative approach. Mock the context and pass it into the second argument of mount. Here's an example for mocking React router:

const router = {
  history: new BrowserRouter().history,
  route: {
    location: {},
    match: {},
  },
};
const wrapper = mount(<Component />, {
  context: { router },
  childContextTypes: { router: shape({}) },
});

This example is adapted/stolen from this solution on how to to do enzyme testing with react router.

@mattcarlotta
Copy link

mattcarlotta commented Jul 3, 2019

Using the @flq solution, you can build a reusable test function:

For example, wrapping a component with a MemoryRouter -- can be substituted for Context or a Provider and/or some other HOC that is dynamically passed into this function as well -- however, state still needs to be set on the component itself (unless the component state is derived from props).:

/**
 * Factory function to create a Mounted MemoryRouter Wrapper for a component
 * @function HOCWrap
 * @param {node} Component - Component to be mounted
 * @param {object} initialProps - Component props specific to this setup.
 * @param {object} state - Component initial state for setup.
 * @returns {MountedRouterWrapper}
 */

import React, { createElement } from 'react';
import { MemoryRouter } from 'react-router-dom';
import { mount } from 'enzyme';

export const HOCWrap = (Component, initialProps = {}, state = null) => {
  const wrapper = mount(
    createElement(
      props => (
        <MemoryRouter>
	  <Component {...props} />
	</MemoryRouter>
    ), initialProps )
  );
   
   if (state) wrapper.find(Component).setState(state);
   return wrapper;
};

import it into your setupTests.js file:

import Adapter from "enzyme-adapter-react-16";
import { configure } from "enzyme";
import { HOCWrap } from "./utils/testing";

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

global.HOCWrap = HOCWrap;

Use it whenever you need to wrap something in a MemoryRouter:

import Example from "../path/to/example.js"

const initialProps = { 
  message: "Loading your preferences..." 
};

const initialState = { 
  isLoading: true 
};

const wrapper = HOCWrapper(Example, initialProps, initialState);

describe("Example", () => {

  it("contains a message and is loading", () => {
     expect(wrapper.find(Example).props("message")).toEqual("Loading your preferences...");
     expect(wrapper.find(Example).state("isLoading")).toBeTruthy();
  });

  it("updates the message and is no longer loading", () => {
     wrapper.setProps({ message: "Welcome!" });
     wrapper.find(Example).setState({ isLoading: false });

     expect(wrapper.find(Example).props("message")).toEqual("Welcome!");
     expect(wrapper.find(Example).state("isLoading")).toBeFalsy();
  });
});

@bethcodes
Copy link

Given the rise of Context as an architectural model, the idea that props will be on the root node simply isn't true anymore. All of these dozens-of-lines-long work arounds don't address the basic issue, which is that enzyme was designed for a different React architecture than the one we are now working with.

@ljharb
Copy link
Member

ljharb commented Dec 13, 2019

@bethcodes that idea hasn't ever been entirely true, because of components that provide old-style context. The basic issue is that React has introduced many new features without sufficient regard for how people will test them in a more granular way than "ReactDOM.render everything".

@pardoman
Copy link

pardoman commented Jun 7, 2021

The workaround proposed by @jaydlawrence was very helpful and I was able to make use of it successfully, thanks!

@MerzDaniel
Copy link

Yes that is the way to go for the moment 👍
#1925 (comment)

@Shaju06
Copy link

Shaju06 commented Sep 24, 2021

I'm trying this setProps form mapatataprop but it doesn't update it and the props still remains same as coming from store

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