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

Listening to renders #728

Closed
rommguy opened this issue Dec 15, 2016 · 33 comments
Closed

Listening to renders #728

rommguy opened this issue Dec 15, 2016 · 33 comments

Comments

@rommguy
Copy link

rommguy commented Dec 15, 2016

Hi,
Thanks for this tool, it's very useful.
Do you plan to add some way to register to component renders ?
I need to test the results of an action on a component, but to assert only after the component renders, and I don't want to add another render in the test, only to have a callback called when the render is done.
So some kind of hook on componentDidUpdate.
Thoughts ?

@blainekasten
Copy link
Contributor

Can you explain in more detail what you are trying to do? The existing API has worked for every use case we've seen thus far.

It sounds like you are wanting to trigger an action and assert that the component responded appropriately. That's usually as simple as this:

const wrapper = mount(<Component />);
triggerAction();

// wrapper.update() could be necassary
// https://github.com/airbnb/enzyme/blob/master/docs/api/mount.md#update--reactwrapper

// inspect expected changes in wrapper
expect(wrapper.find('.updatedElement')).toBe('span')

@rommguy
Copy link
Author

rommguy commented Dec 15, 2016

Well, at a certain point, not specified exactly in documentation, React renders become async.
Documentation of setState method contains this -

There is no guarantee of synchronous operation of calls to setState

In my project, the renders are usually async, and a test like the one you wrote will fail, or worse, pass sometimes.
I want to do a test like this -

it('should have updated element after actions is triggered, (done) => {
    wrapper.onRender(() => {
        expect(wrapper.find('.updatedElement')).toBe('span')
        done()
    })

   triggerAction();
})

@ljharb
Copy link
Member

ljharb commented Dec 15, 2016

That's why .setState takes a callback, and why .update exists.

@rommguy
Copy link
Author

rommguy commented Dec 15, 2016

@ljharb Not sure what you meant. Do you suggest I call setState in the test ?
My tests usually trigger actions, not call setState - that is part of the implementation
I want to listen to renders, to check that async stuff happened, and if it did, call done().
There could be more than one render as a result of an action.

Calling setState as part of the test, or for that matter .update, will cause an additional render, that's isn't there in production scenario

@blainekasten
Copy link
Contributor

I think you are conflating two different tests into one. You should test your actions and your components independently for unit tests.

@rommguy
Copy link
Author

rommguy commented Dec 16, 2016

Another more specific example -
I have a component, consisting of a button, and 2 possible children.
Clicking the button switches between the currently rendered child
The implementation registers onClick, and calls setState when clicked...
Now, I want to write a test, that simulates click, and than checks that the correct child is rendered.
@blainekasten please explain what you meant in this case.

Thanks

@ljharb
Copy link
Member

ljharb commented Dec 16, 2016

@rommguy wrapper.simulate('click'); wrapper.update() and then make your assertions.

@rommguy
Copy link
Author

rommguy commented Dec 16, 2016

@ljharb The test you wrote adds an additional render compared to the production flow of the components.
Like I wrote in previous comments, I don't want additional renders just for the sake of testing, to avoid possible side effects of that extra render.

@ljharb
Copy link
Member

ljharb commented Dec 16, 2016

Renders should never have side effects, and in React, calling setState does trigger an extra render, so that's what should be happening in tests too.

@rommguy
Copy link
Author

rommguy commented Dec 16, 2016

Your test has 2 renders - one from the setState, and another one because you call update()
The production flow only has 1.

@blainekasten
Copy link
Contributor

@roommguy, what @ljharb is getting at is that it shouldn't matter if you render once, or 100 times. The point of a react component render is to be pure based on props and state, meaning it returns the same thing every time while props and state do not change.

Why are you concerned about multiple renders?

@blainekasten
Copy link
Contributor

Another more specific example -
I have a component, consisting of a button, and 2 possible children.
Clicking the button switches between the currently rendered child
The implementation registers onClick, and calls setState when clicked...
Now, I want to write a test, that simulates click, and than checks that the correct child is rendered.
@blainekasten please explain what you meant in this case.

I would say in this situation you still have 2 different tests happening.

  1. simulating onClick updates state
  2. Given a certain state, an expected UI happens

@rommguy
Copy link
Author

rommguy commented Dec 16, 2016

