-
Notifications
You must be signed in to change notification settings - Fork 2.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
Upgrades React #4107
Upgrades React #4107
Conversation
import { renderIntoDocument, act, Simulate } from 'react-dom/test-utils'; | ||
import { render, findDOMNode } from 'react-dom'; | ||
import { fireEvent, act, render } from '@testing-library/react'; | ||
import { Simulate } from 'react-dom/test-utils'; |
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.
Simulate is still being used?
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 for taking the time to review @heath-freenome. There are two cases where fireEvent did not behave as I had expected and that's why I continued to use Simulate.
-
In the case of a select component with the option to select multiple values. I simply couldn't figure out how to set the selected attribute on more than one option element.
-
In the case of an input field of type number. fireEvent will not set a value to a non numeric.
I'm happy to take any suggestions.
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.
Just wanted to follow up on the React upgrade. I've been developing quite a bit on top of the React 18 library and haven't had any major issues. There are peer dependency errors that I've come across in libraries that are fixed to React 17. You'll also see the following warning:
Warning: ReactDOM.render is no longer supported in React 18.
@@ -1,5 +1,6 @@ | |||
import { expect } from 'chai'; | |||
import { Simulate, act } from 'react-dom/test-utils'; | |||
import { Simulate } from 'react-dom/test-utils'; |
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.
Where is Simulate
still being used?
], | ||
}, | ||
act(() => { | ||
Simulate.change(node.querySelector('.field select'), { |
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 you make this use fireEvent
instead?
Simulate.change(node.querySelector('.field select'), { | |
fireEvent.change(node.querySelector('.field select'), { |
@orenf your tests are failing and this needs a rebase. Are you able to fix these? |
@heath-freenome , you might be missing the second commit that I added. The second commit will fix the failing test that I believe you're referencing. I just confirmed that all the tests are passing. |
@orenf The github pipelines are failing still. Can you please fix |
Thanks @heath-freenome , I looked the error raised by Node 18.x build. I've not been able to reproduce the error. I'll need some time to figure out the reason for the discrepancies. |
@orenf Have you figured out how to fix the build for your PR yet? |
I haven't had a chance to review this issue and I likely won't have the opportunity to do so this week either. I do, however, keep rebasing against the main branch. |
@heath-freenome , the build appears to be passing now. |
Upgrades React to version 18
Adds @testing-library/react
Updates tests accordingly