bito-code-review[bot] commented on code in PR #38342: URL: https://github.com/apache/superset/pull/38342#discussion_r2875087987
########## superset-frontend/spec/helpers/a11yTestHelper.tsx: ########## @@ -0,0 +1,75 @@ +/** + * 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. + */ + +/** + * Accessibility Test Helper + * + * Shared utilities for axe-core based a11y testing. + * Uses jest-axe to run automated WCAG 2.1 Level A+AA checks. + */ +import { configureAxe, toHaveNoViolations } from 'jest-axe'; + +expect.extend(toHaveNoViolations); + +// WCAG 2.1 Level A + AA rules relevant to our 15 criteria +export const WCAG_RULES = [ + 'image-alt', // 1.1.1 + 'button-name', // 1.1.1 + 'aria-toggle-field-name', // 1.3.3 + 'link-in-text-block', // 1.4.1 + 'color-contrast', // 1.4.11 + 'label', // 2.4.6 + 'autocomplete-valid', // 1.3.5 + 'heading-order', // 2.4.6 + 'aria-required-attr', // general + 'aria-valid-attr', // general + 'aria-valid-attr-value', // general +] as const; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Invalid Axe-Core Rule</b></div> <div id="fix"> The WCAG_RULES array includes 'aria-toggle-field-name', which is not a valid axe-core rule. This could lead to incomplete accessibility checks or runtime errors. Remove this invalid rule to ensure proper a11y testing. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion export const WCAG_RULES = [ 'image-alt', // 1.1.1 'button-name', // 1.1.1 'link-in-text-block', // 1.4.1 'color-contrast', // 1.4.11 'label', // 2.4.6 'autocomplete-valid', // 1.3.5 'heading-order', // 2.4.6 'aria-required-attr', // general 'aria-valid-attr', // general 'aria-valid-attr-value', // general ] as const; ```` </div> </details> </div> <small><i>Code Review Run #ce4a60</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/pages/Home/Home.a11y.test.tsx: ########## @@ -0,0 +1,108 @@ +/** + * 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. + */ + +/** + * Home/Welcome Page - Accessibility Tests (WCAG 2.1 Level A + AA) + * + * Tests cover: + * - WCAG 1.1.1: Non-text content has alternatives + * - WCAG 1.3.3: Sensory characteristics (toggle switches) + * - WCAG 2.4.6: Headings and labels + * - WCAG 2.4.7: Focus visible on navigation elements + */ +import fetchMock from 'fetch-mock'; +import { render, waitFor } from 'spec/helpers/testing-library'; +import { checkA11y } from 'spec/helpers/a11yTestHelper'; +import Welcome from 'src/pages/Home'; + +// API mocks matching Home.test.tsx patterns +fetchMock.get('glob:*/api/v1/chart/?*', { result: [] }); +fetchMock.get('glob:*/api/v1/chart/_info?*', { permissions: [] }); +fetchMock.get('glob:*/api/v1/chart/favorite_status?*', { result: [] }); +fetchMock.get('glob:*/api/v1/dashboard/?*', { result: [] }); +fetchMock.get('glob:*/api/v1/dashboard/_info?*', { permissions: [] }); +fetchMock.get('glob:*/api/v1/dashboard/favorite_status/?*', { result: [] }); +fetchMock.get('glob:*/api/v1/saved_query/?*', { result: [] }); +fetchMock.get('glob:*/api/v1/saved_query/_info?*', { permissions: [] }); +fetchMock.get('glob:*/api/v1/log/recent_activity/*', { result: [] }); + +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + isFeatureEnabled: jest.fn().mockReturnValue(false), +})); + +const mockedProps = { + user: { + username: 'alpha', + firstName: 'alpha', + lastName: 'alpha', + createdOn: '2016-11-11T12:34:17', + userId: 5, + email: '[email protected]', + isActive: true, + isAnonymous: false, + permissions: {}, + roles: { + sql_lab: [['can_read', 'SavedQuery']], + }, + }, +}; + +// eslint-disable-next-line no-restricted-globals +describe('Home/Welcome Page - Accessibility', () => { + afterEach(() => { + fetchMock.clearHistory(); + }); + + test('should have no axe-core violations', async () => { + const { container } = await waitFor(() => + render(<Welcome {...mockedProps} />, { + useRedux: true, + useRouter: true, + }), + ); + const results = await checkA11y(container); + expect(results).toHaveNoViolations(); + }); + + test('navigation panels should have accessible headings', async () => { + const { findAllByText } = await waitFor(() => + render(<Welcome {...mockedProps} />, { + useRedux: true, + useRouter: true, + }), + ); + const panels = await findAllByText(/Dashboards|Charts|Recents/); + expect(panels.length).toBeGreaterThan(0); + }); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Test assertion mismatch for headings</b></div> <div id="fix"> The test claims to check for accessible headings but only verifies text presence, which doesn't ensure WCAG 2.4.6 compliance. It could pass if the text appears in non-heading elements. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion test('navigation panels should have accessible headings', async () => { const { getAllByRole } = render(<Welcome {...mockedProps} />, { useRedux: true, useRouter: true, }); const headings = getAllByRole('heading', { name: /Dashboards|Charts|Recents/ }); expect(headings.length).toBeGreaterThan(0); }); ```` </div> </details> </div> <small><i>Code Review Run #ce4a60</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/pages/SqlLab/SqlLab.a11y.test.tsx: ########## @@ -0,0 +1,171 @@ +/** + * 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. + */ + +/** + * SQL Lab Page - Accessibility Tests (WCAG 2.1 Level A + AA) + * + * Tests cover: + * - WCAG 1.1.1: Editor and result areas have accessible labels + * - WCAG 2.4.6: Tab headings are descriptive + * - WCAG 2.4.7: Focus management in tabbed interface + * - WCAG 3.3.1: SQL error messages are programmatically identifiable + */ +import fetchMock from 'fetch-mock'; +import { + render, + act, + waitFor, + defaultStore as store, + createStore, +} from 'spec/helpers/testing-library'; +import reducers from 'spec/helpers/reducerIndex'; +import { api } from 'src/hooks/apiResources/queryApi'; +import { DEFAULT_COMMON_BOOTSTRAP_DATA } from 'src/constants'; +import { checkA11y } from 'spec/helpers/a11yTestHelper'; + +import SqlLab from '.'; + +const fakeApiResult = { + result: { + common: DEFAULT_COMMON_BOOTSTRAP_DATA, + tab_state_ids: [], + databases: [], + queries: {}, + user: { + userId: 1, + username: 'test_user', + isActive: true, + isAnonymous: false, + firstName: 'Test', + lastName: 'User', + permissions: {}, + roles: {}, + }, + }, +}; + +const sqlLabInitialStateApiRoute = `glob:*/api/v1/sqllab/`; + +jest.mock('src/SqlLab/components/App', () => () => ( + <div data-test="mock-sqllab-app" role="application" aria-label="SQL Lab editor"> + <div role="tablist" aria-label="Query tabs"> + <button role="tab" aria-selected="true" aria-controls="tab-panel-1"> + Query 1 + </button> + </div> + <div id="tab-panel-1" role="tabpanel" aria-labelledby="tab-1"> + <label htmlFor="sql-editor">SQL Query</label> + <textarea id="sql-editor" aria-label="SQL query editor" /> + <button type="button" aria-label="Run query"> + Run + </button> + <section aria-label="Query results"> + <table aria-label="Query result table"> + <thead> + <tr> + <th scope="col">Column</th> + </tr> + </thead> + <tbody> + <tr> + <td>Value</td> + </tr> + </tbody> + </table> + </section> + </div> + </div> +)); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Mocked component structure doesn't match real App</b></div> <div id="fix"> The test mocks the App component with a simplified static JSX structure that doesn't reflect the real App component's rendering (which includes TabbedSqlEditors, AppLayout, etc.). This means the accessibility tests are verifying attributes on a mock rather than the actual SQL Lab interface, potentially missing real a11y regressions. Consider either making the mock more accurate to the real component's output or testing the real component with full state/API mocking. </div> </div> <small><i>Code Review Run #ce4a60</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/pages/DatabaseList/DatabaseList.a11y.test.tsx: ########## @@ -0,0 +1,161 @@ +/** + * 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. + */ + +/** + * Database List Page - Accessibility Tests (WCAG 2.1 Level A + AA) + * + * Tests cover: + * - WCAG 1.1.1: Action buttons have accessible names + * - WCAG 1.4.11: Non-text contrast for table controls + * - WCAG 2.4.6: Table headers and column labels + */ +import { render } from 'spec/helpers/testing-library'; +import { checkA11y } from 'spec/helpers/a11yTestHelper'; + +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + isFeatureEnabled: jest.fn().mockReturnValue(false), +})); + +// Mock DatabaseList with accessible ListView structure +jest.mock('./index', () => ({ + __esModule: true, + default: () => ( + <div data-test="database-list-page"> + <header> + <h2>Databases</h2> + <div role="toolbar" aria-label="Database actions"> + <button type="button" aria-label="Add new database connection"> + + Database + </button> + </div> + </header> + <section aria-label="Database filters"> + <label htmlFor="database-search">Search databases</label> + <input + id="database-search" + type="search" + aria-label="Search databases" + /> + </section> + <table aria-label="Databases list"> + <thead> + <tr> + <th scope="col">Database name</th> + <th scope="col">Backend</th> + <th scope="col">Allow DML</th> + <th scope="col">CSV upload</th> + <th scope="col">Expose in SQL Lab</th> + <th scope="col">Modified</th> + <th scope="col">Actions</th> + </tr> + </thead> + <tbody> + <tr> + <td>PostgreSQL Production</td> + <td>PostgreSQL</td> + <td> + <span role="img" aria-label="Enabled"> + Yes + </span> + </td> + <td> + <span role="img" aria-label="Disabled"> + No + </span> + </td> + <td> + <span role="img" aria-label="Enabled"> + Yes + </span> + </td> + <td>2024-01-01</td> + <td> + <button + type="button" + aria-label="Edit PostgreSQL Production database" + > + Edit + </button> + <button + type="button" + aria-label="Delete PostgreSQL Production database" + > + Delete + </button> + </td> + </tr> + </tbody> + </table> + </div> + ), +})); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Test mocks component inaccurately</b></div> <div id="fix"> The test mocks the DatabaseList component with a custom JSX structure that differs significantly from the actual component's output. The real component uses ListView with spans (role='button') for actions and Icons.CheckOutlined/Icons.CloseOutlined for boolean displays, while the mock uses proper buttons with aria-label and spans with role='img' and aria-label. This means the test validates an idealized accessible structure but doesn't ensure the real Database List page is accessible. The real component's boolean displays lack aria-labels, and actions use spans instead of buttons, potentially failing accessibility standards. To properly test the page's accessibility, remove the component mock and mock the underlying hooks/data instead. </div> </div> <small><i>Code Review Run #ce4a60</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/a11y-regression.test.tsx: ########## @@ -0,0 +1,132 @@ +/** + * A11y Regression Test Suite + * + * These tests verify that critical accessibility patches survive upstream updates. + * Each test reads the actual source files and checks for required a11y attributes, + * ensuring that merges or rebases do not silently remove accessibility fixes. + * + * Run: npx jest a11y-regression.test + */ + Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Jest Environment Mismatch</b></div> <div id="fix"> This test file uses the Node.js 'fs' module to read source files, but Jest is configured with jsdom environment which doesn't provide 'fs'. The tests will fail at runtime. Add the Jest environment pragma to specify Node environment. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion */ /** @jest-environment node */ ```` </div> </details> </div> <small><i>Code Review Run #ce4a60</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
