-
Notifications
You must be signed in to change notification settings - Fork 4.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
Switch away from using enzyme.mount in tests (viewport/test/with-viewport-match.js) #7828
Switch away from using enzyme.mount in tests (viewport/test/with-viewport-match.js) #7828
Conversation
This switches all tests in `viewport/test/with-viewport-match.js` from using enzyme.mount to `React.TestUtils`. This is because `enzyme` does not fully support React 16.3+ (and movement to do so is really slow). This will fix issues with breakage due to the enzyme incompatibility as components receive React 16.3+ features (such as `forwardRef` usage in #7557).
|
||
expect( wrapper.find( Component ).props() ).toEqual( { isWide: true } ); | ||
expect( wrapper.root.findByType( ChildComponent ).props.isWide ) |
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 isn't exactly the same check. Can we update it to make sure it doesn't change anything?
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'm sorry I'm not sure exactly what you mean by "Can we update it to make sure it doesn't change anything?". I'm also not sure why you think this isn't the same check. I had to wrap ChildComponent
in the TestComponent
so effectively I'm doing the same test, just doing a different method for getting the component to test. This same method will apply when HOCs are returning a forwardRef wrapped component (in 7557).
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.
Another alternative here would be to use TestUtils
to do a full mount into the DOM and then traverse actual HTMLElement nodes to verify expected results? But I'm not sure that is any better than what is happening 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.
expect( wrapper.root.findByType( ChildComponent ).props ).toEqual( { isWide: true });
This is what I thought about. Does ref
causes any issues 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.
I ensured these tests passed against 7557 (while preserving the intent of the test). Then ensured the same tests passed against master (if they didn't pass against master then I knew there was a problem).
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's minor, we can skip 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.
LGTM 👍
Description
This switches all tests in
viewport/test/with-viewport-match.js
from using enzyme.mount toReact.TestUtils
. This is becauseenzyme
does not fully support React 16.3+ (and movement to do so is really slow). This will fix issues with breakage due to the enzyme incompatibility as components receive React 16.3+ features (such asforwardRef
usage in #7557).Checklist: