Test custom async hooks

[helabenkhalfallah]

I use Enzyme version 3.8.0.

// functional component
const UsersList = ({ onItemSelected, }) => {
  // async service fetch hooks
  const url = 'service-url';
  const { error, loading, data, } = useService(Service, url, {
    amount: 0,
  });

  return (
    <Fragment>
      <div className="user-list">
          {
            data.map(user => (
              <UserItem
                key={user.id}
                user={user}
                onItemSelected={onItemSelected}
              />
            ))
          }
        </div>
    </Fragment>
  );
};

My useService make an async call to get user list data.

Test :

jest.mock('axios');
it('Should Mount Component & Display Data', async () => {
    // mock result
    axiosMock.get.mockResolvedValue(UserListMock);
    
    // mount component
    const wrapper = mount(<UserList />);

    // wait to have data
    await expect(wrapper.state().data).to.not.be.null;

    // test
    expect(wrapper.find('h2').first().text()).toEqual('Mock Title');
  });

But I have this error :
ReactWrapper::state() can only be called on class components

[swapkats]

As the error states .state is only valid for class components. your component is a functional component using hooks. This is not supported at the moment. You can subscribe for progress on hook support on this issue. #2011

Meanwhile looks like this forked branch will suffice your use case #2008

[helabenkhalfallah]

OK thank you, I check it :) :)

[helabenkhalfallah]

Hello, I locally used the branch #2008 :

"enzyme": "file:../enzyme/packages/enzyme",
"enzyme-adapter-react-16": "^1.9.1",
"enzyme-to-json": "^3.3.5",

But I still get the error :

    ReactWrapper::state() can only be called on class components

      1034 |
      1035 |         if (this.instance() === null || this[RENDERER].getNode().nodeType !== 'class') {
    > 1036 |           throw new Error('ReactWrapper::state() can only be called on class components');
           |                 ^
      1037 |         }
      1038 |         var _state = this.single('state', function () {
      1039 |           return _this16.instance().state;

      at ReactWrapper.state (../enzyme/packages/enzyme/build/ReactWrapper.js:1036:17)
      at Object.state (pages/mocks/MockSelectComponent.test.jsx:25:23)
      at tryCatch (node_modules/@babel/runtime/node_modules/regenerator-runtime/runtime.js:62:40)
      at Generator.invoke [as _invoke] (node_modules/@babel/runtime/node_modules/regenerator-runtime/runtime.js:288:22)
      at Generator.prototype.(anonymous function) [as next] (node_modules/@babel/runtime/node_modules/regenerator-runtime/runtime.js:114:21)

I will wait for enzyme update, Thanks 👍 :)

[chenesan]

Hi @helabenkhalfallah I'm still working on #2008 and currently it only has test code to verify what is not working (will fix the test in the future), so I think it won't work in your test for now. Also the state() currenly is used for class component state -- I'm not sure if it's the api to get hooks state in the future.

[ljharb]

For the record, the way I'd like to approach supporting hooks in enyzme is in this order (as separate PRs):

1. components that use hooks should Just Work in both mount and shallow without any user test changes
2. since hooks can be used with class components as well, setState and state probably shouldn't interact with hooks. So, we may need to explore a new API just for interacting with hooks on a component.

[helabenkhalfallah]

@ljharb
1. components that use hooks should Just Work in both mountandshallow without any user test changes
Perfect, we need only to mock Service Response if we have Backend Call (exactly like before).

2. since hooks can be used with class components as well
Hooks can only been used inside functional component not class.
You can’t use Hooks inside of a class component, but you can definitely mix classes and function components with Hooks in a single tree.
I don't understand this point.
Thanks.

[ljharb]

I'm pretty confident that despite the docs, hooks can be used in any kind of component.

[helabenkhalfallah]

OK but using hooks inside a Class will not going have a big advantage because inside a Class I can directly use state, dimount for Http Call, Redux, ... But to reuse statefull logic (useState, custom Hooks, ...) inside Funtional Component here is the big advantage (single independent atomic parts : UI + Hooks).

[ljharb]

Whether it's an advantage or not is utterly irrelevant; if it works in React, enzyme has to support it.

[helabenkhalfallah]

I don't know your use case but Hooks inside class doesn't work, you will get :
React Hooks Error: Hooks can only be called inside the body of a function component.
Thanks :)

[ljharb]

Thanks, if that's indeed the case then that might make the second item in the list above much easier.

[helabenkhalfallah]

Yes, effectively ;)

[bdwain]

2. since hooks can be used with class components as well, setState and state probably shouldn't interact with hooks. So, we may need to explore a new API just for interacting with hooks on a component.

One of the few complaints I've had about enzyme is the existence of the setState and state helpers. I know that they seem helpful, but I think they encourage bad test design (i'll explain why below). Rather than try to support something similar for functional components using hooks, I would love it if Enzyme took this opportunity to drop support for those helpers by not providing an alternative that works with functional components and encouraging people to write their tests without them.

The reason I don't like the setState and state helpers is because they encourage people to rely on internal implementation details of a component that should not be publicly exposed. For example, this is a common thing I've seen.

class Counter extends Component {
  state = {
     counter: 0
  };

  render(){
    const {counter} = this.state;
    return (
       <div>
          count: {counter}
          <button onClick={() => this.setState({counter: counter + 1}}>
            increment
          </button>
       </div>
    );
  }
}

//test
test('button increments counter', () => {
  expect(component.state().counter).toBe(0);
  component.find('button').simulate('click');
  expect(component.state().counter).toBe(1);
});

That test is bad for a few reasons. First, it's testing an implementation detail. The fact that your test would break if you renamed state.counter to state.count is a sign you're testing the wrong things. State is not part of the public API of a component. It's not part of the props and not part of the return value (except where you return it in render), so you shouldn't read or manipulate it directly.

In addition to the above, the test is also not necessarily correct, because testing that state.counter is correct is useless if you are not using state.counter correctly. The test would still pass if you said count: {someOtherStateValue} instead of count: {counter}.

The correctness issue can be prevented if you add a second test like this

  component.setState({counter: 1});
  expect(component.text()).toInclude('count: 1')

but that's now two tests when you could have done one

    component.find('button').simulate('click');
    expect(component.text()).toInclude('count: 1')

This test is correct, won't break if the public API doesn't change, is concise, tests only one thing, and also simulates the actual usage of the component, which is what tests are supposed to do.

I'm happy to talk about this more. But that's the gist of my argument. I think this would lead to people writing much better tests, and while I know there will be a lot of pushback, this seems like the perfect opportunity to do it. And if they are not removed from the API, I would at least encourage making them more difficult to use to make it clear they are escape hatches and not everyday methods. The same can be said for component.instance(), but that at least is impossible for functional components.

[ljharb]

(turns out hooks can't be used on class components, so that's my mistake).

Good tests for X should be dealing with the implementation details of X - it's everything else that shouldn't.

[bdwain]

Can you explain your position a bit more? Why do you think the implementation details are worth testing?

To me, it's like testing private methods of a class, which can't even be tested in languages like C++, and in Java you need reflection for. Not testing them seems to be a popular opinion. And state seems like a great candidate for something private if javascript supported it.

[ljharb]

Certainly private things should never be tested! State, however, isn't private. Either way, it's because in order to test various states of a component, you'd have to be able to get in them - and often that requires cooperation from children that setState callbacks are passed to, or browser interaction that's difficult to test, etc.

[bdwain]

I definitely get the difficult browser interaction thing, but those cases are rare enough that if they were the only reason to support it I feel like it would be good to mark the methods like dangerouslySetState or something to make clear they are escape hatches. The other one though has never been a problem for me. If I want to test a set state callback passed to a child, I just do something like `component.find(Child).props().theCallback()`. I don’t think that’s any less convenient than calling setstate.

[ljharb]

Sure, that's fine too - but it doesn't satisfy every case.

[bdwain]

Yea I get that. My point was that if it’s generally preferable to avoid setState except in exceptional circumstances, maybe the api could self document that In the name of the method somehow. It wouldn’t prevent you from doing it, but it would make you think about it before doing it, similar to how needing to disable a lint rule in exceptional circumstances is fine but having to explicitly do it forces you to think about why it’s not an ideal thing to use.

…

On Sun, Feb 17, 2019 at 12:25 AM Jordan Harband ***@***.***> wrote: Sure, that's fine too - but it doesn't satisfy every case. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2012 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADzDDmcnt1eX-iEw-N5pXY045zVOJj-Kks5vOPXBgaJpZM4avWFU> .

[bdwain]

Seeing @gaearon say (in #1938) supporting setState for hooks would be very difficult in its current form reminded me of this thread again, so I thought I'd try one more time.

It sounds like it would be a large effort to maintain support for state and setState (and other things that deal with "internals" of a component). While I think it's good to make it easier to test hard-to-test things with helpers like setState (if you make it clear they are for exceptional cases), there's also something to be said for simplicity. Whatever is necessary to support a setState method that works with hooks is likely to be extremely complex and difficult to maintain.

I've already noticed enzyme start to fall behind React features a bit (new context support took a long time after it came out). I'm sure that's mostly due to needing more contributions from the community (and I'd be happy to help), but adding more complexity to support exceptional cases seems like it would hold the library back even more, and people starting new projects will see that the newest React features are not supported and will look at other libraries.

I think for now, the best thing to do is to deprecate state, setState, and possibly the context helpers (if they suffer from the same problem) and remove them in the next major version of enzyme. This way, there won't be a split between methods that work in functional components and methods that don't (that would cause a lot of confusion I think). And it will make supporting new features of react easier. If in the future, there is more bandwidth and people have time to explore something to inspect the state of a component in a way that works with both hooks and class components, then that's great (though I would recommend messaging it as for exceptional cases, unlike today).

Thoughts @ljharb ?

[ljharb]

@bdwain I agree that the current setState method isn't a good idea to have interact with hooks. Whether a new API method could/should be added is a different story.

enzyme certainly has fell behind; we went from 5 contributors down to 1 (hi!) and this is far from my only project. The main concern would be that time spent building some new hooks API is time not spent on the rest of #1553.

As for whether various methods should be deprecated, I think that's a much bigger and different question. We already have some methods that only work (or are only useful) for certain kinds of components, so I don't think that split is really a problem in practice.

[bdwain]

Fair enough. I didn't realize there was a split between functional and class components besides the limitations of the actual components.

[ljharb]

That is the split - and in the case of hooks, that also differs by the component type (createClass and class components can't use hooks; only SFCs can)