codeant-ai-for-open-source[bot] commented on code in PR #37522:
URL: https://github.com/apache/superset/pull/37522#discussion_r2746174876
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -51,6 +88,36 @@ function getYAxisFormatter(
return yAxis.axisLabel!.formatter!;
}
+/**
+ * Creates a properly typed EchartsTimeseriesChartProps for testing.
+ * Uses shared createEchartsTimeseriesTestChartProps with Timeseries defaults.
+ */
+function createTestChartProps(config: {
+ formData?: Partial<EchartsTimeseriesFormData>;
+ queriesData?: ChartDataResponseResult[];
+ datasource?: {
+ verboseMap?: Record<string, string>;
+ columnFormats?: Record<string, string>;
+ currencyFormats?: Record<
+ string,
+ { symbol: string; symbolPosition: string }
+ >;
+ currencyCodeColumn?: string;
+ };
+ width?: number;
+ height?: number;
+}): EchartsTimeseriesChartProps {
+ return createEchartsTimeseriesTestChartProps<
+ EchartsTimeseriesFormData,
+ EchartsTimeseriesChartProps
+ >({
+ defaultFormData: DEFAULT_FORM_DATA,
+ defaultVizType: 'my_viz',
+ defaultQueriesData: queriesData as unknown as ChartDataResponseResult[],
Review Comment:
**Suggestion:** Unsafe and unnecessary double-cast: `queriesData as unknown
as ChartDataResponseResult[]` forces an incorrect type and hides real type
issues; replace the cast by reading `config.queriesData` and providing a proper
default (no forced casts). [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ createTestChartProps test helper type-safety masked.
- ⚠️ createEchartsTimeseriesTestChartProps defaultQueriesData mismatch risk.
- ⚠️ Tests may accept malformed queriesData silently.
```
</details>
```suggestion
defaultQueriesData: config.queriesData ?? [],
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the test helper factory in
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts
where createTestChartProps is defined (function start at ~95, added block at
lines
110-118). Note the current line 116 sets:
`defaultQueriesData: queriesData as unknown as
ChartDataResponseResult[],` which forces
a double-cast.
2. Observe createEchartsTimeseriesTestChartProps implementation in
test/helpers.ts (file
path: superset-frontend/plugins/plugin-chart-echarts/test/helpers.ts,
function at lines
70-106). That function expects defaultQueriesData: ChartDataResponseResult[].
3. Reproduce the typing/masking issue: change the module-scoped `const
queriesData =
[...]` (defined below createTestChartProps in transformProps.test.ts, around
lines
~121-130) to an intentionally incompatible shape (for example, an array of
numbers). Run
TypeScript compilation (npm run typecheck or tsc). Because the code uses `as
unknown as
ChartDataResponseResult[]` on line 116, the compiler will not flag the
mismatch — the
unsafe cast silences the type error. This demonstrates the double-cast hides
real type
issues.
4. Also reproduce a behavioral surprise: call createTestChartProps({}) from
a test (e.g.,
the test at transformProps.test.ts where `const chartProps =
createTestChartProps({});` is
used) and observe that when callers expect an empty default, the helper
instead provides
the module-level `queriesData` as the default. Using `config.queriesData ??
[]` ensures
the caller-provided value (or an empty default) is used instead of relying
on the module
variable. This is the intended, safer behavior.
```
</details>
<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/Timeseries/transformProps.test.ts
**Line:** 116:116
**Comment:**
*Type Error: Unsafe and unnecessary double-cast: `queriesData as
unknown as ChartDataResponseResult[]` forces an incorrect type and hides real
type issues; replace the cast by reading `config.queriesData` and providing a
proper default (no forced casts).
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>
##########
superset-frontend/plugins/plugin-chart-echarts/test/helpers.ts:
##########
@@ -0,0 +1,106 @@
+/**
+ * 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 { ChartProps, ChartDataResponseResult } from '@superset-ui/core';
+import { supersetTheme } from '@apache-superset/core/ui';
+
+/**
+ * Datasource shape used by Echarts Timeseries and Mixed Timeseries chart
props.
+ */
+export interface EchartsTimeseriesTestDatasource {
+ verboseMap?: Record<string, string>;
+ columnFormats?: Record<string, string>;
+ currencyFormats?: Record<string, { symbol: string; symbolPosition: string }>;
+ currencyCodeColumn?: string;
+}
+
+const DEFAULT_DATASOURCE: EchartsTimeseriesTestDatasource = {
+ verboseMap: {},
+ columnFormats: {},
+ currencyFormats: {},
+};
Review Comment:
**Suggestion:** Module-level `DEFAULT_DATASOURCE` is a mutable object shared
across tests; returning it directly allows callers to mutate a shared instance
which can cause cross-test interference. Freeze or otherwise ensure callers
receive a copy to avoid shared mutable state. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Timeseries transformProps tests may become flaky.
- ⚠️ Mixed Timeseries plugin tests may see unexpected state.
- ⚠️ Test debugging time increases for chart-echarts tests.
- ⚠️ Suggestion addresses test infrastructure, not runtime code.
```
</details>
```suggestion
const DEFAULT_DATASOURCE: EchartsTimeseriesTestDatasource = Object.freeze({
verboseMap: {},
columnFormats: {},
currencyFormats: {},
}) as EchartsTimeseriesTestDatasource;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the helper at
superset-frontend/plugins/plugin-chart-echarts/test/helpers.ts. The
helper's purpose is documented at lines 48-50 as "Shared by Timeseries and
Mixed
Timeseries transformProps tests". The factory function
createEchartsTimeseriesTestChartProps starts at line 70.
2. In a Timeseries transformProps test (those tests import this shared
helper per file
comment at lines 48-50), call createEchartsTimeseriesTestChartProps(...)
without providing
a datasource argument. Inside that function (lines 85-90) fullFormData is
built, and at
lines 92-99 a ChartProps instance is created with datasource:
customDatasource ??
DEFAULT_DATASOURCE (see line 98).
3. Because customDatasource is undefined, the ChartProps and the returned
object reference
the module-level DEFAULT_DATASOURCE object defined at lines 32-36. If a test
mutates that
object (for example, mutating chartProps.datasource.verboseMap = { 'Time
Label': 'x' }),
the module-level DEFAULT_DATASOURCE is changed for the entire process.
4. A subsequent test that also calls createEchartsTimeseriesTestChartProps()
(again
relying on the default) will receive the mutated DEFAULT_DATASOURCE, causing
unexpected
test behavior or flakiness. This reproduces cross-test state leakage
originating from the
DEFAULT_DATASOURCE defined at lines 32-36.
```
</details>
<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/helpers.ts
**Line:** 32:36
**Comment:**
*Possible Bug: Module-level `DEFAULT_DATASOURCE` is a mutable object
shared across tests; returning it directly allows callers to mutate a shared
instance which can cause cross-test interference. Freeze or otherwise ensure
callers receive a copy to avoid shared mutable state.
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>
--
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]
