Copilot commented on code in PR #37497:
URL: https://github.com/apache/superset/pull/37497#discussion_r2740518936
##########
superset-frontend/src/dashboard/actions/dashboardState.test.js:
##########
@@ -234,4 +236,61 @@ describe('dashboardState actions', () => {
);
});
});
+
+ // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
+ describe('fetchFaveStar', () => {
+ test('dispatches TOGGLE_FAVE_STAR on successful fetch', async () => {
+ const dashboardId = 123;
+ const dispatch = sinon.stub();
+
+ getStub.restore();
+ getStub = sinon.stub(SupersetClient, 'get').resolves({
+ json: {
+ result: [{ id: dashboardId, value: true }],
+ },
+ });
+
+ const thunk = fetchFaveStar(dashboardId);
+ await thunk(dispatch);
+
+ await waitFor(() => expect(dispatch.callCount).toBe(1));
+ expect(dispatch.getCall(0).args[0].type).toBe(TOGGLE_FAVE_STAR);
+ expect(dispatch.getCall(0).args[0].isStarred).toBe(true);
+ });
+
+ test('does not dispatch error toast on 404 response', async () => {
+ const dashboardId = 999;
+ const dispatch = sinon.stub();
+
+ getStub.restore();
+ const error404 = new Error('Not found');
+ error404.status = 404;
+ getStub = sinon.stub(SupersetClient, 'get').rejects(error404);
+
Review Comment:
The mocked 404 failure here rejects with a plain `Error` that has a `status`
field, but `fetchFaveStar` now calls `getClientErrorObject(error)` and checks
`errorObject.status`. `getClientErrorObject` does not read `status` off a
generic `Error`, so `errorObject.status` will be undefined and this test will
fail (it will dispatch the danger toast instead of skipping). Update the stub
to reject with the same shape SupersetClient uses (e.g., `{ response: new
Response(..., { status: 404 }) }`) so `getClientErrorObject` can derive
`status` correctly.
##########
superset-frontend/src/dashboard/actions/dashboardState.test.js:
##########
@@ -234,4 +236,61 @@ describe('dashboardState actions', () => {
);
});
});
+
+ // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
+ describe('fetchFaveStar', () => {
+ test('dispatches TOGGLE_FAVE_STAR on successful fetch', async () => {
+ const dashboardId = 123;
+ const dispatch = sinon.stub();
+
+ getStub.restore();
+ getStub = sinon.stub(SupersetClient, 'get').resolves({
+ json: {
+ result: [{ id: dashboardId, value: true }],
+ },
+ });
+
+ const thunk = fetchFaveStar(dashboardId);
+ await thunk(dispatch);
+
+ await waitFor(() => expect(dispatch.callCount).toBe(1));
+ expect(dispatch.getCall(0).args[0].type).toBe(TOGGLE_FAVE_STAR);
+ expect(dispatch.getCall(0).args[0].isStarred).toBe(true);
+ });
+
+ test('does not dispatch error toast on 404 response', async () => {
+ const dashboardId = 999;
+ const dispatch = sinon.stub();
+
+ getStub.restore();
+ const error404 = new Error('Not found');
+ error404.status = 404;
+ getStub = sinon.stub(SupersetClient, 'get').rejects(error404);
+
+ const thunk = fetchFaveStar(dashboardId);
+ await thunk(dispatch);
+
+ // Should not dispatch any action (no toast, no toggle)
+ expect(dispatch.callCount).toBe(0);
+ });
+
+ test('dispatches error toast on non-404 errors', async () => {
+ const dashboardId = 123;
+ const dispatch = sinon.stub();
+
+ getStub.restore();
+ const error500 = new Error('Internal server error');
+ error500.status = 500;
+ getStub = sinon.stub(SupersetClient, 'get').rejects(error500);
Review Comment:
Same as the 404 case: this test rejects with a plain `Error` and sets
`error500.status`, but `getClientErrorObject` won’t propagate that into
`errorObject.status`. This makes the test less representative of real
SupersetClient failures and can cause unexpected behavior. Prefer rejecting
with `{ response: new Response(..., { status: 500 }) }` (or whatever
SupersetClient actually rejects with) so the thunk exercises the same parsing
path as production.
##########
superset-frontend/src/dashboard/actions/dashboardState.js:
##########
@@ -113,15 +113,29 @@ export function fetchFaveStar(id) {
.then(({ json }) => {
dispatch(toggleFaveStar(!!json?.result?.[0]?.value));
})
- .catch(() =>
+ .catch(async error => {
+ const errorObject = await getClientErrorObject(error);
+
Review Comment:
There’s trailing whitespace on the blank line after
`getClientErrorObject(error);` (shown in the diff). Please remove it to avoid
failing lint rules like `no-trailing-spaces` / prettier formatting checks.
```suggestion
```
##########
superset-frontend/src/dashboard/actions/dashboardState.test.js:
##########
@@ -234,4 +236,61 @@ describe('dashboardState actions', () => {
);
});
});
+
+ // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
+ describe('fetchFaveStar', () => {
+ test('dispatches TOGGLE_FAVE_STAR on successful fetch', async () => {
+ const dashboardId = 123;
+ const dispatch = sinon.stub();
+
+ getStub.restore();
+ getStub = sinon.stub(SupersetClient, 'get').resolves({
+ json: {
+ result: [{ id: dashboardId, value: true }],
+ },
+ });
+
+ const thunk = fetchFaveStar(dashboardId);
+ await thunk(dispatch);
+
+ await waitFor(() => expect(dispatch.callCount).toBe(1));
+ expect(dispatch.getCall(0).args[0].type).toBe(TOGGLE_FAVE_STAR);
+ expect(dispatch.getCall(0).args[0].isStarred).toBe(true);
+ });
+
+ test('does not dispatch error toast on 404 response', async () => {
+ const dashboardId = 999;
+ const dispatch = sinon.stub();
+
+ getStub.restore();
+ const error404 = new Error('Not found');
+ error404.status = 404;
+ getStub = sinon.stub(SupersetClient, 'get').rejects(error404);
+
+ const thunk = fetchFaveStar(dashboardId);
+ await thunk(dispatch);
+
+ // Should not dispatch any action (no toast, no toggle)
+ expect(dispatch.callCount).toBe(0);
+ });
+
+ test('dispatches error toast on non-404 errors', async () => {
+ const dashboardId = 123;
+ const dispatch = sinon.stub();
+
+ getStub.restore();
+ const error500 = new Error('Internal server error');
+ error500.status = 500;
+ getStub = sinon.stub(SupersetClient, 'get').rejects(error500);
+
+ const thunk = fetchFaveStar(dashboardId);
+ await thunk(dispatch);
+
+ await waitFor(() => expect(dispatch.callCount).toBe(1));
+ // Verify error toast action was dispatched (ADD_TOAST with danger type)
+ const dispatchedAction = dispatch.getCall(0).args[0];
+ expect(dispatchedAction.type).toBe('ADD_TOAST');
+ expect(dispatchedAction.payload.toastType).toBe('danger');
Review Comment:
For consistency with the rest of the codebase, avoid asserting on the raw
string `'ADD_TOAST'` and instead import and use the `ADD_TOAST` constant from
`src/components/MessageToasts/actions` (e.g. as done in
`src/dashboard/actions/dashboardLayout.test.js:380`). This reduces brittleness
if the action type ever changes.
--
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]
