From 683d03a6c3faa5b7ade46c1d08e1c8eddb387fba Mon Sep 17 00:00:00 2001 From: Davis McPhee Date: Tue, 3 Dec 2024 15:29:07 -0400 Subject: [PATCH] [Discover] Fix flaky cell actions tests (#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 #193367. Resolves #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) (cherry picked from commit 01de8870602d032f0c49c791552966fa556e634e) --- .../main/data_fetching/fetch_all.test.ts | 3 +++ .../main/hooks/use_esql_mode.test.tsx | 15 ++++++----- .../discover_app_state_container.test.ts | 17 ++++++------ .../discover_app_state_container.ts | 3 +++ .../discover_data_state_container.test.ts | 5 ++-- .../discover_data_state_container.ts | 7 +++++ .../discover_internal_state_container.ts | 26 +++++++++++++++---- .../utils/change_data_view.ts | 12 +++++---- .../utils/get_default_profile_state.test.ts | 6 +++++ 9 files changed, 67 insertions(+), 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 index c7b80181867e0..bbf893a937171 100644 --- 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 index 410406b7eb2d7..f50b8135e2a9f 100644 --- 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/src/plugins/discover/public/application/main/state_management/discover_app_state_container.test.ts b/src/plugins/discover/public/application/main/state_management/discover_app_state_container.test.ts index 361688f10c393..41a4569a37683 100644 --- a/src/plugins/discover/public/application/main/state_management/discover_app_state_container.test.ts +++ b/src/plugins/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/src/plugins/discover/public/application/main/state_management/discover_app_state_container.ts b/src/plugins/discover/public/application/main/state_management/discover_app_state_container.ts index 2e2fd0c6be084..af3541d89c376 100644 --- a/src/plugins/discover/public/application/main/state_management/discover_app_state_container.ts +++ b/src/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/src/plugins/discover/public/application/main/state_management/discover_data_state_container.test.ts b/src/plugins/discover/public/application/main/state_management/discover_data_state_container.test.ts index 40b688c0d2152..810bc7ffe9766 100644 --- a/src/plugins/discover/public/application/main/state_management/discover_data_state_container.test.ts +++ b/src/plugins/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/src/plugins/discover/public/application/main/state_management/discover_data_state_container.ts b/src/plugins/discover/public/application/main/state_management/discover_data_state_container.ts index 5bc27fdb45a60..478d0ca06ea1c 100644 --- a/src/plugins/discover/public/application/main/state_management/discover_data_state_container.ts +++ b/src/plugins/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(DEFAULT_COLUMNS_SETTING, []); const postFetchStateUpdate = defaultProfileState?.getPostFetchState({ diff --git a/src/plugins/discover/public/application/main/state_management/discover_internal_state_container.ts b/src/plugins/discover/public/application/main/state_management/discover_internal_state_container.ts index 67b6551e985f4..8aad8eb91738b 100644 --- a/src/plugins/discover/public/application/main/state_management/discover_internal_state_container.ts +++ b/src/plugins/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; } 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) => ({ ...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 index d2815800fa9c6..8d5b429c43dd7 100644 --- 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/src/plugins/discover/public/application/main/state_management/utils/get_default_profile_state.test.ts b/src/plugins/discover/public/application/main/state_management/utils/get_default_profile_state.test.ts index 6ba709bdd04ec..a3a904260f2f5 100644 --- a/src/plugins/discover/public/application/main/state_management/utils/get_default_profile_state.test.ts +++ b/src/plugins/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,