codeant-ai-for-open-source[bot] commented on code in PR #35459:
URL: https://github.com/apache/superset/pull/35459#discussion_r3598531894
##########
superset-frontend/plugins/plugin-chart-echarts/test/Pie/transformProps.test.ts:
##########
@@ -634,3 +638,198 @@ describe('legend sorting', () => {
]);
});
});
+
+describe('Half-donut', () => {
+ const getAngleChartProps = (
+ donut: boolean,
+ sweptAngle: number,
+ startAngle: number = 180,
+ ) => {
+ const formData: SqlaFormData = {
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ granularity_sqla: 'ds',
+ metric: 'sum__num',
+ groupby: ['category'],
+ viz_type: 'pie',
+ donut,
+ startAngle,
+ sweptAngle,
+ show_total: true,
+ };
+
+ return new ChartProps({
+ formData,
+ width: 800,
+ height: 600,
+ queriesData: [
+ {
+ data: [
+ { category: 'A', sum__num: 10, sum__num__contribution: 0.5 },
+ { category: 'B', sum__num: 10, sum__num__contribution: 0.5 },
+ ],
+ },
+ ],
+ theme: supersetTheme,
+ }) as EchartsPieChartProps;
+ };
+
+ test('sets center to 70% for half-donut', () => {
+ const props = getAngleChartProps(true, 180);
+ const transformed = transformProps(props);
+ const series = transformed.echartOptions.series as PieSeriesOption[];
+ expect(series[0].center).toEqual(['50%', '70%']);
+ });
+
+ test('keeps center at 50% for full donut', () => {
+ const props = getAngleChartProps(true, 360);
+ const transformed = transformProps(props);
+ const series = transformed.echartOptions.series as PieSeriesOption[];
+ expect(series[0].center).toEqual(['50%', '50%']);
+ });
+
+ test('calculates endAngle for a quarter donut', () => {
+ const props = getAngleChartProps(true, 90);
+ const transformed = transformProps(props);
+ const series = transformed.echartOptions.series as PieSeriesOption[];
+ expect(series[0].endAngle).toBe(90);
+ });
+
+ test('sets center to 30% for bottom half-donut (startAngle=0)', () => {
+ const props = getAngleChartProps(true, 180, 0);
+ const transformed = transformProps(props);
+ const series = transformed.echartOptions.series as PieSeriesOption[];
+ expect(series[0].center).toEqual(['50%', '30%']);
+ });
+
+ test('sets center to 30% for bottom half-donut (startAngle=360)', () => {
+ const props = getAngleChartProps(true, 180, 360);
+ const transformed = transformProps(props);
+ const series = transformed.echartOptions.series as PieSeriesOption[];
+ expect(series[0].center).toEqual(['50%', '30%']);
+ });
+
+ test.each([
+ [180, 180, 'top'],
+ [180, 90, 'top'],
+ [180, 45, 'top'],
+ [0, 180, 'bottom'],
+ [360, 180, 'bottom'],
+ [360, 90, 'bottom'],
+ [90, 180, 'none'],
+ [270, 180, 'none'],
+ [180, 360, 'none'],
+ [0, 360, 'none'],
+ ])('startAngle=%i, sweptAngle=%i → %s (%s)', (start, swept, expected) => {
Review Comment:
**Suggestion:** The parameterized test title contains four printf
placeholders but only three runtime values are provided, so Jest will render an
incorrect/undefined suffix in test names. Remove the extra placeholder to keep
test output accurate and debuggable. [typo]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Jest output shows confusing names for half-donut cases.
⚠️ Harder to match failing case to angle parameters.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/plugins/plugin-chart-echarts/test/Pie/transformProps.test.ts`
and locate the parameterized test at lines 93–105 (around hunk line 723)
that calls
`test.each([...])('startAngle=%i, sweptAngle=%i → %s (%s)', (start, swept,
expected) =>
{`.
2. Note that the test title template string contains four printf-style
placeholders (`%i,
%i, %s, %s`) while the callback only passes three arguments (`start`,
`swept`, `expected`)
to Jest’s `test.each` formatter.
3. Run the Jest test suite for this file; Jest will format the test names
using the
provided template and arguments, but with the extra `%s` placeholder lacking
a
corresponding runtime value, leading to an incorrect or `undefined` suffix
in the
displayed test case names.
4. Observe in Jest output that each `getHalfDonut` parameterized case has a
misleading
title where the last `%s` placeholder does not represent any real value,
making it harder
to correlate failures with specific `(startAngle, sweptAngle, expected)`
combinations.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=07d4d5d61d9f4830a6a5dadae708044c&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=07d4d5d61d9f4830a6a5dadae708044c&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/Pie/transformProps.test.ts
**Line:** 723:723
**Comment:**
*Typo: The parameterized test title contains four printf placeholders
but only three runtime values are provided, so Jest will render an
incorrect/undefined suffix in test names. Remove the extra placeholder to keep
test output accurate and debuggable.
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%2F35459&comment_hash=0e93a1b252df62b184299caf67c386e5fc775c3f942f56088b9994a1cd26e0d7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35459&comment_hash=0e93a1b252df62b184299caf67c386e5fc775c3f942f56088b9994a1cd26e0d7&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Pie/transformProps.test.ts:
##########
@@ -634,3 +638,198 @@ describe('legend sorting', () => {
]);
});
});
+
+describe('Half-donut', () => {
+ const getAngleChartProps = (
+ donut: boolean,
+ sweptAngle: number,
+ startAngle: number = 180,
+ ) => {
+ const formData: SqlaFormData = {
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ granularity_sqla: 'ds',
+ metric: 'sum__num',
+ groupby: ['category'],
+ viz_type: 'pie',
+ donut,
+ startAngle,
+ sweptAngle,
+ show_total: true,
+ };
+
+ return new ChartProps({
+ formData,
+ width: 800,
+ height: 600,
+ queriesData: [
+ {
+ data: [
+ { category: 'A', sum__num: 10, sum__num__contribution: 0.5 },
+ { category: 'B', sum__num: 10, sum__num__contribution: 0.5 },
+ ],
+ },
+ ],
+ theme: supersetTheme,
+ }) as EchartsPieChartProps;
+ };
+
+ test('sets center to 70% for half-donut', () => {
+ const props = getAngleChartProps(true, 180);
+ const transformed = transformProps(props);
+ const series = transformed.echartOptions.series as PieSeriesOption[];
+ expect(series[0].center).toEqual(['50%', '70%']);
+ });
+
+ test('keeps center at 50% for full donut', () => {
+ const props = getAngleChartProps(true, 360);
+ const transformed = transformProps(props);
+ const series = transformed.echartOptions.series as PieSeriesOption[];
+ expect(series[0].center).toEqual(['50%', '50%']);
+ });
+
+ test('calculates endAngle for a quarter donut', () => {
+ const props = getAngleChartProps(true, 90);
+ const transformed = transformProps(props);
+ const series = transformed.echartOptions.series as PieSeriesOption[];
+ expect(series[0].endAngle).toBe(90);
+ });
+
+ test('sets center to 30% for bottom half-donut (startAngle=0)', () => {
+ const props = getAngleChartProps(true, 180, 0);
+ const transformed = transformProps(props);
+ const series = transformed.echartOptions.series as PieSeriesOption[];
+ expect(series[0].center).toEqual(['50%', '30%']);
+ });
+
+ test('sets center to 30% for bottom half-donut (startAngle=360)', () => {
+ const props = getAngleChartProps(true, 180, 360);
+ const transformed = transformProps(props);
+ const series = transformed.echartOptions.series as PieSeriesOption[];
+ expect(series[0].center).toEqual(['50%', '30%']);
+ });
+
+ test.each([
+ [180, 180, 'top'],
+ [180, 90, 'top'],
+ [180, 45, 'top'],
+ [0, 180, 'bottom'],
+ [360, 180, 'bottom'],
+ [360, 90, 'bottom'],
+ [90, 180, 'none'],
+ [270, 180, 'none'],
+ [180, 360, 'none'],
+ [0, 360, 'none'],
+ ])('startAngle=%i, sweptAngle=%i → %s (%s)', (start, swept, expected) => {
+ expect(getHalfDonut(start, swept)).toBe(expected);
+ });
+
+ const baseProps = {
+ donut: true,
+ width: 800,
+ height: 600,
+ startAngle: 180,
+ sweptAngle: 360,
+ };
+
+ test('returns "middle" for donut without padding and not half', () => {
+ const result = getTotalValuePadding({
+ ...baseProps,
+ chartPadding: { top: 0, bottom: 0, left: 0, right: 0 },
+ });
+ expect(result.top).toBe('middle');
+ });
+
+ test('returns "0" for non-donut without padding and not half', () => {
+ const result = getTotalValuePadding({
+ ...baseProps,
+ donut: false,
+ chartPadding: { top: 0, bottom: 0, left: 0, right: 0 },
+ });
+ expect(result.top).toBe('0');
+ });
+
+ test('adjusts top for donut with bottom padding', () => {
+ const result = getTotalValuePadding({
+ ...baseProps,
+ chartPadding: { top: 0, bottom: 60, left: 0, right: 0 },
+ });
+ expect(result.top).toBe('45%');
+ });
+
+ test('returns "0" for non-donut with bottom padding', () => {
+ const result = getTotalValuePadding({
+ ...baseProps,
+ donut: false,
+ chartPadding: { top: 0, bottom: 60, left: 0, right: 0 },
+ });
+ expect(result.top).toBe('0');
+ });
+
+ test('adjusts top for donut with top padding', () => {
+ const result = getTotalValuePadding({
+ ...baseProps,
+ chartPadding: { top: 60, bottom: 0, left: 0, right: 0 },
+ });
+ expect(result.top).toBe('55%');
+ });
+
+ test('adjusts top for non-donut with top padding', () => {
+ const result = getTotalValuePadding({
+ ...baseProps,
+ donut: false,
+ chartPadding: { top: 60, bottom: 0, left: 0, right: 0 },
+ });
+ expect(result.top).toBe('10%');
+ });
+
+ test('uses HALF_DONUT_OFFSET (68.5) when isHalf is true and no top padding',
() => {
Review Comment:
**Suggestion:** This test description references `HALF_DONUT_OFFSET`, but
that symbol does not exist in the implementation, so the description is stale
and misleading for future maintenance. Update the wording to match the current
layout-based logic/value source. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
⚠️ Test descriptions reference nonexistent HALF_DONUT_OFFSET constant.
⚠️ Future maintainers misled about layout tuning source.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/plugins/plugin-chart-echarts/test/Pie/transformProps.test.ts`
and locate the test at lines 167–174 (around hunk line 786) with description
`uses
HALF_DONUT_OFFSET (68.5) when isHalf is true and no top padding`.
2. Note that this test calls `getTotalValuePadding` from the implementation
and asserts
`result.top` equals `'68.5%'`, implying the existence of a
`HALF_DONUT_OFFSET` constant
controlling that value.
3. Open the implementation file
`superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts`
and inspect the
geometry-related constants at lines 79–100: `HALF_DONUT_SWEEP_LIMIT` and
`HALF_DONUT_LAYOUT`, where `totalTopBase` for the `'top'` layout is `68.5`,
but there is
no exported or declared `HALF_DONUT_OFFSET` symbol.
4. Run `grep "HALF_DONUT_OFFSET" -R superset-frontend` (as reflected by the
earlier Grep
tool output) and observe that only the test descriptions reference
`HALF_DONUT_OFFSET`,
confirming the name is stale and does not correspond to any real constant in
the
production code, which can mislead maintainers about where the 68.5%
positioning
originates.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=38691f82ce4d4d0bb58aac42c4806f80&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=38691f82ce4d4d0bb58aac42c4806f80&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/Pie/transformProps.test.ts
**Line:** 786:786
**Comment:**
*Comment Mismatch: This test description references
`HALF_DONUT_OFFSET`, but that symbol does not exist in the implementation, so
the description is stale and misleading for future maintenance. Update the
wording to match the current layout-based logic/value source.
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%2F35459&comment_hash=a1d664d14c8422b7745596829544e86880bd57c4e4d1718a285186919d7fa8b8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35459&comment_hash=a1d664d14c8422b7745596829544e86880bd57c4e4d1718a285186919d7fa8b8&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]
