ref: Consolidate test related rules into plugin specific eslint confi…

…g objects, and auto-fix (#82737) I'm refactoring the eslint file to consolidate rules with their plugins. I think it'll result in more readable configs, and will give structure for enabling new rules & plugins over time, if we wanted to do that. To start I pulled all the test-related plugins into their own config objects, and override rules as needed. With this change we're able to target test-related rules only at the test files, so that's a win for ci perf. However, it turns out that the `testing-library/react` rules were not applied to `.spec.tsx` files, so there's a lot of violations there and running rules will hurt perf again. Overall perf is not my priority, in most cases we run rules only against changed files. To deal with that I created an extra config object and set those rules to be 'warn' instead of 'error' so we can fix them later & over time. As each of those warning rules is addressed we can delete that line from the eslint.config file, and once they're all done we can merge those two config objects together.
getsentry · Dec 30, 2024 · e8cec11 · e8cec11
1 parent 9d9cd2f
commit e8cec11
Show file tree

Hide file tree

Showing 12 changed files with 91 additions and 88 deletions.
diff --git a/eslint.config.mjs b/eslint.config.mjs
@@ -454,14 +454,9 @@ const reactImportRules = {
   ],
 };
 
-const reactJestRules = {
-  'jest/no-disabled-tests': 'error',
-};
-
 const reactRules = {
   ...reactReactRules,
   ...reactImportRules,
-  ...reactJestRules,
   /**
    * React hooks
    */
@@ -477,21 +472,6 @@ const reactRules = {
    */
   // highlights literals in JSX components w/o translation tags
   'getsentry/jsx-needs-il8n': ['off'],
-  'testing-library/render-result-naming-convention': 'off',
-  'testing-library/no-unnecessary-act': 'off',
-
-  // Disabled as we have many tests which render as simple validations
-  'jest/expect-expect': 'off',
-
-  // Disabled as we have some comment out tests that cannot be
-  // uncommented due to typescript errors.
-  'jest/no-commented-out-tests': 'off',
-
-  // Disabled as we do sometimes have conditional expects
-  'jest/no-conditional-expect': 'off',
-
-  // Useful for exporting some test utilities
-  'jest/no-export': 'off',
 
   'typescript-sort-keys/interface': [
     'error',
@@ -745,8 +725,6 @@ const strictRules = {
   // This is now considered legacy, callback refs preferred
   'react/no-string-refs': ['error'],
 
-  'jest/no-large-snapshots': ['error', {maxSize: 2000}],
-
   'sentry/no-styled-shortcut': ['error'],
 };
 
@@ -810,8 +788,6 @@ export default typescript.config([
     // TODO: move these potential overrides and plugin-specific rules into the
     // corresponding configuration object where the plugin is initially included
     plugins: {
-      ...jest.configs['flat/recommended'].plugins,
-      ...jestDom.configs['flat/recommended'].plugins,
       ...react.configs.flat.plugins,
       ...react.configs.flat['jsx-runtime'].plugins,
       '@emotion': emotion,
@@ -939,17 +915,60 @@ export default typescript.config([
       ],
     },
   },
+  {
+    name: 'jest',
+    files: ['**/*.spec.{ts,js,tsx,jsx}', 'tests/js/**/*.{ts,js,tsx,jsx}'],
+    plugins: jest.configs['flat/recommended'].plugins,
+    rules: {
+      'jest/no-disabled-tests': 'error',
+
+      // Disabled as we have many tests which render as simple validations
+      'jest/expect-expect': 'off',
+
+      // Disabled as we have some comment out tests that cannot be
+      // uncommented due to typescript errors.
+      'jest/no-commented-out-tests': 'off',
+
+      // Disabled as we do sometimes have conditional expects
+      'jest/no-conditional-expect': 'off',
+
+      // Useful for exporting some test utilities
+      'jest/no-export': 'off',
 
+      // We don't recommend snapshots, but if there are any keep it small
+      'jest/no-large-snapshots': ['error', {maxSize: 2000}],
+    },
+  },
+  {
+    name: 'jest-dom',
+    files: ['**/*.spec.{ts,js,tsx,jsx}', 'tests/js/**/*.{ts,js,tsx,jsx}'],
+    plugins: jestDom.configs['flat/recommended'].plugins,
+  },
   {
-    name: 'testing-library/react',
-    files: ['static/**/*.spec.{ts,js}', 'tests/js/**/*.{ts,js}'],
+    name: 'testing-library/react - ts files',
+    files: ['**/*.spec.{ts,js,tsx,jsx}', 'tests/js/**/*.{ts,js,tsx,jsx}'],
     ...testingLibrary.configs['flat/react'],
     rules: {
-      ...baseRules,
-      ...reactRules,
-      ...appRules,
-      ...strictRules,
       ...testingLibrary.configs['flat/react'].rules,
+      'testing-library/render-result-naming-convention': 'off',
+      'testing-library/no-unnecessary-act': 'off',
+    },
+  },
+  {
+    name: 'testing-library/react - tsx files',
+    files: ['**/*.spec.{tsx,jsx}', 'tests/js/**/*.{tsx,jsx}'],
+    ...testingLibrary.configs['flat/react'],
+    rules: {
+      'testing-library/await-async-queries': 'warn', // TODO(ryan953): Fix the violations, then delete this line
+      'testing-library/no-await-sync-events': 'warn', // TODO(ryan953): Fix the violations, then delete this line
+      'testing-library/no-await-sync-queries': 'warn', // TODO(ryan953): Fix the violations, then delete this line
+      'testing-library/no-container': 'warn', // TODO(ryan953): Fix the violations, then delete this line
+      'testing-library/no-node-access': 'warn', // TODO(ryan953): Fix the violations, then delete this line
+      'testing-library/no-render-in-lifecycle': 'warn', // TODO(ryan953): Fix the violations, then delete this line
+      'testing-library/no-wait-for-multiple-assertions': 'warn', // TODO(ryan953): Fix the violations, then delete this line
+      'testing-library/prefer-presence-queries': 'warn', // TODO(ryan953): Fix the violations, then delete this line
+      'testing-library/prefer-query-by-disappearance': 'warn', // TODO(ryan953): Fix the violations, then delete this line
+      'testing-library/prefer-screen-queries': 'warn', // TODO(ryan953): Fix the violations, then delete this line
     },
   },
   {

diff --git a/static/app/components/charts/releaseSeries.spec.tsx b/static/app/components/charts/releaseSeries.spec.tsx
@@ -237,8 +237,8 @@ describe('ReleaseSeries', function () {
       </Fragment>
     );
 
-    await waitFor(() => expect(screen.getByText('Series 1')).toBeInTheDocument());
-    await waitFor(() => expect(screen.getByText('Series 2')).toBeInTheDocument());
+    await screen.findByText('Series 1');
+    await screen.findByText('Series 2');
 
     await waitFor(() => expect(releasesMock).toHaveBeenCalledTimes(1));
   });

diff --git a/static/app/components/onboardingWizard/newSidebar.spec.tsx b/static/app/components/onboardingWizard/newSidebar.spec.tsx
@@ -1,11 +1,5 @@
 import {initializeOrg} from 'sentry-test/initializeOrg';
-import {
-  render,
-  screen,
-  userEvent,
-  waitFor,
-  waitForElementToBeRemoved,
-} from 'sentry-test/reactTestingLibrary';
+import {render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary';
 
 import {NewOnboardingSidebar} from 'sentry/components/onboardingWizard/newSidebar';
 import {type OnboardingTask, OnboardingTaskKey} from 'sentry/types/onboarding';
@@ -80,7 +74,7 @@ describe('NewSidebar', function () {
     expect(screen.queryByText(beyondBasicsTasks[0].title)).not.toBeInTheDocument();
 
     // Manually expand second group
-    userEvent.click(screen.getByRole('button', {name: 'Expand'}));
+    await userEvent.click(screen.getByRole('button', {name: 'Expand'}));
     // Tasks from the second group should be visible
     expect(await screen.findByText(beyondBasicsTasks[0].title)).toBeInTheDocument();
     // task from second group are skippable
@@ -132,19 +126,19 @@ describe('NewSidebar', function () {
     );
 
     // Manually expand second group
-    userEvent.click(screen.getByRole('button', {name: 'Expand'}));
+    await userEvent.click(screen.getByRole('button', {name: 'Expand'}));
     // Tasks from the second group should be visible
     expect(await screen.findByText(beyondBasicsTasks[0].title)).toBeInTheDocument();
 
-    userEvent.click(screen.getByRole('button', {name: 'Skip Task'}));
+    await userEvent.click(screen.getByRole('button', {name: 'Skip Task'}));
 
     // Confirmation to skip should be visible
     expect(await screen.findByText(/Not sure what to do/)).toBeInTheDocument();
     expect(screen.getByRole('button', {name: 'Just Skip'})).toBeInTheDocument();
     expect(screen.getByRole('button', {name: 'Help'})).toBeInTheDocument();
 
     // Click help
-    userEvent.click(screen.getByRole('button', {name: 'Help'}));
+    await userEvent.click(screen.getByRole('button', {name: 'Help'}));
 
     // Show help menu
     expect(await screen.findByText('Search Support, Docs and More')).toBeInTheDocument();
@@ -153,7 +147,7 @@ describe('NewSidebar', function () {
     expect(screen.getByRole('link', {name: 'Visit Help Center'})).toBeInTheDocument();
 
     // Click 'Just Skip'
-    userEvent.click(screen.getByRole('button', {name: 'Just Skip'}));
+    await userEvent.click(screen.getByRole('button', {name: 'Just Skip'}));
     await waitFor(() => {
       expect(mockUpdate).toHaveBeenCalledWith(
         `/organizations/${organization.slug}/onboarding-tasks/`,
@@ -167,7 +161,7 @@ describe('NewSidebar', function () {
     });
 
     // Dismiss skip confirmation
-    userEvent.click(screen.getByRole('button', {name: 'Dismiss Skip'}));
-    await waitForElementToBeRemoved(() => screen.queryByText(/Not sure what to do/));
+    await userEvent.click(screen.getByRole('button', {name: 'Dismiss Skip'}));
+    expect(screen.queryByText(/Not sure what to do/)).not.toBeInTheDocument();
   });
 });
diff --git a/static/app/components/sidebar/newOnboardingStatus.spec.tsx b/static/app/components/sidebar/newOnboardingStatus.spec.tsx
@@ -63,11 +63,11 @@ describe('Onboarding Status', function () {
     expect(screen.getByTestId('pending-seen-indicator')).toBeInTheDocument();
 
     // By hovering over the button, we should refetch the data
-    userEvent.hover(screen.getByRole('button', {name: 'Onboarding'}));
+    await userEvent.hover(screen.getByRole('button', {name: 'Onboarding'}));
     await waitFor(() => expect(getOnboardingTasksMock).toHaveBeenCalled());
 
     // Open the panel
-    userEvent.click(screen.getByRole('button', {name: 'Onboarding'}));
+    await userEvent.click(screen.getByRole('button', {name: 'Onboarding'}));
     await waitFor(() => expect(getOnboardingTasksMock).toHaveBeenCalled());
     await waitFor(() => expect(postOnboardingTasksMock).toHaveBeenCalled());
     expect(handleShowPanel).toHaveBeenCalled();
@@ -116,7 +116,7 @@ describe('Onboarding Status', function () {
     expect(getOnboardingTasksMock).toHaveBeenCalled();
 
     // Hide Panel
-    userEvent.click(screen.getByLabelText('Close Panel'));
+    await userEvent.click(screen.getByLabelText('Close Panel'));
     await waitFor(() => expect(handleHidePanel).toHaveBeenCalled());
   });
 });
diff --git a/static/app/utils/discover/fieldRenderers.spec.tsx b/static/app/utils/discover/fieldRenderers.spec.tsx
@@ -150,9 +150,7 @@ describe('getFieldRenderer', function () {
       const renderer = getFieldRenderer('createdAt', {createdAt: 'date'});
       render(renderer(data, {location, organization}) as React.ReactElement<any, any>);
 
-      await waitFor(() =>
-        expect(screen.getByText('Oct 3, 2019 9:13:14 AM PDT')).toBeInTheDocument()
-      );
+      await screen.findByText('Oct 3, 2019 9:13:14 AM PDT');
     });
 
     it('can render date fields using utc when query string has utc set to true', async function () {
@@ -164,9 +162,7 @@ describe('getFieldRenderer', function () {
         }) as React.ReactElement<any, any>
       );
 
-      await waitFor(() =>
-        expect(screen.getByText('Oct 3, 2019 4:13:14 PM UTC')).toBeInTheDocument()
-      );
+      await screen.findByText('Oct 3, 2019 4:13:14 PM UTC');
     });
   });
 

diff --git a/static/app/views/issueList/customViewsHeader.spec.tsx b/static/app/views/issueList/customViewsHeader.spec.tsx
@@ -466,7 +466,7 @@ describe('CustomViewsHeader', () => {
 
       render(<CustomViewsIssueListHeader {...defaultProps} />);
 
-      userEvent.click(
+      await userEvent.click(
         await screen.findByRole('button', {name: 'High Priority Ellipsis Menu'})
       );
 
@@ -497,7 +497,7 @@ describe('CustomViewsHeader', () => {
 
       render(<CustomViewsIssueListHeader {...defaultProps} router={unsavedTabRouter} />);
 
-      userEvent.click(
+      await userEvent.click(
         await screen.findByRole('button', {name: 'High Priority Ellipsis Menu'})
       );
 
@@ -527,7 +527,7 @@ describe('CustomViewsHeader', () => {
 
       render(<CustomViewsIssueListHeader {...defaultProps} />);
 
-      userEvent.click(
+      await userEvent.click(
         await screen.findByRole('button', {name: 'High Priority Ellipsis Menu'})
       );
 
@@ -560,7 +560,7 @@ describe('CustomViewsHeader', () => {
 
         render(<CustomViewsIssueListHeader {...defaultProps} />, {router: defaultRouter});
 
-        userEvent.click(
+        await userEvent.click(
           await screen.findByRole('button', {name: 'High Priority Ellipsis Menu'})
         );
 
@@ -611,7 +611,7 @@ describe('CustomViewsHeader', () => {
 
         render(<CustomViewsIssueListHeader {...defaultProps} />, {router: defaultRouter});
 
-        userEvent.click(
+        await userEvent.click(
           await screen.findByRole('button', {name: 'High Priority Ellipsis Menu'})
         );
 
@@ -660,7 +660,7 @@ describe('CustomViewsHeader', () => {
 
         render(<CustomViewsIssueListHeader {...defaultProps} />, {router: defaultRouter});
 
-        userEvent.click(
+        await userEvent.click(
           await screen.findByRole('button', {name: 'High Priority Ellipsis Menu'})
         );
 
@@ -699,7 +699,7 @@ describe('CustomViewsHeader', () => {
           {router: unsavedTabRouter}
         );
 
-        userEvent.click(
+        await userEvent.click(
           await screen.findByRole('button', {name: 'High Priority Ellipsis Menu'})
         );
 
@@ -771,7 +771,7 @@ describe('CustomViewsHeader', () => {
           {router: unsavedTabRouter}
         );
 
-        userEvent.click(
+        await userEvent.click(
           await screen.findByRole('button', {name: 'High Priority Ellipsis Menu'})
         );
 

diff --git a/static/app/views/projectDetail/projectScoreCards/projectAnrScoreCard.spec.tsx b/static/app/views/projectDetail/projectScoreCards/projectAnrScoreCard.spec.tsx
@@ -1,5 +1,5 @@
 import {initializeOrg} from 'sentry-test/initializeOrg';
-import {render, screen, waitFor} from 'sentry-test/reactTestingLibrary';
+import {render, screen} from 'sentry-test/reactTestingLibrary';
 
 import type {PageFilters} from 'sentry/types/core';
 import {ProjectAnrScoreCard} from 'sentry/views/projectDetail/projectScoreCards/projectAnrScoreCard';
@@ -109,8 +109,8 @@ describe('ProjectDetail > ProjectAnr', function () {
       })
     );
 
-    await waitFor(() => expect(screen.getByText('11.56%')).toBeInTheDocument());
-    await waitFor(() => expect(screen.getByText('3%')).toBeInTheDocument());
+    await screen.findByText('11.56%');
+    await screen.findByText('3%');
   });
 
   it('renders open in issues CTA', async function () {
@@ -128,7 +128,7 @@ describe('ProjectDetail > ProjectAnr', function () {
       }
     );
 
-    await waitFor(() => expect(screen.getByText('11.56%')).toBeInTheDocument());
+    await screen.findByText('11.56%');
 
     expect(screen.getByRole('button', {name: 'View Issues'})).toHaveAttribute(
       'href',

diff --git a/static/app/views/relocation/relocation.spec.tsx b/static/app/views/relocation/relocation.spec.tsx
@@ -102,12 +102,12 @@ describe('Relocation', function () {
 
   async function waitForRenderSuccess(step: string) {
     renderPage(step);
-    await waitFor(() => expect(screen.getByTestId(step)).toBeInTheDocument());
+    await screen.findByTestId(step);
   }
 
   async function waitForRenderError(step: string) {
     renderPage(step);
-    await waitFor(() => expect(screen.getByTestId('loading-error')).toBeInTheDocument());
+    await screen.findByTestId('loading-error');
   }
 
   describe('Get Started', function () {
@@ -261,7 +261,7 @@ describe('Relocation', function () {
 
       await userEvent.click(screen.getByRole('button', {name: 'Retry'}));
       await waitFor(() => expect(fetchPublicKeys).toHaveBeenCalledTimes(2));
-      await waitFor(() => expect(screen.getByTestId('get-started')).toBeInTheDocument());
+      await screen.findByTestId('get-started');
 
       await waitFor(() =>
         expect(successfulFetchExistingEarthRelocation).toHaveBeenCalledTimes(1)
@@ -347,7 +347,7 @@ describe('Relocation', function () {
       await userEvent.click(screen.getByRole('button', {name: 'Retry'}));
       await waitFor(() => expect(successfulFetchEarthPublicKey).toHaveBeenCalledTimes(1));
       await waitFor(() => expect(successfulFetchMoonPublicKey).toHaveBeenCalledTimes(2));
-      await waitFor(() => expect(screen.getByTestId('public-key')).toBeInTheDocument());
+      await screen.findByTestId('public-key');
 
       expect(fetchExistingRelocations).toHaveBeenCalledTimes(2);
       expect(screen.queryByText('key.pub')).toBeInTheDocument();