enzymejs / enzyme

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

ReactWrapper.update() is not forcing a re-render #2042

Open nicholasrice opened 5 years ago

nicholasrice commented 5 years ago

Current behavior

min-repro here: https://github.com/nicholasrice/enzyme-react-wrapper-update-repro

Using mount to mount a component, calling the update method of the returned ReactWrapper instance does not seem to be forcing a re-render. With slight changes, I implemented the example from https://airbnb.io/enzyme/docs/api/ReactWrapper/update.html and am expierencing a test failure.

On a slight aside, I think the assertions being made in the above documentation should be 1 for the first call and 2 for the second call, instead of 0 for the first call and 1 for the second.

const React = require('react');
const Enzyme = require('enzyme');
const Adapter = require('enzyme-adapter-react-16');

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

test("ReactWrapper.update should re-render a react component", () => {
    const spy = jest.fn();

    class ImpureRender extends React.Component {
        constructor(props) {
            super(props);

            this.count = 0;
        }

        render() {
            this.count += 1;
            spy(this.count);

            return React.createElement("div", {}, this.count);

        }

    }

    const wrapper = Enzyme.mount(React.createElement(ImpureRender));

    expect(spy).toHaveBeenCalledTimes(1)

    // Update the component - should force re-render per https://github.com/airbnb/enzyme/blob/master/docs/api/ReactWrapper/update.md
    wrapper.update();

    expect(spy).toHaveBeenCalledTimes(2)
});

Expected behavior

I would expect that calling the update method of a ReactWrapper would call the render method of the mounted component.

Your environment

API

Version

library version
enzyme 3.9.0
react 16.8.4
react-dom 16.8.4
react-test-renderer 16.8.4
adapter (below) 1.10.0

Adapter

ljharb commented 5 years ago

Since this relies on an impure render (which is obviously a very bad idea in React), can you elaborate on the actual use case where this is popping up? If props and state and context haven't changed, a rerender shouldn't need to call render, even via enzyme's update.

nicholasrice commented 5 years ago

Yea the min-repro isn't a real use-case (but it is pulled from Enzyme documentation). I'm trying to write a test to validate that a context provider is providing the same context object between render cycles, and only providing a new object when a certain prop or parent context is changed. This involves checking object references after multiple renders of a component.

The above component could easily be changed to have a "pure" render by removing this.count and the issue would still manifest. The documentation says: "Forces a re-render." Apologies if I'm not understanding something here, but by that claim I would expect the render method of a React.Component that does not implement shouldComponentUpdate to get executed.

ljharb commented 5 years ago

Hmm, that does make sense. It does stand to reason that a call to update forces a rerender.

msakrejda commented 5 years ago

Hi, I've also just run into the same issue. I definitely understand that impure renders are a bad idea, but in my case I'm testing something that involves mocking out a render prop component that relies on external state. The real version of the component updates its own state and re-renders, but in the mock it's easier to just craft calls to the children passed in. But that means only changing the mock's implementation of render, which means I need update to force a re-render.

Since you tagged as help wanted, is this a good place to start looking?

ljharb commented 5 years ago

@uhoh-itsmaciek yes, but be aware many other methods call into this.update(), and we may not want those to force a rerender.

msakrejda commented 5 years ago

Interesting. Why would they be calling it? The internal documentation is the same: the method forces a re-render.

ljharb commented 5 years ago

They call it when state and/or props change, usually.

I'm just saying we'll have to be careful about it, and that it might be better to refactor so that the current behavior is preserved for all internal update attempts.

ghbakhtiari commented 5 years ago

Any updates on this?

ljharb commented 5 years ago

Nope, it's still open, has the "help wanted" label, and there's no associated PR.

hannaholl commented 5 years ago

I'm having the same issue.

Doing wrapper.instance().forceUpdate() works for me but looking at the documentation I would expect wrapper.update() to do the same. I don't know how "safe" using forceUpdate is but for now it's fixed the problem for me.

ducin commented 5 years ago

Having the same issue.

Having gone through this thread, the easiest solution is to update the docs, stating that it updates only if there were some changes to props/state. If update is an internal tool used by enzyme, there's no need to adapt it to invalid docs. Adapt docs to what update actually is.

Hiroki111 commented 5 years ago

Event wrapped.instance().forceUpdate() doesn't re-render :(

let wrapped;
beforeEach(() => {
    wrapped = mount(
      <Provider store={store}>
        <Test />
      </Provider>
   );
});

it("disables submit button if it's already uploading a file", () => {
    const uploadButton = wrapped.find("#uploadButton");
    expect(uploadButton.prop("disabled")).toEqual(false);

    wrapped.instance().setState({ uploading: true }, () => {
        wrapped.instance().forceUpdate();

        expect(uploadButton.prop("disabled")).toEqual(false);
        // This is supposed to fail, but it passes
    });
});
ljharb commented 5 years ago

What does wrapped.debug() look like, before the setState, and after the forceUpdate?

ghost commented 5 years ago

I'm having the same issue.

Doing wrapper.instance().forceUpdate() works for me but looking at the documentation I would expect wrapper.update() to do the same. I don't know how "safe" using forceUpdate is but for now it's fixed the problem for me.

TypeError: wrapper.instance(...).forceUpdate is not a function ... I got this error

sameekshakumari commented 5 years ago

Event wrapped.instance().forceUpdate() doesn't re-render :(


let wrapped;
beforeEach(() => {
    wrapped = mount(
      <Provider store={store}>
        <Test />
      </Provider>
   );
});

it("disables submit button if it's already uploading a file", () => {
    const uploadButton = wrapped.find("#uploadButton");
    expect(uploadButton.prop("disabled")).toEqual(false);

    wrapped.instance().setState({ uploading: true }, () => {
        wrapped.instance().forceUpdate();

        expect(uploadButton.prop("disabled")).toEqual(false);
        // This is supposed to fail, but it passes
    });
});
```This works
SwinX commented 4 years ago

Event wrapped.instance().forceUpdate() doesn't re-render :(

let wrapped;
beforeEach(() => {
    wrapped = mount(
      <Provider store={store}>
        <Test />
      </Provider>
   );
});

it("disables submit button if it's already uploading a file", () => {
    const uploadButton = wrapped.find("#uploadButton");
    expect(uploadButton.prop("disabled")).toEqual(false);

    wrapped.instance().setState({ uploading: true }, () => {
        wrapped.instance().forceUpdate();

        expect(uploadButton.prop("disabled")).toEqual(false);
        // This is supposed to fail, but it passes
    });
});
```This works

In your test assertion is never triggered because test runner never has a chance to wait until setState callback call: neither promise was returned from test not test finish callback was used.

MrMuzyk commented 4 years ago

I'm having the same issue.

Doing wrapper.instance().forceUpdate() works for me but looking at the documentation I would expect wrapper.update() to do the same. I don't know how "safe" using forceUpdate is but for now it's fixed the problem for me.

Im not 100% sure but i think that:

Enzyme's update() method checks if props/state changed and based on that decides to update component or not.

forceUpdate() in this case is actually React's method. Notice that you don't call it at wrapper but at wrapper.instance(). It causes a re-render no matter what https://reactjs.org/docs/react-component.html#forceupdate.

I'd say that this is acceptable solution.

shaun-weddell commented 4 years ago

forceUpdate won't work in a functional component though, as there is no instance

nathan5x commented 4 years ago

@shaun-weddell - You're absolutely right. For React FC - the instance() method will return null. So, obviously we can't call forceUpdate()

PFight commented 4 years ago

This code works for functional component:

    let component = mount(<test.component />);
    component.setProps({ foo: 42 }); // re-render

Actually, my component even don't need to have a prop foo.

lquixada commented 4 years ago

Following @PFight idea, this generic approach worked for me:

const component = mount(<YourComponent {...anyProps} />);
component.setProps(); // Forces react component tree to re-render.
component.update(); // Syncs the enzyme component tree snapshot with the react component tree.
mmassaki commented 4 years ago

Actually, since setProps expect next props to merge, component.setProps() should be enough to trigger a re-render

lquixada commented 4 years ago

@mmassaki you're right! just fixed the suggestion! thanks!

travisdahl commented 4 years ago

setProps fixed the problem for me but doesnt seem like a real solution

ljharb commented 4 years ago

.update isn't supposed to rerender. It's supposed to update the enzyme tree based on the latest state of the react tree.

In other words, if you haven't changed any props or state, there's nothing to rerender.

travisdahl commented 4 years ago

in my particular case, its not the "rendering" i care about but its the only way I can get the new mock value to be recognized...

Note: in a beforeEach I am doing the shallow mounting and mocking my module for general use.

in this test I want to mock a different return value. if i dont force the rerender with wrapper.setProps() it never uses this new mock value, but instead the one from the before each.

...

  let wrapper;
  let props;

  beforeEach(() => {
    myMockModule = jest.fn().mockReturnValue({
      fizz: 'buzz',
    });

    wrapper = shallow(<App  />);
  });

 ...

    test('my test', () => {
      myMockModule.mockReturnValueOnce({
        foo: 'bar',
      });
      wrapper.setProps(); // without this it doesnt work
      expect(wrapper).toMatchSnapshot();
    });
ljharb commented 4 years ago

What do you mean the new mock value? All mocks should be done before creating the wrapper.

It makes no sense whatsoever to call shallow in a beforeEach. Repetition in tests is good.

travisdahl commented 4 years ago

hmmm interesting.... Ive written a few thousand tests set up like that. 🤣 my bad.

i usually do a shallow in the before each and then set props and state in each test and assert im getting the desired results. gets rid of a lot of boiler plate and ensures I always get a fresh render. why does it "makes no sense whatsoever"? It seems like it is repetition, just automatic? i.e. what is the problem in doing it in a beforeEach if you are gonna do it at the beginning of every test anyway? I'd love to hear more about it, but dont want to hijack this thread either.

ljharb commented 4 years ago

The difference is that each test that has a tiny modification can change it without shenanigans like "oops, let me update the wrapper and rerender it", and that failure messages are clearer.

travisdahl commented 4 years ago

yes, i guess my code snippet above proves your point. If i was mocking before i created the wrapper in each test I wouldnt have the problem I do. Thanks!

The crazy part is the setProps actually does fix it though and let me re-mock the value. Maybe just a happy side-effect

igortas commented 3 years ago

How to force to re-render the component, when I'm updating redux state with store.dispatch(action)? With wrapper.setState(), I'm updating the wrapper prop(s) with the setted state, but that prop is not propagate down below the tree. I've tried with wrapper.update(), but does not help.

xveganxxxedgex commented 2 years ago

I know this is an old issue, but in case anyone ever comes across this issue while trying to test redux useSelector state changes like the comment above mentions, I ended up mocking the selector itself to get the component to have the correct selector state.

Assuming your component has something like this:

const someState = useSelector(getSomeState);

In your test file:

describe('checks some state', () => {
    let stateSpy;

    beforeAll(() => {
        nextActiveStageSpy = jest.spyOn(selectors, "getSomeState").mockImplementation(() => "new-state");
    });

    afterAll(() => {
        nextActiveStageSpy.mockRestore();
    });

    ...
}