chore: move autosuggest tests from Enzyme to RTL #2611

cintnguyen · 2023-09-08T19:53:43Z

Description

Move autosuggest tests from Enzyme to RTL

Deploy Preview

N/A

Merge Checklist

Is there adequate test coverage for your changes?

Post-merge Checklist

Verify your changes were released to NPM at the expected version.
If you'd like, share your contribution in #show-and-tell.
🎉 🙌 Celebrate! Thanks for your contribution.

netlify · 2023-09-08T19:53:49Z

✅ Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`33fb35b`
🔍 Latest deploy log	https://app.netlify.com/sites/paragon-openedx/deploys/64ff8d0881419e00080fd481
😎 Deploy Preview	https://deploy-preview-2611--paragon-openedx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2023-09-08T20:16:01Z

Codecov Report

Patch coverage has no change and project coverage change: +0.16% 🎉

Comparison is base (f1c3596) 91.72% compared to head (71bc0ae) 91.88%.

❗ Current head 71bc0ae differs from pull request most recent head 33fb35b. Consider uploading reports for the commit 33fb35b to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2611      +/-   ##
==========================================
+ Coverage   91.72%   91.88%   +0.16%     
==========================================
  Files         235      235              
  Lines        4217     4214       -3     
  Branches     1020     1021       +1     
==========================================
+ Hits         3868     3872       +4     
+ Misses        345      338       -7     
  Partials        4        4

Files Changed	Coverage Δ
src/Form/FormAutosuggest.jsx	`96.66% <ø> (+0.96%)`	⬆️
src/Form/FormAutosuggestOption.jsx	`100.00% <ø> (ø)`

... and 1 file with indirect coverage changes

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

brian-smith-tcril · 2023-09-08T20:21:59Z

src/Form/FormAutosuggestOption.jsx

