EDSC-4076: Integrate a linter for react-testing-library before the number of those gets too large in EDSC #1844

daniel-zamora · 2024-12-13T07:37:43Z

Overview

What is the feature?

Adds react testing library eslint plugin to edsc

What is the Solution?

Added edsc specific configs to the lint config, the shared linter config updated version, and updated all the tests that were experiencing new linter errors

What areas of the application does this impact?

Component tests that use react testing library.

Testing

Reproduction steps

npm run lint

Attachments

Please include relevant screenshots or files that would be helpful in reviewing and verifying this change.

Checklist

I have added automated tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings

codecov · 2024-12-18T19:09:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.80%. Comparing base (fcae98f) to head (aa84425).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1844      +/-   ##
==========================================
- Coverage   93.81%   93.80%   -0.02%     
==========================================
  Files         777      777              
  Lines       18867    18869       +2     
  Branches     4867     4869       +2     
==========================================
- Hits        17701    17700       -1     
- Misses       1090     1094       +4     
+ Partials       76       75       -1

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

eudoroolivares2016

We do need to figure out why code-cov is missing some files

static/src/js/components/Panels/__tests__/PanelItem.test.jsx

eudoroolivares2016 · 2024-12-20T19:14:24Z

.eslintrc

+        "static/src/js/**/*.js"
+      ],
+      "rules": {
+        "testing-library/await-async-events": "off",


Should these be done in the linter package? otherwise EDSC might have a different setup than say MMT. Some of these feel like they aren't bad to ignore though I understand that it would make this PR even larger some comments on why we're doing these would be good to have though

I think that is the point of the eslintrc, not all repos will want this, we need to exclude tests that haven't been converted from enzyme etc.

Add a comment explaining why these rules are being ignored

I could use a little background as to why all of these rules are being ignored. A comment explaining might be enough 😄

macrouch · 2025-01-03T18:18:38Z

.eslintrc

+        "static/src/js/**/*.js"
+      ],
+      "rules": {
+        "testing-library/await-async-events": "off",


Add a comment explaining why these rules are being ignored

macrouch · 2025-01-03T18:29:44Z

static/src/js/components/CollapsePanel/__tests__/CollapsePanel.test.jsx

@@ -28,8 +28,8 @@ const setup = () => {

 describe('CollapsePanel component', () => {
  test('renders itself correctly', () => {
-    const { container } = setup()
-    expect(container.firstChild).toHaveClass('collapse-panel test-wrap-class')
+    setup()


If no other tests are using the container, don't set it on line 19 and return it on line 22

macrouch · 2025-01-03T18:34:18Z

static/src/js/components/EDSCImage/__tests__/EDSCImage.test.jsx

@@ -1,3 +1,5 @@
+/* eslint-disable testing-library/no-node-access */


Why are these rules ignored? Does the eslintrc rules cover them, or do they need to be addressed?

If they do have to be ignored, we should use the // eslint-disable-next-line syntax only on the lines that need it. In general this means we don't accidentally write code in a file that gets ignored but could have been fixable

macrouch · 2025-01-03T18:39:42Z

static/src/js/components/Facets/Facets.jsx

@@ -55,6 +55,7 @@ const Facets = (props) => {
    featuresFacet.children.push({
      applied: featureFacets.customizable,
      title: 'Customizable',
+      'data-testid': 'customizable-info',


Is this being used in the tests?

macrouch · 2025-01-03T18:46:17Z

static/src/js/components/GranuleResults/GranuleResultsDownloadNotebookButton.jsx

@@ -101,8 +103,15 @@ export const GranuleResultsDownloadNotebookButton = ({
          <Dropdown.Menu
            ref={dropdownMenuRef}
            className="granule-results-download-notebook-button__menu"
+            data-testid="dropdown-menu"


We should avoid using test ids. In this example I don't think it is necessary

const dropdownMenu = screen.getByTestId('dropdown-menu') const downloadButton = within(dropdownMenu).getByRole('button', { name: 'Download Notebook' })

Does screen.getByRole('button', { name: 'Download Notebook' }) work without needing the test id?

macrouch · 2025-01-03T18:47:16Z

static/src/js/components/GranuleResults/__tests__/GranuleResultsFocusedMeta.test.jsx

-              const images = within(modal).queryAllByTestId('mock-edsc-image')
-              const pagination = within(modal).queryByText('2/2')
+              images = within(modal).queryAllByTestId('mock-edsc-image')
+              pagination = within(modal).queryByText('2/2')


I think this line should also be moved below the waitFor

macrouch · 2025-01-03T18:48:19Z

static/src/js/components/GranuleResults/__tests__/GranuleResultsItem.test.jsx

@@ -1,3 +1,4 @@
+/* eslint-disable testing-library/no-unnecessary-act */


Is this covered by eslintrc? If not, use // eslint-disable-next-line instead of disabling for the whole file

macrouch · 2025-01-03T18:50:02Z

static/src/js/components/Panels/PanelItem.jsx

@@ -135,6 +135,9 @@ export class PanelItem extends Component {
      if (scrollable) {
        return (
          <SimpleBar
+            data-testid="simplebar-content-wrapper"


This doesn't seem necessary with both a role and a label on the component

macrouch · 2025-01-03T18:52:43Z

static/src/js/components/ProjectCollections/skeleton.jsx

@@ -51,22 +51,25 @@ export const projectHeader = [
    top: 6,
    height: 12,
    width: 80,
-    radius: 2
+    radius: 2,
+    'data-testid': 'skeleton-item'


Are these being used?

macrouch · 2025-01-03T18:57:58Z

static/src/js/components/TemporalDisplay/__tests__/TemporalSelectionDropdown.test.jsx

    expect(screen.queryByLabelText('Start')).not.toBeInTheDocument()
  })

  test('when clicked toggles the state of show ', async () => {
-    const user = userEvent.setup()
+    const { click } = userEvent.setup()


Not sure why this change was made. But one thing we've done in other repos is call userEvent.setup() first in the setup function, then return the user object. So the test ends up looking like this const { user } = setup({}). That avoids having userEvent.setup() called 18 times in this file

daniel-zamora marked this pull request as ready for review December 16, 2024 18:35

daniel-zamora added 7 commits December 17, 2024 15:05

EDSC-4076 adds react testing library linter

bc3b089

EDSC-4076 renamed component tests to jsx

e191141

EDSC-4076 more renames

b470fd2

EDSC-4076 fixes more linter errors

9f62a8f

EDCS-4076 clean up

152f369

EDSC-4076 cleanup

90a10f3

EDSC-4076 package.json changes

ee6b87b

daniel-zamora force-pushed the EDSC-4076 branch from ad1ea9e to ee6b87b Compare December 18, 2024 19:05

daniel-zamora added 4 commits December 19, 2024 15:33

EDSC-4076 linter fixes

6690d4d

EDSC-4076 eslint-config install

b41a85b

EDSC-4076 test fixes

da7648a

EDSC-4076 test fix

af68918

eudoroolivares2016 reviewed Dec 20, 2024

View reviewed changes

EDSC-4076 clean up

aa84425

daniel-zamora force-pushed the EDSC-4076 branch from 870f22b to aa84425 Compare December 26, 2024 17:41

macrouch reviewed Jan 3, 2025

View reviewed changes

trevorlang self-requested a review January 7, 2025 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EDSC-4076: Integrate a linter for react-testing-library before the number of those gets too large in EDSC #1844

EDSC-4076: Integrate a linter for react-testing-library before the number of those gets too large in EDSC #1844

daniel-zamora commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading

eudoroolivares2016 left a comment

eudoroolivares2016 Dec 20, 2024

daniel-zamora Dec 26, 2024

macrouch Jan 3, 2025

trevorlang Jan 7, 2025

macrouch Jan 3, 2025

macrouch Jan 3, 2025

macrouch Jan 3, 2025

macrouch Jan 3, 2025

macrouch Jan 3, 2025

macrouch Jan 3, 2025

macrouch Jan 3, 2025

macrouch Jan 3, 2025

macrouch Jan 3, 2025

macrouch Jan 3, 2025

		@@ -1,3 +1,5 @@
		/* eslint-disable testing-library/no-node-access */

		@@ -1,3 +1,4 @@
		/* eslint-disable testing-library/no-unnecessary-act */

EDSC-4076: Integrate a linter for react-testing-library before the number of those gets too large in EDSC #1844

Are you sure you want to change the base?

EDSC-4076: Integrate a linter for react-testing-library before the number of those gets too large in EDSC #1844

Conversation

daniel-zamora commented Dec 13, 2024 • edited Loading

Overview

What is the feature?

What is the Solution?

What areas of the application does this impact?

Testing

Reproduction steps

Attachments

Checklist

codecov bot commented Dec 18, 2024 • edited Loading

Codecov Report

eudoroolivares2016 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

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

daniel-zamora commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading