codeant-ai-for-open-source[bot] commented on code in PR #38384: URL: https://github.com/apache/superset/pull/38384#discussion_r2882186627
########## superset-frontend/src/components/CrudThemeProvider.test.tsx: ########## @@ -0,0 +1,132 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { type ReactNode } from 'react'; +import { render, screen } from 'spec/helpers/testing-library'; +import { logging } from '@apache-superset/core'; +import { Theme } from '@apache-superset/core/ui'; +import CrudThemeProvider from './CrudThemeProvider'; + +const MockSupersetThemeProvider = ({ children }: { children: ReactNode }) => ( + <div data-test="dashboard-theme-provider">{children}</div> Review Comment: **Suggestion:** The mock theme provider uses a `data-test` attribute while the tests query for `data-testid`, so the assertions that check for the presence or absence of the themed wrapper will never actually detect it, causing the tests to fail or give misleading results about whether the theme was applied. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ CrudThemeProvider theme wrapper test fails consistently. - ⚠️ Tests cannot accurately detect themed wrapper presence. ``` </details> ```suggestion <div data-testid="dashboard-theme-provider">{children}</div> ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the frontend test suite including `superset-frontend/src/components/CrudThemeProvider.test.tsx` (e.g., `npm test CrudThemeProvider.test.tsx`). 2. During the test `'wraps children with SupersetThemeProvider when valid theme data is provided'` at lines 62-78, `MockSupersetThemeProvider` (defined at lines 25-27) renders `<div data-test="dashboard-theme-provider">`. 3. The assertion `screen.getByTestId('dashboard-theme-provider')` at line 76 uses React Testing Library's test id query, which searches for `data-testid="dashboard-theme-provider"` and does not match the `data-test` attribute. 4. Because no element with `data-testid="dashboard-theme-provider"` exists, `getByTestId` throws an error and the test fails, even though the wrapper div is present in the DOM but incorrectly attributed. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/components/CrudThemeProvider.test.tsx **Line:** 26:26 **Comment:** *Logic Error: The mock theme provider uses a `data-test` attribute while the tests query for `data-testid`, so the assertions that check for the presence or absence of the themed wrapper will never actually detect it, causing the tests to fail or give misleading results about whether the theme was applied. 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%2F38384&comment_hash=5e3cda0e2fc674e5cebd7d7ad10c87c98d4435fa30a50783d1b1b0f84b559e28&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38384&comment_hash=5e3cda0e2fc674e5cebd7d7ad10c87c98d4435fa30a50783d1b1b0f84b559e28&reaction=dislike'>👎</a> ########## superset-frontend/src/dashboard/containers/DashboardPage.test.tsx: ########## @@ -0,0 +1,179 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import type { ReactNode } from 'react'; +import { Suspense } from 'react'; +import { render, screen, waitFor } from 'spec/helpers/testing-library'; +import { + useDashboard, + useDashboardCharts, + useDashboardDatasets, +} from 'src/hooks/apiResources'; +import CrudThemeProvider from 'src/components/CrudThemeProvider'; +import DashboardPage from './DashboardPage'; + +const mockTheme = { + id: 42, + theme_name: 'Branded', + json_data: '{"token":{"colorPrimary":"#1677ff"}}', +}; + +const mockDashboard = { + id: 1, + slug: null, + url: '/superset/dashboard/1/', + dashboard_title: 'Test Dashboard', + thumbnail_url: null, + published: true, + css: null, + json_metadata: '{}', + position_json: '{}', + changed_by_name: 'admin', + changed_by: { id: 1, first_name: 'Admin', last_name: 'User' }, + changed_on: '2024-01-01T00:00:00', + charts: [], + owners: [], + roles: [], + theme: mockTheme, +}; + +jest.mock('src/hooks/apiResources', () => ({ + useDashboard: jest.fn(), + useDashboardCharts: jest.fn(), + useDashboardDatasets: jest.fn(), +})); + +jest.mock('src/dashboard/actions/hydrate', () => ({ + ...jest.requireActual('src/dashboard/actions/hydrate'), + hydrateDashboard: jest.fn(() => ({ type: 'MOCK_HYDRATE' })), +})); + +jest.mock('src/dashboard/actions/datasources', () => ({ + ...jest.requireActual('src/dashboard/actions/datasources'), + setDatasources: jest.fn(() => ({ type: 'MOCK_SET_DATASOURCES' })), +})); + +jest.mock('src/dashboard/actions/dashboardState', () => ({ + ...jest.requireActual('src/dashboard/actions/dashboardState'), + setDatasetsStatus: jest.fn(() => ({ type: 'MOCK_SET_DATASETS_STATUS' })), +})); + +jest.mock('src/components/CrudThemeProvider', () => ({ + __esModule: true, + default: jest.fn(({ children }: { children: ReactNode }) => ( + <div>{children}</div> + )), +})); + +jest.mock('src/dashboard/components/DashboardBuilder/DashboardBuilder', () => ({ + __esModule: true, + default: () => <div data-testid="dashboard-builder">DashboardBuilder</div>, +})); + +jest.mock('src/dashboard/components/SyncDashboardState', () => ({ + __esModule: true, + default: () => null, + getDashboardContextLocalStorage: () => ({}), +})); + +jest.mock('src/dashboard/containers/Dashboard', () => ({ + __esModule: true, + default: ({ children }: { children: ReactNode }) => <div>{children}</div>, +})); + +jest.mock('src/components/MessageToasts/withToasts', () => ({ + useToasts: () => ({ addDangerToast: jest.fn(), addSuccessToast: jest.fn() }), +})); + +jest.mock('src/dashboard/util/injectCustomCss', () => ({ + __esModule: true, + default: () => () => {}, +})); + +jest.mock('src/dashboard/util/activeAllDashboardFilters', () => ({ + getAllActiveFilters: () => ({}), + getRelevantDataMask: () => ({}), +})); + +jest.mock('src/dashboard/util/activeDashboardFilters', () => ({ + getActiveFilters: () => ({}), +})); + +jest.mock('src/utils/urlUtils', () => ({ + getUrlParam: () => null, +})); + +jest.mock('src/dashboard/components/nativeFilters/FilterBar/keyValue', () => ({ + getFilterValue: jest.fn(), + getPermalinkValue: jest.fn(), +})); + +jest.mock('src/dashboard/contexts/AutoRefreshContext', () => ({ + AutoRefreshProvider: ({ children }: { children: ReactNode }) => ( + <>{children}</> + ), +})); + +const mockUseDashboard = useDashboard as jest.Mock; +const mockUseDashboardCharts = useDashboardCharts as jest.Mock; +const mockUseDashboardDatasets = useDashboardDatasets as jest.Mock; +const MockCrudThemeProvider = CrudThemeProvider as unknown as jest.Mock; + +beforeEach(() => { + jest.clearAllMocks(); + mockUseDashboard.mockReturnValue({ + result: mockDashboard, + error: null, + }); + mockUseDashboardCharts.mockReturnValue({ + result: [], + error: null, + }); + mockUseDashboardDatasets.mockReturnValue({ + result: [], + error: null, + status: 'complete', + }); +}); + +test('passes full theme object from dashboard API response to CrudThemeProvider', async () => { + render( + <Suspense fallback="loading"> + <DashboardPage idOrSlug="1" /> + </Suspense>, + { + useRedux: true, + useRouter: true, + initialState: { + dashboardInfo: { id: 1, metadata: {} }, + dashboardState: { sliceIds: [] }, + nativeFilters: { filters: {} }, + dataMask: {}, + }, + }, + ); + + await waitFor(() => { + expect(screen.queryByText('loading')).not.toBeInTheDocument(); + }); + + expect(MockCrudThemeProvider).toHaveBeenCalledWith( + expect.objectContaining({ theme: mockTheme }), + expect.anything(), + ); Review Comment: **Suggestion:** The test expects the mocked React component to be called with two arguments (props and a second value matched by `expect.anything()`), but function components are invoked with a single props argument, so this assertion will fail even when the component receives the correct `theme` prop; the expectation should only assert on the props object. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ DashboardPage theme integration test fails deterministically. - ⚠️ CI for this PR will fail on frontend tests. - ⚠️ Incorrectly suggests CrudThemeProvider not receiving theme prop. ``` </details> ```suggestion expect(MockCrudThemeProvider).toHaveBeenCalledWith( expect.objectContaining({ theme: mockTheme }), ); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the frontend Jest tests including `DashboardPage.test.tsx` (e.g. `cd superset-frontend` then `npm test -- src/dashboard/containers/DashboardPage.test.tsx`). 2. In `superset-frontend/src/dashboard/containers/DashboardPage.test.tsx`, note that `CrudThemeProvider` is mocked at lines 76–81 as `default: jest.fn(({ children }: { children: ReactNode }) => (<div>{children}</div>))`, so `MockCrudThemeProvider` is a function component mock that React calls with a single props argument. 3. Observe in `superset-frontend/src/dashboard/containers/DashboardPage.tsx:294` that the real component usage is `<CrudThemeProvider theme={dashboard?.theme}>…</CrudThemeProvider>`, meaning React will invoke the mock with one argument: a props object containing `theme` and `children`. 4. During the test `passes full theme object from dashboard API response to CrudThemeProvider` (lines 154–179), after rendering `<DashboardPage idOrSlug="1" />`, Jest evaluates `expect(MockCrudThemeProvider).toHaveBeenCalledWith(expect.objectContaining({ theme: mockTheme }), expect.anything(),);` (lines 175–178); because the mock was actually called with a single props object, Jest records only one argument in `mock.calls`, so the two-argument `toHaveBeenCalledWith` assertion fails even though the `theme` prop is correct. ``` </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/containers/DashboardPage.test.tsx **Line:** 175:178 **Comment:** *Logic Error: The test expects the mocked React component to be called with two arguments (props and a second value matched by `expect.anything()`), but function components are invoked with a single props argument, so this assertion will fail even when the component receives the correct `theme` prop; the expectation should only assert on the props object. 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%2F38384&comment_hash=b51ff7da958b729163e0359cb21221cbfa2d0751b79dabae1c8b7b4e4518b725&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38384&comment_hash=b51ff7da958b729163e0359cb21221cbfa2d0751b79dabae1c8b7b4e4518b725&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]
