enzymejs / enzyme

JavaScript Testing utilities for React
https://enzymejs.github.io/enzyme/
MIT License
19.95k stars 2.01k forks source link

How to force update a component after v3? #1245

Open Stupidism opened 6 years ago

Stupidism commented 6 years ago

Before I looked at your demo code at update

class ImpureRender extends React.Component {
  constructor(props) {
    super(props);
    this.count = 0;
  }
  render() {
    this.count += 1;
    return <div>{this.count}</div>;
  }
}

const wrapper = shallow(<ImpureRender />);
expect(wrapper.text()).to.equal('0');
wrapper.update();
expect(wrapper.text()).to.equal('1');

and wrote a stateless(so it has multiple levels) counter and tested it with mount.

const wrapper = mount(<StatelessRender />);
expect(wrapper.text()).to.equal('0');
wrapper.update();
expect(wrapper.text()).to.equal('1');

Now the tests are failed. It seems update() can still forceUpdate a shallowed component but not a mounted one any more.

Now the problem is how do I forceUpdate a mounted component without props changing? There are some breaking changes about update mentioned in migration guide, but It doesn't solve my problem.

Stupidism commented 6 years ago

The temp fix is changing wrapper.update(); to wrapper.setProps({});, but it seems not natual.

ljharb commented 6 years ago

You can't ever assume that side effects in render will be called any specific number of times - do you have an example of an actual real world use case where you'd want an update - with the same props and state - to result in a change? That kind of seems against the entire concept of React.

Stupidism commented 6 years ago

The use case is exactly the ImpureRender component.

<Container>
 render times:
 <ImpureRender />
</Container>
ljharb commented 6 years ago

Why is that a use case tho? Why would using a component that defies all best practices for React be something you want to do?

Stupidism commented 6 years ago

I didn't make it clear enough. It's just a counter that counts how many times a component is rendered. Everytime Container re-renders, it will render its children again, so ImpureRender would +1.

I'm using a component like ImpureRender to show how many times every level of container is rendered in a multiple-level-container component in a document-style webapp. With these counts, I can know which part might have performance issues and locate the bottleneck more effectively. For example: one single click triggers two renders.

It works well with React itself. Now the problem is just enzyme cannot simulate this situation.(It was able to but not any more now)

ljharb commented 6 years ago

The number of renders isn't the important part; the number of updates is. As such, if you write a class-based component with a componentDidUpdate method, then you'll be able to count the actual performance bottlenecks - and it will work well with enzyme too.

Stupidism commented 6 years ago

Unfortunately, it doesn't. I'm using componentWillReceiveProps method, but it's not updated after I call update() on root component. The component is here and the test is here;

Maybe I should use componentDidUpdate instead?

ljharb commented 6 years ago

You'll note that's what I suggested :-)

Stupidism commented 6 years ago

It's not working... Both StatelessRenderCounter and StatefulRenderCounter in my repo

Travis build result is here:

 FAIL  src/RenderCounter/__tests__/RenderCounter.spec.js (7.658s)
  ● StatefulRenderCounter › should change the text after update
    expect(received).toEqual(expected)

    Expected value to equal:
      "2"
    Received:
      "1"
 FAIL  src/RenderCounter/__tests__/withRenderCount.test.js (8.169s)
  ● withRenderCount(BaseComponent): NewComponent › modifies props › should +1 count prop after component update
    expect(received).toBe(expected)

    Expected value to be (using ===):
      "2"
    Received:
      "1"
ljharb commented 6 years ago

Again, https://github.com/Stupidism/stupid-rc-starter/blob/enzyme-update-or-set-props/src/RenderCounter/StatefulRenderCounter.js#L15-L22 is using componentWillReceiveProps, and you want it to be componentDidUpdate.

Separately, you can't count on https://github.com/Stupidism/stupid-rc-starter/blob/enzyme-update-or-set-props/src/RenderCounter/__tests__/RenderCounter.spec.js#L19 actually triggering a rerender; React will see that the props and state haven't changed, and can choose to skip the render entirely.

Stupidism commented 6 years ago

Sorry, I didn't commit the version using componentDidUpdate. I tried all your suggestions but they all failed. So I created a branch with the original version and posted here for further help.

ivanvpan commented 6 years ago

I can confirm that there're issues. For me with these package versions update() produces no result with either mount or shallow rendering:

├─ enzyme-adapter-react-16@1.1.1
│  ├─ enzyme-adapter-utils@^1.3.0
├─ enzyme-adapter-utils@1.3.0
├─ enzyme@3.3.0
tristan-shelton commented 6 years ago

Also experiencing this issue. The reference code at http://airbnb.io/enzyme/docs/api/ShallowWrapper/update.html does not pass using the latest versions of React, Enzyme and the React adapter.

The "this isn't best practice" responses are not helpful when there are lots of ways to use React. Why have an update() method if it doesn't do anything?

In my case I'm using MobX with React and it would be very helpful in writing tests to be able to force a shallow rerender after updating non-state properties that are used in render().

ljharb commented 6 years ago

Does .forceUpdate() not work?

won0089 commented 6 years ago

Experiencing the same issue. In my case I need to to wrapper.instance().forceUpdate() and wrapper.update() in order for the wrapper to be re-rendered. In my opinion wrapper.update() should re-render the wrapper when instructed.

"enzyme": "3.3.0", "enzyme-adapter-react-16": "1.1.1"

tristan-shelton commented 6 years ago

@ljharb forceUpdate did not work for me. setState({}) did, so I used that as a hack. I didn't try the combination won0089 suggested.

MickeyKay commented 6 years ago

Chiming in with another data point. I ran into the same issue recently. After banging my head against the wall for quite a while, a colleague suggested using setContext() in place of update(), and that fixed things immediately.

While all tests are now passing, I'm not confident this is the best approach. The existing test setup uses global state and store vars, which are passed into mount() calls via the context option. It's still unclear to me if there are other implications to bypassing this method and instead using setContext(), however, as mentioned, tests are passing so 🤷‍♂️.

For posterity. . .

Before (not working):

it('should render post title', () => {
  const query = { complete: true, OK: true, entities: [postJson.id] }
  state = merge({}, stateMultipleEntities, { wordpress: { queries: { 0: query } } })
  rendered.update() // Fake store update from completed request
  expect(rendered.html()).toEqual('<div>Architecto enim omnis repellendus</div>')
})

After (working):

it('should render post title', () => {
  const query = { complete: true, OK: true, entities: [postJson.id] }
  state = merge({}, stateMultipleEntities, { wordpress: { queries: { 0: query } } })
  rendered.setContext({ store })
  expect(rendered.html()).toEqual('<div>Architecto enim omnis repellendus</div>')
})

Relevant package.json deps

"dependencies": {
  "react": "^16.4.2",
  "react-dom": "^16.4.2",
  "react-redux": "^5.0.7",
  "redux-saga": "0.13.0"
}

Major props to @franjohn21!

ljharb commented 6 years ago

@MickeyKay what happens if you use .debug() instead of .html()? The latter does a full render, and might not give you what you expect.

lukeAnderson2015 commented 5 years ago

have this same issue. I tried .setProps({}), .setContext(), .instance().forceUpdate(), none of these solutions worked.

resorting to comparing the .html() output to a string stored in the test. feelsbadman.jpg

edit: this has to do with poorly written react components, in that they aren't managing some state via setState() but simply modifying attributes e.g. this.value = x;

ljharb commented 5 years ago

@lukeAnderson2015 i'd suggest using .debug() instead of .html(), fwiw.

For the record, you can certainly get access to those component instances, and stub out instance variables on them, if you need to.

bisubus commented 5 years ago

A common case for component update is testing instance methods:

class Comp extends Component {
  onClick = () => {}

  renderer() {
    return <button onClick={this.onClick}/>
  }
}

const wrapper = mount(<Comp/>);
spyOn(wrapper.instance(), 'onClick');
wrapper.update();
wrapper.find('button').simulate('click');

That update() doesn't do the only thing it's expected to do is very confusing at least.

ljharb commented 5 years ago

@bisubus agreed, but you should never have an arrow function in a class field (also, i'd suggest avoiding simulate, since it doesn't actually simulate anything) - the proper way to do that is:

class Comp extends Component {
  onClick = this.onClick.bind(this);
  onClick() {}

