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

Pass callback to wrapper.setState #509

Closed
stephane-ruhlmann opened this issue Jul 21, 2016 · 25 comments
Closed

Pass callback to wrapper.setState #509

stephane-ruhlmann opened this issue Jul 21, 2016 · 25 comments
Labels

Comments

@stephane-ruhlmann
Copy link

React's setState method allows to pass a callback called after the state has been changed.

I think it would be useful to be able to pass a callback the same way for asynchronous testing purpose.

Example :

it("should render childComponent when state says so", (done) => {
    const wrapper = shallow(<ParentComponent />);
    wrapper.setState({renderChild : true}, () => {
      expect(wrapper.find(childComponent)).to.have.length(1);
      done();
    });
  });
@blainekasten
Copy link
Contributor

Does the test not work if you do these calls subsequentially?

const wrapper = shallow(<ParentComponent />);
wrapper.setState({renderChild : true});
expect(wrapper.find(childComponent)).to.have.length(1);

@stephane-ruhlmann
Copy link
Author

stephane-ruhlmann commented Aug 1, 2016

@blainekasten
Due to the asynchronous nature of setState, we have to wrap the expect like this:

const wrapper = shallow(<ParentComponent />);
wrapper.setState({renderChild : true});
setTimeout(() => {
  expect(wrapper.find(childComponent)).to.have.length(1);
}, 50);

@blainekasten
Copy link
Contributor

I could be wrong. But i'm pretty sure (especially with shallow) that setState is forced to be sychroneous.

For reference: https://github.com/airbnb/enzyme/blob/master/src/ShallowWrapper.js#L254-L256

Could you entertain me and remove the setTimeout call and show me the error you get?

@aweary
Copy link
Collaborator

aweary commented Aug 1, 2016

We still rely on setState on the renderer returned from TestUtils.createRenderer, which as far as I know is still asynchronous.

At least, I didn't see anything in ReactTestUtils that indicates its not using the standard ReactUpdatesQueue which is async.

@blainekasten
Copy link
Contributor

on that note, I have no problem passing the callback through if it works. Would you mind putting up a PR for us to see it in action?

@koba04
Copy link
Contributor

koba04 commented Aug 3, 2016

I'm not sure why setState need a callback in testing with Enzyme.

I may be wrong.
But setState behaves as asynchronous only in batchedUpdates.
React is using batchedUpdates on mounting, unmounting, event handling.

(With the exception when you use ReactDOM.unstable_batchedUpdates as batchedUpdates)

In most cases, I think setState provided by Enzyme is using in the outside of batchedUpdates like this.

const wrapper = shallow(<ParentComponent />);
wrapper.setState({renderChild : true});
expect(wrapper.find(childComponent)).to.have.length(1);

So it behaves synchronously.

I think it's a rare case to need a callback on setState.
Am I missing something?

@aweary
Copy link
Collaborator

aweary commented Aug 3, 2016

@koba04 when you call wrapper.setState enzyme will in turn call setState on the instance returned from React's shallow renderer. Which as far as I can tell, inherits from ReactCompositeComponent

https://github.com/facebook/react/blob/master/src/test/ReactTestUtils.js#L421-L434

Nothing that I can see indicates they are injecting an update strategy other than the default, but of course the React source is really dense so I might be missing something.

@stephane-ruhlmann
Copy link
Author

@blainekasten sure, as soon as I'll have some time

@blainekasten
Copy link
Contributor

Might be worth seeing if someone from React core could comment on. I'll try and reach out to one of them.

@zpao
Copy link

zpao commented Aug 3, 2016

I think it's generally a good idea to assume setState won't always act synchronously. Assuming it in tests can lead to assuming it works that way in actual code. I don't actually know enough about the thinking behind shallow renderer to say if it'll never be possible to hit situations where batching will be used, even today.

@aweary
Copy link
Collaborator

aweary commented Aug 3, 2016

Thanks @zpao, if there's nothing explicit in the shallow renderer that forces synchronous state updates, then we should definitely allow a callback to be passed through.

@chrisui
Copy link

chrisui commented Sep 6, 2016

I've come across this issue where I'm testing handlers with setState(state => nextState) calls in my components.

I wonder if it's possible to have wrapper.resolveState() or something similar which forces sync processing of the react state queue so we can assert on the next line without timeouts.

@aronwoost
Copy link

aronwoost commented Sep 29, 2016

👍 for forcing sync setState() when run in tests.

It would save tons of code lines:

setTimeout(() => {
  expect(foo).toBe("bar");
}, 50);

And than it still fails sometimes. Because async can be longer than 50ms.

@ljharb
Copy link
Member

ljharb commented Sep 29, 2016

@aronwoost

return Promise.resolve().then(() => {
  expect(foo).toBe("bar");
});

@aronwoost
Copy link

@ljharb I don't get it.

@ljharb
Copy link
Member

ljharb commented Sep 29, 2016

@aronwoost then you're returning a promise, which mocha will wait on, you don't have to couple to a timeout value, and any exceptions will correctly fail your tests (which they won't with setTimeout)

@aronwoost
Copy link

@ljharb the promise resolves on next tick. Is it guaranteed that new state is populated and componentDidUpdate()/render() happened by then?

@ljharb
Copy link
Member

ljharb commented Sep 29, 2016

You'd handle that by using React callbacks, for example, like a callback to setState.

@aronwoost
Copy link

Yes, if setState() would allow a callback or return a Promise we could use it (thats what this ticket is about, right).

Until that however, we are stuck with setInterval.

Still, my statement from above (sync setState() would be useful in the case) stand.

@alecrobins
Copy link
Contributor

hey I was facing a similar problems when doing some unit testing recently. I just opened up a PR that lets you pass a callback to the setState and setProps for mount and setState for shallow.

@victorhqc
Copy link

victorhqc commented Nov 17, 2016

I got an unexpected behavior, the callback seems that is not working properly in some cases.

Component

import React, { Component } from 'react';
import isEmpty from 'lodash/isEmpty';
import { validation } from 'custom-validation'; // This is not async stuff

export default class MyComponent extends Component {

  onSubmit() {
   // In real component I perform a simple validation that is not async.
    if(!isEmpty( validation(this.state) )) {
      this.setState({ error: 'Some nasty error' });
    }
  }

  render() {
    const { foo, error } = this.state;
    console.log('error', error);

    return (
      <form onSubmit={this.onSubmit}>
         {foo}
      </form>
    );
  }

  state = {
    foo: 'bar',
    error: null
  };
}

This returned null in console.log

const wrapper = mount(<Component />);
wrapper.setState({foo: 'baz'}, () => {
  // My Test goes here.
});

This returned Some nasty error in console.log, which is the expected behavior.

const wrapper = mount(<Component />);
wrapper.setState({foo: 'baz'});
setTimeout(() => {
  // My Test goes here
}, 0);

Edit:
Hmm after seeing the code maybe my component is broken :)

@octalmage
Copy link

octalmage commented Feb 20, 2017

I came across this too. While testing event based actions I wanted to test them in the top level component. The function updates the state, and I when I went to check the state the state wasn't updated yet. I'd love a stateResolve function, but until then the new callback in setState seems to work well:

    dashboard.instance().updateTablePage(5);
    dashboard.setState({}, () => {
      expect(dashboard.state().currentPage).toEqual(5);
      done();
    });

Thanks @alecrobins!

Edit: just kidding, this isn't a real solution. still need a stateResolve function, might look into it.

Sonna added a commit to Sonna/Locomote-Code-Task that referenced this issue Nov 16, 2017
Fix the commented out tests for loading SearchBox search results and
rendering them with HTML, due to an asynchronous problem when running
tests, where occasionally the answer will change depending on whether
the results come back in time; meaning one of two answers will be true
and the other false, depending on when it is called or if it is called
multiple times; e.g.

```javascript
    it('_getResults (after AJAX request with no params)', function (done) {
      const _subject = new describedClass();

      _subject.reRenderCallback = function expectedResults(component, _) {
        const results = component._getResults();
        expect([
          jasmine.any(NullSearchResult),
          jasmine.any(NullComponent)
        ]).toContain(results);
      };

      _subject._searchFromServer('', '', '', function(_error, searchbox) {
        var results = searchbox._getResults();
        expect(results).toEqual(jasmine.any(NullSearchResult));
        done();
      });
    });
```

The SearchBox will be rendered normally on first load containing a
`NullComponent` as a placeholder component until results are loaded, in
this case it returns no results for an empty search and then renders a
`NullSearchResult` component. However, the test will fail unless it is
true for at least one of them when the other is not present; i.e.

- Is true when it renders `NullComponent`    and not `NullSearchResult`,
  and
- Is true when it renders `NulLSearchResult` and not `NullComponent`,
  but
- Is not true when it only renders one of them (since one or the other
  is rendered at some point in the future)

== Notes:
- Some tests have remained disabled since the
  `'_searchFromServer returns expected flights data'` test
  only returns expected results data if `_searchFromServer()` is not
  called multiple time prior to it (meaning it may fail on other
  computers <sigh>)

== References:
- [Pass callback to wrapper.setState · Issue #509 · airbnb/enzyme]
  (enzymejs/enzyme#509)

- [node.js - How do I change the timeout on a jasmine-node async spec - Stack Overflow]
  (https://stackoverflow.com/questions/9867601/how-do-i-change-the-timeout-on-a-jasmine-node-async-spec)
@mdreyer
Copy link

mdreyer commented Feb 16, 2018

++ force resolving the setState cycle!

this is critical for testing components that depend on setState's callback chain, i don't see any way of accomplishing this without using setTimeout/setInterval, which is far from reliable

ex:
onAction(value) { this.setState({ value }, () => { this.someActionCallback(); }); }

how do I test for things that someActionCallback does?

@adeelibr
Copy link

any follow back on @mdreyer said.

@ljharb
Copy link
Member

ljharb commented Aug 17, 2018

@adeelibr update enzyme to v3.4.3 and try it out.

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

No branches or pull requests