Skip to content

fix(tests): resolve AlertReportModal checkmark test failures #34995

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rusackas merged 2 commits into from
Sep 3, 2025
Merged

fix(tests): resolve AlertReportModal checkmark test failures #34995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eac36ec
fix(tests): resolve AlertReportModal checkmark and chart visibility f…
sadpandajoe Sep 3, 2025
c9abb20
chore: address ai pr comments
sadpandajoe Sep 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions superset-frontend/packages/superset-ui-core/src/components/Collapse/CollapseLabelInModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,9 @@ export const CollapseLabelInModal: React.FC<CollapseLabelInModalProps> = ({
{title}{' '}
{validateCheckStatus !== undefined &&
(validateCheckStatus ? (
<Icons.CheckCircleOutlined
iconColor={theme.colorSuccess}
aria-label="check-circle"
/>
<Icons.CheckCircleOutlined iconColor={theme.colorSuccess} />
) : (
<Icons.ExclamationCircleOutlined
iconColor={theme.colorError}
aria-label="error-circle"
/>
<Icons.ExclamationCircleOutlined iconColor={theme.colorError} />
))}
</Typography.Title>
<Typography.Paragraph
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/packages/superset-ui-core/src/components/Icons/BaseIcon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { AntdIconType, BaseIconProps, CustomIconType, IconType } from './types';

const genAriaLabel = (fileName: string) => {
const name = fileName.replace(/_/g, '-'); // Replace underscores with dashes
const words = name.split(/(?=[A-Z])/); // Split at uppercase letters
const words = name.split(/(?<=[a-z])(?=[A-Z])/); // Split at lowercase-to-uppercase transitions

@bito-code-review bito-code-review Bot Sep 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regex change breaks aria-label generation

The regex change from /(?=[A-Z])/.filter(word => word.length > 0) to /(?<=[a-z])(?=[A-Z])/ fundamentally alters string splitting behavior and breaks aria-label generation for strings with consecutive uppercase letters like 'SLACK', 'XMLHttpRequest', 'HTMLElement'. This will cause incorrect accessibility labels in production. Please revert to the original regex pattern.

Code suggestion
Check the AI-generated fix before applying
Suggested change
const words = name.split(/(?<=[a-z])(?=[A-Z])/); // Split at lowercase-to-uppercase transitions
const words = name.split(/(?=[A-Z])/).filter(word => word.length > 0); // Split at uppercase letters and filter out empty strings

Code Review Run #c3ee1f

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

@sadpandajoe sadpandajoe Sep 4, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both regexes produce identical results for icon names like "CheckCircleOutlined"

  • The updated regex is actually more precise (only splits at lowercase-to-uppercase transitions) - Tests are passing, proving functionality works correctly


if (words.length === 2) {
return words[0].toLowerCase();
Expand Down
31 changes: 27 additions & 4 deletions superset-frontend/src/features/alerts/AlertReportModal.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import {
userEvent,
within,
} from 'spec/helpers/testing-library';
import { VizType } from '@superset-ui/core';
import { buildErrorTooltipMessage } from './buildErrorTooltipMessage';
import AlertReportModal, { AlertReportModalProps } from './AlertReportModal';
import { AlertObject, NotificationMethodOption } from './types';
Expand All @@ -49,6 +48,7 @@ const generateMockPayload = (dashboard = true) => {
database: {
database_name: 'examples',
id: 1,
value: 1,
},
description: 'Some description',
extra: {},
Expand Down Expand Up @@ -93,8 +93,9 @@ const generateMockPayload = (dashboard = true) => {
...mockPayload,
chart: {
id: 1,
slice_name: 'Test Chart',
viz_type: VizType.Table,
slice_name: 'test chart',
viz_type: 'table',
value: 1,
},
Comment on lines 94 to 99

@bito-code-review bito-code-review Bot Sep 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing required label property in chart mock

The chart object mock is missing the required label property. The AlertReportModal component expects chart objects to have a label property as defined in the MetaObject interface, and the component code specifically checks for chart.label in multiple places (lines 871, 889, 1646, 1650). Add label: 'test chart' to the chart object to fix the broken functionality.

Code suggestion
Check the AI-generated fix before applying
Suggested change
chart: {
id: 1,
slice_name: 'Test Chart',
viz_type: VizType.Table,
slice_name: 'test chart',
viz_type: 'table',
value: 1,
},
chart: {
id: 1,
label: 'test chart',
slice_name: 'test chart',
viz_type: 'table',
value: 1,
},

Code Review Run #c3ee1f

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

@sadpandajoe sadpandajoe Sep 4, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Looking at line 170 in validAlert, the label IS provided where needed
  • The generateMockPayload function doesn't need label for all use cases

};
};
Expand Down Expand Up @@ -134,7 +135,7 @@ const validAlert: AlertObject = {
creation_method: 'alerts_reports',
crontab: '0 0 * * *',
dashboard_id: 0,
chart_id: 0,
chart_id: 1,
force_screenshot: false,
last_state: 'Not triggered',
name: 'Test Alert',
Expand All @@ -153,6 +154,24 @@ const validAlert: AlertObject = {
],
timezone: 'America/Rainy_River',
type: 'Alert',
database: {
id: 1,
value: 1,
database_name: 'test_db',
} as any,
sql: 'SELECT COUNT(*) FROM test_table',
validator_config_json: {
op: '>',
threshold: 10.0,
},
working_timeout: 3600,
chart: {
id: 1,
value: 1,
label: 'Test Chart',
slice_name: 'Test Chart',
viz_type: 'table',
} as any,
};

jest.mock('./buildErrorTooltipMessage', () => ({
Expand Down Expand Up @@ -258,6 +277,10 @@ test('renders 5 checkmarks for a valid alert', async () => {
render(<AlertReportModal {...generateMockedProps(false, true, false)} />, {
useRedux: true,
});

// Wait for validation to complete by waiting for the modal to fully render
await screen.findByText('Edit alert');

const checkmarks = await screen.findAllByRole('img', {
name: /check-circle/i,
});
Expand Down
Loading