-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Testing: Upgrade React and Enzyme to the latest version #3079
Conversation
@@ -18,6 +18,7 @@ const { UP, DOWN } = keycodes; | |||
describe( 'NavigableMenu', () => { | |||
// Skipping this this because the `isVisible` check in utils/focus/tabbable.js always returns false in tests | |||
// Probbably a jsdom issue | |||
// eslint-disable-next-line jest/no-disabled-tests | |||
it.skip( 'should navigate by keypresses', () => { |
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 will look into it later. I added this eslint rule to hide warning.
@@ -290,8 +290,8 @@ export class InserterMenu extends Component { | |||
onClick={ this.selectBlock( block.name ) } | |||
ref={ this.bindReferenceNode( block.name ) } | |||
tabIndex="-1" | |||
onMouseEnter={ ! disabled && this.props.showInsertionPoint } | |||
onMouseLeave={ ! disabled && this.props.hideInsertionPoint } | |||
onMouseEnter={ ! disabled ? this.props.showInsertionPoint : null } |
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.
This change fixes warnings in the InserterMenu component:
Warning: Expected
onMouseLeave
listener to be a function, instead got a value ofboolean
type.
Warning: ExpectedonMouseEnter
listener to be a function, instead got a value ofboolean
type.
e3e6eb2
to
54e3057
Compare
afterEach( () => { | ||
expect( spyError ).not.toHaveBeenCalled(); | ||
expect( spyWarn ).not.toHaveBeenCalled(); | ||
} ); |
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.
❤️
@@ -28,12 +28,12 @@ | |||
"moment-timezone": "0.5.13", | |||
"mousetrap": "1.6.1", | |||
"prop-types": "15.5.10", | |||
"react": "15.6.1", | |||
"react": "16.0.0", |
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.
🎉
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.
you did it 👍
Noting that many of our dependencies define a React peer in the 15.x line, so warnings are logged when installing a new version of a package:
I imagine at least some of these should have updated versions available which define React 16 as a valid peer. |
Yes, we can upgrade them after release v1.5. We are using the latest React and React DOM for some time with the bundled code so it seemed like an easier migration path. I can open a follow-up PR on Monday. Thanks for catching this. @youknowriad left a note about this in the preceding PR but I forget to copy it. |
Description
Another attempt to upgrade React and Enzyme to the latest version. We started exploration in #2813 and this PR should finally make it happen. All tests work as expected.
As part of this diff I also improved the behavior of throwing an error when test executes
console.error
. I provided new logic which uses spies andexpect
call to validate if test should fail. New output looks as follow:It's much easier to read and it properly outputs all warnings and errors.
There are still 2 skipped tests. I will take a closer look at them later and decide if we can fix them or they should be removed.
How Has This Been Tested?
Travis should be happy.
Run
npm test
locally.To test new behavior around
console.error
andconsole.warn
simply put such call in one of the tests.Checklist: