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.

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

fix(sqllab): Async queries are now fetched properly #21698

Merged
merged 4 commits into from
Oct 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,26 @@ const newProps = {
},
},
};
const asyncQueryProps = {
...mockedProps,
database: { allow_run_async: true },
};
const asyncRefetchDataPreviewProps = {
...asyncQueryProps,
query: {
state: 'success',
results: undefined,
isDataPreview: true,
},
};
const asyncRefetchResultsTableProps = {
...asyncQueryProps,
query: {
state: 'success',
results: undefined,
resultsKey: 'async results key',
},
};
fetchMock.get('glob:*/api/v1/dataset?*', { result: [] });

const middlewares = [thunk];
Expand All @@ -86,13 +106,13 @@ const setup = (props?: any, store?: Store) =>
});

describe('ResultSet', () => {
it('renders a Table', async () => {
test('renders a Table', async () => {
const { getByTestId } = setup(mockedProps, mockStore(initialState));
const table = getByTestId('table-container');
expect(table).toBeInTheDocument();
});

it('should render success query', async () => {
test('should render success query', async () => {
const { queryAllByText, getByTestId } = setup(
mockedProps,
mockStore(initialState),
Expand All @@ -114,7 +134,7 @@ describe('ResultSet', () => {
expect(exploreButton).toBeInTheDocument();
});

it('should render empty results', async () => {
test('should render empty results', async () => {
const props = {
...mockedProps,
query: { ...mockedProps.query, results: { data: [] } },
Expand All @@ -128,7 +148,7 @@ describe('ResultSet', () => {
expect(alert).toHaveTextContent('The query returned no data');
});

it('should call reRunQuery if timed out', async () => {
test('should call reRunQuery if timed out', async () => {
const store = mockStore(initialState);
const propsWithError = {
...mockedProps,
Expand All @@ -143,26 +163,26 @@ describe('ResultSet', () => {
expect(store.getActions()[0].type).toEqual('START_QUERY');
});

it('should not call reRunQuery if no error', async () => {
test('should not call reRunQuery if no error', async () => {
const store = mockStore(initialState);
setup(mockedProps, store);
expect(store.getActions()).toEqual([]);
});

it('should render cached query', async () => {
test('should render cached query', async () => {
const store = mockStore(initialState);
const { rerender } = setup(cachedQueryProps, store);

// @ts-ignore
rerender(<ResultSet {...newProps} />);
expect(store.getActions()).toHaveLength(1);
expect(store.getActions()).toHaveLength(2);
expect(store.getActions()[0].query.results).toEqual(
cachedQueryProps.query.results,
);
expect(store.getActions()[0].type).toEqual('CLEAR_QUERY_RESULTS');
});

it('should render stopped query', async () => {
test('should render stopped query', async () => {
await waitFor(() => {
setup(stoppedQueryProps, mockStore(initialState));
});
Expand All @@ -171,13 +191,13 @@ describe('ResultSet', () => {
expect(alert).toBeInTheDocument();
});

it('should render running/pending/fetching query', async () => {
test('should render running/pending/fetching query', async () => {
const { getByTestId } = setup(runningQueryProps, mockStore(initialState));
const progressBar = getByTestId('progress-bar');
expect(progressBar).toBeInTheDocument();
});

it('should render fetching w/ 100 progress query', async () => {
test('should render fetching w/ 100 progress query', async () => {
const { getByRole, getByText } = setup(
fetchingQueryProps,
mockStore(initialState),
Expand All @@ -187,7 +207,7 @@ describe('ResultSet', () => {
expect(getByText('fetching')).toBeInTheDocument();
});

it('should render a failed query with an error message', async () => {
test('should render a failed query with an error message', async () => {
await waitFor(() => {
setup(failedQueryWithErrorMessageProps, mockStore(initialState));
});
Expand All @@ -196,21 +216,56 @@ describe('ResultSet', () => {
expect(screen.getByText('Something went wrong')).toBeInTheDocument();
});

it('should render a failed query with an errors object', async () => {
test('should render a failed query with an errors object', async () => {
await waitFor(() => {
setup(failedQueryWithErrorsProps, mockStore(initialState));
});
expect(screen.getByText('Database error')).toBeInTheDocument();
});

it('renders if there is no limit in query.results but has queryLimit', async () => {
test('renders if there is no limit in query.results but has queryLimit', async () => {
const { getByRole } = setup(mockedProps, mockStore(initialState));
expect(getByRole('grid')).toBeInTheDocument();
});

it('renders if there is a limit in query.results but not queryLimit', async () => {
test('renders if there is a limit in query.results but not queryLimit', async () => {
const props = { ...mockedProps, query: queryWithNoQueryLimit };
const { getByRole } = setup(props, mockStore(initialState));
expect(getByRole('grid')).toBeInTheDocument();
});

test('Async queries - renders "Fetch data preview" button when data preview has no results', () => {
setup(asyncRefetchDataPreviewProps, mockStore(initialState));
expect(
screen.getByRole('button', {
name: /fetch data preview/i,
}),
).toBeVisible();
expect(screen.queryByRole('grid')).toBe(null);
});

test('Async queries - renders "Refetch results" button when a query has no results', () => {
setup(asyncRefetchResultsTableProps, mockStore(initialState));
expect(
screen.getByRole('button', {
name: /refetch results/i,
}),
).toBeVisible();
expect(screen.queryByRole('grid')).toBe(null);
});

test('Async queries - renders on the first call', () => {
setup(asyncQueryProps, mockStore(initialState));
expect(screen.getByRole('grid')).toBeVisible();
expect(
screen.queryByRole('button', {
name: /fetch data preview/i,
}),
).toBe(null);
expect(
screen.queryByRole('button', {
name: /refetch results/i,
}),
).toBe(null);
});
});
6 changes: 1 addition & 5 deletions superset-frontend/src/SqlLab/components/ResultSet/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -164,11 +164,7 @@ const ResultSet = ({
setCachedData(query.results.data);
dispatch(clearQueryResults(query));
}
if (
query.resultsKey &&
prevQuery?.resultsKey &&
query.resultsKey !== prevQuery.resultsKey
) {
if (query.resultsKey && query.resultsKey !== prevQuery?.resultsKey) {
Copy link
Member

Choose a reason for hiding this comment

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

is there any case where the refetch button will appear since we are removing this case?

Copy link
Member Author

@lyndsiWilliams lyndsiWilliams Oct 4, 2022

Choose a reason for hiding this comment

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

It should work normally, as it did before the refactor. Before the refactor the if statement looked like this:

if (
      nextProps.query.resultsKey &&
      nextProps.query.resultsKey !== this.props.query.resultsKey
    ) 

So I don't think prevQuery?.resultsKey && was supposed to be there in the first place.

fetchResults(query);
}
}, [query, cache]);
Expand Down