codeant-ai-for-open-source[bot] commented on code in PR #37985:
URL: https://github.com/apache/superset/pull/37985#discussion_r2811310090
##########
superset-frontend/src/pages/ActionLog/index.test.tsx:
##########
@@ -0,0 +1,68 @@
+import React from 'react';
+import '@testing-library/jest-dom';
+import { render, screen } from '@testing-library/react';
+import ActionLogList from './index';
+
+
+jest.mock('src/views/CRUD/hooks', () => ({
+ useListViewResource: () => ({
+ state: {
+ loading: false,
+ resourceCount: 1,
+ resourceCollection: [
+ {
+ action: 'test',
+ user: {
+ username: 'guid-123',
+ first_name: 'John',
+ last_name: 'Doe',
+ },
+ },
+ ],
+ bulkSelectEnabled: false,
+ },
+ fetchData: jest.fn(),
+ refreshData: jest.fn(),
+ toggleBulkSelect: jest.fn(),
+ }),
+}));
+
+jest.mock('src/components/MessageToasts/withToasts', () => ({
+ __esModule: true,
+ default: (Component: any) => Component,
+ useToasts: () => ({
+ addDangerToast: jest.fn(),
+ addSuccessToast: jest.fn(),
+ }),
+}));
+
+jest.mock('src/features/home/SubMenu', () => ({
+ __esModule: true,
+ default: () => <div />,
+}));
+
+jest.mock('src/components/ListView', () => ({
+ __esModule: true,
+ ListView: ({ data }: any) => (
+ <div>
+ {data.map((row: any) => (
+ <div key={row.user.username}>
+ {row.user.first_name} {row.user.last_name}
+ </div>
+ ))}
Review Comment:
**Suggestion:** The `ListView` mock hardcodes rendering
`{row.user.first_name} {row.user.last_name}` from the raw data, bypassing the
actual column/cell rendering logic in `ActionLogList`, so this test will still
pass even if the "User" column regresses to showing only username or any other
incorrect behavior; the mock should instead invoke the column's cell renderer
so the test reflects the real implementation. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ ActionLog `/actionlog/list` user column regressions may go undetected.
- ⚠️ Jest test only validates mocked ListView output, not columns.
```
</details>
```suggestion
ListView: ({ data, columns }: any) => (
<div>
{Array.isArray(data) &&
data.map((row: any) => {
if (!Array.isArray(columns) || columns.length === 0) {
return null;
}
const userColumn =
columns.find(
(col: any) =>
col.key === 'user' ||
col.id === 'user' ||
col.accessor === 'user',
) || columns[0];
if (typeof userColumn?.Cell === 'function') {
// Mimic react-table's Cell props shape used in Superset ListView
return (
<div key={row.user?.username ?? row.id}>
{userColumn.Cell({
row: { original: row },
value: row.user,
})}
</div>
);
}
const value =
typeof userColumn?.accessor === 'function'
? userColumn.accessor(row)
: userColumn?.accessor
? row[userColumn.accessor as keyof typeof row]
: row.user?.username;
return (
<div key={row.user?.username ?? row.id}>
{value != null ? String(value) : ''}
</div>
);
})}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset-frontend/src/pages/ActionLog/index.test.tsx` and note the
`jest.mock('src/components/ListView'...)` definition at lines 44–61, where
the mocked
`ListView` only accepts `{ data }` and always renders `{row.user.first_name}
{row.user.last_name}`.
2. In the same file, see the test `describe('ActionLog User column display',
...)` at
lines 63–67, which calls `render(<ActionLogList />);` and asserts
`screen.getByText('John
Doe')` is in the document.
3. When this test runs, `ActionLogList` (imported from `./index` at line 4)
internally
renders Superset's real `ListView`, but the import is replaced by the mock
from step 1, so
the real `columns` configuration and any `Cell` renderer for the "User"
column are never
used.
4. Because the mock derives `"John Doe"` directly from
`row.user.first_name`/`last_name`
on the raw data, the test will keep passing even if the actual "User" column
implementation in `superset-frontend/src/pages/ActionLog/index.tsx` is later
changed to
render only `username` or any other incorrect value, meaning this regression
test does not
actually cover the real column rendering logic.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/ActionLog/index.test.tsx
**Line:** 46:52
**Comment:**
*Logic Error: The `ListView` mock hardcodes rendering
`{row.user.first_name} {row.user.last_name}` from the raw data, bypassing the
actual column/cell rendering logic in `ActionLogList`, so this test will still
pass even if the "User" column regresses to showing only username or any other
incorrect behavior; the mock should instead invoke the column's cell renderer
so the test reflects the real implementation.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37985&comment_hash=41e8ea47a18fe17b88e078b4d3007408628f45029431e9c27aa2536ef25b8306&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37985&comment_hash=41e8ea47a18fe17b88e078b4d3007408628f45029431e9c27aa2536ef25b8306&reaction=dislike'>👎</a>
--
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]
