Copilot commented on code in PR #44958:
URL: https://github.com/apache/superset/pull/44958#discussion_r4186873589
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -274,6 +275,88 @@ const renderWithRouter = ({
return { ...result, history };
};
+test('fetches datasource metadata when the datasource changes', async () => {
+ const datasourceKey = '2__table';
+ const initialState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ form_data: {
+ datasource: '1__table',
+ viz_type: VizType.Table,
+ metrics: [],
+ },
+ },
+ };
+ fetchMock.get(
+ `glob:*/fetch_datasource_metadata?datasourceKey=${datasourceKey}`,
+ {
+ id: 2,
+ type: 'table',
+ columns: [],
+ metrics: [],
+ },
+ );
+ const store = createStore(initialState, reducerIndex);
+ renderWithRouter({ initialState, store: store as Store });
+
+ act(() => {
+ store.dispatch(
+ exploreActions.setControlValue('datasource', datasourceKey, []),
+ );
+ });
+
+ await waitFor(() =>
+ expect(
+ fetchMock.callHistory.called(
+ `glob:*/fetch_datasource_metadata?datasourceKey=${datasourceKey}`,
+ ),
+ ).toBe(true),
+ );
+});
+
+test('shows an error when fetching datasource metadata fails', async () => {
+ const datasourceKey = '3__table';
+ const initialState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ form_data: {
+ datasource: '1__table',
+ viz_type: VizType.Table,
+ metrics: [],
+ },
+ },
+ };
+ const store = createStore(initialState, reducerIndex);
+ renderWithRouter({ initialState, store: store as Store });
+ jest
+ .spyOn(SupersetClient, 'get')
+ .mockRejectedValueOnce(new Error('Request failed'));
Review Comment:
This test mixes mocking strategies (fetchMock in the previous test, but a
`SupersetClient.get` spy here). Mocking the network via `fetchMock` for the
failing request keeps tests consistent and avoids coupling the test to the
internal implementation (i.e., whether the thunk uses `SupersetClient.get` vs
`SupersetClient.request`/fetch). Consider switching this failure case to a
`fetchMock` 500/rejection for the `/fetch_datasource_metadata` endpoint instead.
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -704,8 +704,11 @@ function ExploreViewContainer(props:
ExploreViewContainerProps) {
(previousControls.datasource == null ||
props.controls.datasource.value !==
previousControls.datasource.value)
) {
- // this should really be handled by actions
- fetchDatasourceMetadata(props.form_data.datasource);
+ Promise.resolve(
+ props.actions.fetchDatasourceMetadata(props.form_data.datasource),
+ ).catch(() => {
+ props.addDangerToast(t('Failed to load datasource metadata'));
+ });
Review Comment:
`Promise.resolve(expr)` does not catch synchronous exceptions thrown while
evaluating `expr` (the call to `fetchDatasourceMetadata` happens before Promise
resolution). If `fetchDatasourceMetadata` (or dispatch) can throw
synchronously, this will bypass the `.catch()` and potentially crash the
effect. Prefer a pattern that catches both sync + async failures (e.g.,
`Promise.resolve().then(() => ...)` or an explicit `try/catch` around the call
plus handling promise rejections).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
