refactor: refactor after review part 1

openedx · Sep 8, 2023 · 7097832 · 7097832
1 parent 8236a5e
commit 7097832
Show file tree

Hide file tree

Showing 119 changed files with 1,033 additions and 1,668 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -95,21 +95,18 @@
     "@testing-library/react": "^12.1.4",
     "@testing-library/react-hooks": "^8.0.1",
     "@testing-library/user-event": "^13.5.0",
-    "@types/enzyme": "^3.10.12",
     "@types/jest": "^27.5.0",
     "@types/react": "17.0.0",
     "@types/react-dom": "17.0.11",
     "@types/react-test-renderer": "^18.0.0",
     "@types/uuid": "^9.0.0",
     "@typescript-eslint/eslint-plugin": "^5.22.0",
     "@typescript-eslint/parser": "^5.22.0",
-    "@wojtekmaj/enzyme-adapter-react-17": "^0.6.7",
     "axios": "^0.27.2",
     "axios-mock-adapter": "^1.21.1",
     "babel-jest": "^28.1.2",
     "babel-loader": "^8.2.4",
     "commander": "^9.3.0",
-    "enzyme": "^3.9.0",
     "eslint": "8.18.0",
     "eslint-config-airbnb": "19.0.4",
     "eslint-config-airbnb-typescript": "^17.0.0",
@@ -142,7 +139,7 @@
       "^icons/index.js$": "babel-jest",
       "^.+\\.tsx?$": "ts-jest"
     },
