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

Test custom async hooks #2012

Open
helabenkhalfallah opened this issue Feb 8, 2019 · 23 comments
Open

Test custom async hooks #2012

helabenkhalfallah opened this issue Feb 8, 2019 · 23 comments

Comments

@helabenkhalfallah
Copy link

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
Copy link

swapkats commented Feb 8, 2019

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
Copy link
Author

OK thank you, I check it :) :)

@helabenkhalfallah
Copy link
Author

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
Copy link
Contributor

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
Copy link
Member

ljharb commented Feb 11, 2019

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
Copy link
Author

helabenkhalfallah commented Feb 11, 2019

@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
Copy link
Member

ljharb commented Feb 11, 2019

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

@helabenkhalfallah
Copy link
Author

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
Copy link
Member

ljharb commented Feb 11, 2019

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

@helabenkhalfallah
Copy link
Author

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
Copy link
Member

ljharb commented Feb 11, 2019

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

@helabenkhalfallah
Copy link
Author

Yes, effectively ;)

@bdwain
Copy link
Contributor

bdwain commented Feb 17, 2019

  1. 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
Copy link
Member

ljharb commented Feb 17, 2019

(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
Copy link
Contributor

bdwain commented Feb 17, 2019

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
Copy link
Member

ljharb commented Feb 17, 2019

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
Copy link
Contributor

bdwain commented Feb 17, 2019 via email

@ljharb
Copy link
Member

ljharb commented Feb 17, 2019

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

@bdwain
Copy link
Contributor

bdwain commented Feb 17, 2019 via email

@bdwain
Copy link
Contributor

bdwain commented Mar 23, 2019

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
Copy link
Member

ljharb commented Mar 24, 2019

@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
Copy link
Contributor

bdwain commented Mar 27, 2019

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

@ljharb
Copy link
Member

ljharb commented Mar 27, 2019

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)

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

No branches or pull requests

5 participants