[Discover] Fix flaky cell actions tests (elastic#202097)

## Summary This PR fixes a tricky race condition when setting default Discover profile state, which was causing functional test failures such as the linked cell actions tests. Resolves elastic#193367. Resolves elastic#193400. ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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 scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
viduni94 · Dec 3, 2024 · 01de887 · 01de887
1 parent 530b4d4
commit 01de887
Show file tree

Hide file tree

Showing 9 changed files with 67 additions and 27 deletions.
diff --git a/src/plugins/discover/public/application/main/data_fetching/fetch_all.test.ts b/src/plugins/discover/public/application/main/data_fetching/fetch_all.test.ts
@@ -76,6 +76,7 @@ describe('test fetchAll', () => {
         customFilters: [],
         overriddenVisContextAfterInvalidation: undefined,
         resetDefaultProfileState: {
+          resetId: 'test',
           columns: false,
           rowHeight: false,
           breakdownField: false,
@@ -269,6 +270,7 @@ describe('test fetchAll', () => {
         customFilters: [],
         overriddenVisContextAfterInvalidation: undefined,
         resetDefaultProfileState: {
+          resetId: 'test',
           columns: false,
           rowHeight: false,
           breakdownField: false,
@@ -392,6 +394,7 @@ describe('test fetchAll', () => {
           customFilters: [],
           overriddenVisContextAfterInvalidation: undefined,
           resetDefaultProfileState: {
+            resetId: 'test',
             columns: false,
             rowHeight: false,
             breakdownField: false,

diff --git a/src/plugins/discover/public/application/main/hooks/use_esql_mode.test.tsx b/src/plugins/discover/public/application/main/hooks/use_esql_mode.test.tsx
@@ -25,6 +25,7 @@ import { DiscoverStateContainer } from '../state_management/discover_state';
 import { VIEW_MODE } from '@kbn/saved-search-plugin/public';
 import { dataViewAdHoc } from '../../../__mocks__/data_view_complex';
 import { buildDataTableRecord, EsHitRecord } from '@kbn/discover-utils';
+import { omit } from 'lodash';
 
 function getHookProps(
   query: AggregateQuery | Query | undefined,
@@ -501,7 +502,7 @@ describe('useEsqlMode', () => {
       FetchStatus.LOADING
     );
     const documents$ = stateContainer.dataState.data$.documents$;
-    expect(stateContainer.internalState.get().resetDefaultProfileState).toEqual({
+    expect(omit(stateContainer.internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
       columns: false,
       rowHeight: false,
       breakdownField: false,
@@ -516,7 +517,7 @@ describe('useEsqlMode', () => {
       query: { esql: 'from pattern1' },
     });
     await waitFor(() =>
-      expect(stateContainer.internalState.get().resetDefaultProfileState).toEqual({
+      expect(omit(stateContainer.internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
         columns: true,
         rowHeight: true,
         breakdownField: true,
@@ -537,7 +538,7 @@ describe('useEsqlMode', () => {
       query: { esql: 'from pattern1' },
     });
     await waitFor(() =>
-      expect(stateContainer.internalState.get().resetDefaultProfileState).toEqual({
+      expect(omit(stateContainer.internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
         columns: false,
         rowHeight: false,
         breakdownField: false,
@@ -553,7 +554,7 @@ describe('useEsqlMode', () => {
       query: { esql: 'from pattern2' },
     });
     await waitFor(() =>
-      expect(stateContainer.internalState.get().resetDefaultProfileState).toEqual({
+      expect(omit(stateContainer.internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
         columns: true,
         rowHeight: true,
         breakdownField: true,
@@ -570,7 +571,7 @@ describe('useEsqlMode', () => {
     const documents$ = stateContainer.dataState.data$.documents$;
     const result1 = [buildDataTableRecord({ message: 'foo' } as EsHitRecord)];
     const result2 = [buildDataTableRecord({ message: 'foo', extension: 'bar' } as EsHitRecord)];
-    expect(stateContainer.internalState.get().resetDefaultProfileState).toEqual({
+    expect(omit(stateContainer.internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
       columns: false,
       rowHeight: false,
       breakdownField: false,
@@ -581,7 +582,7 @@ describe('useEsqlMode', () => {
       result: result1,
     });
     await waitFor(() =>
-      expect(stateContainer.internalState.get().resetDefaultProfileState).toEqual({
+      expect(omit(stateContainer.internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
         columns: false,
         rowHeight: false,
         breakdownField: false,
@@ -593,7 +594,7 @@ describe('useEsqlMode', () => {
       result: result2,
     });
     await waitFor(() =>
-      expect(stateContainer.internalState.get().resetDefaultProfileState).toEqual({
+      expect(omit(stateContainer.internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
         columns: true,
         rowHeight: false,
         breakdownField: false,

diff --git a/...ns/discover/public/application/main/state_management/discover_app_state_container.test.ts b/...ns/discover/public/application/main/state_management/discover_app_state_container.test.ts
@@ -26,6 +26,7 @@ import {
   getSavedSearchContainer,
 } from './discover_saved_search_container';
 import { getDiscoverGlobalStateContainer } from './discover_global_state_container';
+import { omit } from 'lodash';
 
 let history: History;
 let stateStorage: IKbnUrlStateStorage;
@@ -269,13 +270,13 @@ describe('Test discover app state container', () => {
   describe('initAndSync', () => {
     it('should call setResetDefaultProfileState correctly with no initial state', () => {
       const state = getStateContainer();
-      expect(internalState.get().resetDefaultProfileState).toEqual({
+      expect(omit(internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
         columns: false,
         rowHeight: false,
         breakdownField: false,
       });
       state.initAndSync();
-      expect(internalState.get().resetDefaultProfileState).toEqual({
+      expect(omit(internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
         columns: true,
         rowHeight: true,
         breakdownField: true,
@@ -286,13 +287,13 @@ describe('Test discover app state container', () => {
       const stateStorageGetSpy = jest.spyOn(stateStorage, 'get');
       stateStorageGetSpy.mockReturnValue({ columns: ['test'] });
       const state = getStateContainer();
-      expect(internalState.get().resetDefaultProfileState).toEqual({
+      expect(omit(internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
         columns: false,
         rowHeight: false,
         breakdownField: false,
       });
       state.initAndSync();
-      expect(internalState.get().resetDefaultProfileState).toEqual({
+      expect(omit(internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
         columns: false,
         rowHeight: true,
         breakdownField: true,
@@ -303,13 +304,13 @@ describe('Test discover app state container', () => {
       const stateStorageGetSpy = jest.spyOn(stateStorage, 'get');
       stateStorageGetSpy.mockReturnValue({ rowHeight: 5 });
       const state = getStateContainer();
-      expect(internalState.get().resetDefaultProfileState).toEqual({
+      expect(omit(internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
         columns: false,
         rowHeight: false,
         breakdownField: false,
       });
       state.initAndSync();
-      expect(internalState.get().resetDefaultProfileState).toEqual({
+      expect(omit(internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
         columns: true,
         rowHeight: false,
         breakdownField: true,
@@ -326,13 +327,13 @@ describe('Test discover app state container', () => {
         managed: false,
       });
       const state = getStateContainer();
-      expect(internalState.get().resetDefaultProfileState).toEqual({
+      expect(omit(internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
         columns: false,
         rowHeight: false,
         breakdownField: false,
       });
       state.initAndSync();
-      expect(internalState.get().resetDefaultProfileState).toEqual({
+      expect(omit(internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
         columns: false,
         rowHeight: false,
         breakdownField: false,

diff --git a/...plugins/discover/public/application/main/state_management/discover_app_state_container.ts b/...plugins/discover/public/application/main/state_management/discover_app_state_container.ts
@@ -262,9 +262,12 @@ export const getDiscoverAppStateContainer = ({
 
     addLog('[appState] initialize state and sync with URL', currentSavedSearch);
 
+    // Set the default profile state only if not loading a saved search,
+    // to avoid overwriting saved search state
     if (!currentSavedSearch.id) {
       const { breakdownField, columns, rowHeight } = getCurrentUrlState(stateStorage, services);
 
+      // Only set default state which is not already set in the URL
       internalStateContainer.transitions.setResetDefaultProfileState({
         columns: columns === undefined,
         rowHeight: rowHeight === undefined,

diff --git a/...s/discover/public/application/main/state_management/discover_data_state_container.test.ts b/...s/discover/public/application/main/state_management/discover_data_state_container.test.ts
@@ -16,6 +16,7 @@ import { FetchStatus } from '../../types';
 import { DataDocuments$ } from './discover_data_state_container';
 import { getDiscoverStateMock } from '../../../__mocks__/discover_state.mock';
 import { fetchDocuments } from '../data_fetching/fetch_documents';
+import { omit } from 'lodash';
 
 jest.mock('../data_fetching/fetch_documents', () => ({
   fetchDocuments: jest.fn().mockResolvedValue({ records: [] }),
@@ -190,7 +191,7 @@ describe('test getDataStateContainer', () => {
     await waitFor(() => {
       expect(dataState.data$.main$.value.fetchStatus).toBe(FetchStatus.COMPLETE);
     });
-    expect(stateContainer.internalState.get().resetDefaultProfileState).toEqual({
+    expect(omit(stateContainer.internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
       columns: false,
       rowHeight: false,
       breakdownField: false,
@@ -221,7 +222,7 @@ describe('test getDataStateContainer', () => {
     await waitFor(() => {
       expect(dataState.data$.main$.value.fetchStatus).toBe(FetchStatus.COMPLETE);
     });
-    expect(stateContainer.internalState.get().resetDefaultProfileState).toEqual({
+    expect(omit(stateContainer.internalState.get().resetDefaultProfileState, 'resetId')).toEqual({
       columns: false,
       rowHeight: false,
       breakdownField: false,

diff --git a/...lugins/discover/public/application/main/state_management/discover_data_state_container.ts b/...lugins/discover/public/application/main/state_management/discover_data_state_container.ts
@@ -293,6 +293,13 @@ export function getDataStateContainer({
               ...commonFetchDeps,
             },
             async () => {
+              const { resetDefaultProfileState: currentResetDefaultProfileState } =
+                internalStateContainer.getState();
+
+              if (currentResetDefaultProfileState.resetId !== resetDefaultProfileState.resetId) {
+                return;
+              }
+
               const { esqlQueryColumns } = dataSubjects.documents$.getValue();
               const defaultColumns = uiSettings.get<string[]>(DEFAULT_COLUMNS_SETTING, []);
               const postFetchStateUpdate = defaultProfileState?.getPostFetchState({

diff --git a/...ns/discover/public/application/main/state_management/discover_internal_state_container.ts b/...ns/discover/public/application/main/state_management/discover_internal_state_container.ts
@@ -7,6 +7,7 @@
  * License v3.0 only", or the "Server Side Public License, v 1".
  */
 
+import { v4 as uuidv4 } from 'uuid';
 import {
   createStateContainer,
   createStateContainerReactHelpers,
@@ -26,7 +27,12 @@ export interface InternalState {
   customFilters: Filter[];
   overriddenVisContextAfterInvalidation: UnifiedHistogramVisContext | {} | undefined; // it will be used during saved search saving
   isESQLToDataViewTransitionModalVisible?: boolean;
-  resetDefaultProfileState: { columns: boolean; rowHeight: boolean; breakdownField: boolean };
+  resetDefaultProfileState: {
+    resetId: string;
+    columns: boolean;
+    rowHeight: boolean;
+    breakdownField: boolean;
+  };
 }
 
 export interface InternalStateTransitions {
@@ -56,7 +62,9 @@ export interface InternalStateTransitions {
   ) => (isVisible: boolean) => InternalState;
   setResetDefaultProfileState: (
     state: InternalState
-  ) => (resetDefaultProfileState: InternalState['resetDefaultProfileState']) => InternalState;
+  ) => (
+    resetDefaultProfileState: Omit<InternalState['resetDefaultProfileState'], 'resetId'>
+  ) => InternalState;
 }
 
 export type DiscoverInternalStateContainer = ReduxLikeStateContainer<
@@ -77,7 +85,12 @@ export function getInternalStateContainer() {
       expandedDoc: undefined,
       customFilters: [],
       overriddenVisContextAfterInvalidation: undefined,
-      resetDefaultProfileState: { columns: false, rowHeight: false, breakdownField: false },
+      resetDefaultProfileState: {
+        resetId: '',
+        columns: false,
+        rowHeight: false,
+        breakdownField: false,
+      },
     },
     {
       setDataView: (prevState: InternalState) => (nextDataView: DataView) => ({
@@ -151,9 +164,12 @@ export function getInternalStateContainer() {
       }),
       setResetDefaultProfileState:
         (prevState: InternalState) =>
-        (resetDefaultProfileState: InternalState['resetDefaultProfileState']) => ({
+        (resetDefaultProfileState: Omit<InternalState['resetDefaultProfileState'], 'resetId'>) => ({
           ...prevState,
-          resetDefaultProfileState,
+          resetDefaultProfileState: {
+            ...resetDefaultProfileState,
+            resetId: uuidv4(),
+          },
         }),
     },
     {},

diff --git a/src/plugins/discover/public/application/main/state_management/utils/change_data_view.ts b/src/plugins/discover/public/application/main/state_management/utils/change_data_view.ts
@@ -43,11 +43,6 @@ export async function changeDataView(
   let nextDataView: DataView | null = null;
 
   internalState.transitions.setIsDataViewLoading(true);
-  internalState.transitions.setResetDefaultProfileState({
-    columns: true,
-    rowHeight: true,
-    breakdownField: true,
-  });
 
   try {
     nextDataView = typeof id === 'string' ? await dataViews.get(id, false) : id;
@@ -56,6 +51,13 @@ export async function changeDataView(
   }
 
   if (nextDataView && dataView) {
+    // Reset the default profile state if we are switching to a different data view
+    internalState.transitions.setResetDefaultProfileState({
+      columns: true,
+      rowHeight: true,
+      breakdownField: true,
+    });
+
     const nextAppState = getDataViewAppState(
       dataView,
       nextDataView,

diff --git a/...discover/public/application/main/state_management/utils/get_default_profile_state.test.ts b/...discover/public/application/main/state_management/utils/get_default_profile_state.test.ts
@@ -27,6 +27,7 @@ describe('getDefaultProfileState', () => {
       let appState = getDefaultProfileState({
         profilesManager: profilesManagerMock,
         resetDefaultProfileState: {
+          resetId: 'test',
           columns: false,
           rowHeight: false,
           breakdownField: true,
@@ -39,6 +40,7 @@ describe('getDefaultProfileState', () => {
       appState = getDefaultProfileState({
         profilesManager: profilesManagerMock,
         resetDefaultProfileState: {
+          resetId: 'test',
           columns: false,
           rowHeight: false,
           breakdownField: true,
@@ -54,6 +56,7 @@ describe('getDefaultProfileState', () => {
       let appState = getDefaultProfileState({
         profilesManager: profilesManagerMock,
         resetDefaultProfileState: {
+          resetId: 'test',
           columns: true,
           rowHeight: false,
           breakdownField: false,
@@ -79,6 +82,7 @@ describe('getDefaultProfileState', () => {
       appState = getDefaultProfileState({
         profilesManager: profilesManagerMock,
         resetDefaultProfileState: {
+          resetId: 'test',
           columns: true,
           rowHeight: false,
           breakdownField: false,
@@ -107,6 +111,7 @@ describe('getDefaultProfileState', () => {
       const appState = getDefaultProfileState({
         profilesManager: profilesManagerMock,
         resetDefaultProfileState: {
+          resetId: 'test',
           columns: false,
           rowHeight: true,
           breakdownField: false,
@@ -125,6 +130,7 @@ describe('getDefaultProfileState', () => {
       const appState = getDefaultProfileState({
         profilesManager: profilesManagerMock,
         resetDefaultProfileState: {
+          resetId: 'test',
           columns: false,
           rowHeight: false,
           breakdownField: false,