-    "setupFiles": [
+    "setupFilesAfterEnv": [
       "./src/setupTest.js"
     ],
     "moduleNameMapper": {

diff --git a/src/Annotation/Annotation.test.jsx b/src/Annotation/Annotation.test.jsx
@@ -1,7 +1,7 @@
 import React from 'react';
 import renderer from 'react-test-renderer';
 import { render, screen } from '@testing-library/react';
-import Annotation from './index';
+import Annotation from '.';
 
 const VARIANTS = ['error', 'success', 'warning', 'light', 'dark'];
 const ARROW_PLACEMENTS = ['top', 'right', 'bottom', 'left'];

diff --git a/src/Breadcrumb/Breadcrumb.test.jsx b/src/Breadcrumb/Breadcrumb.test.jsx
@@ -2,89 +2,75 @@ import React from 'react';
 import { render, screen } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 
-import Breadcrumb from './index';
+import Breadcrumb from '.';
 
 const baseProps = {
   links: [
     {
       label: 'Link 1',
       href: '/link-1',
-      'data-testid': 'link',
     },
     {
       label: 'Link 2',
       href: '/link-2',
-      'data-testid': 'link',
     },
     {
       label: 'Link 3',
       href: '/link-3',
-      'data-testid': 'link',
     },
   ],
-  'data-testid': 'breadcrumb',
 };
 
 describe('<Breadcrumb />', () => {
   it('renders with just links', () => {
     render(<Breadcrumb {...baseProps} />);
-    const breadcrumb = screen.getByTestId('breadcrumb');
-    const list = breadcrumb.querySelectorAll('ol li');
-    expect(list.length).toBe(5);
-    expect(screen.getAllByTestId('link').length).toBe(3);
+    expect(screen.queryAllByRole('list').length).toBe(1);
+    expect(screen.queryAllByRole('listitem').length).toBe(baseProps.links.length);
   });
 
   it('renders with links and active label', () => {
     const label = 'Current Page';
     render(<Breadcrumb {...baseProps} activeLabel={label} />);
+    const list = screen.queryAllByRole('list');
+    const listItems = screen.queryAllByRole('listitem');
 
-    const breadcrumb = screen.getByTestId('breadcrumb');
-    const list = breadcrumb.querySelectorAll('ol li');
-    expect(list.length).toBe(7);
-    expect(screen.getAllByTestId('link').length).toBe(3);
-    expect(list[list.length - 1].textContent).toBe(label);
+    expect(list.length).toBe(1);
+    expect(listItems.length).toBe(baseProps.links.length + 1);
+    expect(listItems[listItems.length - 1].textContent).toBe(label);
   });
 
   it('renders custom spacer', () => {
     render(
-      <Breadcrumb
-        {...baseProps}
-        spacer={<span data-testid="custom-spacer">/</span>}
-      />,
+      <Breadcrumb {...baseProps} spacer={<span>/</span>} />,
     );
-
-    const breadcrumb = screen.getByTestId('breadcrumb');
-    const list = breadcrumb.querySelectorAll('ol li');
-    expect(list.length).toBe(5);
-    expect(screen.getAllByTestId('link').length).toBe(3);
-    expect(screen.getAllByTestId('custom-spacer').length).toBe(2);
+    const listItems = screen.queryAllByRole('listitem');
+    expect(listItems.length).toBe(baseProps.links.length);
+    expect(screen.getAllByRole('presentation').length).toBe(2);
   });
 
-  it('fires the passed in click handler', async () => {
+  it('fires the passed in click handler', () => {
     const clickHandler = jest.fn();
     render(<Breadcrumb {...baseProps} clickHandler={clickHandler} />);
 
-    const links = screen.getAllByTestId('link');
-    expect(links.length).toBe(3);
+    const listItems = screen.queryAllByRole('listitem');
+    const links = screen.queryAllByRole('link');
+    expect(listItems.length).toBe(baseProps.links.length);
 
-    await userEvent.click(links[0]);
+    userEvent.click(links[0]);
     expect(clickHandler).toHaveBeenCalled();
   });
 
   it('renders in mobile view', () => {
     render(<Breadcrumb {...baseProps} isMobile />);
-    const breadcrumb = screen.getByTestId('breadcrumb');
-
-    const list = breadcrumb.querySelector('ol');
-    const listItems = breadcrumb.querySelectorAll('ol li');
-    expect(listItems.length).toBe(2);
+    const list = screen.getByRole('list');
+    const listItems = screen.getAllByRole('listitem');
+    expect(listItems.length).toBe(1);
     expect(list.className).toContain('is-mobile');
   });
 
   it('renders links as custom elements', () => {
     render(<Breadcrumb {...baseProps} linkAs="div" />);
-    const breadcrumb = screen.getByTestId('breadcrumb');
-    const list = breadcrumb.querySelector('ol');
+    const list = screen.getByRole('list');
 
     const anchors = list.querySelectorAll('a');
     expect(anchors.length).toBe(0);
@@ -99,14 +85,13 @@ describe('<Breadcrumb />', () => {
       href: '/link-1',
       className: 'my-link',
       target: '_blank',
-      'data-testid': 'link',
     };
 
     render(<Breadcrumb links={[linkProps]} />);
 
-    const links = screen.getAllByTestId('link');
-    expect(links[0].className).toContain('my-link');
-    expect(links[0].getAttribute('target')).toBe('_blank');
-    expect(links[0].getAttribute('href')).toBe('/link-1');
+    const links = screen.getByRole('link');
+    expect(links.className).toContain('my-link');
+    expect(links.getAttribute('target')).toBe('_blank');
+    expect(links.getAttribute('href')).toBe('/link-1');
   });
 });
diff --git a/src/Button/Button.test.jsx b/src/Button/Button.test.jsx
@@ -4,7 +4,7 @@ import userEvent from '@testing-library/user-event';
 import renderer from 'react-test-renderer';
 
 import { Close } from '../../icons';
-import Button from './index';
+import Button from '.';
 import Hyperlink from '../Hyperlink';
 
 describe('<Button />', () => {
@@ -71,24 +71,25 @@ describe('<Button />', () => {
         expect(tree).toMatchSnapshot();
       });
 
-      test('cannot click if disabled', () => {
+      test('cannot click if disabled', async () => {
         const onClick = jest.fn();
         render(<Button as="a" href="https://edx.org" disabled onClick={onClick}>Button</Button>);
         const link = screen.getByRole('link');
-        userEvent.click(link);
+        await userEvent.click(link);
         expect(onClick).not.toHaveBeenCalled();
       });
 
-      test('invalid disabled if without href', () => {
+      test('invalid disabled if without href', async () => {
         const onClick = jest.fn();
-        render(<Button as="a" disabled onClick={onClick} data-testid="button">Button</Button>);
-        const link = screen.getAllByTestId('button');
-        userEvent.click(link[0]);
+        const { rerender } = render(<Button as="a" disabled onClick={onClick}>Button</Button>);
+        const link = screen.getByText('Button');
+        await userEvent.click(link);
         expect(onClick).toHaveBeenCalled();
+        onClick.mockClear();
 
-        render(<Button as="a" href="" disabled onClick={onClick} data-testid="button">Button</Button>);
-        const emptyHrefLink = screen.getAllByTestId('button');
-        userEvent.click(emptyHrefLink[1]);
+        rerender(<Button as="a" href="" disabled onClick={onClick}>Button</Button>);
+        const emptyHrefLink = screen.getByRole('link', { name: 'Button' });
+        await userEvent.click(emptyHrefLink);
         expect(onClick).toHaveBeenCalled();
       });
 

diff --git a/src/Button/deprecated/Button.test.jsx b/src/Button/deprecated/Button.test.jsx
@@ -1,8 +1,7 @@
 import React from 'react';
 import { render } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
-import '@testing-library/jest-dom/extend-expect';
-import Button from './index';
+import Button from '.';
 
 const defaultProps = {
   label: 'Click me!',

diff --git a/src/Card/CardBody.jsx b/src/Card/CardBody.jsx
@@ -2,8 +2,8 @@ import React from 'react';
 import PropTypes from 'prop-types';
 import classNames from 'classnames';
 
-const CardBody = React.forwardRef(({ className, children }, ref) => (
-  <div className={classNames('pgn__card-body', className)} ref={ref}>
+const CardBody = React.forwardRef(({ className, children, ...rest }, ref) => (
+  <div className={classNames('pgn__card-body', className)} ref={ref} {...rest}>
     {children}
   </div>
 ));

diff --git a/src/Card/tests/CardBody.test.jsx b/src/Card/tests/CardBody.test.jsx
@@ -1,5 +1,5 @@
 import React from 'react';
-import { render } from '@testing-library/react';
+import { render, screen } from '@testing-library/react';
 import renderer from 'react-test-renderer';
 import CardBody from '../CardBody';
 import Card from '..';
@@ -13,21 +13,21 @@ describe('correct rendering', () => {
   });
 
   it('renders correct base className', () => {
-    const { container } = render(<CardBody />);
-    const body = container.querySelector('div');
+    render(<CardBody data-testid="card-body" />);
+    const body = screen.getByTestId('card-body');
     expect(body.classList.contains('pgn__card-body')).toBe(true);
   });
 
   it('renders correct variant', () => {
-    const { container } = render(<Card variant="dark" />);
-    const wrapper = container.querySelector('div');
+    render(<Card variant="dark" data-testid="card" />);
+    const wrapper = screen.getByTestId('card');
     expect(wrapper.classList.contains('pgn__card-dark')).toBe(true);
   });
 
   it('renders body with custom className', () => {
     const className = 'my-class-name';
-    const { container } = render(<CardBody className={className} />);
-    const body = container.querySelector('div');
+    render(<CardBody className={className} data-testid="card-body" />);
+    const body = screen.getByTestId('card-body');
     expect(body.classList.contains(className)).toBe(true);
   });
 });
diff --git a/src/Card/tests/CardImageCap.test.jsx b/src/Card/tests/CardImageCap.test.jsx
@@ -1,7 +1,6 @@
 import React from 'react';
 import renderer from 'react-test-renderer';
 import { render, fireEvent, screen } from '@testing-library/react';
-import '@testing-library/jest-dom';
 import CardImageCap from '../CardImageCap';
 import CardContext from '../CardContext';
 

diff --git a/src/CheckBox/CheckBox.test.jsx b/src/CheckBox/CheckBox.test.jsx
@@ -1,21 +1,20 @@
 import React from 'react';
-import { render, fireEvent } from '@testing-library/react';
-import '@testing-library/jest-dom/extend-expect';
-import CheckBox from './index';
+import { render, fireEvent, screen } from '@testing-library/react';
+import CheckBox from '.';
 
 describe('<CheckBox />', () => {
   it('attributes are set correctly', () => {
-    const { getByLabelText } = render(<CheckBox name="checkbox" label="check me out!" />);
-    const checkbox = getByLabelText('check me out!');
+    render(<CheckBox name="checkbox" label="check me out!" />);
+    const checkbox = screen.getByLabelText('check me out!');
     expect(checkbox).toBeInTheDocument();
     expect(checkbox).toHaveAttribute('type', 'checkbox');
     expect(checkbox).not.toBeChecked();
     expect(checkbox).toHaveAttribute('aria-checked', 'false');
   });
 
   it('aria-label changes after click', () => {
-    const { getByLabelText } = render(<CheckBox name="checkbox" label="check me out!" />);
-    const checkbox = getByLabelText('check me out!');
+    render(<CheckBox name="checkbox" label="check me out!" />);
+    const checkbox = screen.getByLabelText('check me out!');
     expect(checkbox).toHaveAttribute('aria-checked', 'false');
     fireEvent.click(checkbox);
     expect(checkbox).toHaveAttribute('aria-checked', 'true');
@@ -25,8 +24,8 @@ describe('<CheckBox />', () => {
 
   it('check that callback function is triggered when clicked', () => {
     const onChangeSpy = jest.fn();
-    const { getByLabelText } = render(<CheckBox name="checkbox" label="check me out!" onChange={onChangeSpy} />);
-    const checkbox = getByLabelText('check me out!');
+    render(<CheckBox name="checkbox" label="check me out!" onChange={onChangeSpy} />);
+    const checkbox = screen.getByLabelText('check me out!');
     expect(onChangeSpy).toHaveBeenCalledTimes(0);
     fireEvent.click(checkbox);
     expect(onChangeSpy).toHaveBeenCalledTimes(1);
@@ -37,8 +36,8 @@ describe('<CheckBox />', () => {
   });
 
   it('checks if start state can be set to checked', () => {
-    const { getByLabelText } = render(<CheckBox name="checkbox" label="I start checked" checked />);
-    const checkbox = getByLabelText('I start checked');
+    render(<CheckBox name="checkbox" label="I start checked" checked />);
+    const checkbox = screen.getByLabelText('I start checked');
     expect(checkbox).toBeChecked();
     expect(checkbox).toHaveAttribute('aria-checked', 'true');
     fireEvent.click(checkbox);
@@ -47,16 +46,16 @@ describe('<CheckBox />', () => {
   });
 
   it('checkbox can be disabled', () => {
-    const { getByLabelText } = render(<CheckBox name="checkbox" label="I am disabled" disabled />);
-    const checkbox = getByLabelText('I am disabled');
+    render(<CheckBox name="checkbox" label="I am disabled" disabled />);
+    const checkbox = screen.getByLabelText('I am disabled');
     expect(checkbox).toBeDisabled();
     fireEvent.click(checkbox);
     expect(checkbox).toBeDisabled();
   });
 
   it('overrides state value when props value changes', () => {
-    const { getByLabelText, rerender } = render(<CheckBox name="checkbox" label="I start checked" checked />);
-    const checkbox = getByLabelText('I start checked');
+    const { rerender } = render(<CheckBox name="checkbox" label="I start checked" checked />);
+    const checkbox = screen.getByLabelText('I start checked');
     expect(checkbox).toBeChecked();
     rerender(<CheckBox name="checkbox" label="I start checked" checked={false} />);
     expect(checkbox).not.toBeChecked();

diff --git a/src/CheckBoxGroup/CheckBoxGroup.test.jsx b/src/CheckBoxGroup/CheckBoxGroup.test.jsx
@@ -1,7 +1,6 @@
 import React from 'react';
 import { render } from '@testing-library/react';
-import '@testing-library/jest-dom/extend-expect';
-import CheckBoxGroup from './index';
+import CheckBoxGroup from '.';
 import CheckBox from '../CheckBox';
 
 describe('<CheckBoxGroup />', () => {