Browse Source

ref: Consolidate test related rules into plugin specific eslint config 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.
Ryan Albrecht 2 months ago
parent
commit
2209d58f24

+ 49 - 30
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
     },
   },
   {

+ 2 - 2
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));
   });

+ 8 - 14
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,11 +126,11 @@ 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();
@@ -144,7 +138,7 @@ describe('NewSidebar', function () {
     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();
   });
 });

+ 3 - 3
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());
   });
 });

+ 2 - 6
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');
     });
   });
 

+ 8 - 8
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'})
         );
 

+ 4 - 4
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',

+ 4 - 4
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();

+ 3 - 9
static/app/views/replays/list/listContent.spec.tsx

@@ -88,9 +88,7 @@ describe('ReplayList', () => {
       organization: mockOrg,
     });
 
-    await waitFor(() =>
-      expect(screen.getByText('Get to the root cause faster')).toBeInTheDocument()
-    );
+    await screen.findByText('Get to the root cause faster');
     expect(mockFetchReplayListRequest).not.toHaveBeenCalled();
   });
 
@@ -110,9 +108,7 @@ describe('ReplayList', () => {
       organization: mockOrg,
     });
 
-    await waitFor(() =>
-      expect(screen.getByText('Get to the root cause faster')).toBeInTheDocument()
-    );
+    await screen.findByText('Get to the root cause faster');
     expect(mockFetchReplayListRequest).not.toHaveBeenCalled();
   });
 
@@ -132,9 +128,7 @@ describe('ReplayList', () => {
       organization: mockOrg,
     });
 
-    await waitFor(() =>
-      expect(screen.getByText('Get to the root cause faster')).toBeInTheDocument()
-    );
+    await screen.findByText('Get to the root cause faster');
     expect(mockFetchReplayListRequest).not.toHaveBeenCalled();
   });
 

+ 3 - 3
static/app/views/replays/list/setupReplaysCTA.spec.tsx

@@ -1,4 +1,4 @@
-import {render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary';
+import {render, screen, userEvent} from 'sentry-test/reactTestingLibrary';
 
 import {SetupReplaysCTA} from 'sentry/views/replays/list/replayOnboardingPanel';
 
@@ -12,7 +12,7 @@ describe('SetupReplaysCTA', () => {
     render(<SetupReplaysCTA primaryAction="setup" orgSlug="foo" disabled />);
     const setupBtn = screen.getByTestId('setup-replays-btn');
     await userEvent.hover(setupBtn);
-    await waitFor(() => screen.getByTestId('setup-replays-tooltip'));
+    await screen.findByTestId('setup-replays-tooltip');
     expect(screen.getByTestId('setup-replays-tooltip')).toBeInTheDocument();
   });
 
@@ -27,7 +27,7 @@ describe('SetupReplaysCTA', () => {
     render(<SetupReplaysCTA primaryAction="create" orgSlug="foo" disabled />);
     const createBtn = screen.getByTestId('create-project-btn');
     await userEvent.hover(createBtn);
-    await waitFor(() => screen.getByTestId('create-project-tooltip'));
+    await screen.findByTestId('create-project-tooltip');
     expect(screen.getByTestId('create-project-tooltip')).toBeInTheDocument();
   });
 });

Some files were not shown because too many files changed in this diff