Skip to content

fix(tests): fix flakey tests with PropertiesModal.test.tsx, FiltersConfigModal.test.tsx and ChartList.listview.test.tsx #36037

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

Merged
sadpandajoe merged 5 commits into from
Nov 10, 2025
Merged

fix(tests): fix flakey tests with PropertiesModal.test.tsx, FiltersConfigModal.test.tsx and ChartList.listview.test.tsx #36037

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a97a8c3
fix(tests): add await to userEvent calls to fix race conditions
sadpandajoe Nov 7, 2025
42b2ac9
refactor(tests): fix waitFor anti-patterns and improve performance
sadpandajoe Nov 7, 2025
449bce9
refactor(tests): fix test failures and flatten ChartList structure
sadpandajoe Nov 7, 2025
98dcc5d
refactor(tests): fix timer cleanup, mock pollution, and migrate to us…
sadpandajoe Nov 8, 2025
e9eac10
refactor(tests): address PR #36037 Copilot feedback
sadpandajoe Nov 8, 2025
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
151 changes: 85 additions & 66 deletions ...end/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,9 +184,6 @@ afterEach(() => {
jest.restoreAllMocks();
});

// Set timeout for all tests in this file to prevent CI timeouts
jest.setTimeout(60000);

function defaultRender(initialState: any = defaultState(), modalProps = props) {
return render(<FiltersConfigModal {...modalProps} />, {
initialState,
Expand Down Expand Up @@ -226,9 +223,12 @@ test('renders a value filter type', () => {
test('renders a numerical range filter type', async () => {
defaultRender();

userEvent.click(screen.getByText(VALUE_REGEX));
await userEvent.click(screen.getByText(VALUE_REGEX));

await waitFor(() => userEvent.click(screen.getByText(NUMERICAL_RANGE_REGEX)));
const numericalRangeOption = await waitFor(() =>
screen.getByText(NUMERICAL_RANGE_REGEX),
);
Comment thread
sadpandajoe marked this conversation as resolved.
Outdated
await userEvent.click(numericalRangeOption);

expect(screen.getByText(FILTER_TYPE_REGEX)).toBeInTheDocument();
expect(screen.getByText(FILTER_NAME_REGEX)).toBeInTheDocument();
Expand All @@ -250,9 +250,12 @@ test('renders a numerical range filter type', async () => {
test('renders a time range filter type', async () => {
defaultRender();

userEvent.click(screen.getByText(VALUE_REGEX));
await userEvent.click(screen.getByText(VALUE_REGEX));

await waitFor(() => userEvent.click(screen.getByText(TIME_RANGE_REGEX)));
const timeRangeOption = await waitFor(() =>
screen.getByText(TIME_RANGE_REGEX),
);
await userEvent.click(timeRangeOption);
Comment thread
sadpandajoe marked this conversation as resolved.
Outdated

expect(screen.getByText(FILTER_TYPE_REGEX)).toBeInTheDocument();
expect(screen.getByText(FILTER_NAME_REGEX)).toBeInTheDocument();
Expand All @@ -265,9 +268,12 @@ test('renders a time range filter type', async () => {
test('renders a time column filter type', async () => {
defaultRender();

userEvent.click(screen.getByText(VALUE_REGEX));
await userEvent.click(screen.getByText(VALUE_REGEX));

await waitFor(() => userEvent.click(screen.getByText(TIME_COLUMN_REGEX)));
const timeColumnOption = await waitFor(() =>
screen.getByText(TIME_COLUMN_REGEX),
);
await userEvent.click(timeColumnOption);
Comment thread
sadpandajoe marked this conversation as resolved.
Outdated

expect(screen.getByText(FILTER_TYPE_REGEX)).toBeInTheDocument();
expect(screen.getByText(FILTER_NAME_REGEX)).toBeInTheDocument();
Expand All @@ -280,9 +286,12 @@ test('renders a time column filter type', async () => {
test('renders a time grain filter type', async () => {
defaultRender();

userEvent.click(screen.getByText(VALUE_REGEX));
await userEvent.click(screen.getByText(VALUE_REGEX));

await waitFor(() => userEvent.click(screen.getByText(TIME_GRAIN_REGEX)));
const timeGrainOption = await waitFor(() =>
screen.getByText(TIME_GRAIN_REGEX),
);
await userEvent.click(timeGrainOption);
Comment thread
sadpandajoe marked this conversation as resolved.
Outdated

expect(screen.getByText(FILTER_TYPE_REGEX)).toBeInTheDocument();
expect(screen.getByText(FILTER_NAME_REGEX)).toBeInTheDocument();
Expand All @@ -295,7 +304,7 @@ test('renders a time grain filter type', async () => {
test('render time filter types as disabled if there are no temporal columns in the dataset', async () => {
defaultRender(noTemporalColumnsState());

userEvent.click(screen.getByText(VALUE_REGEX));
await userEvent.click(screen.getByText(VALUE_REGEX));

const timeRange = await screen.findByText(TIME_RANGE_REGEX);
const timeGrain = await screen.findByText(TIME_GRAIN_REGEX);
Expand All @@ -309,7 +318,7 @@ test('render time filter types as disabled if there are no temporal columns in t

test('validates the name', async () => {
defaultRender();
userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
await userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
await waitFor(
async () => {
expect(await screen.findByText(NAME_REQUIRED_REGEX)).toBeInTheDocument();
Expand All @@ -320,16 +329,16 @@ test('validates the name', async () => {

test('validates the column', async () => {
defaultRender();
userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
await userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
expect(await screen.findByText(COLUMN_REQUIRED_REGEX)).toBeInTheDocument();
});

// eslint-disable-next-line jest/no-disabled-tests
test.skip('validates the default value', async () => {
defaultRender(noTemporalColumnsState());
expect(await screen.findByText('birth_names')).toBeInTheDocument();
userEvent.type(screen.getByRole('combobox'), `Column A{Enter}`);
userEvent.click(getCheckbox(DEFAULT_VALUE_REGEX));
await userEvent.type(screen.getByRole('combobox'), `Column A{Enter}`);
await userEvent.click(getCheckbox(DEFAULT_VALUE_REGEX));
await waitFor(() => {
expect(
screen.queryByText(FILL_REQUIRED_FIELDS_REGEX),
Expand All @@ -345,10 +354,15 @@ test('validates the pre-filter value', async () => {
try {
defaultRender();

userEvent.click(screen.getByText(FILTER_SETTINGS_REGEX));
userEvent.click(getCheckbox(PRE_FILTER_REGEX));
await userEvent.click(screen.getByText(FILTER_SETTINGS_REGEX));
await userEvent.click(getCheckbox(PRE_FILTER_REGEX));

jest.runAllTimers();

await waitFor(() => {
const errorMessages = screen.getAllByText(PRE_FILTER_REQUIRED_REGEX);
expect(errorMessages.length).toBeGreaterThan(0);
});
} finally {
jest.useRealTimers();
}
Expand All @@ -359,7 +373,8 @@ test('validates the pre-filter value', async () => {
// Wait for validation to complete after timer switch
await waitFor(
() => {
expect(screen.getByText(PRE_FILTER_REQUIRED_REGEX)).toBeInTheDocument();
const errorMessages = screen.queryAllByText(PRE_FILTER_REQUIRED_REGEX);
expect(errorMessages.length).toBeGreaterThan(0);
},
{ timeout: 15000 },
);
Expand All @@ -368,13 +383,13 @@ test('validates the pre-filter value', async () => {
// eslint-disable-next-line jest/no-disabled-tests
test.skip("doesn't render time range pre-filter if there are no temporal columns in datasource", async () => {
defaultRender(noTemporalColumnsState());
userEvent.click(screen.getByText(DATASET_REGEX));
await waitFor(() => {
await userEvent.click(screen.getByText(DATASET_REGEX));
await waitFor(async () => {
expect(screen.queryByLabelText('Loading')).not.toBeInTheDocument();
userEvent.click(screen.getByText('birth_names'));
await userEvent.click(screen.getByText('birth_names'));
});
Comment on lines +376 to 379

Copilot AI Nov 7, 2025

Copy link

Choose a reason for hiding this comment

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

Using an async callback function inside waitFor is an anti-pattern. The waitFor utility expects synchronous checks that can be repeatedly executed until they pass. An async function inside waitFor can lead to unpredictable behavior.

Instead, await the user interaction before waitFor, and only include the synchronous expectation inside:

await userEvent.click(screen.getByText(DATASET_REGEX));
await userEvent.click(screen.getByText('birth_names'));
await waitFor(() => {
  expect(screen.queryByLabelText('Loading')).not.toBeInTheDocument();
});

Copilot uses AI. Check for mistakes.

@sadpandajoe sadpandajoe Nov 7, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This test is skipped so we can omit for now

userEvent.click(screen.getByText(FILTER_SETTINGS_REGEX));
userEvent.click(getCheckbox(PRE_FILTER_REGEX));
await userEvent.click(screen.getByText(FILTER_SETTINGS_REGEX));
await userEvent.click(getCheckbox(PRE_FILTER_REGEX));
await waitFor(() =>
expect(
screen.queryByText(TIME_RANGE_PREFILTER_REGEX),
Expand Down Expand Up @@ -439,9 +454,9 @@ test('deletes a filter', async () => {
const removeButtons = screen.getAllByRole('button', {
name: 'delete',
});
userEvent.click(removeButtons[2]);
await userEvent.click(removeButtons[2]);

userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
await userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));

await waitFor(() =>
expect(onSave).toHaveBeenCalledWith(
Expand Down Expand Up @@ -476,8 +491,8 @@ test('deletes a filter including dependencies', async () => {
const removeButtons = screen.getAllByRole('button', {
name: 'delete',
});
userEvent.click(removeButtons[1]);
userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
await userEvent.click(removeButtons[1]);
await userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
await waitFor(() =>
expect(onSave).toHaveBeenCalledWith(
expect.objectContaining({
Expand Down Expand Up @@ -525,7 +540,7 @@ test('switches the order between two filters', async () => {

fireEvent.dragEnd(draggableFilters[0]);

userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
await userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));

await waitFor(() =>
expect(onSave).toHaveBeenCalledWith(
Expand Down Expand Up @@ -568,14 +583,14 @@ test('rearranges three filters and deletes one of them', async () => {
const deleteButtons = screen.getAllByRole('button', {
name: 'delete',
});
userEvent.click(deleteButtons[1]);
await userEvent.click(deleteButtons[1]);

fireEvent.dragStart(draggableFilters[0]);
fireEvent.dragOver(draggableFilters[2]);
fireEvent.drop(draggableFilters[2]);
fireEvent.dragEnd(draggableFilters[0]);

userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
await userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));

await waitFor(() =>
expect(onSave).toHaveBeenCalledWith(
Expand All @@ -594,47 +609,51 @@ test('rearranges three filters and deletes one of them', async () => {

test('modifies the name of a filter', async () => {
jest.useFakeTimers();
const nativeFilterState = [
buildNativeFilter('NATIVE_FILTER-1', 'state', []),
buildNativeFilter('NATIVE_FILTER-2', 'country', []),
];

const state = {
...defaultState(),
dashboardInfo: {
metadata: { native_filter_configuration: nativeFilterState },
},
dashboardLayout,
};

const onSave = jest.fn();
try {
const nativeFilterState = [
buildNativeFilter('NATIVE_FILTER-1', 'state', []),
buildNativeFilter('NATIVE_FILTER-2', 'country', []),
];

const state = {
...defaultState(),
dashboardInfo: {
metadata: { native_filter_configuration: nativeFilterState },
},
dashboardLayout,
};

defaultRender(state, {
...props,
createNewOnOpen: false,
onSave,
});
const onSave = jest.fn();

const filterNameInput = screen.getByRole('textbox', {
name: FILTER_NAME_REGEX,
});
defaultRender(state, {
...props,
createNewOnOpen: false,
onSave,
});

userEvent.clear(filterNameInput);
userEvent.type(filterNameInput, 'New Filter Name');
const filterNameInput = screen.getByRole('textbox', {
name: FILTER_NAME_REGEX,
});

jest.runAllTimers();
await userEvent.clear(filterNameInput);
await userEvent.type(filterNameInput, 'New Filter Name');

userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
jest.runAllTimers();

await waitFor(() =>
expect(onSave).toHaveBeenCalledWith(
expect.objectContaining({
modified: expect.arrayContaining([
expect.objectContaining({ name: 'New Filter Name' }),
]),
}),
),
);
await userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));

await waitFor(() =>
expect(onSave).toHaveBeenCalledWith(
expect.objectContaining({
modified: expect.arrayContaining([
expect.objectContaining({ name: 'New Filter Name' }),
]),
}),
),
);
} finally {
jest.useRealTimers();
}
});

test('renders a filter with a chart containing BigInt values', async () => {
Expand Down
Loading
Loading