  renderer() {
    return <button type="button" onClick={this.onClick} />
  }
}

spyOn(Comp.prototype, 'onClick');
const wrapper = mount(<Comp/>);
wrapper.find('button').prop('onClick')();
bisubus commented 5 years ago

@ljharb Your example is exactly my own preferred way to write things, proto methods provide extra layer of testability. On the other hand, setting up spies/stubs on instance allows structure tests in DRYer way and move mount to beforeEach where needed:

beforeEach(() => {
  wrapper = mount(<Comp/>)
});
it('should stub specific method', () => {
  wrapper.instance().onClick = spy();
  wrapper.update();
  // ...
});

Not my cup of tea but it's possible. I also believe that my own code style isn't the only one that has the right to exist. Even if it were, there are situations where I cannot dictate it. If simulate had no uses, it wouldn't exist in library API.

ljharb commented 5 years ago

Tests shouldn’t be DRY, that’s for implementation.

Simulate as implemented has no uses; it only exists in enzyme because it existed in react-test-renderer, and we didn’t realize it was so useless until it was too late to remove. It will be removed in v4.

conatus commented 5 years ago

@ljharb

Thanks for your work on this. Bit concerned that removing .simulate will break a huge number of people's tests. Is there going to be a code-mod upgrade path for this?

ljharb commented 5 years ago

@conatus v4 isn't coming any time soon. The reality, though, is that many tests that use simulate are broken already, because of mistaken assumptions about how it works.

conatus commented 5 years ago

Sure but these mistaken assumptions are extremely widespead. I'd estimate most React codebases have these mistaken assumptions in.

What is the right way to test these kind of interactions?

ljharb commented 5 years ago

@conartist6 .prop('onClick')(), eg.

avegancafe commented 5 years ago

I'm curious @ljharb , what mistaken assumptions are you referring to? Want to write as good tests as I can over here! lol

ljharb commented 5 years ago

@kyleholzinger for one, that .simulate actually simulates anything - it does not. It is nothing more than sugar for invoking a prop function, sometimes with a mock event object (depending on mount vs shallow). It's trivial to make your own mock event object (and if not, then that might be something enzyme should support), but invoking a prop directly is much more straightforward.

avegancafe commented 5 years ago

Ahh I see, yeah that makes total sense (your explanation, not that implementation necessarily). Are there good guides/libs/methods for creating that own mock event object? I'm curious to know what it would look like, beyond making some { target: {} } type fake. Totally agree that of course invoking the prop directly is not only more straightforward, but aligns better with react's "everything is just javascript" paradigm!

ljharb commented 5 years ago

Typically I just match whatever the implementation needs - { target: {} }, { preventDefault() {} }, etc.

makr28 commented 4 years ago

I'm using a functional component. When I debug into it, the component renders but my wrapper in my test doesnt show the changes.

wrapper.update() isn't fixing this issue.

In the component I have a usestate hook. And I am triggering a function inside the functional component which calls the the state values setter. If I debug into the code it renders it with the correct state value (used as a prop in a child component).

But in the test, If I try to see what value is being passed to that child component, it is still the old value.

Here's a quick example of what i want to test (component and test) ` const wrapper= ({}) => { const [selected, setSelected] = React.useState(false);

function handleOnSomething(idx) { setSelected(idx) }

return (

})

it('test', () => { const wrapper= shallowWIntl();

viewer.find('selector').prop('onSomething')(1); // this method call the setter

const item2 = viewer.find(selector2ndItem);

// should be correct and is when debugging but wrapper doesnt have the changes expect(item2.props().isSelected).toEqual(true); expect(wrapper).toMatchSnapshot(); });
` Basically the test should set the 2nd items' isSelected to true. It does in the code when I debug but the wrapper in the test doesnt show those changes.

I have the following versions "enzyme": "^3.3.0", "enzyme-adapter-react-16": "^1.1.1", "enzyme-react-intl": "^1.4.8", "enzyme-redux": "^0.2.1", "react-test-renderer": "^16.2.0", "enzyme-to-json": "^3.3.3",

ljharb commented 4 years ago

Please file a new issue; this one from 2017 isn’t the same problem as hooks.