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

[Tests] add hooks tests #2008

Merged
merged 6 commits into from
Jun 11, 2019
Merged

[Tests] add hooks tests #2008

merged 6 commits into from
Jun 11, 2019

Conversation

chenesan
Copy link
Contributor

@chenesan chenesan commented Feb 7, 2019

This PR adds support for [basic hooks API] (https://reactjs.org/docs/hooks-reference.html#basic-hooks) (useState, useEffect, useContext, related issue #1938 #1996). Currently I'm not sure what hooks API is already working in enzyme shallow / mount and what isn't, so I'll start from adding test cases to confirm the current supports. I'll try to finish the following checklist in the next few days. Also tell me if it's better to have single PR for each api :-)

I've added some test cases for useState and found it doesn't work in shallow to setState.

(Updated: Created a checklist in #2011 to try to track any issue / PR in hooks, and concentrate on support useState in this PR)

@wodenx
Copy link

wodenx commented Feb 7, 2019

Would you consider adding useRef to the shortlist?

@chenesan chenesan mentioned this pull request Feb 8, 2019
14 tasks
@chenesan chenesan changed the title [WIP] Support React hooks [WIP] Support React hooks useState Feb 8, 2019
@chenesan
Copy link
Contributor Author

chenesan commented Feb 8, 2019

@wodenx I just created #2011 to try to track all of the hooks API support. And in this PR I'd like to work on useState. Feel free to open issue / PR for useRef :)

expect(wrapper.find('div').length).to.equal(1);
expect(wrapper.find('div').text()).to.equal('0');
});
it('handles setState returned from useState', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('handles setState returned from useState', () => {
it('handles setState returned from useState', () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb Can we add setprops test case to simulate componentwillReceiveProps and test using act ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, that sounds great.

describeIf(is('>= 16.8'), 'hooks', () => {
it('handles useState', () => {
const ComponentUsingStateHook = () => {
const [count] = React.useState(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be pulled in react-compat, and not accessed off of React.

@perrin4869
Copy link

Hm... Any clue how you'll get this working? The upstream shallow renderer doesn't seem to support this

@hisapy
Copy link

hisapy commented Feb 8, 2019

Hi ...

I just upgraded to React v16.8.1 and I started having these warnings when testing a Hooks based component.

console.error ../../node_modules/react-dom/cjs/react-dom.development.js:506
    Warning: An update to _default inside a test was not wrapped in act(...).
    
    When testing, code that causes React state updates should be wrapped into act(...):
    
    act(() => {
      /* fire events that update state */
    });
    /* assert on the output */
    
    This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act

Is this something that should be handled by enzyme? I didn't have these warnings when I was at the Hooks alphas

@ljharb
Copy link
Member

ljharb commented Feb 8, 2019

@hisapy enzyme has zero support for hooks right now; as for the warnings, that's because react added them between the alphas and the real release.

@hisapy
Copy link

hisapy commented Feb 8, 2019

Thx for the quick answer @ljharb ... I wish I could get rid of the warnings ...

Right now I'm trying to use act to see where it takes me.

Just for the record, I'm not actually testing the hooks. My component useState but in my tests I just mount the component and do the usual simulate stuff, like I would without hooks.

@chenesan
Copy link
Contributor Author

chenesan commented Feb 9, 2019

@perrin4869 I'd try to see what I can do in react shallow renderer.

@Jessidhia
Copy link

act() should be used by any React code that may interact with state, whether you use hooks or not (it'll be important for ConcurrentMode too). It's expected to be reexported and/or automatically wrapped by testing libraries. See https://github.com/threepointone/react-act-examples/blob/master/README.md

@ljharb
Copy link
Member

ljharb commented Feb 18, 2019

It seems like something enzyme could/should perhaps abstract away from the user?

@chenesan
Copy link
Contributor Author

I think that the failed shallow test is related to facebook/react#14840 . After facebook/react#14841 fixes the issue the test should pass.
As for act() I guess we could wrap the call inside returned render of ReactSixteenAdapter.createMountRenderer. But I'm not sure if this should be included in this PR.

@ljharb
Copy link
Member

ljharb commented Mar 5, 2019

@chenesan we should probably merge the act change in a separate PR first.

@chenesan
Copy link
Contributor Author

chenesan commented Mar 6, 2019

@ljharb I would try to work on act() this week with some use cases in https://github.com/threepointone/react-act-examples/blob/master/README.md

(Update: Send PR in #2034)

@pgangwani
Copy link

pgangwani commented Mar 6, 2019

@ljharb @chenesan : I would like to contribute here. Please help me to get kickstarted. Reading Contributing guide.

@chenesan
Copy link
Contributor Author

chenesan commented Mar 6, 2019

Hi @pgangwani glad to hear this. What could I do to help you?
If you're not sure where to start maybe you could find issues with help wanted label. Or maybe you could help add some test cases for react hooks

@pgangwani
Copy link

pgangwani commented Mar 7, 2019 via email

@ljharb
Copy link
Member

ljharb commented Mar 7, 2019

@pgangwani please post a link to a branch on #2029, and i'll cherry-pick it into that PR.

@pgangwani
Copy link

@ljharb : Is there any chatroom ? As I am new, may need little help to start contributing so kickstart well and try sinking in the pattern to work with you

@chenesan
Copy link
Contributor Author

chenesan commented Mar 7, 2019

Hi @pgangwani there is gitter chatroom :-)

@pgangwani
Copy link

@chenesan Added PR #2041 for test cases. Please review

@chenesan
Copy link
Contributor Author

chenesan commented Mar 9, 2019

@pgangwani Glad to see your work! I'm not a maintainer here, though. So I have no permission to approve the changes and I think my review doesn't count a lot. Let's wait for @ljharb 's review.

@pgangwani
Copy link

@chenesan @ljharb All these cases are already covered in #2041

@chenesan
Copy link
Contributor Author

@pgangwani good to see #2041 ;)
@ljharb If these cases are included in #2041, should I close this?

@chenesan
Copy link
Contributor Author

chenesan commented May 29, 2019

@Acesmndr I believe that works with shallow after react@16.8.5 (See facebook/react#15120)

@Acesmndr
Copy link

@chenesan I tried but even with React v16.8.6 Enzyme V3.9.0 and Enzyme Adapter v1.13.2 The test is failing. Here is a sample file along with its specs.

import React, { useLayoutEffect, useState } from 'react';

type FormElement = React.FocusEvent<HTMLInputElement>;

const NewForm: React.FC = () => {
  const [email, setEmail] = useState<string>('');
  useLayoutEffect(() => {
    console.log('it should be called but isn\'t called', email);
  }, [email]);

  const handleEmailChange = (evt: FormElement) => {
    const emailValue = (evt.target as HTMLInputElement).value.trim();
    setEmail(emailValue);
    console.log('This gets called but the setEmail is not called');
  };
  
  return (
    <form>
      <input
        id="batman"
        value={email}
        onChange={handleEmailChange}
      />
    </form>
  );
};

export default NewForm;

and the specs:

import * as React from 'react';
import { shallow } from 'enzyme';

import NewForm from './test';

describe('Form', () => {
  let container;

  beforeAll(() => {
    container = shallow(<NewForm />);
  });
  describe('email', () => {
    it('sets value', () => {
      container.find('input').prop('onChange')({
        target: {
          value: 'acesmndr@gmail.com',
        },
      });
      expect(container.find('input').prop('value')).toEqual('acesmndr@gmail.com');
      // console.log(container.find('input').props());
    });
  });
});

And the error it's throwing is that email isn't updated even though setEmail is called.
image
Am I doing anything wrong?

@yuritoledo
Copy link

@Acesmndr
Have u tried like this?

<input
  id="batman"
  value={email}
  onChange={e => setEmail(e.target.value)}
/>

@Acesmndr
Copy link

Acesmndr commented Jun 4, 2019

@yuritoledo Must be one of the dependencies in my project as it didn't work even with that approach. But once I setup everything from scratch using a boilerplate it did indeed work (even my typescript code). :) Must be some rogue npm package which I need to figure out.

@JonRCahill
Copy link

JonRCahill commented Jun 8, 2019

I am unable to get shallow working with the useState hook either. Maybe it is because I am using react-native?

My code is as follows:

import React, { useState } from "react";
import { View, Button } from "react-native";
import { shallow } from "enzyme";

const Example = function({ button2Press }) {
const [name, setName] = useState("");

  return (
    <View>
      <Button title="Button 1" onPress={() => setName("Hello")} />
      <Button title="Button 2" onPress={() => button2Press(name)} />
    </View>
  );
};

describe("Example", () => {
  it("updates the state", () => {
    const button2Press = jest.fn();
    const wrapper = shallow(<Example button2Press={button2Press} />)
    const button1 = wrapper.findWhere(node => node.prop("title") === "Button 1")
                        .first();
    const button2 = wrapper.findWhere(node => node.prop("title") === "Button 2")
                        .first();

    button1.props().onPress();
    button2.props().onPress();

    expect(button2Press).toHaveBeenCalledWith("Hello");
  });
});

The error I get is that button2Press is always called with an empty string rather then what is being set in state.

I am using the dependencies:

  • react 16.8.6
  • react-native 0.59.9
  • enzyme 3.9.0
  • enzyme-adapter-react-16 1.13.1

@ljharb
Copy link
Member

ljharb commented Jun 8, 2019

@BobaFaux update to enzyme v3.10 and try again. Either way tho, this is a PR to add useState support, so it's likely that until this is merged, it won't work.

@chenesan
Copy link
Contributor Author

chenesan commented Jun 9, 2019

@BobaFaux Try to call update() after the first button pressed and find the button2 again:

const button1 = wrapper.findWhere(node => node.prop('title') === 'Button 1')
        .first();
      button1.props().onPress();
      wrapper.update();
      const button2 = wrapper.findWhere(node => node.prop('title') === 'Button 2')
        .first();
      button2.props().onPress(); 
      wrapper.update();
      expect(button2Press).toHaveBeenCalledWith("Hello");

@JonRCahill
Copy link

@chenesan thanks that worked for me. I had tried the wrapper.update(); before but my problem was then finding the element again after update is called.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rebased this on top of a commit that adds a new way to share hook tests across shallow and mount. cc @chenesan for final tweaks to get this in

});
});

// todo: enable when useEffect works in the shallow renderer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the github issue this is linked to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

packages/enzyme-test-suite/test/shared/hooks/custom.jsx Outdated Show resolved Hide resolved
);
};

// TODO; useContext seems to not work in the shallow renderer's initial render
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we file a bug on react about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems not a bug in react side -- I've tried to write a test to render text with the context initial value and that works in shallow. I think it's more about the bug mentioned in #2008 (comment) . But I don't know where the related issue is (e.g. does enzyme has supported .dive() into inner component to get the value from outside provider in shallow?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, ideally that is supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a bug. It looks like even we wrap a Provider outside UiComponent, the context value read by useContext still does not change

});
});

// TODO: useContext: enable when shallow dive supports createContext
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and also add the link here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

} from '../../_helpers/react-compat';

export default function describeUseEffect({
hasHooks,
Wrap,
isShallow,
}) {
describeIf(hasHooks, 'hooks: useEffect', () => {
// TODO: enable when the shallow renderer fixes its bug
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have an issue link here?

expect(setDocumentTitle.args).to.deep.equal([[expectedCountString(0)]]);
});

// TODO: useEffect fixme
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this broken on mount?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not broken, just need a .act wrap; I'd fix this with .simulate .

);
}

