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

Migrate enzyme tests to testing-library #52

Merged
merged 6 commits into from
Apr 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
"src/index.d.ts"
],
"scripts": {
"lint": "eslint ./src ./examples",
"lint": "eslint ./src ./tests ./examples",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curiously the test files weren't checked by eslint.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, interesting. Thanks for adding them!

"test": "yarn lint && yarn test:only",
"test:only": "jest --no-cache --verbose --coverage",
"test:dev": "jest --watchAll --no-cache --verbose --coverage",
Expand Down Expand Up @@ -61,19 +61,23 @@
"@babel/preset-env": "^7.16.11",
"@babel/preset-react": "^7.16.7",
"@babel/preset-stage-2": "^7.8.3",
"@wojtekmaj/enzyme-adapter-react-17": "^0.6.7",
"@testing-library/dom": "^8.13.0",
"@testing-library/jest-dom": "^5.16.4",
"@testing-library/react": "^13.0.0",
"@testing-library/user-event": "^14.1.0",
"babel-jest": "~27.5.1",
"babel-loader": "~8.2.4",
"coveralls": "~3.1.1",
"cross-env": "~7.0.3",
"css-loader": "~6.7.1",
"enzyme": "~3.11.0",
"enzyme-to-json": "~3.6.2",
"eslint": "~8.11.0",
"eslint-config-vkbansal": "~5.2.1",
"eslint-import-resolver-webpack": "~0.13.2",
"eslint-plugin-import": "~2.25.4",
"eslint-plugin-jest": "^26.1.4",
"eslint-plugin-jest-dom": "^4.0.1",
"eslint-plugin-react": "~7.29.4",
"eslint-plugin-testing-library": "^5.2.1",
"history": "~5.3.0",
"html-webpack-plugin": "~5.5.0",
"http-server": "~14.1.0",
Expand All @@ -83,19 +87,15 @@
"react": "~18.0.0",
"react-dom": "~18.0.0",
"react-router-dom": "~6.3.0",
"react-test-renderer": "~18.0.0",
"rimraf": "~3.0.0",
"style-loader": "~3.3.1",
"webpack": "5.72.0",
"webpack-cli": "^4.9.2"
},
"jest": {
"setupFiles": [
"setupFilesAfterEnv": [
"<rootDir>/tests/.setup.js"
],
"snapshotSerializers": [
"enzyme-to-json/serializer"
],
"roots": [
"<rootDir>/tests"
],
Expand Down
19 changes: 12 additions & 7 deletions src/AbstractMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,24 +25,29 @@ export default class AbstractMenu extends Component {
return;
}

switch (e.keyCode) {
case 37: // left arrow
case 27: // escape
switch (e.key) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me. I'm not really worried about IE, but good to include them too :)

case 'ArrowLeft': // left arrow
case 'Left': // IE specific value
case 'Escape': // escape
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess some of the comments are now redundant, but I don't mind keeping them.

case 'Esc': // IE specific value
e.preventDefault();
this.hideMenu(e);
break;
case 38: // up arrow
case 'ArrowUp': // up arrow
case 'Up': // IE specific value
e.preventDefault();
this.selectChildren(true);
break;
case 40: // down arrow
case 'ArrowDown': // down arrow
case 'Down': // IE specific value
e.preventDefault();
this.selectChildren(false);
break;
case 39: // right arrow
case 'ArrowRight': // right arrow
case 'Right': // IE specific value
this.tryToOpenSubMenu(e);
break;
case 13: // enter
case 'Enter': // enter
e.preventDefault();
this.tryToOpenSubMenu(e);
{
Expand Down
2 changes: 1 addition & 1 deletion src/ContextMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ export default class ContextMenu extends AbstractMenu {
// Disabling this rule for more consistency.
/* eslint-disable-next-line class-methods-use-this */
hideMenu = (e) => {
if (e.keyCode === 27 || e.keyCode === 13) { // ECS or enter
if (e.key === 'Escape' || e.key === 'Esc' || e.key === 'Enter') {
hideMenu();
}
};
Expand Down
21 changes: 16 additions & 5 deletions tests/.eslintrc.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,17 @@
---
env:
jest: true
rules:
prefer-arrow-callback: 0
no-mixed-requires: 0
plugins:
- jest
- jest-dom
- testing-library
extends:
- plugin:jest/recommended
- plugin:jest-dom/recommended
- plugin:testing-library/react
env:
jest: true
rules:
prefer-arrow-callback: 0
no-mixed-requires: 0
# This disallows using direct Node properties (eg: firstChild), but we have
# legitimate uses:
testing-library/no-node-access: 0
21 changes: 6 additions & 15 deletions tests/.setup.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,8 @@
const jsdom = require('jsdom');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I feel like these tests are really not enough. But this is something for the future.

// Importing this here makes it work everywhere.
import '@testing-library/jest-dom';

const documentHTML = '<!doctype html><html><body><div id="root"></div></body></html>';
const dom = new jsdom.JSDOM(documentHTML);
global.document = dom.window.document;
global.window = dom.window;
global.window.resizeTo = (width, height) => {
global.window.innerWidth = width || global.window.innerWidth;
global.window.innerHeight = width || global.window.innerHeight;
global.window.dispatchEvent(new Event('resize'));
window.resizeTo = (width, height) => {
window.innerWidth = width || window.innerWidth;
window.innerHeight = width || window.innerHeight;
window.dispatchEvent(new Event('resize'));
};
global.window.requestAnimationFrame = jest.fn();

const Enzyme = require('enzyme');
const Adapter = require('@wojtekmaj/enzyme-adapter-react-17');

Enzyme.configure({ adapter: new Adapter() });
Loading