[Hooks Testing] Added Test cases for useState, useEffects & customHooks #2041
Conversation
pgangwani
changed the title
|
@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
|
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
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 ? |
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
So, what i'd like to end up with is:
- no code remains commented out
- no
console.logs 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
its anddescribes, 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. |
|
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. |
4 participants
@ljharb @chenesan : Please review and add comments as this is my first PR. I expect some rework. Otherwise, cherrypick the commits as you said.