-
-
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
[Hooks Testing] Added Test cases for useState, useEffects & customHooks #2041
Conversation
@ljharb : I know I have written test cases for ReactWrapper only. I will copy paste in shallowWrapper as its compatible , wanted to get review comments and then copy to ShallowWrapper spec file. |
[Update]: @ljharb : I have
|
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.
Thanks! Some initial feedback.
@ljharb Incorporated all comments/ suggested changes except one, opened a discussion. |
@ljharb : Please review one more time as incorporated changes |
Added test cases for useContext & useReducer |
40cc703
to
0a17404
Compare
@pgangwani What I'd love to see is all the passing tests merged, and the failing tests remaining in an open PR, and then hopefully someone (not necessarily me) can submit a PR to fix them. |
@ljharb Did we get any update from facebook on shallowRender? Meanwhile I will try to re-rrun & update here, what is failing & why, then you may suggest me if I need to create seperate passing test cases vs failing or may be comment/skip for you ( anyone ) to fix. Is that ok ? |
My understanding is that useEffect shouldn't work on shallow (so we can skip those tests for now) but I think the other hooks should. |
@ljharb : I have skipped test cases failing and added TODO comments. Can you please review and then will raise one more PR by undoing the last 2 commits. Is that what you expected ? |
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.
So, what i'd like to end up with is:
- no code remains commented out
- no
console.log
s remain - no noop matchers are used (ie, every expectation must end in a function call)
- any tests that fail, are explicitly skipped with a detailed comment describing when they fail, and if possible, why they fail and how they might be fixed
- line breaks between all sibling
it
s anddescribe
s, but not directly inside a describe.
and finally, although I'm happy to take care of it, things rebased down to a minimal number of atomic commits, but retaining authorship - ie, each person should keep at least one commit in the final branch.
@ljharb : Sorry, I was away for bit as I didn't hear on it . I will definitely resolve the review comments. I need one favor from you. Could you please either merge or comment if anything outstanding after I resolve comment? |
@pgangwani yes, if you can update the PR into separate commits that have passing tests, and another batch that has failing tests, and then ping me, I'll merge it. |
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 is the status of this PR? where is the documentation about the implementation? |
It’s merged, and only affects enzyme’s own tests, so there’s no documentation expected or needed. |
@ljharb @chenesan : Please review and add comments as this is my first PR. I expect some rework. Otherwise, cherrypick the commits as you said.