This is an automated email from the ASF dual-hosted git repository. elizabeth pushed a commit to branch fix/modal-confirm-dark-mode-theming in repository https://gitbox.apache.org/repos/asf/superset.git
commit 5e324c3b69404153b53d925ebb1c6dd9cad75bba Author: Elizabeth Thompson <[email protected]> AuthorDate: Fri Oct 3 15:28:09 2025 -0700 test(modals): flatten test structure in ThemeList and EmbeddedModal - Remove nested describe/it blocks in favor of flat test() calls - Follow "avoid nesting when testing" best practices - Improves test readability and maintainability 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> --- .../EmbeddedModal/EmbeddedModal.test.tsx | 112 +++++------ .../src/pages/ThemeList/ThemeList.test.tsx | 221 ++++++++++----------- 2 files changed, 159 insertions(+), 174 deletions(-) diff --git a/superset-frontend/src/dashboard/components/EmbeddedModal/EmbeddedModal.test.tsx b/superset-frontend/src/dashboard/components/EmbeddedModal/EmbeddedModal.test.tsx index 5b9c6b089c..b37dcd1a41 100644 --- a/superset-frontend/src/dashboard/components/EmbeddedModal/EmbeddedModal.test.tsx +++ b/superset-frontend/src/dashboard/components/EmbeddedModal/EmbeddedModal.test.tsx @@ -174,84 +174,78 @@ test('adds extension to DashboardEmbedModal', async () => { extensionsRegistry.set('embedded.modal', undefined); }); -describe('Modal.useModal integration', () => { - beforeEach(() => { - jest.clearAllMocks(); - }); +test('uses Modal.useModal hook for confirmation dialogs', () => { + const useModalSpy = jest.spyOn(Modal, 'useModal'); + setup(); - test('uses Modal.useModal hook for confirmation dialogs', () => { - const useModalSpy = jest.spyOn(Modal, 'useModal'); - setup(); + // Verify that useModal is called when the component mounts + expect(useModalSpy).toHaveBeenCalled(); - // Verify that useModal is called when the component mounts - expect(useModalSpy).toHaveBeenCalled(); + useModalSpy.mockRestore(); +}); - useModalSpy.mockRestore(); +test('renders contextHolder for proper theming', async () => { + const { container } = render(<DashboardEmbedModal {...defaultProps} />, { + useRedux: true, }); - test('renders contextHolder for proper theming', async () => { - const { container } = render(<DashboardEmbedModal {...defaultProps} />, { - useRedux: true, - }); + // Wait for component to be rendered + await screen.findByText('Embed'); - // Wait for component to be rendered - await screen.findByText('Embed'); + // The contextHolder is rendered in the component tree + // Check that modal root elements exist for theming + const modalRootElements = container.querySelectorAll('.ant-modal-root'); + expect(modalRootElements).toBeDefined(); +}); - // The contextHolder is rendered in the component tree - // Check that modal root elements exist for theming - const modalRootElements = container.querySelectorAll('.ant-modal-root'); - expect(modalRootElements).toBeDefined(); +test('confirmation modal inherits theme context', async () => { + setup(); + + // Click deactivate to trigger the confirmation modal + const deactivate = await screen.findByRole('button', { + name: 'Deactivate', }); + fireEvent.click(deactivate); - test('confirmation modal inherits theme context', async () => { - setup(); + // Wait for the modal to appear + const modalTitle = await screen.findByText('Disable embedding?'); + expect(modalTitle).toBeInTheDocument(); - // Click deactivate to trigger the confirmation modal - const deactivate = await screen.findByRole('button', { - name: 'Deactivate', - }); - fireEvent.click(deactivate); + // Check that the modal is rendered within the component tree (not on body directly) + const modal = modalTitle.closest('.ant-modal-wrap'); + expect(modal).toBeInTheDocument(); +}); - // Wait for the modal to appear - const modalTitle = await screen.findByText('Disable embedding?'); - expect(modalTitle).toBeInTheDocument(); +test('does not use Modal.confirm directly', () => { + // Spy on the static Modal.confirm method + const confirmSpy = jest.spyOn(Modal, 'confirm'); - // Check that the modal is rendered within the component tree (not on body directly) - const modal = modalTitle.closest('.ant-modal-wrap'); - expect(modal).toBeInTheDocument(); - }); + setup(); - test('does not use Modal.confirm directly', () => { - // Spy on the static Modal.confirm method - const confirmSpy = jest.spyOn(Modal, 'confirm'); + // The component should not call Modal.confirm directly + expect(confirmSpy).not.toHaveBeenCalled(); - setup(); + confirmSpy.mockRestore(); +}); - // The component should not call Modal.confirm directly - expect(confirmSpy).not.toHaveBeenCalled(); +test('modal actions work correctly with useModal', async () => { + setup(); - confirmSpy.mockRestore(); + // Click deactivate + const deactivate = await screen.findByRole('button', { + name: 'Deactivate', }); + fireEvent.click(deactivate); - test('modal actions work correctly with useModal', async () => { - setup(); - - // Click deactivate - const deactivate = await screen.findByRole('button', { - name: 'Deactivate', - }); - fireEvent.click(deactivate); - - // Modal should appear - expect(await screen.findByText('Disable embedding?')).toBeInTheDocument(); + // Modal should appear + expect(await screen.findByText('Disable embedding?')).toBeInTheDocument(); - // Click Cancel to close modal - const cancelBtn = screen.getByRole('button', { name: 'Cancel' }); - fireEvent.click(cancelBtn); + // Click Cancel to close modal + const cancelBtn = screen.getByRole('button', { name: 'Cancel' }); + fireEvent.click(cancelBtn); - // Modal should close - await waitFor(() => { - expect(screen.queryByText('Disable embedding?')).not.toBeInTheDocument(); - }); + // Modal should close + await waitFor(() => { + expect(screen.queryByText('Disable embedding?')).not.toBeInTheDocument(); }); }); diff --git a/superset-frontend/src/pages/ThemeList/ThemeList.test.tsx b/superset-frontend/src/pages/ThemeList/ThemeList.test.tsx index 13cf75e8a3..9efc23f492 100644 --- a/superset-frontend/src/pages/ThemeList/ThemeList.test.tsx +++ b/superset-frontend/src/pages/ThemeList/ThemeList.test.tsx @@ -100,162 +100,153 @@ const renderThemesList = (props = {}) => }, ); -// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks -describe('ThemesList', () => { - beforeEach(() => { - fetchMock.resetHistory(); - }); +beforeEach(() => { + fetchMock.resetHistory(); +}); - test('renders', async () => { - renderThemesList(); - expect(await screen.findByText('Themes')).toBeInTheDocument(); - }); +test('renders', async () => { + renderThemesList(); + expect(await screen.findByText('Themes')).toBeInTheDocument(); +}); - test('renders a ListView', async () => { - renderThemesList(); - expect(await screen.findByTestId('themes-list-view')).toBeInTheDocument(); - }); +test('renders a ListView', async () => { + renderThemesList(); + expect(await screen.findByTestId('themes-list-view')).toBeInTheDocument(); +}); - test('renders theme information', async () => { - renderThemesList(); +test('renders theme information', async () => { + renderThemesList(); - // Wait for list to load - await screen.findByTestId('themes-list-view'); + // Wait for list to load + await screen.findByTestId('themes-list-view'); - // Wait for data to load - await waitFor(() => { - mockThemes.forEach(theme => { - expect(screen.getByText(theme.theme_name)).toBeInTheDocument(); - }); + // Wait for data to load + await waitFor(() => { + mockThemes.forEach(theme => { + expect(screen.getByText(theme.theme_name)).toBeInTheDocument(); }); }); +}); - test('shows system theme tags correctly', async () => { - renderThemesList(); +test('shows system theme tags correctly', async () => { + renderThemesList(); - // Wait for list to load - await screen.findByTestId('themes-list-view'); + // Wait for list to load + await screen.findByTestId('themes-list-view'); - // System theme should have a "System" tag - await waitFor(() => { - expect(screen.getByText('System')).toBeInTheDocument(); - }); + // System theme should have a "System" tag + await waitFor(() => { + expect(screen.getByText('System')).toBeInTheDocument(); }); +}); - test('handles theme deletion for non-system themes', async () => { - renderThemesList(); +test('handles theme deletion for non-system themes', async () => { + renderThemesList(); - // Wait for list to load - await screen.findByTestId('themes-list-view'); + // Wait for list to load + await screen.findByTestId('themes-list-view'); - // Find delete buttons (should only exist for non-system themes) - const deleteButtons = await screen.findAllByTestId('delete-action'); - expect(deleteButtons.length).toBeGreaterThan(0); + // Find delete buttons (should only exist for non-system themes) + const deleteButtons = await screen.findAllByTestId('delete-action'); + expect(deleteButtons.length).toBeGreaterThan(0); - fireEvent.click(deleteButtons[0]); + fireEvent.click(deleteButtons[0]); - // Confirm deletion modal should appear - await waitFor(() => { - expect(screen.getByText('Delete Theme?')).toBeInTheDocument(); - }); + // Confirm deletion modal should appear + await waitFor(() => { + expect(screen.getByText('Delete Theme?')).toBeInTheDocument(); }); +}); - test('shows apply action for themes', async () => { - renderThemesList(); +test('shows apply action for themes', async () => { + renderThemesList(); - // Wait for list to load - await screen.findByTestId('themes-list-view'); + // Wait for list to load + await screen.findByTestId('themes-list-view'); - // Find apply buttons - const applyButtons = await screen.findAllByTestId('apply-action'); - expect(applyButtons.length).toBe(mockThemes.length); - }); + // Find apply buttons + const applyButtons = await screen.findAllByTestId('apply-action'); + expect(applyButtons.length).toBe(mockThemes.length); +}); - test('fetches themes data on load', async () => { - renderThemesList(); +test('fetches themes data on load', async () => { + renderThemesList(); - await waitFor(() => { - const calls = fetchMock.calls(/api\/v1\/theme\/\?/); - expect(calls.length).toBeGreaterThan(0); - }); + await waitFor(() => { + const calls = fetchMock.calls(/api\/v1\/theme\/\?/); + expect(calls.length).toBeGreaterThan(0); }); +}); - test('shows bulk select when user has permissions', async () => { - renderThemesList(); - - // Wait for list to load - await screen.findByText('Themes'); +test('shows bulk select when user has permissions', async () => { + renderThemesList(); - // Should show bulk select button - expect(screen.getByText('Bulk select')).toBeInTheDocument(); - }); + // Wait for list to load + await screen.findByText('Themes'); - test('shows create theme button when user has permissions', async () => { - renderThemesList(); + // Should show bulk select button + expect(screen.getByText('Bulk select')).toBeInTheDocument(); +}); - // Wait for list to load - await screen.findByText('Themes'); +test('shows create theme button when user has permissions', async () => { + renderThemesList(); - // Should show theme creation button - const addButton = screen.getByLabelText('plus'); - expect(addButton).toBeInTheDocument(); - }); + // Wait for list to load + await screen.findByText('Themes'); - describe('Modal.useModal integration', () => { - beforeEach(() => { - jest.clearAllMocks(); - }); + // Should show theme creation button + const addButton = screen.getByLabelText('plus'); + expect(addButton).toBeInTheDocument(); +}); - it('uses Modal.useModal hook instead of Modal.confirm', () => { - const useModalSpy = jest.spyOn(Modal, 'useModal'); - renderThemesList(); +test('uses Modal.useModal hook instead of Modal.confirm', () => { + const useModalSpy = jest.spyOn(Modal, 'useModal'); + renderThemesList(); - // Verify that useModal is called when the component mounts - expect(useModalSpy).toHaveBeenCalled(); + // Verify that useModal is called when the component mounts + expect(useModalSpy).toHaveBeenCalled(); - useModalSpy.mockRestore(); - }); + useModalSpy.mockRestore(); +}); - it('renders contextHolder for modal theming', async () => { - const { container } = renderThemesList(); +test('renders contextHolder for modal theming', async () => { + const { container } = renderThemesList(); - // Wait for component to be rendered - await screen.findByText('Themes'); + // Wait for component to be rendered + await screen.findByText('Themes'); - // The contextHolder is rendered but invisible, so we check for its presence in the DOM - // Modal.useModal returns elements that get rendered in the component tree - const contextHolderExists = container.querySelector('.ant-modal-root'); - expect(contextHolderExists).toBeDefined(); - }); + // The contextHolder is rendered but invisible, so we check for its presence in the DOM + // Modal.useModal returns elements that get rendered in the component tree + const contextHolderExists = container.querySelector('.ant-modal-root'); + expect(contextHolderExists).toBeDefined(); +}); - it('confirms system theme changes using themed modal', async () => { - const mockSetSystemDefault = jest.fn().mockResolvedValue({}); - fetchMock.post( - 'glob:*/api/v1/theme/*/set_system_default', - mockSetSystemDefault, - ); +test('confirms system theme changes using themed modal', async () => { + const mockSetSystemDefault = jest.fn().mockResolvedValue({}); + fetchMock.post( + 'glob:*/api/v1/theme/*/set_system_default', + mockSetSystemDefault, + ); - renderThemesList(); + renderThemesList(); - // Wait for list to load - await screen.findByTestId('themes-list-view'); + // Wait for list to load + await screen.findByTestId('themes-list-view'); - // Since the test data doesn't render actual action buttons, we'll verify - // that the modal system is properly set up by checking the hook was called - // This is validated in the "uses Modal.useModal hook" test - expect(true).toBe(true); - }); + // Since the test data doesn't render actual action buttons, we'll verify + // that the modal system is properly set up by checking the hook was called + // This is validated in the "uses Modal.useModal hook" test + expect(true).toBe(true); +}); - it('does not use deprecated Modal.confirm directly', () => { - // Create a spy on the static Modal.confirm method - const confirmSpy = jest.spyOn(Modal, 'confirm'); +test('does not use deprecated Modal.confirm directly', () => { + // Create a spy on the static Modal.confirm method + const confirmSpy = jest.spyOn(Modal, 'confirm'); - renderThemesList(); + renderThemesList(); - // The component should not call Modal.confirm directly - expect(confirmSpy).not.toHaveBeenCalled(); + // The component should not call Modal.confirm directly + expect(confirmSpy).not.toHaveBeenCalled(); - confirmSpy.mockRestore(); - }); - }); + confirmSpy.mockRestore(); });
