codeant-ai-for-open-source[bot] commented on code in PR #38342: URL: https://github.com/apache/superset/pull/38342#discussion_r2874700799
########## superset-frontend/src/pages/Chart/Chart.a11y.test.tsx: ########## @@ -0,0 +1,107 @@ +/** + * 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. + */ + +/** + * Explore/Chart Builder Page - Accessibility Tests (WCAG 2.1 Level A + AA) + * + * Tests cover: + * - WCAG 1.1.1: Control panels have accessible labels + * - WCAG 1.3.3: Dropdown controls convey info beyond color + * - WCAG 2.4.6: Headings and labels for control sections + * - WCAG 2.4.7: Focus visible on chart builder controls + * - WCAG 3.3.1: Error identification in chart configuration + */ +import fetchMock from 'fetch-mock'; +import { render, waitFor } from 'spec/helpers/testing-library'; +import { checkA11y } from 'spec/helpers/a11yTestHelper'; + +jest.mock('src/hooks/useUnsavedChangesPrompt', () => ({ + useUnsavedChangesPrompt: jest.fn().mockReturnValue({ + showModal: false, + setShowModal: jest.fn(), + handleConfirmNavigation: jest.fn(), + handleSaveAndCloseModal: jest.fn(), + }), +})); + +jest.mock('re-resizable', () => ({ + Resizable: () => <div data-test="mock-re-resizable" />, +})); + +jest.mock( + 'src/explore/components/ExploreChartPanel', + () => + () => ( + <div data-test="mock-explore-chart-panel" role="region" aria-label="Chart visualization"> + <div role="img" aria-label="Chart output">Chart content</div> + </div> + ), +); + +jest.mock('src/dashboard/util/charts/getFormDataWithExtraFilters'); +jest.mock('src/explore/exploreUtils/getParsedExploreURLParams', () => ({ + getParsedExploreURLParams: jest.fn().mockReturnValue(new Map()), +})); + +import ChartPage from '.'; + +// eslint-disable-next-line no-restricted-globals +describe('Explore/Chart Builder Page - Accessibility', () => { + beforeEach(() => { + fetchMock.get('glob:*/api/v1/explore/*', { + result: { + dataset: { id: 1 }, + form_data: { viz_type: 'table', datasource: '1__table' }, + }, + }); + }); + + afterEach(() => { + fetchMock.clearHistory().removeRoutes(); + }); + + test('should have no axe-core violations', async () => { + const { container } = render(<ChartPage />, { + useRouter: true, + useRedux: true, + useDnd: true, + }); + await waitFor(() => + expect( + fetchMock.callHistory.calls('glob:*/api/v1/explore/*').length, + ).toBeGreaterThanOrEqual(1), + ); Review Comment: **Suggestion:** The test relies on `fetchMock.callHistory.calls(...)`, but `fetch-mock` exposes `calls(...)` directly rather than through a `callHistory` object, so this expression will be `undefined` at runtime and throw when accessing `.calls`, preventing the test from running correctly; use `fetchMock.calls(...)` instead. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ ChartPage accessibility test crashes before executing assertions. - ⚠️ CI a11y coverage for Explore/ChartPage becomes unreliable. ``` </details> ```suggestion await waitFor(() => expect(fetchMock.calls('glob:*/api/v1/explore/*').length).toBeGreaterThanOrEqual(1), ); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the Jest test suite including `superset-frontend/src/pages/Chart/Chart.a11y.test.tsx` (for example via `npm test` or the Superset frontend test command). 2. The test case `should have no axe-core violations` starting at line 79 mounts `<ChartPage />` and then reaches the `await waitFor(...)` block at lines 85–89. 3. Inside the `waitFor` callback, Jest executes `fetchMock.callHistory.calls('glob:*/api/v1/explore/*')` (line 87); in the `fetch-mock` library used by this project, the public API is `fetchMock.calls(...)` and there is no `callHistory` property, so `fetchMock.callHistory` is `undefined` and accessing `.calls` throws `TypeError: Cannot read properties of undefined`. 4. Because this exception is thrown inside `waitFor`, the test fails before asserting that any fetch was made or running the subsequent `checkA11y(container)` call, causing the accessibility test for `ChartPage` to be permanently broken in CI. ``` </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/Chart/Chart.a11y.test.tsx **Line:** 85:89 **Comment:** *Possible Bug: The test relies on `fetchMock.callHistory.calls(...)`, but `fetch-mock` exposes `calls(...)` directly rather than through a `callHistory` object, so this expression will be `undefined` at runtime and throw when accessing `.calls`, preventing the test from running correctly; use `fetchMock.calls(...)` instead. 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%2F38342&comment_hash=2208b20c7c08ffee87345cbe6a4105f8c7488d71c75b835bdea6df7f3499986e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38342&comment_hash=2208b20c7c08ffee87345cbe6a4105f8c7488d71c75b835bdea6df7f3499986e&reaction=dislike'>👎</a> ########## 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 + */ + +import fs from 'fs'; +import path from 'path'; + +const ROOT = path.resolve(__dirname, '..'); + +function readSource(relativePath: string): string { + const fullPath = path.join(ROOT, relativePath); + if (!fs.existsSync(fullPath)) { + throw new Error(`Source file not found: ${fullPath}`); + } + return fs.readFileSync(fullPath, 'utf-8'); +} + +describe('A11y Regression Tests', () => { + describe('Phase 1: Level A Fixes', () => { + test('ECharts Echart component uses SVGRenderer instead of CanvasRenderer', () => { + const source = readSource( + 'plugins/plugin-chart-echarts/src/components/Echart.tsx', + ); + expect(source).toContain('SVGRenderer'); + // Must NOT use CanvasRenderer in the echarts.use() call + expect(source).not.toMatch(/use\(\[.*CanvasRenderer/); + }); Review Comment: **Suggestion:** The negative test that checks ECharts does not use `CanvasRenderer` relies on a regex `use\(\[.*CanvasRenderer` where `.*` does not match newlines, so if the `echarts.use([...])` call is formatted across multiple lines (which is very likely in real code), the regex will fail to detect `CanvasRenderer` and the test will incorrectly pass, allowing regressions to slip through. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ A11y regression test misses CanvasRenderer reintroduction. - ⚠️ ECharts charts may silently lose text scalability. - ⚠️ WCAG 1.4.4 compliance for ECharts views at risk. ``` </details> ```suggestion // Must NOT use CanvasRenderer in the echarts.use() call expect(source).not.toMatch(/use\(\[[\s\S]*CanvasRenderer/); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In `plugins/plugin-chart-echarts/src/components/Echart.tsx`, edit the echarts registration to use a multi-line array including `CanvasRenderer`, e.g.: `echarts.use([` on one line, `CanvasRenderer,` on the next line, then closing `]);` (exact line numbers not provided in the PR context). 2. Ensure `superset-frontend/src/a11y-regression.test.tsx` remains as in this PR, with the negative check at line 33: `expect(source).not.toMatch(/use\(\[.*CanvasRenderer/);`. 3. From `superset-frontend/`, run the regression suite as documented in the header comment: `npx jest a11y-regression.test`. 4. Observe that the test `"ECharts Echart component uses SVGRenderer instead of CanvasRenderer"` still passes even though `CanvasRenderer` is now present in the multi-line `echarts.use([...])` call in `Echart.tsx`, because the `.*` in the regex at line 33 does not match newlines and fails to see `CanvasRenderer`. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/a11y-regression.test.tsx **Line:** 32:33 **Comment:** *Logic Error: The negative test that checks ECharts does not use `CanvasRenderer` relies on a regex `use\(\[.*CanvasRenderer` where `.*` does not match newlines, so if the `echarts.use([...])` call is formatted across multiple lines (which is very likely in real code), the regex will fail to detect `CanvasRenderer` and the test will incorrectly pass, allowing regressions to slip through. 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%2F38342&comment_hash=1bc52c6984ded0d237330870e5bc87626ef886d4f2f2285823d9fc4a4a98a6a7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38342&comment_hash=1bc52c6984ded0d237330870e5bc87626ef886d4f2f2285823d9fc4a4a98a6a7&reaction=dislike'>👎</a> ########## 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 Review Comment: **Suggestion:** The tabpanel's `aria-labelledby="tab-1"` references an element ID that doesn't exist, which violates ARIA relationships and is likely to trigger axe-core accessibility violations, causing the a11y test to fail; add a matching `id="tab-1"` to the tab button so the reference is valid. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ SQL Lab accessibility Jest test fails every run. - ⚠️ CI a11y gate blocked by invalid ARIA mock. - ⚠️ Accessibility coverage for SQL Lab cannot be trusted. - ⚠️ Issue exists only in Jest mock, not production UI. ``` </details> ```suggestion <button id="tab-1" role="tab" aria-selected="true" aria-controls="tab-panel-1"> ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the Jest test suite that includes `superset-frontend/src/pages/SqlLab/SqlLab.a11y.test.tsx` (the test file added in this PR). 2. Inside that file, the `jest.mock('src/SqlLab/components/App', ...)` at lines 66–94 returns markup where the tab button (line 69) has no `id`, but the tabpanel `<div id="tab-panel-1" role="tabpanel" aria-labelledby="tab-1">` (line 72) references `aria-labelledby="tab-1"`. 3. The test `"should have no axe-core violations"` at lines 109–123 renders `<SqlLab />`, which uses the mocked App, and then calls `checkA11y(container)` from `spec/helpers/a11yTestHelper`. 4. `checkA11y` runs axe-core on this DOM and axe reports an ARIA relationship violation because `aria-labelledby="tab-1"` points to a non-existent element; the expectation `expect(results).toHaveNoViolations()` then fails, causing the test (and CI a11y check) to fail consistently. ``` </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/SqlLab/SqlLab.a11y.test.tsx **Line:** 69:69 **Comment:** *Logic Error: The tabpanel's `aria-labelledby="tab-1"` references an element ID that doesn't exist, which violates ARIA relationships and is likely to trigger axe-core accessibility violations, causing the a11y test to fail; add a matching `id="tab-1"` to the tab button so the reference is valid. 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%2F38342&comment_hash=cf3e76b08fdc1b604d79fcdcc68584867037f4b56942359a30ed9ecfe5be77a4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38342&comment_hash=cf3e76b08fdc1b604d79fcdcc68584867037f4b56942359a30ed9ecfe5be77a4&reaction=dislike'>👎</a> ########## 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(); + }); Review Comment: **Suggestion:** The test suite calls `fetchMock.clearHistory()`, but `fetch-mock` exposes `resetHistory()` (and `reset()`/`restore()`), so using a non-existent method will cause a runtime TypeError and prevent the accessibility tests from running; switch to `resetHistory()` to clear call history while keeping the route mocks. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Home accessibility tests crash during Jest teardown. - ⚠️ Axe WCAG regressions may go undetected. - ⚠️ CI pipelines running this test suite may fail. ``` </details> ```suggestion afterEach(() => { fetchMock.resetHistory(); }); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the Jest test file `superset-frontend/src/pages/Home/Home.a11y.test.tsx` (e.g., via `npm test Home.a11y.test.tsx`). 2. Jest loads the test module, registering the `afterEach` hook at lines 69–71 which calls `fetchMock.clearHistory()`. 3. The first test in the `describe('Home/Welcome Page - Accessibility', ...)` block completes, and Jest invokes the `afterEach` callback. 4. At runtime, `fetchMock` (imported from `fetch-mock` at line 29) does not define a `clearHistory` function (the documented API provides `resetHistory()` instead), so `fetchMock.clearHistory()` throws `TypeError: fetchMock.clearHistory is not a function`, causing the entire accessibility test suite for the Home/Welcome page to error out rather than report Axe results. ``` </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/Home/Home.a11y.test.tsx **Line:** 69:71 **Comment:** *Logic Error: The test suite calls `fetchMock.clearHistory()`, but `fetch-mock` exposes `resetHistory()` (and `reset()`/`restore()`), so using a non-existent method will cause a runtime TypeError and prevent the accessibility tests from running; switch to `resetHistory()` to clear call history while keeping the route mocks. 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%2F38342&comment_hash=8593dff4ca585810d0dd795581d1a7e0deaf2138a31353ee500c8540278f0654&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38342&comment_hash=8593dff4ca585810d0dd795581d1a7e0deaf2138a31353ee500c8540278f0654&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]
