codeant-ai-for-open-source[bot] commented on code in PR #42315: URL: https://github.com/apache/superset/pull/42315#discussion_r3633395025
########## superset-frontend/plugins/plugin-chart-echarts/test/components/echartsLocale.test.ts: ########## @@ -0,0 +1,78 @@ +/** + * 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. + */ +import { init, use, registerLocale } from 'echarts/core'; +import { LineChart } from 'echarts/charts'; +import { SVGRenderer } from 'echarts/renderers'; +import { GridComponent } from 'echarts/components'; +import { LOCALE_LOADERS, loadLocale } from '../../src/components/echartsLocale'; + +type LocaleWithTime = { + time?: { month?: string[]; dayOfWeek?: string[] }; +}; + +test('loadLocale returns undefined for locales echarts does not ship', async () => { + expect(await loadLocale('XX')).toBeUndefined(); +}); + +test.each(Object.keys(LOCALE_LOADERS))( + 'locale %s loads a locale object with time names', + async localeKey => { + const localeObj = (await loadLocale(localeKey)) as LocaleWithTime; + // A locale object without content would clobber echarts' builtin + // locales when registered; missing `time` names crash every + // time-axis chart inside echarts' time formatter (see #42314 + // discussion — "Cannot read properties of null (reading '0')"). + expect(localeObj).toBeDefined(); + expect(localeObj.time?.month).toHaveLength(12); + expect(localeObj.time?.dayOfWeek).toHaveLength(7); + }, +); + +test('registering the loaded EN locale keeps time-axis charts rendering', async () => { + use([SVGRenderer, LineChart, GridComponent]); + const localeObj = await loadLocale('EN'); + expect(localeObj).toBeDefined(); + registerLocale('EN', localeObj!); + + const chart = init(null, null, { + renderer: 'svg', + ssr: true, + width: 400, + height: 300, + locale: 'EN', + }); + chart.setOption( + { + xAxis: { type: 'time' }, + yAxis: { type: 'value' }, + series: [ + { + type: 'line', + data: [ + [new Date(1965, 0, 1).getTime(), 100], + [new Date(1985, 0, 1).getTime(), 200], + [new Date(2005, 0, 1).getTime(), 150], + ], + }, + ], + }, + { notMerge: true, lazyUpdate: false }, + ); + expect(chart.renderToSVGString()).toContain('<svg'); Review Comment: **Suggestion:** This test creates an ECharts instance but never disposes it, which leaks chart instances across the test process and can cause memory growth or cross-test interference in longer suites. Dispose the chart in a finally block (or after assertions) to ensure cleanup always runs. [resource leak] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Test suite retains chart instances until process exit. - ⚠️ Minor memory growth in repeated or long-running tests. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the Jest test `registering the loaded EN locale keeps time-axis charts rendering` in `superset-frontend/plugins/plugin-chart-echarts/test/components/echartsLocale.test.ts` (starts at line 47). 2. Inside this test, ECharts is initialized via `init(null, null, {...})` at lines 53-59, creating a chart instance configured for SVG SSR rendering with locale `EN`. 3. The test sets chart options with a time-axis line series (lines 60-75) and calls `chart.renderToSVGString()` at line 77 to assert that SVG output is produced. 4. The chart instance created in step 2 is never disposed (no call to `chart.dispose()` after the assertion), so each execution of this test leaves an ECharts chart instance registered inside the library for the lifetime of the test process, causing unnecessary resource retention across the test suite. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=656daccddddd4fe88b5feb3856f08cdd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=656daccddddd4fe88b5feb3856f08cdd&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/plugins/plugin-chart-echarts/test/components/echartsLocale.test.ts **Line:** 53:77 **Comment:** *Resource Leak: This test creates an ECharts instance but never disposes it, which leaks chart instances across the test process and can cause memory growth or cross-test interference in longer suites. Dispose the chart in a finally block (or after assertions) to ensure cleanup always runs. 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%2F42315&comment_hash=a33ece0fd5c23927a36cab7b1ed3002031a4f78441c8a12fa983848be0f7506e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42315&comment_hash=a33ece0fd5c23927a36cab7b1ed3002031a4f78441c8a12fa983848be0f7506e&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]