@@ -13,7 +13,7 @@ function FormAutosuggestOption({
    <MenuItem
      as="li"
      role="option"
-      tabindex="-1"


good catch!

adamstankiewicz · 2023-09-11T18:51:55Z

src/Form/tests/FormAutosuggest.test.jsx

+  it('renders component with options', () => {
+    const { getByTestId, container } = render(<FormAutosuggestTestComponent />);
+    const input = getByTestId('autosuggest_textbox_input');
+    fireEvent.click(input);


[curious] Is there a reason to use fireEvent here (and elsewhere throughout the tests) versus using userEvent?

If it's possible to use userEvent.click instead, it's generally preferable since it simulates more of the true user interactions in the browser, not just the low-level click itself.

For example, userEvent.click also simulates a previous hover event that a real user would trigger prior to clicking.

Resources:

https://testing-library.com/docs/guide-events/

https://blog.mimacom.com/react-testing-library-fireevent-vs-userevent/

Fixed in : https://github.com/openedx/paragon/compare/71bc0ae4c69bf3875142148f22a1ff7a204a2381..299441986865ab87377629aa4d99f13ed8fc47d7
Thank you!

adamstankiewicz · 2023-09-11T18:55:58Z

src/Form/tests/FormAutosuggest.test.jsx

-    });
+  it('renders without loading state', () => {
+    const { container } = render(<FormAutosuggestTestComponent />);
+    expect(container.querySelector('.pgn__form-autosuggest__dropdown-loading')).toBeNull();


Is there a way to avoid using the class name implementation detail in this test?

For example, if the component renders a Spinner with the supplied screenReaderText prop, could this test assert that the Spinner is not rendered based on a data-testid attribute versus relying on the class name that could change at any time. If the class name did change down the road, this test would still pass.

Fixed in:
https://github.com/openedx/paragon/compare/299441986865ab87377629aa4d99f13ed8fc47d7..6c9a96ea3e2ba2ef4ab37d5f266b4d7205f5c718

adamstankiewicz · 2023-09-11T18:56:49Z

src/Form/tests/FormAutosuggest.test.jsx

-      const wrapper = mount(<FormAutosuggestWrapper value="Test Value" />);
+  it('render with loading state', () => {
+    const { container } = render(<FormAutosuggestWrapper isLoading />);
+    expect(container.querySelector('.pgn__form-autosuggest__dropdown-loading')).toBeTruthy();


Similar to above comment, could we either assert that the screenReaderText passed to the Spinner is rendered, or alternatively, rely on a more stable data-testid instead of the class name?

Fixed in:
https://github.com/openedx/paragon/compare/299441986865ab87377629aa4d99f13ed8fc47d7..6c9a96ea3e2ba2ef4ab37d5f266b4d7205f5c718

adamstankiewicz

LGTM!

brian-smith-tcril · 2023-09-11T21:35:07Z

So the current failing test is the error state test. It was written by having the simulated user enter the textbox and press escape. This worked before 4816baa#diff-96e13c1a773ec8d218ea5667d3dec6c8f6eeadcb00c81584f50d6c3d4d52135fL133, but now that doesn't trigger the error state.

It should probably be updated to simulate a click outside, as that will still trigger the error state (I verified on https://paragon-openedx.netlify.app/components/form/form-autosuggest/)

Co-authored-by: Mena Hassan <mhassan@axim.org>

cintnguyen · 2023-09-11T21:58:55Z

So the current failing test is the error state test. It was written by having the simulated user enter the textbox and press escape. This worked before 4816baa#diff-96e13c1a773ec8d218ea5667d3dec6c8f6eeadcb00c81584f50d6c3d4d52135fL133, but now that doesn't trigger the error state.

It should probably be updated to simulate a click outside, as that will still trigger the error state (I verified on https://paragon-openedx.netlify.app/components/form/form-autosuggest/)

Fix here: https://github.com/openedx/paragon/compare/ba5085a9a58e9a35f545070541decaba69c8130f..33fb35b955f6e7de8b872791fe5794616300cff9

PKulkoRaccoonGang · 2023-09-12T07:25:37Z

src/Form/FormAutosuggest.jsx

@@ -102,6 +102,7 @@ function FormAutosuggest({
  const iconToggle = (
    <IconButton
      className="pgn__form-autosuggest__icon-button"
+      data-testid="autosuggest_iconbutton"


We recently rewrote all tests (except FormAutosuggest) to RTL. For uniformity of test identifiers, we agreed to use a kebab case. Can you convert the names of the identifiers to a uniform format? autosuggest-iconbutton

PKulkoRaccoonGang · 2023-09-12T07:26:34Z

src/Form/FormAutosuggest.jsx

@@ -253,7 +255,7 @@ function FormAutosuggest({
      >
        {isLoading ? (
          <div className="pgn__form-autosuggest__dropdown-loading">
-            <Spinner animation="border" variant="dark" screenReaderText={screenReaderText} />
+            <Spinner animation="border" variant="dark" screenReaderText={screenReaderText} data-testid="autosuggest_loading_spinner" />


Maybe more readable

Suggested change

<Spinner animation="border" variant="dark" screenReaderText={screenReaderText} data-testid="autosuggest_loading_spinner" />

<Spinner

animation="border"

variant="dark"

screenReaderText={screenReaderText}

data-testid="autosuggest-loading-spinner"

/>

PKulkoRaccoonGang · 2023-09-12T07:27:18Z

src/Form/FormAutosuggest.jsx

@@ -230,6 +231,7 @@ function FormAutosuggest({
          onChange={handleOnChange}
          onClick={handleClick}
          trailingElement={iconToggle}
+          data-testid="autosuggest_textbox_input"


Suggested change

data-testid="autosuggest_textbox_input"

data-testid="autosuggest-textbox-input"

PKulkoRaccoonGang · 2023-09-12T07:27:35Z

src/Form/FormAutosuggestOption.jsx

@@ -12,8 +12,9 @@ function FormAutosuggestOption({
  return (
    <MenuItem
      as="li"
+      data-testid="autosuggest_optionitem"


Suggested change

data-testid="autosuggest_optionitem"

data-testid="autosuggest-optionitem"

PKulkoRaccoonGang · 2023-09-12T07:28:20Z

src/Form/tests/FormAutosuggest.test.jsx

+import PropTypes from 'prop-types';
+import { render, screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import '@testing-library/jest-dom';


Is it better to move this line to setupTests.js? Did you encounter some problems with this?

brian-smith-tcril · 2023-09-12T16:32:49Z

@PKulkoRaccoonGang I'm sorry I was a bit hasty and didn't see these comments before hitting merge. We should be able to address them in a quick follow-up PR.

edx-semantic-release · 2023-09-13T06:49:40Z

🎉 This PR is included in version 21.1.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

edx-semantic-release · 2023-09-22T13:21:48Z

🎉 This PR is included in version 22.0.0-alpha.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

cintnguyen force-pushed the formAutosuggest-rtl branch from 47739c5 to 71bc0ae Compare September 8, 2023 19:59

brian-smith-tcril approved these changes Sep 8, 2023

View reviewed changes

adamstankiewicz reviewed Sep 11, 2023

View reviewed changes

cintnguyen force-pushed the formAutosuggest-rtl branch 3 times, most recently from 6c9a96e to 0248452 Compare September 11, 2023 21:14

adamstankiewicz approved these changes Sep 11, 2023

View reviewed changes

cintnguyen force-pushed the formAutosuggest-rtl branch from 0248452 to ba5085a Compare September 11, 2023 21:47

chore: move autosuggest tests from Enzyme to RTL

33fb35b

Co-authored-by: Mena Hassan <mhassan@axim.org>

cintnguyen force-pushed the formAutosuggest-rtl branch from ba5085a to 33fb35b Compare September 11, 2023 21:56

brian-smith-tcril approved these changes Sep 11, 2023

View reviewed changes

PKulkoRaccoonGang reviewed Sep 12, 2023

View reviewed changes

brian-smith-tcril merged commit 23b83c5 into openedx:master Sep 12, 2023
9 checks passed

brian-smith-tcril mentioned this pull request Sep 12, 2023

[docs] RTL best practices and naming/structure conventions #2619

Closed

edx-semantic-release added the released label Sep 13, 2023

edx-semantic-release added the released on @alpha label Sep 22, 2023

arbrandes mentioned this pull request Mar 6, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: move autosuggest tests from Enzyme to RTL #2611

chore: move autosuggest tests from Enzyme to RTL #2611

cintnguyen commented Sep 8, 2023

netlify bot commented Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 8, 2023 •

edited

Loading

brian-smith-tcril Sep 8, 2023

adamstankiewicz Sep 11, 2023

cintnguyen Sep 11, 2023

adamstankiewicz Sep 11, 2023

cintnguyen Sep 11, 2023

adamstankiewicz Sep 11, 2023

cintnguyen Sep 11, 2023

adamstankiewicz left a comment

brian-smith-tcril commented Sep 11, 2023

cintnguyen commented Sep 11, 2023

PKulkoRaccoonGang Sep 12, 2023

PKulkoRaccoonGang Sep 12, 2023

PKulkoRaccoonGang Sep 12, 2023

PKulkoRaccoonGang Sep 12, 2023

PKulkoRaccoonGang Sep 12, 2023

brian-smith-tcril commented Sep 12, 2023

edx-semantic-release commented Sep 13, 2023

edx-semantic-release commented Sep 22, 2023

	data-testid="autosuggest_textbox_input"
	data-testid="autosuggest-textbox-input"

	data-testid="autosuggest_optionitem"
	data-testid="autosuggest-optionitem"

chore: move autosuggest tests from Enzyme to RTL #2611

chore: move autosuggest tests from Enzyme to RTL #2611

Conversation

cintnguyen commented Sep 8, 2023

Description

Deploy Preview

Merge Checklist

Post-merge Checklist

netlify bot commented Sep 8, 2023 • edited Loading

✅ Deploy Preview for paragon-openedx ready!

codecov bot commented Sep 8, 2023 • edited Loading

Codecov Report

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

adamstankiewicz left a comment

Choose a reason for hiding this comment

brian-smith-tcril commented Sep 11, 2023

cintnguyen commented Sep 11, 2023

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

brian-smith-tcril commented Sep 12, 2023

edx-semantic-release commented Sep 13, 2023

edx-semantic-release commented Sep 22, 2023

netlify bot commented Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 8, 2023 •

edited

Loading