@blainekasten Regarding the question why I'm concerned about additional renders - I agree 100% that components should be pure, and additional renders should not matter, but I'm writing a test, and when writing tests, you can't assume the component is written as it should be - if you can assume that you don't need tests...
That's why when you test stuff, you want the test flow to be as identical as possible to the production flow, you don't know what people have changed, and maybe someone made a serious mistake and caused the render to have side effects.

Splitting to 2 separate tests like you suggested is actually an anti-pattern to testing code, especially in React.
You are testing inner component implementation, instead of the product.
Tests should always focus around user flows - click leads to DOM change, not to setState
Testing inner implementation details makes future refactor harder, instead of making it easier, which is one of the main purposes of tests.

@ljharb
Copy link
Member

ljharb commented Dec 16, 2016

@rommguy react calls render potentially a thousand times in production, so if your render isn't pure, you are already broken and violating the rules of react. "Identical to the production flow in tests" means "render a whole bunch of times", plain and simple.

@rommguy
Copy link
Author

rommguy commented Dec 16, 2016

@ljharb As far as I know React doesn't initiate render. It is you who initiates render, by updating props or state, or calling forceUpdate.
I'm unfamiliar with render initiated by React library without relation to your code. That would make very bad performance apps, if they just render all the time without reason.

I was hoping for a more serious debate or reply, but you guys keep trying to convince me that I don't need to test what I want to test
Thanks for the replies.
closing the issue

@rommguy rommguy closed this as completed Dec 16, 2016
@blainekasten
Copy link
Contributor

I'm sorry you feel this wasn't productive. We did have 16 comments here discussing. We can keep discussing if you want. I also would keep arguing that your intention is not quite accurate for what you want to test.

I agree 100% that components should be pure, and additional renders should not matter, but I'm writing a test, and when writing tests, you can't assume the component is written as it should be

This actually proves my point a bit more. If you are writing tests and find that tests break by causing re-renders. Your test has just done it's job in showing you that your render is not pure. That's a test well done. So therefore, I still think that you shouldn't be concerned with causing re-renders in your tests.

@jwbay
Copy link
Contributor

jwbay commented Dec 18, 2016

From this provided example, it seems like all you want is to wait until React has flushed any async setState calls to DOM, then assert:

    wrapper.onRender(() => {
        expect(wrapper.find('.updatedElement')).toBe('span')
        done()
    })

In this case, you just need to ensure your test waits until whatever causes the render has happened. So if it's a promise, e.g.

  onClick() {
    api.getData().then(data => {
        this.setState({ data: data }) //should re-render when the promise resolves, and our test should assert after
      })
  }

Your test needs to take that into account, usually by mocking the ajax call and keeping a handle on that promise.

it('works async', done => {
  const response = Promise.resolve({ fakeData: 42 })
  setUpMockApiCall(api.getData, response)
  const wrapper = mount(<MyComponent />)
  wrapper.find('button').first().simulate('click')
  response.then(() => {
    expect(wrapper.find('.updatedElement')).toBe('span')
    done()
  })
});

In this example, since the .then in the component's onClick is attached before the .then in the test, React will flush state updates to DOM before we make our assertion.

You can find something similar in enzyme's own test suite: https://github.com/airbnb/enzyme/blob/master/test/ShallowWrapper-spec.jsx#L3973

@rommguy rommguy reopened this Dec 18, 2016
@rommguy
Copy link
Author

rommguy commented Dec 18, 2016

Hi @jwbay and thank you for the detailed response.
Maybe I didn't understand something there, but I believe the test you suggested fails, because setState is async, and you assert synchronously after the promise resolved, which means the render didn't happen yet.

@jwbay
Copy link
Contributor

jwbay commented Dec 18, 2016

I think the sticking point here is likely setState being 'async'. It's important to note that when you call setState, React doesn't wait until some random future tick of the event loop to start flushing your state changes to DOM*. It would be next to impossible to test React components if this were the case.

setState noted as being 'async' was more about batching, so people wouldn't do this and expect it to work:

this.setState({ x: this.state.x + 1 })
if (cond) {
  this.setState({ x: this.state.x + 1 })
}

This code may not add 2 to state.x even if cond is true. It depends on whether the two setState calls are batched.

See docs: https://facebook.github.io/react/docs/react-component.html#setstate

