[Cases] Fix cases flaky tests (#176863)

## Summary With the help of @umbopepato, @js-jankisalvi, and @adcoelho, we realized that one possible source of flakiness is the `CasesContextProvider` that every test uses under the hood. In this PR I refactor the `CasesContextProvider` to render immediately its children and not wait for the `appId` and `appTitle` to be defined. I make them optional and make all changes needed in the code to accommodate the presence of an optional `appId`. Also, I refactored some components that I believed could introduce flakiness. ## Issues Fixes #175570 Fixes #175956 Fixes #175935 Fixes #175934 Fixes #175655 Fixes #176643 Fixes #175204 Flaky test runner: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5255, https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5261, https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5264, https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5267 ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenario ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Umberto Pepato <umberto.pepato@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
elastic · Feb 22, 2024 · 0dd21e5 · 0dd21e5
1 parent 162426c
commit 0dd21e5
Show file tree

Hide file tree

Showing 36 changed files with 578 additions and 398 deletions.
diff --git a/x-pack/plugins/cases/public/common/apm/use_cases_transactions.test.ts b/x-pack/plugins/cases/public/common/apm/use_cases_transactions.test.ts
@@ -80,6 +80,15 @@ describe('cases transactions', () => {
       );
       expect(mockAddLabels).toHaveBeenCalledWith({ alert_count: 3 });
     });
+
+    it('should not start any transactions if the app ID is not defined', () => {
+      const { result } = renderUseCreateCaseWithAttachmentsTransaction();
+
+      result.current.startTransaction();
+
+      expect(mockStartTransaction).not.toHaveBeenCalled();
+      expect(mockAddLabels).not.toHaveBeenCalled();
+    });
   });
 
   describe('useAddAttachmentToExistingCaseTransaction', () => {
@@ -104,5 +113,14 @@ describe('cases transactions', () => {
       );
       expect(mockAddLabels).toHaveBeenCalledWith({ alert_count: 3 });
     });
+
+    it('should not start any transactions if the app ID is not defined', () => {
+      const { result } = renderUseAddAttachmentToExistingCaseTransaction();
+
+      result.current.startTransaction({ attachments: bulkAttachments });
+
+      expect(mockStartTransaction).not.toHaveBeenCalled();
+      expect(mockAddLabels).not.toHaveBeenCalled();
+    });
   });
 });
diff --git a/x-pack/plugins/cases/public/common/apm/use_cases_transactions.ts b/x-pack/plugins/cases/public/common/apm/use_cases_transactions.ts
@@ -17,8 +17,8 @@ const BULK_ADD_ATTACHMENT_TO_NEW_CASE = 'bulkAddAttachmentsToNewCase' as const;
 const ADD_ATTACHMENT_TO_EXISTING_CASE = 'addAttachmentToExistingCase' as const;
 const BULK_ADD_ATTACHMENT_TO_EXISTING_CASE = 'bulkAddAttachmentsToExistingCase' as const;
 
