codeant-ai-for-open-source[bot] commented on code in PR #37497:
URL: https://github.com/apache/superset/pull/37497#discussion_r2740617140
##########
superset-frontend/src/dashboard/actions/dashboardState.test.js:
##########
@@ -234,4 +237,71 @@ 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 = {
+ response: {
+ status: 404,
+ statusText: 'Not Found',
+ bodyUsed: false,
+ },
+ };
+ 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 = {
+ response: {
+ status: 500,
+ statusText: 'Internal Server Error',
+ bodyUsed: false,
+ },
Review Comment:
**Suggestion:** The mocked 500 error object also lacks a Response-like
`json()` method; similar to the 404 case, `getClientErrorObject` could attempt
to parse the response body and throw — add an async `json` method to the mocked
`response` so the test exercises error-handling code rather than causing an
unrelated TypeError. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unit test for non-404 fetchFaveStar error path broken.
- ⚠️ CI failure potential for dashboardState tests.
```
</details>
```suggestion
json: async () => ({ message: 'Internal Server Error' }),
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the unit test "dispatches error toast on non-404 errors" in
superset-frontend/src/dashboard/actions/dashboardState.test.js (the test
block is located near lines ~283-305). The test replaces
SupersetClient.get
with a rejection using the error500 object defined at lines 287-295.
2. The test invokes fetchFaveStar(dashboardId) (thunk in
src/dashboard/actions/dashboardState.js around lines 108-140) which
catches the rejection and calls getClientErrorObject(error) to normalize
the error.
3. If getClientErrorObject attempts to call response.json() on the mocked
response, the missing json() implementation on error500.response (lines
288-294) can raise a TypeError, causing the test to fail unexpectedly
instead of validating the non-404 error handling (dispatching an
ADD_TOAST action).
4. Adding an async json() method to the mocked response exercises the real
code path and ensures the test verifies dispatching of the error toast.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/actions/dashboardState.test.js
**Line:** 293:293
**Comment:**
*Possible Bug: The mocked 500 error object also lacks a Response-like
`json()` method; similar to the 404 case, `getClientErrorObject` could attempt
to parse the response body and throw — add an async `json` method to the mocked
`response` so the test exercises error-handling code rather than causing an
unrelated TypeError.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/dashboard/actions/dashboardState.test.js:
##########
@@ -234,4 +237,71 @@ 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 = {
+ response: {
+ status: 404,
+ statusText: 'Not Found',
+ bodyUsed: false,
+ },
Review Comment:
**Suggestion:** The mocked 404 error object lacks a Response-like `json()`
method; the production helper `getClientErrorObject` may attempt to call
`response.json()` on the rejected value and throw a TypeError, causing the test
to fail for the wrong reason — include a `json` async method on the mocked
`response` so it behaves like a real fetch Response. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Unit test 'does not dispatch error toast on 404 response' affected.
- ⚠️ CI dashboard tests may fail with parsing TypeError.
```
</details>
```suggestion
json: async () => ({ message: 'Not Found' }),
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the unit test "does not dispatch error toast on 404 response" in
superset-frontend/src/dashboard/actions/dashboardState.test.js (test block
starts at the fetchFaveStar tests around lines 262-281). The test file
stubs SupersetClient.get to reject with the object defined at
lines 266-274 (the error404 object).
2. The test calls fetchFaveStar(dashboardId) (thunk defined in
src/dashboard/actions/dashboardState.js: export function fetchFaveStar(id)
— see the implementation at lines ~108-140) which executes
SupersetClient.get
and hits the .catch branch.
3. Inside fetchFaveStar catch, the code invokes getClientErrorObject(error)
(see src/dashboard/actions/dashboardState.js catch at ~lines 128-136),
which in the real client code may call response.json() on the rejected
value to parse the body.
4. Because the mocked error404.response object (lines 267-273 in the test)
lacks an async json() method, getClientErrorObject could throw a
TypeError while attempting to parse the body, causing the test to fail
for the wrong reason (TypeError) instead of exercising the intended
404-handling path. Adding response.json resolves this mismatch.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/actions/dashboardState.test.js
**Line:** 272:272
**Comment:**
*Possible Bug: The mocked 404 error object lacks a Response-like
`json()` method; the production helper `getClientErrorObject` may attempt to
call `response.json()` on the rejected value and throw a TypeError, causing the
test to fail for the wrong reason — include a `json` async method on the mocked
`response` so it behaves like a real fetch Response.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
