-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[Tests] add hooks tests #2008
Conversation
Would you consider adding useRef to the shortlist? |
expect(wrapper.find('div').length).to.equal(1); | ||
expect(wrapper.find('div').text()).to.equal('0'); | ||
}); | ||
it('handles setState returned from useState', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it('handles setState returned from useState', () => { | |
it('handles setState returned from useState', () => { |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
.
Hm... Any clue how you'll get this working? The upstream shallow renderer doesn't seem to support this |
Hi ... I just upgraded to React v16.8.1 and I started having these warnings when testing a Hooks based component.
Is this something that should be handled by enzyme? I didn't have these warnings when I was at the Hooks alphas |
@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. |
Thx for the quick answer @ljharb ... I wish I could get rid of the warnings ... Right now I'm trying to use Just for the record, I'm not actually testing the hooks. My component |
@perrin4869 I'd try to see what I can do in react shallow renderer. |
|
It seems like something enzyme could/should perhaps abstract away from the user? |
9f3439a
to
dee77bd
Compare
I think that the failed shallow test is related to facebook/react#14840 . After facebook/react#14841 fixes the issue the test should pass. |
@chenesan we should probably merge the |
@ljharb I would try to work on (Update: Send PR in #2034) |
Hi @pgangwani glad to hear this. What could I do to help you? |
I want to start with test cases.
…On Wed 6 Mar, 2019, 8:25 PM Yi-Shan, Chen, ***@***.***> wrote:
Hi @pgangwani <https://github.com/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
<https://github.com/airbnb/enzyme/issues?q=is%3Aissue+is%3Aopen+label%3A%22help+wanted%22>.
Or maybe you could help add some test cases for react hooks
<#2011>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2008 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ANpzLENY29DZcF6jBCQ9l20sFeOoMSlmks5vT9b-gaJpZM4am4xH>
.
|
@pgangwani please post a link to a branch on #2029, and i'll cherry-pick it into that PR. |
@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 |
Hi @pgangwani there is gitter chatroom :-) |
@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 good to see #2041 ;) |
@Acesmndr I believe that works with shallow after [email protected] (See facebook/react#15120) |
@chenesan I tried but even with 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: '[email protected]',
},
});
expect(container.find('input').prop('value')).toEqual('[email protected]');
// console.log(container.find('input').props());
});
});
}); And the error it's throwing is that email isn't updated even though setEmail is called. |
@Acesmndr
|
@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. |
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:
The error I get is that I am using the dependencies:
|
@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. |
@BobaFaux Try to call 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"); |
@chenesan thanks that worked for me. I had tried the |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
); | ||
}; | ||
|
||
// TODO; useContext seems to not work in the shallow renderer's initial render |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #2008 (comment)
} from '../../_helpers/react-compat'; | ||
|
||
export default function describeUseEffect({ | ||
hasHooks, | ||
Wrap, | ||
isShallow, | ||
}) { | ||
describeIf(hasHooks, 'hooks: useEffect', () => { | ||
// TODO: enable when the shallow renderer fixes its bug |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
@ljharb Just responsed all the comments and also fixed the lint issues. could be reviewed again :-) |
b458b99
to
aae8ad1
Compare
It's strange the the statement in describe of |
@chenesan the describe bodies run even if they're skipped, so they still have to work in all versions. |
…react-hooks [Hooks Testing] Added Test cases for useState, useEffects & customHooks
Hey @chenesan were you able to get this to work for |
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 inshallow
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)