bito-code-review[bot] commented on code in PR #43571:
URL: https://github.com/apache/superset/pull/43571#discussion_r4163554247
##########
tests/unit_tests/mcp_service/chart/test_chart_utils.py:
##########
@@ -160,6 +160,60 @@ def test_merge_bubble_preserves_omitted_defaults(
assert {key: merged[key] for key in expected} == expected
[email protected](
+ "updates,expected",
+ [
+ ({}, {"color_scheme": "lyftColors", "row_limit": 42}),
+ (
+ {"color_scheme": "googleCategory10c", "row_limit": 200},
+ {"color_scheme": "googleCategory10c", "row_limit": 200},
+ ),
+ ],
+)
+def test_merge_radar_preserves_omitted_defaults(
+ updates: dict[str, Any], expected: dict[str, Any]
+) -> None:
+ """Normalizing a radar config must not mark unsent fields as set.
+
+ ``merge_chart_form_data`` keeps an omitted color scheme or row limit only
+ when the field is absent from ``model_fields_set``. A plugin that dumps
+ without ``exclude_unset`` hands back a config where every field is set, so
+ an update that mentions neither still resets both.
+ """
+ from superset.mcp_service.chart.chart_utils import map_radar_config
+ from superset.mcp_service.chart.schemas import RadarChartConfig
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inline imports violate rule 12745</b></div>
<div id="fix">
Inline imports of `map_radar_config` and `RadarChartConfig` inside the test
body have no circular-dependency justification: this module already imports
`merge_chart_form_data` from `chart_utils` and `ColumnRef` from `schemas` at
top level, so the same modules are safely importable at module scope. Repo rule
[12745] requires module-level imports unless a documented cycle exists. Please
hoist these two imports into the existing top-level import blocks. ([BITO.md
rule 12745](BITO.md))
</div>
</div>
<small><i>Code Review Run #722d04</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/test_radar_chart.py:
##########
@@ -0,0 +1,253 @@
+# 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.
+
+"""Tests for the radar chart type plugin.
+
+Schema validation, form_data mapping (matching the frontend Radar buildQuery
+contract for viz_type ``radar`` — multiple ``metrics`` forming the axes plus
+an optional ``groupby`` for the series polygons), and registry integration.
+"""
+
+import pytest
+from pydantic import TypeAdapter, ValidationError
+
+from superset.mcp_service.chart.chart_utils import map_radar_config
+from superset.mcp_service.chart.schemas import ChartConfig, RadarChartConfig
+
+
+class TestRadarChartConfigSchema:
+ """RadarChartConfig schema validation."""
+
+ def test_basic_radar_config(self) -> None:
+ config = RadarChartConfig(
+ chart_type="radar",
+ metrics=[
+ {"name": "speed", "aggregate": "AVG"},
+ {"name": "power", "aggregate": "AVG"},
+ {"name": "range", "aggregate": "AVG"},
+ ],
+ )
+ assert len(config.metrics) == 3
+ assert config.groupby is None # series grouping is optional
+ assert config.row_limit == 10 # frontend controlPanel default
+
+ def test_radar_missing_metrics(self) -> None:
+ with pytest.raises(ValidationError):
+ RadarChartConfig(chart_type="radar", groupby=[{"name": "model"}])
+
+ def test_radar_empty_metrics_rejected(self) -> None:
+ with pytest.raises(ValidationError):
+ RadarChartConfig(chart_type="radar", metrics=[])
+
+ def test_radar_rejects_extra_fields(self) -> None:
+ with pytest.raises(ValidationError):
+ RadarChartConfig(
+ chart_type="radar",
+ metrics=[{"name": "speed", "aggregate": "AVG"}],
+ bogus=1,
+ )
+
+ def test_radar_groupby_rejects_aggregate(self) -> None:
+ """An aggregate makes a series column metric-like; reject it."""
+ with pytest.raises(ValidationError):
+ RadarChartConfig(
+ chart_type="radar",
+ metrics=[{"name": "speed", "aggregate": "AVG"}],
+ groupby=[{"name": "model", "aggregate": "SUM"}],
+ )
+
+ def test_radar_bare_axis_records_sum(self) -> None:
+ """Each axis is a metric slot, so a bare column is summed.
+
+ create_metric_object applies that default when it builds form_data.
+ Recording it here keeps the aggregate-compatibility check from
+ skipping the ref, so SUM of a text column is rejected up front
+ instead of rendering a polygon of zeros.
+ """
+ config = RadarChartConfig(
+ chart_type="radar",
+ metrics=[{"name": "speed"}, {"name": "power", "aggregate": "AVG"}],
+ )
+
+ assert config.metrics[0].aggregate == "SUM"
+ assert config.metrics[0].is_metric
+ assert config.metrics[1].aggregate == "AVG" # explicit one untouched
+
+ def test_radar_bare_axis_is_aggregate_checked(self) -> None:
+ """The recorded SUM must reach the dataset aggregate check."""
+ from superset.mcp_service.chart.plugins.radar import RadarChartPlugin
+ from superset.mcp_service.chart.validation.dataset_validator import (
+ DatasetValidator,
+ )
+ from superset.mcp_service.common.error_schemas import DatasetContext
+
+ context = DatasetContext(
+ id=1,
+ table_name="cars",
+ schema=None,
+ database_name="database",
+ available_columns=[
+ {"name": "model", "type": "VARCHAR", "is_numeric": False},
+ {"name": "speed", "type": "BIGINT", "is_numeric": True},
+ ],
+ available_metrics=[],
+ )
+ config = RadarChartConfig(chart_type="radar", metrics=[{"name":
"model"}])
+
+ errors = DatasetValidator._validate_aggregations(
+ RadarChartPlugin().extract_column_refs(config), context
+ )
+
+ assert errors, "SUM(model) on a VARCHAR column must be rejected"
+ assert errors[0].error_type == "invalid_aggregation"
+
+ def test_radar_groupby_rejects_saved_metric(self) -> None:
+ with pytest.raises(ValidationError):
+ RadarChartConfig(
+ chart_type="radar",
+ metrics=[{"name": "speed", "aggregate": "AVG"}],
+ groupby=[{"name": "count", "saved_metric": True}],
+ )
+
+ def test_radar_metric_accepts_saved_metric(self) -> None:
+ """A saved metric is a valid radar axis."""
+ config = RadarChartConfig(
+ chart_type="radar",
+ metrics=[{"name": "efficiency_score", "saved_metric": True}],
+ )
+ assert config.metrics[0].saved_metric is True
+
+ def test_chart_config_union_dispatches_radar(self) -> None:
+ config = TypeAdapter(ChartConfig).validate_python(
+ {
+ "chart_type": "radar",
+ "metrics": [{"name": "speed", "aggregate": "AVG"}],
+ }
+ )
+ assert isinstance(config, RadarChartConfig)
+
+
+class TestMapRadarConfig:
+ """form_data mapping must match the frontend Radar buildQuery."""
+
+ def test_basic_radar_form_data(self) -> None:
+ config = RadarChartConfig(
+ chart_type="radar",
+ metrics=[
+ {"name": "speed", "aggregate": "AVG"},
+ {"name": "power", "aggregate": "AVG"},
+ ],
+ )
+ form_data = map_radar_config(config)
+ assert form_data["viz_type"] == "radar"
+ assert [m["label"] for m in form_data["metrics"]] == [
+ "AVG(speed)",
+ "AVG(power)",
+ ]
+ assert form_data["groupby"] == []
+ assert form_data["row_limit"] == 10
+
+ def test_radar_form_data_with_series_and_filters(self) -> None:
+ config = RadarChartConfig(
+ chart_type="radar",
+ metrics=[{"name": "speed", "aggregate": "AVG"}],
+ groupby=[{"name": "model"}],
+ filters=[{"column": "year", "op": "=", "value": 2026}],
+ )
+ form_data = map_radar_config(config)
+ assert form_data["groupby"] == ["model"]
+ assert form_data["adhoc_filters"], "filters must map to adhoc_filters"
+
+ def test_radar_saved_metric_maps_to_name_string(self) -> None:
+ config = RadarChartConfig(
+ chart_type="radar",
+ metrics=[{"name": "efficiency_score", "saved_metric": True}],
+ )
+ assert map_radar_config(config)["metrics"] == ["efficiency_score"]
+
+
+class TestRadarQueryContext:
+ """The built query must ORDER BY the first metric descending.
+
+ Radar's frontend buildQuery always orders by the first metric; the MCP
+ mapper emits sort_by_metric so the shared builder does the same, keeping
+ the top-N polygons deterministic under a row_limit.
+ """
+
+ def test_orders_by_first_metric_descending(self, monkeypatch) -> None:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Untyped monkeypatch fixture param</b></div>
<div id="fix">
BITO.md rule 11810 requires annotating fixture-injected parameters. Annotate
as `monkeypatch: pytest.MonkeyPatch`; 32 of 43 sibling monkeypatch parameters
in tests/unit_tests/mcp_service/chart/ are already annotated, so this matches
house style and lets static typing cover the stubbed
`resolve_datasource_engine` call.
</div>
</div>
<small><i>Code Review Run #722d04</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/test_chart_utils.py:
##########
@@ -160,6 +160,60 @@ def test_merge_bubble_preserves_omitted_defaults(
assert {key: merged[key] for key in expected} == expected
[email protected](
+ "updates,expected",
+ [
+ ({}, {"color_scheme": "lyftColors", "row_limit": 42}),
+ (
+ {"color_scheme": "googleCategory10c", "row_limit": 200},
+ {"color_scheme": "googleCategory10c", "row_limit": 200},
+ ),
+ ],
+)
+def test_merge_radar_preserves_omitted_defaults(
+ updates: dict[str, Any], expected: dict[str, Any]
+) -> None:
+ """Normalizing a radar config must not mark unsent fields as set.
+
+ ``merge_chart_form_data`` keeps an omitted color scheme or row limit only
+ when the field is absent from ``model_fields_set``. A plugin that dumps
+ without ``exclude_unset`` hands back a config where every field is set, so
+ an update that mentions neither still resets both.
+ """
+ from superset.mcp_service.chart.chart_utils import map_radar_config
+ from superset.mcp_service.chart.schemas import RadarChartConfig
+
+ config = RadarChartConfig(
+ chart_type="radar",
+ metrics=[ColumnRef(name="revenue", aggregate="SUM")],
+ groupby=[ColumnRef(name="product")],
+ **updates,
+ )
+ config = DatasetValidator.normalize_column_names(
+ config,
+ dataset_id=1,
+ dataset_context=DatasetContext(
+ id=1,
+ table_name="sales",
+ database_name="db",
+ available_columns=[{"name": "Product"}, {"name": "Revenue"}],
+ available_metrics=[],
+ ),
+ )
+ assert config.groupby is not None
+ assert config.groupby[0].name == "Product"
+ new_form_data = map_radar_config(config)
+ existing = {
+ "viz_type": new_form_data["viz_type"],
+ "color_scheme": "lyftColors",
+ "row_limit": 42,
+ }
+
+ merged = merge_chart_form_data(existing, new_form_data, config)
+
+ assert {key: merged[key] for key in expected} == expected
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Under-asserted merge result</b></div>
<div id="fix">
The final assertion inspects only the `color_scheme`/`row_limit` keys named
in `expected`, so a regression in
`merge_chart_form_data`/`_merge_shared_form_data` affecting any other key (e.g.
dropping `groupby` or corrupting `metrics`) would pass silently. The sibling
`test_merge_chart_preserves_omitted_defaults` also asserts `merged["metric"] ==
new_form_data["metric"]`; mirror that here for the radar payload.
</div>
</div>
<small><i>Code Review Run #722d04</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
