feat: migrate enzyme to rtl #915
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,12 +8,12 @@ | |
| ], | ||
| "dependencies": { | ||
| "@edx/brand": "npm:@openedx/[email protected]", | ||
| "@edx/frontend-component-footer": "13.0.4", | ||
| "@edx/frontend-enterprise-catalog-search": "9.0.0", | ||
| "@edx/frontend-enterprise-hotjar": "6.0.0", | ||
| "@edx/frontend-enterprise-logistration": "8.0.0", | ||
| "@edx/frontend-enterprise-utils": "8.0.0", | ||
| "@edx/frontend-platform": "7.1.1", | ||
| "@edx/openedx-atlas": "^0.6.0", | ||
| "@edx/reactifex": "^2.2.0", | ||
| "@loadable/component": "5.16.3", | ||
|
|
@@ -61,16 +61,15 @@ | |
| }, | ||
| "devDependencies": { | ||
| "@edx/browserslist-config": "1.1.1", | ||
| "@edx/react-unit-test-utils": "^2.0.0", | ||
| "@openedx/frontend-build": "13.0.28", | ||
| "@tanstack/eslint-plugin-query": "4.29.9", | ||
| "@testing-library/jest-dom": "5.11.9", | ||
| "@testing-library/react": "12.1.5", | ||
| "@testing-library/react-hooks": "3.7.0", | ||
| "@testing-library/user-event": "13.5.0", | ||
| "acorn": "8.5.0", | ||
| "axios-mock-adapter": "1.19.0", | ||
| "jest-canvas-mock": "^2.4.0", | ||
| "match-media-mock": "0.1.1", | ||
| "mockdate": "3.0.5", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,5 @@ | ||
| import React from 'react'; | ||
| import { fireEvent, render, screen } from '@testing-library/react'; | ||
|
|
||
| import { EmailSettingsModal } from '../EmailSettingsModal'; | ||
| import { updateEmailSettings } from '../data'; | ||
|
|
@@ -11,7 +10,7 @@ describe('<EmailSettingsModal />', () => { | |
| let wrapper; | ||
|
|
||
| beforeEach(() => { | ||
| wrapper = render(( | ||
| <EmailSettingsModal | ||
| onClose={() => {}} | ||
| courseRunId="my+course+key" | ||
|
|
@@ -23,28 +22,38 @@ describe('<EmailSettingsModal />', () => { | |
| // is initially `false` until the modal is opened, at which point it's | ||
| // set to whatever the correct value is for that particular course run. | ||
| // Setting the `hasEmailsEnabled` prop here simulates that behavior. | ||
| wrapper.rerender(( | ||
| <EmailSettingsModal | ||
| onClose={() => {}} | ||
| courseRunId="my+course+key" | ||
| hasEmailsEnabled | ||
| /> | ||
| )); | ||
|
Comment on lines
+25
to
+31
There was a problem hiding this comment. [note] It's unfortunate we will need to |
||
| }); | ||
|
|
||
| it('statefulbutton component state is initially set to default and disabled', () => { | ||
| const buttonElement = screen.getAllByRole('button'); | ||
|
There was a problem hiding this comment. [curious/suggestion] Why use For example, something like: const buttonElement = screen.getByRole('button', { name: 'Save' });Same feedback for similar usages in other test cases as well. |
||
|
|
||
| expect(buttonElement[buttonElement.length - 1].getAttribute('aria-disabled')).toEqual('true'); | ||
| expect(buttonElement[buttonElement.length - 1].disabled).toEqual(false); | ||
|
Comment on lines
+37
to
+38
There was a problem hiding this comment. [suggestion] Would expect(buttonElement).toBeDisabled(); |
||
| expect(screen.getByText('Save')).toBeTruthy(); | ||
|
There was a problem hiding this comment. When making assertions with |
||
| }); | ||
|
|
||
| it('statefulbutton component state is set to complete after click event', async () => { | ||
| // Note: The following line is needed to properly resolve the | ||
| // `updateEmailSettings` promise. | ||
| const flushPromises = () => new Promise(setImmediate); | ||
|
There was a problem hiding this comment. With tests using RTL, any use of this approach with |
||
| const buttonElement = screen.getAllByRole('button'); | ||
|
|
||
| expect(screen.getByText('Save')).toBeTruthy(); | ||
| expect(screen.queryByText('Saved')).toBeFalsy(); | ||
|
There was a problem hiding this comment. Similar to using |
||
|
|
||
| fireEvent.click(screen.getByRole('checkbox')); | ||
|
There was a problem hiding this comment.
The only time
See these related resources for more details: |
||
| fireEvent.click(buttonElement[buttonElement.length - 1]); | ||
| await flushPromises(); | ||
|
|
||
| expect(updateEmailSettings).toHaveBeenCalledTimes(1); | ||
| expect(screen.getByText('Saved')).toBeTruthy(); | ||
| expect(screen.queryByText('Save')).toBeFalsy(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import React from 'react'; | ||
| import { fireEvent, render, screen } from '@testing-library/react'; | ||
| import { act } from 'react-dom/test-utils'; | ||
| import { AppContext } from '@edx/frontend-platform/react'; | ||
|
|
||
|
|
@@ -30,56 +30,59 @@ describe('<MarkCompleteModal />', () => { | |
| course_run_status: 'completed', | ||
| }, | ||
| })); | ||
| render(( | ||
| <AppContext.Provider value={{ enterpriseConfig }}> | ||
| <MarkCompleteModal | ||
| {...initialProps} | ||
| /> | ||
| </AppContext.Provider> | ||
| )); | ||
| const buttonElement = screen.getAllByRole('button'); | ||
| fireEvent.click(buttonElement[buttonElement.length - 1]); | ||
| expect(service.updateCourseCompleteStatusRequest).toBeCalledWith({ | ||
| course_id: initialProps.courseId, | ||
| enterprise_id: enterpriseConfig.uuid, | ||
| saved_for_later: true, | ||
| }); | ||
| expect(buttonElement[buttonElement.length - 1].textContent).toEqual(MARK_SAVED_FOR_LATER_PENDING_LABEL); | ||
| }); | ||
|
|
||
| it('handles confirm click with error', async () => { | ||
| // eslint-disable-next-line no-import-assign | ||
| service.markCourseAsCompleteRequest = jest.fn() | ||
| .mockImplementation(() => Promise.reject(new Error('test error'))); | ||
| render(( | ||
| <AppContext.Provider value={{ enterpriseConfig }}> | ||
| <MarkCompleteModal | ||
| {...initialProps} | ||
| /> | ||
| </AppContext.Provider> | ||
| )); | ||
| const buttonElement = screen.getAllByRole('button'); | ||
| await act(async () => { | ||
| fireEvent.click(buttonElement[buttonElement.length - 1]); | ||
| }); | ||
| expect(service.updateCourseCompleteStatusRequest).toBeCalledWith({ | ||
| course_id: initialProps.courseId, | ||
| enterprise_id: enterpriseConfig.uuid, | ||
| saved_for_later: true, | ||
| }); | ||
| expect(buttonElement[buttonElement.length - 1].textContent).toEqual(MARK_SAVED_FOR_LATER_DEFAULT_LABEL); | ||
| }); | ||
|
|
||
| it('handles close modal', () => { | ||
| const mockOnClose = jest.fn(); | ||
| render(( | ||
| <AppContext.Provider value={{ enterpriseConfig }}> | ||
| <MarkCompleteModal | ||
| {...initialProps} | ||
| onClose={mockOnClose} | ||
| /> | ||
| </AppContext.Provider> | ||
| )); | ||
| const buttonElement = screen.getAllByRole('button'); | ||
| act(() => { | ||
| fireEvent.click(buttonElement[1]); | ||
| }); | ||
|
Comment on lines
84
to
86
There was a problem hiding this comment. With RTL, side effects such as this |
||
| expect(mockOnClose).toBeCalledTimes(1); | ||
| }); | ||
|
|
||
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.
[inform/suggestion] Note, while
@edx/react-unit-test-utilsis acceptable as a short-term replacement for previous Enzyme tests to minimize initial refactoring, we generally would prefer not to use this NPM package in favor of using React Testing Library (RTL) directly.Rationale: RTL's testing philosophy is vastly different from Enzyme, and while
@edx/react-unit-test-utilsis using RTL under-the-hood, it generally continues to propagate Enzyme's testing philosophy of asserting based on implementation details (no longer desired).It does look like you're using RTL directly in all but 2 test cases, which is great. Perhaps considering refactoring those 2 test cases to also use RTL directly.