codeant-ai-for-open-source[bot] commented on code in PR #40821: URL: https://github.com/apache/superset/pull/40821#discussion_r3366001996
########## superset-frontend/playwright/tests/dashboard/gauge-interval-colors.spec.ts: ########## @@ -0,0 +1,195 @@ +/** + * 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. + */ + +/** + * Regression for #28766: a Gauge chart configured with interval bounds and + * interval colors (mapped to a categorical color scheme) sometimes renders the + * wrong interval colors when first loaded on a dashboard — a refresh fixes it. + * + * The gauge renders to a <canvas>, so this test reads pixels back from the + * rendered gauge and asserts the configured interval colors are present in the + * correct mapping. With `color_scheme: supersetColors` and + * `interval_color_indices: '1,2'`, the gauge axis must paint the scheme's 1st + * and 2nd colors (#1FA8C9 and #454E7C) and must NOT paint the 3rd (#5AC189), + * which would indicate a shifted / fallback palette. + * + * CI green => the gauge paints the configured interval colors on first load; + * merging closes #28766 and guards against regressions. + * CI red => the interval colors are wrong on first load; the bug is live in + * plugin-chart-echarts/src/Gauge (color-scheme resolution). + */ +import { testWithAssets, expect } from '../../helpers/fixtures'; +import { apiPost, apiPut } from '../../helpers/api/requests'; +import { apiPostDashboard } from '../../helpers/api/dashboard'; +import { DashboardPage } from '../../pages/DashboardPage'; + +const DATASET_NAME = 'birth_names'; + +// supersetColors palette: index 1 = #1FA8C9, index 2 = #454E7C, index 3 = #5AC189 +const COLOR_INTERVAL_1: [number, number, number] = [31, 168, 201]; +const COLOR_INTERVAL_2: [number, number, number] = [69, 78, 124]; +const COLOR_UNUSED_3: [number, number, number] = [90, 193, 137]; + +async function findDatasetIdByName(page: any, name: string): Promise<number> { + const query = `(filters:!((col:table_name,opr:eq,value:'${name}')))`; + const resp = await page.request.get(`api/v1/dataset/?q=${query}`); + const body = await resp.json(); Review Comment: **Suggestion:** This dataset lookup bypasses the shared API helper layer and calls `page.request.get` directly, so it loses the transient network retry behavior used by other Playwright API calls. In CI, intermittent connection drops can fail this request before the test reaches its assertions. Use the existing dataset helper/API wrapper so retries and consistent request handling are applied. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Gauge dashboard test intermittently fails on transient dataset requests. - ⚠️ CI becomes flaky due to unretried dataset fetch. - ⚠️ Shared retry wrapper unused for dataset lookups in tests. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The test in `superset-frontend/playwright/tests/dashboard/gauge-interval-colors.spec.ts:25–161` calls `const datasetId = await findDatasetIdByName(page, DATASET_NAME);` at line 62 to look up the `birth_names` dataset before creating the gauge chart. 2. `findDatasetIdByName` is defined at `gauge-interval-colors.spec.ts:15–22` and issues a raw request with `const resp = await page.request.get(\`api/v1/dataset/?q=${query}\`);` and `const body = await resp.json();` (diff lines 51–52), bypassing the shared `apiGet` helper in `playwright/helpers/api/requests.ts:141–152`, which wraps `page.request.get` in `withTransientRetry` at lines 44–60. 3. Under concurrent CI load, when the Werkzeug dev server drops a connection mid-request (explicitly documented as `socket hang up` / `ECONNRESET` / `ERR_EMPTY_RESPONSE` in the comment at `requests.ts:30–40`), this direct `page.request.get` call fails immediately with a transport error because no retry logic is applied. 4. The test aborts before any chart or dashboard is created, while other API calls in the same test (chart creation via `apiPost` at `gauge-interval-colors.spec.ts:48–55` and dashboard creation via `apiPostDashboard` at lines 89–95) succeed thanks to `withTransientRetry`; refactoring the dataset lookup to use `apiGet` or a dataset API helper would route it through the same retry mechanism and eliminate this source of flakiness. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=630baae0f3264079a22c2634795eac9e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=630baae0f3264079a22c2634795eac9e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/tests/dashboard/gauge-interval-colors.spec.ts **Line:** 51:52 **Comment:** *Possible Bug: This dataset lookup bypasses the shared API helper layer and calls `page.request.get` directly, so it loses the transient network retry behavior used by other Playwright API calls. In CI, intermittent connection drops can fail this request before the test reaches its assertions. Use the existing dataset helper/API wrapper so retries and consistent request handling are 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40821&comment_hash=e8f40506b8ffb78282cd4725d19b11e88e1817e8630e8304bec7a603e7e73422&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40821&comment_hash=e8f40506b8ffb78282cd4725d19b11e88e1817e8630e8304bec7a603e7e73422&reaction=dislike'>👎</a> ########## superset-frontend/playwright/tests/dashboard/gauge-interval-colors.spec.ts: ########## @@ -0,0 +1,195 @@ +/** + * 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. + */ + +/** + * Regression for #28766: a Gauge chart configured with interval bounds and + * interval colors (mapped to a categorical color scheme) sometimes renders the + * wrong interval colors when first loaded on a dashboard — a refresh fixes it. + * + * The gauge renders to a <canvas>, so this test reads pixels back from the + * rendered gauge and asserts the configured interval colors are present in the + * correct mapping. With `color_scheme: supersetColors` and + * `interval_color_indices: '1,2'`, the gauge axis must paint the scheme's 1st + * and 2nd colors (#1FA8C9 and #454E7C) and must NOT paint the 3rd (#5AC189), + * which would indicate a shifted / fallback palette. + * + * CI green => the gauge paints the configured interval colors on first load; + * merging closes #28766 and guards against regressions. + * CI red => the interval colors are wrong on first load; the bug is live in + * plugin-chart-echarts/src/Gauge (color-scheme resolution). + */ +import { testWithAssets, expect } from '../../helpers/fixtures'; +import { apiPost, apiPut } from '../../helpers/api/requests'; +import { apiPostDashboard } from '../../helpers/api/dashboard'; +import { DashboardPage } from '../../pages/DashboardPage'; + +const DATASET_NAME = 'birth_names'; + +// supersetColors palette: index 1 = #1FA8C9, index 2 = #454E7C, index 3 = #5AC189 +const COLOR_INTERVAL_1: [number, number, number] = [31, 168, 201]; +const COLOR_INTERVAL_2: [number, number, number] = [69, 78, 124]; +const COLOR_UNUSED_3: [number, number, number] = [90, 193, 137]; + +async function findDatasetIdByName(page: any, name: string): Promise<number> { + const query = `(filters:!((col:table_name,opr:eq,value:'${name}')))`; + const resp = await page.request.get(`api/v1/dataset/?q=${query}`); + const body = await resp.json(); + if (!body.result?.length) { + throw new Error(`Dataset ${name} not found`); + } + return body.result[0].id; +} + +testWithAssets( + 'Gauge renders configured interval colors on a dashboard (#28766)', + async ({ page, testAssets }) => { + const datasetId = await findDatasetIdByName(page, DATASET_NAME); + + const chartParams = { + datasource: `${datasetId}__table`, + viz_type: 'gauge_chart', + metric: 'count', + adhoc_filters: [], + groupby: [], + row_limit: 10, + color_scheme: 'supersetColors', + min_val: 0, + max_val: 100, + start_angle: 225, + end_angle: -45, + intervals: '50,100', + interval_color_indices: '1,2', + show_pointer: true, + number_format: 'SMART_NUMBER', + value_formatter: '{value}', + }; + const chartResp = await apiPost(page, 'api/v1/chart/', { + slice_name: `gauge_interval_colors_${Date.now()}`, + viz_type: 'gauge_chart', + datasource_id: datasetId, + datasource_type: 'table', + params: JSON.stringify(chartParams), + }); + expect(chartResp.ok()).toBe(true); + const chartId: number = (await chartResp.json()).id; + testAssets.trackChart(chartId); Review Comment: **Suggestion:** The chart creation response is parsed as `body.id` only, but this API is handled elsewhere in the suite as either `{ id }` or `{ result: { id } }`. If the backend returns the nested shape, `chartId` becomes `undefined`, which then breaks chart linking and dashboard rendering with a misleading failure. Parse both shapes and fail fast when no ID is returned. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Gauge interval-colors test fails with undefined chartId. - ⚠️ Dashboard linking uses invalid or null chart identifier. - ⚠️ Orphaned test charts remain when cleanup sees undefined. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. From `superset-frontend/`, run `npx playwright test playwright/tests/dashboard/gauge-interval-colors.spec.ts` to execute the test defined at `superset-frontend/playwright/tests/dashboard/gauge-interval-colors.spec.ts:25` (`testWithAssets('Gauge renders configured interval colors on a dashboard (#28766)', ...)`). 2. Inside this test, a chart is created via `apiPost(page, 'api/v1/chart/', ...)` at lines 48–55 in `gauge-interval-colors.spec.ts`, hitting the same `api/v1/chart/` endpoint wrapped by `apiPostChart` in `playwright/helpers/api/chart.ts:52–56`. 3. When the backend returns a JSON payload where the chart ID is only present as `result.id` and not as a top-level `id` (a pattern already handled defensively in `playwright/tests/dashboard/clear-all-filters.spec.ts:41–43` and documented for datasets in `playwright/helpers/api/dataset.ts:38–41`), the line `const chartId: number = (await chartResp.json()).id;` at `gauge-interval-colors.spec.ts:56` (diff line 90) assigns `undefined` to `chartId`. 4. This `undefined` `chartId` is immediately passed to `testAssets.trackChart(chartId)` at `gauge-interval-colors.spec.ts:57` (diff line 91) and later used in `apiPut(page, \`api/v1/chart/${chartId}\`, { dashboards: [dashboardId] })` at line 100, as well as in the layout meta `meta: { chartId }` at lines 81–83, causing subsequent dashboard linking and navigation (`DashboardPage.gotoById(dashboardId)` at lines 102–103) to operate on an invalid chart reference and fail in a non-obvious way. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=56fe5e918ac44cd59301fa6049c79fa3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=56fe5e918ac44cd59301fa6049c79fa3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/tests/dashboard/gauge-interval-colors.spec.ts **Line:** 90:91 **Comment:** *Api Mismatch: The chart creation response is parsed as `body.id` only, but this API is handled elsewhere in the suite as either `{ id }` or `{ result: { id } }`. If the backend returns the nested shape, `chartId` becomes `undefined`, which then breaks chart linking and dashboard rendering with a misleading failure. Parse both shapes and fail fast when no ID is returned. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40821&comment_hash=5599ff0be6d31a44ce81bab5b3f4a4260929d2dc4b20a79ca7971506b9cbe902&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40821&comment_hash=5599ff0be6d31a44ce81bab5b3f4a4260929d2dc4b20a79ca7971506b9cbe902&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]
