Skip to content

Commit

Permalink
[8.x] [Discover] Fix flaky cell actions tests (#202097) (#202805)
Browse files Browse the repository at this point in the history
# Backport

This will backport the following commits from `main` to `8.x`:
- [[Discover] Fix flaky cell actions tests
(#202097)](#202097)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Davis
McPhee","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-03T19:29:07Z","message":"[Discover]
Fix flaky cell actions tests (#202097)\n\n## Summary\r\n\r\nThis PR
fixes a tricky race condition when setting default Discover\r\nprofile
state, which was causing functional test failures such as the\r\nlinked
cell actions tests.\r\n\r\nResolves #193367.\r\nResolves
#193400.\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [x] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] If a plugin
configuration key changed, check if it needs to be\r\nallowlisted in the
cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[ ] This was checked for breaking HTTP API changes, and any
breaking\r\nchanges have been approved by the breaking-change committee.
The\r\n`release_note:breaking` label should be applied in these
situations.\r\n- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [x] The PR description includes
the appropriate Release Notes section,\r\nand the correct
`release_note:*` label is applied per
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"01de8870602d032f0c49c791552966fa556e634e","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Discover","release_note:skip","v9.0.0","Team:DataDiscovery","backport:prev-minor"],"title":"[Discover]
Fix flaky cell actions
tests","number":202097,"url":"https://github.com/elastic/kibana/pull/202097","mergeCommit":{"message":"[Discover]
Fix flaky cell actions tests (#202097)\n\n## Summary\r\n\r\nThis PR
fixes a tricky race condition when setting default Discover\r\nprofile
state, which was causing functional test failures such as the\r\nlinked
cell actions tests.\r\n\r\nResolves #193367.\r\nResolves
#193400.\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [x] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] If a plugin
configuration key changed, check if it needs to be\r\nallowlisted in the
cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[ ] This was checked for breaking HTTP API changes, and any
breaking\r\nchanges have been approved by the breaking-change committee.
The\r\n`release_note:breaking` label should be applied in these
situations.\r\n- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [x] The PR description includes
the appropriate Release Notes section,\r\nand the correct
`release_note:*` label is applied per
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"01de8870602d032f0c49c791552966fa556e634e"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/202097","number":202097,"mergeCommit":{"message":"[Discover]
Fix flaky cell actions tests (#202097)\n\n## Summary\r\n\r\nThis PR
fixes a tricky race condition when setting default Discover\r\nprofile
state, which was causing functional test failures such as the\r\nlinked
cell actions tests.\r\n\r\nResolves #193367.\r\nResolves
#193400.\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [x] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] If a plugin
configuration key changed, check if it needs to be\r\nallowlisted in the
cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[ ] This was checked for breaking HTTP API changes, and any
breaking\r\nchanges have been approved by the breaking-change committee.
The\r\n`release_note:breaking` label should be applied in these
situations.\r\n- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [x] The PR description includes
the appropriate Release Notes section,\r\nand the correct
`release_note:*` label is applied per
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"01de8870602d032f0c49c791552966fa556e634e"}}]}]
BACKPORT-->

Co-authored-by: Davis McPhee <[email protected]>
  • Loading branch information
kibanamachine and davismcphee authored Dec 3, 2024
1 parent 0e1734d commit 2f42e16
Show file tree
Hide file tree
Showing 9 changed files with 67 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ describe('test fetchAll', () => {
customFilters: [],
overriddenVisContextAfterInvalidation: undefined,
resetDefaultProfileState: {
resetId: 'test',
columns: false,
rowHeight: false,
breakdownField: false,
Expand Down Expand Up @@ -269,6 +270,7 @@ describe('test fetchAll', () => {
customFilters: [],
overriddenVisContextAfterInvalidation: undefined,
resetDefaultProfileState: {
resetId: 'test',
columns: false,
rowHeight: false,
breakdownField: false,
Expand Down Expand Up @@ -392,6 +394,7 @@ describe('test fetchAll', () => {
customFilters: [],
overriddenVisContextAfterInvalidation: undefined,
resetDefaultProfileState: {
resetId: 'test',
columns: false,
rowHeight: false,
breakdownField: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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: [] }),
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 {
Expand Down Expand Up @@ -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<
Expand All @@ -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) => ({
Expand Down Expand Up @@ -151,9 +164,12 @@ export function getInternalStateContainer() {
}),
setResetDefaultProfileState:
(prevState: InternalState) =>
(resetDefaultProfileState: InternalState['resetDefaultProfileState']) => ({
(resetDefaultProfileState: Omit<InternalState['resetDefaultProfileState'], 'resetId'>) => ({
...prevState,
resetDefaultProfileState,
resetDefaultProfileState: {
...resetDefaultProfileState,
resetId: uuidv4(),
},
}),
},
{},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ describe('getDefaultProfileState', () => {
let appState = getDefaultProfileState({
profilesManager: profilesManagerMock,
resetDefaultProfileState: {
resetId: 'test',
columns: false,
rowHeight: false,
breakdownField: true,
Expand All @@ -39,6 +40,7 @@ describe('getDefaultProfileState', () => {
appState = getDefaultProfileState({
profilesManager: profilesManagerMock,
resetDefaultProfileState: {
resetId: 'test',
columns: false,
rowHeight: false,
breakdownField: true,
Expand All @@ -54,6 +56,7 @@ describe('getDefaultProfileState', () => {
let appState = getDefaultProfileState({
profilesManager: profilesManagerMock,
resetDefaultProfileState: {
resetId: 'test',
columns: true,
rowHeight: false,
breakdownField: false,
Expand All @@ -79,6 +82,7 @@ describe('getDefaultProfileState', () => {
appState = getDefaultProfileState({
profilesManager: profilesManagerMock,
resetDefaultProfileState: {
resetId: 'test',
columns: true,
rowHeight: false,
breakdownField: false,
Expand Down Expand Up @@ -107,6 +111,7 @@ describe('getDefaultProfileState', () => {
const appState = getDefaultProfileState({
profilesManager: profilesManagerMock,
resetDefaultProfileState: {
resetId: 'test',
columns: false,
rowHeight: true,
breakdownField: false,
Expand All @@ -125,6 +130,7 @@ describe('getDefaultProfileState', () => {
const appState = getDefaultProfileState({
profilesManager: profilesManagerMock,
resetDefaultProfileState: {
resetId: 'test',
columns: false,
rowHeight: false,
breakdownField: false,
Expand Down

0 comments on commit 2f42e16

Please sign in to comment.