feat: migrate enzyme to rtl #915

Syed-Ali-Abbas-Zaidi · 2024-01-02T12:28:06Z

For all changes

Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

Ensure to attach screenshots
Ensure to have UX team confirm screenshots

…al-enterprise into Ali-Abbas/enzyme-to-rtl

codecov · 2024-03-04T12:18:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.65%. Comparing base (d36b78a) to head (4ad3d21).
Report is 178 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #915      +/-   ##
==========================================
- Coverage   84.87%   84.65%   -0.22%     
==========================================
  Files         320      343      +23     
  Lines        6399     7520    +1121     
  Branches     1552     1849     +297     
==========================================
+ Hits         5431     6366     +935     
- Misses        941     1118     +177     
- Partials       27       36       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…al-enterprise into Ali-Abbas/enzyme-to-rtl

adamstankiewicz

Thanks for the PR, @Syed-Ali-Abbas-Zaidi! I left some feedback/suggestions, primarily to ensure the updated tests more closely follow best practices with RTL. Thanks.

adamstankiewicz · 2024-03-25T13:58:57Z

...ain-content/course-enrollments/course-cards/email-settings/tests/EmailSettingsModal.test.jsx

+
+    expect(buttonElement[buttonElement.length - 1].getAttribute('aria-disabled')).toEqual('true');
+    expect(buttonElement[buttonElement.length - 1].disabled).toEqual(false);
+    expect(screen.getByText('Save')).toBeTruthy();


When making assertions with .getByText, it's recommended to use .toBeInTheDocument(), not .toBeTruthy(). This would be more consistent with other usages of getByText throughout this repo and more aligned with the best practices recommended with RTL (e.g., https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#using-get-variants-as-assertions).

adamstankiewicz · 2024-03-25T14:00:46Z

...ain-content/course-enrollments/course-cards/email-settings/tests/EmailSettingsModal.test.jsx

-    const defaultState = 'default';
-    expect(wrapper.find(StatefulButton).prop('state')).toEqual(defaultState);
-    expect(wrapper.find(StatefulButton).prop('disabledStates')).toContain(defaultState);
+    const buttonElement = screen.getAllByRole('button');


[curious/suggestion] Why use .getAllByRole when you're only expecting/asserting on a single button? I would recommend making this query for the specific button in question, not all buttons...

For example, something like:

const buttonElement = screen.getByRole('button', { name: 'Save' });

Same feedback for similar usages in other test cases as well.

adamstankiewicz · 2024-03-25T14:01:21Z

...ain-content/course-enrollments/course-cards/email-settings/tests/EmailSettingsModal.test.jsx

+    expect(buttonElement[buttonElement.length - 1].getAttribute('aria-disabled')).toEqual('true');
+    expect(buttonElement[buttonElement.length - 1].disabled).toEqual(false);


[suggestion] Would .toBeDisabled() simplify these lines?

expect(buttonElement).toBeDisabled();

adamstankiewicz · 2024-03-25T14:02:39Z

...ain-content/course-enrollments/course-cards/email-settings/tests/EmailSettingsModal.test.jsx

-    wrapper.find('input[type="checkbox"]').simulate('change', { target: { checked: false } });
-    wrapper.find(StatefulButton).simulate('click');
+    expect(screen.getByText('Save')).toBeTruthy();
+    expect(screen.queryByText('Saved')).toBeFalsy();


Similar to using .toBeInTheDocument() for such assertions, this should be using .not.toBeInTheDocument(), not toBeFalsy().

adamstankiewicz · 2024-03-25T14:04:08Z

...ain-content/course-enrollments/course-cards/email-settings/tests/EmailSettingsModal.test.jsx

+    expect(screen.getByText('Save')).toBeTruthy();
+    expect(screen.queryByText('Saved')).toBeFalsy();
+
+    fireEvent.click(screen.getByRole('checkbox'));


userEvent is generally preferred over fireEvent. userEvent utilizes fireEvent but userEvent more closely mimics real user behavior (which is one of the overall benefits of RTL).

The only time fireEvent should be called, per the following resources, is if the user interaction is not yet implemented in userEvent.

There are, however, some user interactions or aspects of these that aren't yet implemented and thus can't yet be described with user-event. In these cases you can use fireEvent to dispatch the concrete events that your software relies on.

See these related resources for more details:

https://testing-library.com/docs/user-event/intro/#differences-from-fireevent

https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#not-using-testing-libraryuser-event

https://testing-library.com/docs/user-event/intro/#differences-from-fireevent

adamstankiewicz · 2024-03-25T14:45:55Z

src/components/my-career/tests/SearchJobRole.test.jsx

+
+    expect(buttonElement[1].getAttribute('aria-disabled')).not.toEqual('true');
+    expect(buttonElement[1].disabled).toEqual(false);
+    expect(screen.getByText('Save')).toBeTruthy();
    wrapper.unmount();


Same question as above whether wrapper.unmount() can be removed (question applies to the below test cases as well).

adamstankiewicz · 2024-03-25T14:56:06Z

src/components/my-career/tests/SearchJobRole.test.jsx

-    wrapper.find(StatefulButton).simulate('click');
-    wrapper.update();
+
+    fireEvent.change(screen.getByTestId('dropdown').children[0], { target: { value: 'Software Engineer' } });


[suggestion] I believe it'd be more in line with RTL's philosophy/recommendations to not reach into the children of the returned element from screen.getByTestId('dropdown') but instead query for the input field directly (e.g., based on its role or label).

screen.getByRole('input', { name: 'Type to find a current job' })

https://testing-library.com/docs/queries/byrole/

adamstankiewicz · 2024-03-25T14:58:13Z

package.json

@@ -61,16 +61,15 @@
  },
  "devDependencies": {
    "@edx/browserslist-config": "1.1.1",
+    "@edx/react-unit-test-utils": "^2.0.0",


[inform/suggestion] Note, while @edx/react-unit-test-utils is 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-utils is 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.

adamstankiewicz · 2024-03-25T15:01:37Z

src/components/search/content-highlights/tests/HighlightedContentCard.test.jsx

@@ -1,5 +1,5 @@
 import { AppContext } from '@edx/frontend-platform/react';
-import { shallow } from 'enzyme';
+import { shallow } from '@edx/react-unit-test-utils';


[suggestion] Would recommend not using @edx/react-unit-test-utils here in favor of refactoring the one test using shallow to use RTL directly instead;shallow rendering is not something we want to continue as a paradigm in this MFE.

https://buttondown.email/kentcdodds/archive/76ce5bb1-0681-490b-9e1e-62f27e5cfa4d

adamstankiewicz · 2024-03-25T15:05:16Z

...ain-content/course-enrollments/course-cards/email-settings/tests/EmailSettingsModal.test.jsx

+    wrapper.rerender((
+      <EmailSettingsModal
+        onClose={() => {}}
+        courseRunId="my+course+key"
+        hasEmailsEnabled
+      />
+    ));


[note] It's unfortunate we will need to rerender() here, but don't seem an immediate way around it without refactoring the underlying EmailSettingsModal itself, which is likely out of scope for this particular work to replace Enzyme. Just wanted to leave a note that, while OK to leave as is, this generally seems to be a code smell.

feat: migrate enzyme to rtl

a3ddfdd

Syed-Ali-Abbas-Zaidi marked this pull request as draft January 2, 2024 12:34

abdullahwaheed added 2 commits March 4, 2024 16:46

Merge branch 'master' of github.com:openedx/frontend-app-learner-port…

300e707

…al-enterprise into Ali-Abbas/enzyme-to-rtl

fix: fixed dependency issues

3866582

abdullahwaheed marked this pull request as ready for review March 4, 2024 12:11

arbrandes mentioned this pull request Mar 6, 2024

[DEPR]: Enzyme openedx/public-engineering#195

Open

abdullahwaheed added 4 commits March 12, 2024 11:51

Merge branch 'master' of github.com:openedx/frontend-app-learner-port…

f2b69fb

…al-enterprise into Ali-Abbas/enzyme-to-rtl

refactor: updated footer version

3e87d06

Merge branch 'master' of github.com:openedx/frontend-app-learner-port…

b146921

…al-enterprise into Ali-Abbas/enzyme-to-rtl

Merge branch 'master' of github.com:openedx/frontend-app-learner-port…

4ad3d21

…al-enterprise into Ali-Abbas/enzyme-to-rtl

adamstankiewicz reviewed Mar 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migrate enzyme to rtl #915

feat: migrate enzyme to rtl #915

Syed-Ali-Abbas-Zaidi commented Jan 2, 2024

codecov bot commented Mar 4, 2024 •

edited

Loading

adamstankiewicz left a comment

adamstankiewicz Mar 25, 2024

adamstankiewicz Mar 25, 2024

adamstankiewicz Mar 25, 2024

adamstankiewicz Mar 25, 2024

adamstankiewicz Mar 25, 2024

adamstankiewicz Mar 25, 2024

adamstankiewicz Mar 25, 2024

adamstankiewicz Mar 25, 2024

adamstankiewicz Mar 25, 2024

adamstankiewicz Mar 25, 2024

		expect(buttonElement[buttonElement.length - 1].getAttribute('aria-disabled')).toEqual('true');
		expect(buttonElement[buttonElement.length - 1].disabled).toEqual(false);

feat: migrate enzyme to rtl #915

Are you sure you want to change the base?

feat: migrate enzyme to rtl #915

Conversation

Syed-Ali-Abbas-Zaidi commented Jan 2, 2024

For all changes

Only if submitting a visual change

codecov bot commented Mar 4, 2024 • edited Loading

Codecov Report

adamstankiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 4, 2024 •

edited

Loading