-export type StartCreateCaseWithAttachmentsTransaction = (param: {
-  appId: string;
+export type StartCreateCaseWithAttachmentsTransaction = (param?: {
+  appId?: string;
   attachments?: CaseAttachmentsWithoutOwner;
 }) => Transaction | undefined;
 
@@ -28,11 +28,17 @@ export const useCreateCaseWithAttachmentsTransaction = () => {
 
   const startCreateCaseWithAttachmentsTransaction =
     useCallback<StartCreateCaseWithAttachmentsTransaction>(
-      ({ appId, attachments }) => {
+      ({ appId, attachments } = {}) => {
+        if (!appId) {
+          return;
+        }
+
         if (!attachments) {
           return startTransaction(`Cases [${appId}] ${CREATE_CASE}`);
         }
+
         const alertCount = getAlertCount(attachments);
+
         if (alertCount <= 1) {
           return startTransaction(`Cases [${appId}] ${ADD_ATTACHMENT_TO_NEW_CASE}`);
         }
@@ -48,7 +54,7 @@ export const useCreateCaseWithAttachmentsTransaction = () => {
 };
 
 export type StartAddAttachmentToExistingCaseTransaction = (param: {
-  appId: string;
+  appId?: string;
   attachments: CaseAttachmentsWithoutOwner;
 }) => Transaction | undefined;
 
@@ -59,13 +65,20 @@ export const useAddAttachmentToExistingCaseTransaction = () => {
   const startAddAttachmentToExistingCaseTransaction =
     useCallback<StartAddAttachmentToExistingCaseTransaction>(
       ({ appId, attachments }) => {
+        if (!appId) {
+          return;
+        }
+
         const alertCount = getAlertCount(attachments);
+
         if (alertCount <= 1) {
           return startTransaction(`Cases [${appId}] ${ADD_ATTACHMENT_TO_EXISTING_CASE}`);
         }
+
         const transaction = startTransaction(
           `Cases [${appId}] ${BULK_ADD_ATTACHMENT_TO_EXISTING_CASE}`
         );
+
         transaction?.addLabels({ alert_count: alertCount });
         return transaction;
       },

diff --git a/x-pack/plugins/cases/public/common/lib/kibana/__mocks__/index.ts b/x-pack/plugins/cases/public/common/lib/kibana/__mocks__/index.ts
@@ -25,7 +25,6 @@ export const useKibana = jest.fn().mockReturnValue({
 export const useHttp = jest.fn().mockReturnValue(createStartServicesMock().http);
 export const useTimeZone = jest.fn();
 export const useDateFormat = jest.fn();
-export const useBasePath = jest.fn(() => '/test/base/path');
 export const useToasts = jest
   .fn()
   .mockReturnValue(notificationServiceMock.createStartContract().toasts);

diff --git a/x-pack/plugins/cases/public/common/lib/kibana/hooks.ts b/x-pack/plugins/cases/public/common/lib/kibana/hooks.ts
@@ -30,8 +30,6 @@ export const useTimeZone = (): string => {
   return timeZone === 'Browser' ? moment.tz.guess() : timeZone;
 };
 
-export const useBasePath = (): string => useKibana().services.http.basePath.get();
-
 export const useToasts = (): StartServices['notifications']['toasts'] =>
   useKibana().services.notifications.toasts;
 
@@ -116,12 +114,12 @@ export const useCurrentUser = (): AuthenticatedElasticUser | null => {
  * Returns a full URL to the provided page path by using
  * kibana's `getUrlForApp()`
  */
-export const useAppUrl = (appId: string) => {
+export const useAppUrl = (appId?: string) => {
   const { getUrlForApp } = useKibana().services.application;
 
   const getAppUrl = useCallback(
     (options?: { deepLinkId?: string; path?: string; absolute?: boolean }) =>
-      getUrlForApp(appId, options),
+      getUrlForApp(appId ?? '', options),
     [appId, getUrlForApp]
   );
   return { getAppUrl };
@@ -131,7 +129,7 @@ export const useAppUrl = (appId: string) => {
  * Navigate to any app using kibana's `navigateToApp()`
  * or by url using `navigateToUrl()`
  */
-export const useNavigateTo = (appId: string) => {
+export const useNavigateTo = (appId?: string) => {
   const { navigateToApp, navigateToUrl } = useKibana().services.application;
 
   const navigateTo = useCallback(
@@ -144,7 +142,7 @@ export const useNavigateTo = (appId: string) => {
       if (url) {
         navigateToUrl(url);
       } else {
-        navigateToApp(appId, options);
+        navigateToApp(appId ?? '', options);
       }
     },
     [appId, navigateToApp, navigateToUrl]
@@ -156,7 +154,7 @@ export const useNavigateTo = (appId: string) => {
  * Returns navigateTo and getAppUrl navigation hooks
  *
  */
-export const useNavigation = (appId: string) => {
+export const useNavigation = (appId?: string) => {
   const { navigateTo } = useNavigateTo(appId);
   const { getAppUrl } = useAppUrl(appId);
   return { navigateTo, getAppUrl };

diff --git a/x-pack/plugins/cases/public/common/lib/kibana/kibana_react.mock.tsx b/x-pack/plugins/cases/public/common/lib/kibana/kibana_react.mock.tsx
@@ -9,6 +9,7 @@ import React from 'react';
 import { BehaviorSubject } from 'rxjs';
 
 import type { PublicAppInfo } from '@kbn/core/public';
+import { AppStatus } from '@kbn/core/public';
 import type { RecursivePartial } from '@elastic/eui/src/components/common';
 import { coreMock } from '@kbn/core/public/mocks';
 import { KibanaContextProvider } from '@kbn/kibana-react-plugin/public';
@@ -52,7 +53,21 @@ export const createStartServicesMock = ({ license }: StartServiceArgs = {}): Sta
 
   services.application.currentAppId$ = new BehaviorSubject<string>('testAppId');
   services.application.applications$ = new BehaviorSubject<Map<string, PublicAppInfo>>(
-    new Map([['testAppId', { category: { label: 'Test' } } as unknown as PublicAppInfo]])
+    new Map([
+      [
+        'testAppId',
+        {
+          id: 'testAppId',
+          title: 'test-title',
+          category: { id: 'test-label-id', label: 'Test' },
+          status: AppStatus.accessible,
+          visibleIn: ['globalSearch'],
+          appRoute: `/app/some-id`,
+          keywords: [],
+          deepLinks: [],
+        },
+      ],
+    ])
   );
 
   services.triggersActionsUi.actionTypeRegistry.get = jest.fn().mockReturnValue({

diff --git a/x-pack/plugins/cases/public/common/mock/test_providers.tsx b/x-pack/plugins/cases/public/common/mock/test_providers.tsx
@@ -19,7 +19,7 @@ import type { ScopedFilesClient } from '@kbn/files-plugin/public';
 import { euiDarkVars } from '@kbn/ui-theme';
 import { I18nProvider } from '@kbn/i18n-react';
 import { createMockFilesClient } from '@kbn/shared-ux-file-mocks';
-import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
+import { QueryClient } from '@tanstack/react-query';
 
 import { KibanaContextProvider } from '@kbn/kibana-react-plugin/public';
 import { FilesContext } from '@kbn/shared-ux-file-context';
@@ -108,10 +108,9 @@ const TestProvidersComponent: React.FC<TestProviderProps> = ({
                 permissions,
                 getFilesClient,
               }}
+              queryClient={queryClient}
             >
-              <QueryClientProvider client={queryClient}>
-                <FilesContext client={createMockFilesClient()}>{children}</FilesContext>
-              </QueryClientProvider>
+              <FilesContext client={createMockFilesClient()}>{children}</FilesContext>
             </CasesProvider>
           </MemoryRouter>
         </ThemeProvider>
@@ -191,8 +190,9 @@ export const createAppMockRenderer = ({
                 releasePhase,
                 getFilesClient,
               }}
+              queryClient={queryClient}
             >
-              <QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
+              {children}
             </CasesProvider>
           </MemoryRouter>
         </ThemeProvider>

diff --git a/x-pack/plugins/cases/public/common/use_cases_toast.test.tsx b/x-pack/plugins/cases/public/common/use_cases_toast.test.tsx
@@ -14,13 +14,16 @@ import { alertComment, basicComment, mockCase } from '../containers/mock';
 import React from 'react';
 import userEvent from '@testing-library/user-event';
 import type { SupportedCaseAttachment } from '../types';
-import { getByTestId } from '@testing-library/react';
+import { getByTestId, queryByTestId, screen } from '@testing-library/react';
 import { OWNER_INFO } from '../../common/constants';
+import { useCasesContext } from '../components/cases_context/use_cases_context';
 
 jest.mock('./lib/kibana');
+jest.mock('../components/cases_context/use_cases_context');
 
 const useToastsMock = useToasts as jest.Mock;
 const useKibanaMock = useKibana as jest.Mocked<typeof useKibana>;
+const useCasesContextMock = useCasesContext as jest.Mock;
 
 describe('Use cases toast hook', () => {
   const successMock = jest.fn();
@@ -66,6 +69,8 @@ describe('Use cases toast hook', () => {
       getUrlForApp,
       navigateToUrl,
     };
+
+    useCasesContextMock.mockReturnValue({ appId: 'testAppId' });
   });
 
   describe('showSuccessAttach', () => {
@@ -147,6 +152,7 @@ describe('Use cases toast hook', () => {
     describe('Toast content', () => {
       let appMockRender: AppMockRenderer;
       const onViewCaseClick = jest.fn();
+
       beforeEach(() => {
         appMockRender = createAppMockRenderer();
         onViewCaseClick.mockReset();
@@ -213,9 +219,16 @@ describe('Use cases toast hook', () => {
         const result = appMockRender.render(
           <CaseToastSuccessContent onViewCaseClick={onViewCaseClick} />
         );
+
         userEvent.click(result.getByTestId('toaster-content-case-view-link'));
         expect(onViewCaseClick).toHaveBeenCalled();
       });
+
+      it('hides the view case link when onViewCaseClick is not defined', () => {
+        appMockRender.render(<CaseToastSuccessContent />);
+
+        expect(screen.queryByTestId('toaster-content-case-view-link')).not.toBeInTheDocument();
+      });
     });
 
     describe('Toast navigation', () => {
@@ -267,6 +280,31 @@ describe('Use cases toast hook', () => {
           path: '/mock-id',
         });
       });
+
+      it('does not navigates to a case if the appId is not defined', () => {
+        useCasesContextMock.mockReturnValue({ appId: undefined });
+
+        const { result } = renderHook(
+          () => {
+            return useCasesToast();
+          },
+          { wrapper: TestProviders }
+        );
+
+        result.current.showSuccessAttach({
+          theCase: { ...mockCase, owner: 'in-valid' },
+          title: 'Custom title',
+        });
+
+        const mockParams = successMock.mock.calls[0][0];
+        const el = document.createElement('div');
+        mockParams.text(el);
+        const button = queryByTestId(el, 'toaster-content-case-view-link');
+
+        expect(button).toBeNull();
+        expect(getUrlForApp).not.toHaveBeenCalled();
+        expect(navigateToUrl).not.toHaveBeenCalled();
+      });
     });
   });
 

diff --git a/x-pack/plugins/cases/public/common/use_cases_toast.tsx b/x-pack/plugins/cases/public/common/use_cases_toast.tsx
@@ -140,13 +140,18 @@ export const useCasesToast = () => {
         ? OWNER_INFO[theCase.owner].appId
         : appId;
 
-      const url = getUrlForApp(appIdToNavigateTo, {
-        deepLinkId: 'cases',
-        path: generateCaseViewPath({ detailName: theCase.id }),
-      });
+      const url =
+        appIdToNavigateTo != null
+          ? getUrlForApp(appIdToNavigateTo, {
+              deepLinkId: 'cases',
+              path: generateCaseViewPath({ detailName: theCase.id }),
+            })
+          : null;
 
       const onViewCaseClick = () => {
-        navigateToUrl(url);
+        if (url) {
+          navigateToUrl(url);
+        }
       };
 
       const renderTitle = getToastTitle({ theCase, title, attachments });
@@ -157,7 +162,10 @@ export const useCasesToast = () => {
         iconType: 'check',
         title: toMountPoint(<Title>{renderTitle}</Title>),
         text: toMountPoint(
-          <CaseToastSuccessContent content={renderContent} onViewCaseClick={onViewCaseClick} />
+          <CaseToastSuccessContent
+            content={renderContent}
+            onViewCaseClick={url != null ? onViewCaseClick : undefined}
+          />
         ),
       });
     },
@@ -186,7 +194,7 @@ export const CaseToastSuccessContent = ({
   onViewCaseClick,
   content,
 }: {
-  onViewCaseClick: () => void;
+  onViewCaseClick?: () => void;
   content?: string;
 }) => {
   return (
@@ -196,14 +204,16 @@ export const CaseToastSuccessContent = ({
           {content}
         </EuiTextStyled>
       ) : null}
-      <EuiButtonEmpty
-        size="xs"
-        flush="left"
-        onClick={onViewCaseClick}
-        data-test-subj="toaster-content-case-view-link"
-      >
-        {VIEW_CASE}
-      </EuiButtonEmpty>
+      {onViewCaseClick !== undefined ? (
+        <EuiButtonEmpty
+          size="xs"
+          flush="left"
+          onClick={onViewCaseClick}
+          data-test-subj="toaster-content-case-view-link"
+        >
+          {VIEW_CASE}
+        </EuiButtonEmpty>
+      ) : null}
     </>
   );
 };

diff --git a/x-pack/plugins/cases/public/components/add_comment/index.tsx b/x-pack/plugins/cases/public/components/add_comment/index.tsx
@@ -75,7 +75,7 @@ export const AddComment = React.memo(
       const [focusOnContext, setFocusOnContext] = useState(false);
       const { permissions, owner, appId } = useCasesContext();
       const { isLoading, mutate: createAttachments } = useCreateAttachments();
-      const draftStorageKey = getMarkdownEditorStorageKey(appId, caseId, id);
+      const draftStorageKey = getMarkdownEditorStorageKey({ appId, caseId, commentId: id });
 
       const { form } = useForm<AddCommentFormSchema>({
         defaultValue: initialCommentValue,

diff --git a/x-pack/plugins/cases/public/components/all_cases/table_filter_config/use_filter_config.tsx b/x-pack/plugins/cases/public/components/all_cases/table_filter_config/use_filter_config.tsx
@@ -14,7 +14,7 @@ import { LOCAL_STORAGE_KEYS } from '../../../../common/constants';
 import type { FilterConfig, FilterConfigState } from './types';
 import { useCustomFieldsFilterConfig } from './use_custom_fields_filter_config';
 import { useCasesContext } from '../../cases_context/use_cases_context';
-import { deflattenCustomFieldKey, isFlattenCustomField } from '../utils';
+import { deflattenCustomFieldKey, getLocalStorageKey, isFlattenCustomField } from '../utils';
 
 const mergeSystemAndCustomFieldConfigs = ({
   systemFilterConfig,
@@ -52,7 +52,7 @@ const shouldBeActive = ({
 const useActiveByFilterKeyState = ({ filterOptions }: { filterOptions: FilterOptions }) => {
   const { appId } = useCasesContext();
   const [activeByFilterKey, setActiveByFilterKey] = useLocalStorage<FilterConfigState[]>(
-    `${appId}.${LOCAL_STORAGE_KEYS.casesTableFiltersConfig}`,
+    getLocalStorageKey(LOCAL_STORAGE_KEYS.casesTableFiltersConfig, appId),
     []
   );