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

[Hooks Testing] Added Test cases for useState, useEffects & customHooks #2041

Merged
merged 7 commits into from
Jun 11, 2019
Merged

[Hooks Testing] Added Test cases for useState, useEffects & customHooks #2041

merged 7 commits into from
Jun 11, 2019

Conversation

pgangwani
Copy link

@ljharb @chenesan : Please review and add comments as this is my first PR. I expect some rework. Otherwise, cherrypick the commits as you said.

@pgangwani pgangwani changed the title Added Test cases for useState, useEffects & customHooks [Hooks Testing] Added Test cases for useState, useEffects & customHooks Mar 8, 2019
@pgangwani
Copy link
Author

@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.

@pgangwani
Copy link
Author

[Update]: @ljharb : I have

  • copied test to shallowWrapper spec
  • Removed mount usage in shallowWrapper spec (review careefully)

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.

Thanks! Some initial feedback.

packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
@pgangwani
Copy link
Author

@ljharb Incorporated all comments/ suggested changes except one, opened a discussion.

@pgangwani
Copy link
Author

@ljharb : Please review one more time as incorporated changes

@pgangwani
Copy link
Author

Added test cases for useContext & useReducer

@pgangwani
Copy link
Author

@ljharb @chenesan : Hi, I took some break and now I am back, may I know whats the status and plan ahead ?

@ljharb
Copy link
Member

ljharb commented Apr 13, 2019

@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.

@pgangwani
Copy link
Author

pgangwani commented Apr 13, 2019

@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 ?

@ljharb
Copy link
Member

ljharb commented Apr 13, 2019

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.

@pgangwani
Copy link
Author

@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 ?

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.

So, what i'd like to end up with is:

  1. no code remains commented out
  2. no console.logs remain
  3. no noop matchers are used (ie, every expectation must end in a function call)
  4. 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
  5. line breaks between all sibling its and describes, 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.

packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
@pgangwani
Copy link
Author

@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?

@ljharb
Copy link
Member

ljharb commented May 22, 2019

@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.

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.

#2041 and #2008 combined look great!

@ljharb ljharb merged commit 081554e into enzymejs:master Jun 11, 2019
@josuevalrob
Copy link

what is the status of this PR? where is the documentation about the implementation?

@ljharb
Copy link
Member

ljharb commented Mar 17, 2021

It’s merged, and only affects enzyme’s own tests, so there’s no documentation expected or needed.

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

Successfully merging this pull request may close these issues.

4 participants