Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Discover] Fix flaky cell actions tests #202097

Merged
merged 8 commits into from
Dec 3, 2024
Merged

[Discover] Fix flaky cell actions tests #202097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
351e02b
Attempt to fix flaky cell actions tests
davismcphee Nov 28, 2024
f4269d2
Merge branch 'main' into fix-flaky-cell-actions-tests
davismcphee Nov 28, 2024
25e272f
Attempt to keep profile resets in sync
davismcphee Nov 29, 2024
6c68149
Fix types and tests, remove temp code
davismcphee Nov 29, 2024
984bc38
Remove comment
davismcphee Nov 29, 2024
2213f43
Remove addLog
davismcphee Nov 29, 2024
b53f68a
Merge branch 'main' into fix-flaky-cell-actions-tests
davismcphee Dec 1, 2024
1a611db
Merge branch 'main' into fix-flaky-cell-actions-tests
davismcphee Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/plugins/discover/public/application/main/data_fetching/fetch_all.test.ts
View file
Open in desktop
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
15 changes: 8 additions & 7 deletions src/plugins/discover/public/application/main/hooks/use_esql_mode.test.tsx
View file
Open in desktop
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
17 changes: 9 additions & 8 deletions ...ns/discover/public/application/main/state_management/discover_app_state_container.test.ts
View file
Open in desktop
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
3 changes: 3 additions & 0 deletions ...plugins/discover/public/application/main/state_management/discover_app_state_container.ts
View file
Open in desktop
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
5 changes: 3 additions & 2 deletions ...s/discover/public/application/main/state_management/discover_data_state_container.test.ts
View file
Open in desktop
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
7 changes: 7 additions & 0 deletions ...lugins/discover/public/application/main/state_management/discover_data_state_container.ts
View file
Open in desktop
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;
}
Comment on lines +296 to +301
Copy link
Contributor Author

@davismcphee davismcphee Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue was that there is a 100ms delay between when we update the app state data source to the new data view in changeDataView (and trigger default state reset) and when the fetch$ callback is actually called (due to the debounce in getFetch$). If within that 100ms period, the previous fetch finishes, this code was being run and clearing out the default state reset here:

kibana/src/plugins/discover/public/application/main/state_management/discover_data_state_container.ts

Lines 314 to 320 in 2213f43

// Clear the default profile state flags after the data fetching
// is done so refetches don't reset the state again
internalStateContainer.transitions.setResetDefaultProfileState({
columns: false,
rowHeight: false,
breakdownField: false,
});

This causes the following code which checks for the default state reset in the fetch$ callback to receive all false values for the subsequent fetch, and the correct default state does not get set:

kibana/src/plugins/discover/public/application/main/state_management/discover_data_state_container.ts

Lines 268 to 271 in 2213f43

const { resetDefaultProfileState, dataView } = internalStateContainer.getState();
const defaultProfileState = dataView
? getDefaultProfileState({ profilesManager, resetDefaultProfileState, dataView })
: undefined;

The solution was to ensure the default state reset call associated with this fetch is actually the latest one when this code is run, by using a resetId that gets updated on each reset call. This is not the most elegant solution to this issue and is a symptom of a deeper issue, but it was the least invasive one I could come up with. I believe the long term / correct solution to this issue is to refactor Discover data fetching so logic runs sequentially and is easier to reason about.


const { esqlQueryColumns } = dataSubjects.documents$.getValue();
const defaultColumns = uiSettings.get<string[]>(DEFAULT_COLUMNS_SETTING, []);
const postFetchStateUpdate = defaultProfileState?.getPostFetchState({
Expand Down
26 changes: 21 additions & 5 deletions ...ns/discover/public/application/main/state_management/discover_internal_state_container.ts
View file
Open in desktop
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
12 changes: 7 additions & 5 deletions src/plugins/discover/public/application/main/state_management/utils/change_data_view.ts
View file
Open in desktop
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
6 changes: 6 additions & 0 deletions ...discover/public/application/main/state_management/utils/get_default_profile_state.test.ts
View file
Open in desktop
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
Loading