setState batching is an implementation detail of React and your tests really don't need to care. Its effects are not exposed to your tests unless you're spying on MyComponent.prototype.render and tracking calls to it.

*This is actually exactly what may happen in a future version of React (though less random and more carefully scheduled), but I would count on either having a flag to flip to force the current behavior or a special test renderer available.

@rommguy
Copy link
Author

rommguy commented Dec 18, 2016

Wouldn't waiting for render to finish cover this whether it's sync or async?
If I had some hook on componentDidUpdate, I can assert there, and I wouldn't care how React implemented the render process
Currently my tests fail unless I initiate an additional render with a callback that contains the assertions. This doesn't usually happen on small components, only when writing an integration test, where I render the entire app tree, which contains lots of components.

@jwbay
Copy link
Contributor

jwbay commented Dec 18, 2016

Wouldn't waiting for render to finish cover this whether it's sync or async?

Which render? You'd need some kind of wrapper.onUpdate(3, () => {, to get the 3rd render if that's the soonest you can make your assertions. This seems pretty fragile and tightly coupled to both your component's implementation and how React implements updates.

Currently my tests fail unless I initiate an additional render with a callback that contains the assertions

More than likely you've either got impure renders as discussed above, or you're not waiting for some async flow to finish within the test. Per above, async flow here does not mean setState calls, it means setTimeouts, http requests, or Deferreds/Promises.

For your integration test, a component doesn't know when its child components update. That information is not bubbled up by React. So if you're trying to wait until a child component has 'finished' an update/render after a setState, that's a nonstarter. The wrapper couldn't tell you, even if you had an onUpdate hook available on it.

@rommguy
Copy link
Author

rommguy commented Dec 18, 2016

I actually cause only a single render with each action, so I don't need a counter of some kind like you suggested.

React invokes DidUpdate only after all children have finished rendering.
All the "will" method are called top to bottom, and all the "did" methods are called bottom top
So componentDidUpdate is a good place to know all the subtree has updated.

@jwbay
Copy link
Contributor

jwbay commented Dec 18, 2016

I actually cause only a single render with each action, so I don't need a counter of some kind like you suggested.

While you don't need it for your specific use case, others may want it once such an API is available. See: #279. The relationship is orthogonal, but it's an example of API creep. As soon as someone makes two calls at once with two independent setStates, they'd want an onUpdate(2, () => { to be able to 'wait' long enough to make their assertions.

So componentDidUpdate is a good place to know all the subtree has updated.

Assuming the wrapper is what kicked off the update, yes, absolutely. Otherwise, no. Let's say you make an ajax call in didMount of a child component while the initial tree is being mounted, per idiomatic React. The setState that happens in the callback will be kicked off by the child component itself, so its didUpdate will be called but the parent's will not.

Again, you may not care for your use case, but such things would likely come up as part of introducing a new method to the Enzyme API.


Note that if you're absolutely bound and determined to hook into your component's didUpdate, you can always spy on it yourself in your test, either by stubbing/mocking according to your test runner/framework, or proxying it manually yourself. If this is a repeated pattern you could even have a helper method do it for you. I'd still recommend trying to figure out why you need a second manual render to make your tests pass, though. It may uncover unsupported patterns or other strangeness. Best of luck!

@rommguy
Copy link
Author

rommguy commented Dec 19, 2016

I agree with your points, thank you for the thorough explanation.
Closing the issue.

@rommguy rommguy closed this as completed Dec 19, 2016
@viridia
Copy link

viridia commented Jul 9, 2017

A strong use case for what the OP asked for is when testing with react-apollo/test-utils. Components rendered with <MockedProvider><Component></MockedProvider> render asynchronously (in fact, they render twice, once with blank data, and a second time with mocked data), and there is no way to get a reference to the internal promise, so you can't wait on it.

Thus it seems the only solution is to wait until rendering is complete.

@ljharb
Copy link
Member

ljharb commented Jul 9, 2017

What's react-apollo, and what test utils would you be trying to use with enzyme?

@viridia
Copy link

viridia commented Jul 9, 2017

React-apollo is part of the Apollo project, which is a client-side library for Facebook's GraphQL. Components can be wrapped in a HoC connector which associates them with a GraphQL query; when the query results are available, your component can be rendered.

