Migrate enzyme tests to testing-library #52
Conversation
✅ Deploy Preview for firefox-devtools-react-contextmenu ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| @@ -19,7 +19,7 @@ | |||
| "src/index.d.ts" | |||
| ], | |||
| "scripts": { | |||
| "lint": "eslint ./src ./examples", | |||
| "lint": "eslint ./src ./tests ./examples", | |||
There was a problem hiding this comment.
Curiously the test files weren't checked by eslint.
There was a problem hiding this comment.
Ah, interesting. Thanks for adding them!
| switch (e.keyCode) { | ||
| case 37: // left arrow | ||
| case 27: // escape | ||
| switch (e.key) { |
There was a problem hiding this comment.
The user-event library doesn't use keyCode in the event it sends, and this is on purpose. Therefore I moved to e.key in this PR too. I took care to use the older values that Internet Explorer supports as well. I didn't test IE but I trust MDN on those.
See https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key/Key_Values for the values for this property.
There was a problem hiding this comment.
Works for me. I'm not really worried about IE, but good to include them too :)
| @@ -1,17 +1,8 @@ | |||
| const jsdom = require('jsdom'); | |||
There was a problem hiding this comment.
This commit has the bulk of the changes. The test files aren't perfect but good enough to move us forward. In the future we can try to improve the coverage.
There was a problem hiding this comment.
Yeah, I feel like these tests are really not enough. But this is something for the future.
| @@ -19,7 +19,7 @@ | |||
| "src/index.d.ts" | |||
| ], | |||
| "scripts": { | |||
| "lint": "eslint ./src ./examples", | |||
| "lint": "eslint ./src ./tests ./examples", | |||
There was a problem hiding this comment.
Ah, interesting. Thanks for adding them!
| switch (e.key) { | ||
| case 'ArrowLeft': // left arrow | ||
| case 'Left': // IE specific value | ||
| case 'Escape': // escape |
There was a problem hiding this comment.
I guess some of the comments are now redundant, but I don't mind keeping them.
| switch (e.keyCode) { | ||
| case 37: // left arrow | ||
| case 27: // escape | ||
| switch (e.key) { |
There was a problem hiding this comment.
Works for me. I'm not really worried about IE, but good to include them too :)
| @@ -1,29 +0,0 @@ | |||
|
|
|||
|
|
|||
| import React from 'react'; | |||
There was a problem hiding this comment.
Unlike sub-menu-tests.js, I don't see the new file of this SubMenuWrapper component. Is that intentional or is it because of a slipped git add? :)
There was a problem hiding this comment.
as we discussed, this test file wasn't run before, and is about a component that doesn't seem present anymore.
| @@ -1,17 +1,8 @@ | |||
| const jsdom = require('jsdom'); | |||
There was a problem hiding this comment.
Yeah, I feel like these tests are really not enough. But this is something for the future.
I believe that looking at commits individually should make it easier.