// TODO: fixme when useEffect works in the shallow renderer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's link the issue here too

packages/enzyme-test-suite/test/shared/hooks/useState.jsx Outdated Show resolved Hide resolved
@chenesan
Copy link
Contributor Author

I think those todos are not from this PR originally but the rebased code in #2041 ; I'll look into them to make progress of #2011, though.

@chenesan
Copy link
Contributor Author

@ljharb Just responsed all the comments and also fixed the lint issues. could be reviewed again :-)

@chenesan chenesan force-pushed the react-hooks branch 2 times, most recently from b458b99 to aae8ad1 Compare June 11, 2019 06:47
@chenesan
Copy link
Contributor Author

It's strange the the statement in describe of useContext test is still running under React v16.2 even though we have describeIf(hasHook, ...) check, which cause ci failed. I just moved common logic outside of it() into it().

@ljharb
Copy link
Member

ljharb commented Jun 11, 2019

@chenesan the describe bodies run even if they're skipped, so they still have to work in all versions.

@ljharb ljharb changed the title [WIP] Support React hooks useState [Tests] add hooks tests Jun 11, 2019
@ljharb ljharb merged commit d9fa3f0 into enzymejs:master Jun 11, 2019
ljharb added a commit that referenced this pull request Jun 11, 2019
…react-hooks

[Hooks Testing] Added Test cases for useState, useEffects & customHooks
@rodoabad
Copy link

Hey @chenesan were you able to get this to work for shallow.

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

Successfully merging this pull request may close these issues.