For testing, you'd use a mock provider (provided as part of the react-apollo test utils) which allows you to simulate the query results. However, the mock provider isn't synchronous, so when you try to use enzyme mock(), what it returns is a component that is not fully populated, and thus the assertions that follow the mount() call fail.

The react-apollo test utils show examples working with Jest (although they claim it should work with other frameworks), so apparently there is some way of handling this issue, but I don't know what it would be.

I want to continue using enzyme + mocha + certainty, I don't want to have to switch my large project over to using Jest just because I'm adopting GraphQL.

In the mean time, I have found a workaround with the enzyme-wait package, but this only works in cases where the element has some attribute or feature that can be waited on via an enzyme selector expression.

@ljharb
Copy link
Member

ljharb commented Jul 9, 2017

Thanks for the explanation.

I'd say if there's no way to directly get at the underlying promise, that's a flaw in the library. It should be stored on the wrapper instance, or in the state of it, and then that can be accessed by a testing library.

@viridia
Copy link

viridia commented Jul 10, 2017

My point is that there may be a lot of HoCs that work like this - bear in mind that many HoCs are functions, not classes, and whatever asynchronous mechanisms they use might be represented only by local variables.

@ljharb
Copy link
Member

ljharb commented Jul 11, 2017

@viridia if an HOC works like this, then it's intentionally made itself non-testable - there's not really anything enzyme can do about that.

In other words, if it's not storing a promise somewhere accessible, then it's explicitly chosen to deny you the ability to respond to the result of the async action.

@treshugart
Copy link

treshugart commented Jan 22, 2018

Could one just use https://github.com/sindresorhus/p-wait-for and do something like:

import waitFor from 'p-wait-for';

const wrapper = mount(<Node />);
wrapper.simulate('something');

await waitFor(() => wrapper.find(Something).length);

// do stuff now

I'm probably missing something. This could totally slow down tests, of course, but could possibly be useful if you need to do this on occasion. We're thinking that our use case for what we originally saw in this may be solved by #728 (comment), but just thought I'd post it here in case it'd help anyone needing to wait for something.

@laukaichung
Copy link

laukaichung commented Jun 4, 2018

For React Apollo, I use setTimeout and wrapper.update() in every it function to wait for data fetched from my local database before starting assertion:

describe('Query', () => {
    let wrapper;
    before( ()=>{
        wrapper = mount(
            (
                <TestProvider>
                    <Query query={gql`...`}>
                        {
                           ({data:{something}})=>
                                 <CustomComponent something={something} />
                        }
                    </Query>
                </TestProvider>
            )
        );
    })

    it('exists', done => {
        setTimeout( ()=>{
            wrapper.update();
            console.log(wrapper.debug());
            expect(wrapper.find(CustomImage).length).to.be.above(0);
            done();
        },1000)
    })
});

It works, but I'd like to see some alternatives. I want to avoid using setTimeout.

@tmoody460
Copy link

NOTE: I'm using Enzyme and Jest with Typescript / React.

I am unable to test state changes triggered asynchronously from componentDidMount(). I want to avoid putting a timeout or delay in the code to wait for the render method to be called. Is there a promise-based or synchronous way to achieve this? Here's a simplified version of my use case:

export class Demo extends React.PureComponent<{ getTestValue: () => Promise<string> }, { testValue: string }> {
    constructor(props) {
        super(props);
        this.state = { testValue: "" };
    }
    componentDidMount() {
        this.props.getTestValue()
            .then((value) => {
                this.setState({ testValue: value })
            });
    }
    render() {
        return <div>{this.state.testValue}</div>;
    }
}

And here are my two tests, the first one fails and the second (with a simple delay) succeeds:

describe('Demo.render', () => {

    it('fails', () => {
        const value = "test";

        const mock = jest.fn().mockReturnValue(Promise.resolve(value));

        const demo = shallow(<Demo getTestValue={mock} ></Demo>);
        demo.update();

        expect(demo.find("div").html()).toBe("test");
    });

    it('succeeds', () => {
        const value = "test";

        const mock = jest.fn().mockReturnValue(Promise.resolve(value));

        const demo = shallow(<Demo getTestValue={mock} ></Demo>);
        demo.update();

        delay(100).then(() => {
            expect(demo.find("div").html()).toBe("test");
        });

    });
});

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

8 participants