bito-code-review[bot] commented on code in PR #43570: URL: https://github.com/apache/superset/pull/43570#discussion_r4052433427
########## tests/unit_tests/mcp_service/chart/test_heatmap_chart.py: ########## @@ -0,0 +1,288 @@ +# 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 heatmap chart type plugin. + +Schema validation, form_data mapping (matching the frontend Heatmap +buildQuery contract for viz_type ``heatmap_v2`` — an ``x_axis`` column, a +single ``groupby`` Y column, and one ``metric``), native ``groupby`` +aliasing for the Y axis, and registry integration. +""" + +import pytest +from pydantic import TypeAdapter, ValidationError + +from superset.mcp_service.chart.chart_utils import map_heatmap_config +from superset.mcp_service.chart.schemas import ChartConfig, HeatmapChartConfig + + +class TestHeatmapChartConfigSchema: + """HeatmapChartConfig schema validation.""" + + def test_basic_heatmap_config(self) -> None: + config = HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + ) + assert config.x_axis.name == "day_of_week" + assert config.y_axis.name == "hour" + assert config.normalize_across == "heatmap" # frontend default + + def test_heatmap_missing_x_axis(self) -> None: + with pytest.raises(ValidationError): + HeatmapChartConfig( + chart_type="heatmap_v2", + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + ) + + def test_heatmap_missing_y_axis(self) -> None: + with pytest.raises(ValidationError): + HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + metric={"name": "trips", "aggregate": "COUNT"}, + ) + + def test_heatmap_missing_metric(self) -> None: + with pytest.raises(ValidationError): + HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + ) + + def test_heatmap_rejects_extra_fields(self) -> None: + with pytest.raises(ValidationError): + HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + bogus=1, + ) + + def test_heatmap_axis_rejects_aggregate(self) -> None: + """An aggregate makes an axis metric-like; x_axis/y_axis are dims.""" + with pytest.raises(ValidationError): + HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week", "aggregate": "COUNT"}, + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + ) + with pytest.raises(ValidationError): + HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour", "aggregate": "COUNT"}, + metric={"name": "trips", "aggregate": "COUNT"}, + ) + + def test_heatmap_y_axis_rejects_saved_metric(self) -> None: + with pytest.raises(ValidationError): + HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "count", "saved_metric": True}, + metric={"name": "trips", "aggregate": "COUNT"}, + ) + + def test_heatmap_invalid_normalize_across_rejected(self) -> None: + with pytest.raises(ValidationError): + HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + normalize_across="diagonal", + ) + + def test_groupby_alias_for_y_axis(self) -> None: + """Superset-native 'groupby' is accepted for the Y-axis field.""" + config = HeatmapChartConfig.model_validate( + { + "chart_type": "heatmap_v2", + "x_axis": {"name": "day_of_week"}, + "groupby": {"name": "hour"}, + "metric": {"name": "trips", "aggregate": "COUNT"}, + } + ) + assert config.y_axis.name == "hour" + + def test_chart_config_union_dispatches_heatmap(self) -> None: + config = TypeAdapter(ChartConfig).validate_python( + { + "chart_type": "heatmap_v2", + "x_axis": {"name": "day_of_week"}, + "y_axis": {"name": "hour"}, + "metric": {"name": "trips", "aggregate": "COUNT"}, + } + ) + assert isinstance(config, HeatmapChartConfig) + + +class TestMapHeatmapConfig: + """form_data mapping must match the frontend Heatmap buildQuery.""" + + def test_basic_heatmap_form_data(self) -> None: + config = HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + ) + form_data = map_heatmap_config(config) + assert form_data["viz_type"] == "heatmap_v2" + assert form_data["x_axis"] == "day_of_week" + # Y axis uses the groupby key as a single column (control is multi:false) + assert form_data["groupby"] == "hour" + assert form_data["metric"]["label"] == "COUNT(trips)" + assert form_data["normalize_across"] == "heatmap" + + def test_heatmap_form_data_with_normalize_and_filters(self) -> None: + config = HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + normalize_across="x", + filters=[{"column": "year", "op": "=", "value": 2026}], + ) + form_data = map_heatmap_config(config) + assert form_data["normalize_across"] == "x" + assert form_data["adhoc_filters"], "filters must map to adhoc_filters" + + def test_normalized_defaults_false(self) -> None: + # normalize_across has no visual effect on the frontend unless the + # 'normalized' flag is also set, so it must be threaded through. + config = HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + ) + assert map_heatmap_config(config)["normalized"] is False + + def test_normalized_true_maps_through(self) -> None: + config = HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + normalize_across="x", + normalized=True, + ) + assert map_heatmap_config(config)["normalized"] is True + + def test_heatmap_saved_metric_maps_to_name_string(self) -> None: + config = HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + metric={"name": "avg_fare", "saved_metric": True}, + ) + assert map_heatmap_config(config)["metric"] == "avg_fare" + + +class TestHeatmapQueryContext: + """The built query must GROUP BY both axes, not just the Y (groupby) column. + + map_heatmap_config emits X under 'x_axis' and Y under 'groupby'; the query + builder folds x_axis into the columns only for time-series viz types, so + heatmap_v2 needs an explicit fold or its X dimension is dropped. + """ + + def test_x_axis_reaches_group_by(self, monkeypatch) -> None: Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Untyped monkeypatch fixture</b></div> <div id="fix"> BITO rule 11810 requires fixture-injected parameters to be typed. `monkeypatch` is untyped at line 212; 24 of 26 monkeypatch parameters under tests/unit_tests/mcp_service annotate it as `pytest.MonkeyPatch` (the only other exception is pre-existing test_gauge_chart.py:319). Annotate for consistency and static type coverage. </div> </div> <small><i>Code Review Run #15921f</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_heatmap_chart.py: ########## @@ -0,0 +1,288 @@ +# 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 heatmap chart type plugin. + +Schema validation, form_data mapping (matching the frontend Heatmap +buildQuery contract for viz_type ``heatmap_v2`` — an ``x_axis`` column, a +single ``groupby`` Y column, and one ``metric``), native ``groupby`` +aliasing for the Y axis, and registry integration. +""" + +import pytest +from pydantic import TypeAdapter, ValidationError + +from superset.mcp_service.chart.chart_utils import map_heatmap_config +from superset.mcp_service.chart.schemas import ChartConfig, HeatmapChartConfig + + +class TestHeatmapChartConfigSchema: + """HeatmapChartConfig schema validation.""" + + def test_basic_heatmap_config(self) -> None: + config = HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + ) + assert config.x_axis.name == "day_of_week" + assert config.y_axis.name == "hour" + assert config.normalize_across == "heatmap" # frontend default + + def test_heatmap_missing_x_axis(self) -> None: + with pytest.raises(ValidationError): + HeatmapChartConfig( + chart_type="heatmap_v2", + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + ) + + def test_heatmap_missing_y_axis(self) -> None: + with pytest.raises(ValidationError): + HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + metric={"name": "trips", "aggregate": "COUNT"}, + ) + + def test_heatmap_missing_metric(self) -> None: + with pytest.raises(ValidationError): + HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + ) + + def test_heatmap_rejects_extra_fields(self) -> None: + with pytest.raises(ValidationError): + HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + bogus=1, + ) + + def test_heatmap_axis_rejects_aggregate(self) -> None: + """An aggregate makes an axis metric-like; x_axis/y_axis are dims.""" + with pytest.raises(ValidationError): + HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week", "aggregate": "COUNT"}, + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + ) + with pytest.raises(ValidationError): + HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour", "aggregate": "COUNT"}, + metric={"name": "trips", "aggregate": "COUNT"}, + ) + + def test_heatmap_y_axis_rejects_saved_metric(self) -> None: + with pytest.raises(ValidationError): + HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "count", "saved_metric": True}, + metric={"name": "trips", "aggregate": "COUNT"}, + ) + + def test_heatmap_invalid_normalize_across_rejected(self) -> None: + with pytest.raises(ValidationError): + HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + normalize_across="diagonal", + ) + + def test_groupby_alias_for_y_axis(self) -> None: + """Superset-native 'groupby' is accepted for the Y-axis field.""" + config = HeatmapChartConfig.model_validate( + { + "chart_type": "heatmap_v2", + "x_axis": {"name": "day_of_week"}, + "groupby": {"name": "hour"}, + "metric": {"name": "trips", "aggregate": "COUNT"}, + } + ) + assert config.y_axis.name == "hour" + + def test_chart_config_union_dispatches_heatmap(self) -> None: + config = TypeAdapter(ChartConfig).validate_python( + { + "chart_type": "heatmap_v2", + "x_axis": {"name": "day_of_week"}, + "y_axis": {"name": "hour"}, + "metric": {"name": "trips", "aggregate": "COUNT"}, + } + ) + assert isinstance(config, HeatmapChartConfig) + + +class TestMapHeatmapConfig: + """form_data mapping must match the frontend Heatmap buildQuery.""" + + def test_basic_heatmap_form_data(self) -> None: + config = HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + ) + form_data = map_heatmap_config(config) + assert form_data["viz_type"] == "heatmap_v2" + assert form_data["x_axis"] == "day_of_week" + # Y axis uses the groupby key as a single column (control is multi:false) + assert form_data["groupby"] == "hour" + assert form_data["metric"]["label"] == "COUNT(trips)" + assert form_data["normalize_across"] == "heatmap" + + def test_heatmap_form_data_with_normalize_and_filters(self) -> None: + config = HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + normalize_across="x", + filters=[{"column": "year", "op": "=", "value": 2026}], + ) + form_data = map_heatmap_config(config) + assert form_data["normalize_across"] == "x" + assert form_data["adhoc_filters"], "filters must map to adhoc_filters" + + def test_normalized_defaults_false(self) -> None: + # normalize_across has no visual effect on the frontend unless the + # 'normalized' flag is also set, so it must be threaded through. + config = HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + ) + assert map_heatmap_config(config)["normalized"] is False + + def test_normalized_true_maps_through(self) -> None: + config = HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + metric={"name": "trips", "aggregate": "COUNT"}, + normalize_across="x", + normalized=True, + ) + assert map_heatmap_config(config)["normalized"] is True + + def test_heatmap_saved_metric_maps_to_name_string(self) -> None: + config = HeatmapChartConfig( + chart_type="heatmap_v2", + x_axis={"name": "day_of_week"}, + y_axis={"name": "hour"}, + metric={"name": "avg_fare", "saved_metric": True}, + ) + assert map_heatmap_config(config)["metric"] == "avg_fare" + + +class TestHeatmapQueryContext: + """The built query must GROUP BY both axes, not just the Y (groupby) column. + + map_heatmap_config emits X under 'x_axis' and Y under 'groupby'; the query + builder folds x_axis into the columns only for time-series viz types, so + heatmap_v2 needs an explicit fold or its X dimension is dropped. + """ + + def test_x_axis_reaches_group_by(self, monkeypatch) -> None: + from superset.mcp_service.chart import chart_helpers Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Inline imports in tests</b></div> <div id="fix"> BITO rule 12745 requires module-level imports absent a documented circular dependency. Seven test methods import inline: `chart_helpers` (213), `columns_from_form_data` (237), `registry` (253, 265), `display_name_for_viz_type` (260), `_VIZ_CATEGORY` (279), `_CHART_TYPE_ADAPTERS` (284) — none with a justification comment. Hoist them to the top import block. </div> </div> <small><i>Code Review Run #15921f</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 ########## superset/mcp_service/chart/schemas.py: ########## @@ -1454,6 +1454,67 @@ def reject_metric_style_groupby(self) -> "TreemapChartConfig": return self +class HeatmapChartConfig(BaseChartConfig): + """Config for heatmap charts (viz_type ``heatmap_v2``). + + Matches the frontend Heatmap buildQuery contract: an ``x_axis`` column, a + single ``groupby`` column for the Y axis, and one ``metric`` colouring each + cell. ``normalize_across`` drives the server-side rank normalization + (whole heatmap, per-x, or per-y). + """ + + model_config = ConfigDict(extra="ignore", populate_by_name=True) + + chart_type: Literal["heatmap_v2"] = "heatmap_v2" + x_axis: ColumnRef = Field( + ..., + description="Column along the X axis", + ) + y_axis: ColumnRef = Field( + ..., + description="Column along the Y axis (form_data 'groupby'; single-select)", + validation_alias=AliasChoices("y_axis", "groupby"), + ) + metric: ColumnRef = Field( + ..., + description="Value metric colouring each cell (use aggregate e.g. SUM, " + "COUNT for ad-hoc, or set saved_metric=True for a saved dataset metric)", + ) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Metric role not enforced</b></div> <div id="fix"> Unlike `GaugeChartConfig` and `BigNumberChartConfig` (schemas.py:1328, schemas.py:2140), this validator never checks `self.metric.is_metric`, so `metric={"name": "trips"}` passes validation and `create_metric_object` silently defaults the aggregate to SUM (chart_utils.py:1004) — or surfaces a confusing DB error for non-numeric columns. Add the sibling `is_metric` check. </div> </div> <small><i>Code Review Run #15921f</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 ########## superset/mcp_service/chart/plugins/heatmap.py: ########## @@ -0,0 +1,150 @@ +# 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. + +"""Heatmap chart type plugin.""" + +from __future__ import annotations + +from collections.abc import Mapping +from typing import Any, ClassVar + +from superset.mcp_service.chart.chart_utils import ( + _heatmap_chart_what, + _summarize_filters, + map_heatmap_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, HeatmapChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class HeatmapChartPlugin(BaseChartPlugin): + """Plugin for heatmap chart type.""" + + chart_type = "heatmap_v2" + display_name = "Heatmap" + native_viz_types: ClassVar[Mapping[str, str]] = { + "heatmap_v2": "Heatmap", + } + + def pre_validate( + self, + config: dict[str, Any], + ) -> ChartGenerationError | None: + missing_fields = [] + + if "x_axis" not in config: + missing_fields.append("'x_axis' (column along the X axis)") + if "y_axis" not in config and "groupby" not in config: + missing_fields.append("'y_axis' (column along the Y axis)") + if "metric" not in config: + missing_fields.append("'metric' (value colouring each cell)") + + if missing_fields: + return ChartGenerationError( + error_type="missing_heatmap_fields", + message=( + f"Heatmap chart missing required fields: " + f"{', '.join(missing_fields)}" + ), + details=( + "Heatmaps plot a metric across two dimensions — one on the " + "x_axis and one on the y_axis — colouring each cell by the " + "metric value" + ), + suggestions=[ + "Add 'x_axis': {'name': 'day_of_week'}", + "Add 'y_axis': {'name': 'hour'}", + "Add 'metric': {'name': 'trips', 'aggregate': 'COUNT'}", + "Example: {'chart_type': 'heatmap_v2', " + "'x_axis': {'name': 'day_of_week'}, " + "'y_axis': {'name': 'hour'}, " + "'metric': {'name': 'trips', 'aggregate': 'COUNT'}}", + ], + error_code="MISSING_HEATMAP_FIELDS", + ) + + return None + + def extract_column_refs(self, config: Any) -> list[ColumnRef]: + if not isinstance(config, HeatmapChartConfig): + return [] + refs: list[ColumnRef] = [config.x_axis, config.y_axis, config.metric] + if config.filters: + for f in config.filters: + refs.append(ColumnRef(name=f.column)) + return refs + + def to_form_data( + self, config: Any, dataset_id: int | str | None = None + ) -> dict[str, Any]: + return map_heatmap_config(config) + + def generate_name(self, config: Any, dataset_name: str | None = None) -> str: + what = _heatmap_chart_what(config) + context = _summarize_filters(config.filters) + return self._with_context(what, context) + + def resolve_viz_type(self, config: Any) -> str: + return "heatmap_v2" + + def normalize_column_refs(self, config: Any, dataset_context: Any) -> Any: + config_dict = config.model_dump() + + for key in ("x_axis", "y_axis"): + col = config_dict.get(key) + if col and not col.get("sql_expression") and not col.get("saved_metric"): + col["name"] = DatasetValidator.get_canonical_column_name( + col["name"], dataset_context + ) + if config_dict.get("metric"): + if config_dict["metric"].get("sql_expression"): + pass + elif config_dict["metric"].get("saved_metric"): + config_dict["metric"]["name"] = ( + DatasetValidator.get_canonical_metric_name( + config_dict["metric"]["name"], dataset_context + ) + ) + else: + config_dict["metric"]["name"] = ( + DatasetValidator.get_canonical_column_name( + config_dict["metric"]["name"], dataset_context + ) + ) + DatasetValidator.normalize_filters(config_dict, dataset_context) + return HeatmapChartConfig.model_validate(config_dict) + + def schema_error_hint(self) -> ChartGenerationError | None: + return ChartGenerationError( + error_type="heatmap_validation_error", + message="Heatmap chart configuration validation failed", + details=( + "The heatmap chart configuration is missing required " + "fields or has invalid structure" + ), + suggestions=[ + "Ensure 'x_axis' and 'y_axis' each have a 'name'", + "Ensure 'metric' field has 'name' and 'aggregate'", + "Example: {'chart_type': 'heatmap_v2', " + "'x_axis': {'name': 'day_of_week'}, " + "'y_axis': {'name': 'hour'}, " + "'metric': {'name': 'trips', 'aggregate': 'COUNT'}}", + ], + error_code="HEATMAP_VALIDATION_ERROR", + ) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing method docstrings (BITO 12147)</b></div> <div id="fix"> None of the seven overridden methods carries an inline docstring; their contracts live only on `BaseChartPlugin`/`ChartTypePlugin`. BITO adaptive rule 12147 requires a docstring on every newly introduced function. Brief per-method docstrings (even one line noting heatmap-specific behavior, e.g. the y_axis/groupby alias handling in `pre_validate`) keep this file self-describing. </div> </div> <small><i>Code Review Run #15921f</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_heatmap_chart.py: ########## @@ -0,0 +1,288 @@ +# 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 heatmap chart type plugin. + +Schema validation, form_data mapping (matching the frontend Heatmap +buildQuery contract for viz_type ``heatmap_v2`` — an ``x_axis`` column, a +single ``groupby`` Y column, and one ``metric``), native ``groupby`` +aliasing for the Y axis, and registry integration. +""" + +import pytest +from pydantic import TypeAdapter, ValidationError + +from superset.mcp_service.chart.chart_utils import map_heatmap_config +from superset.mcp_service.chart.schemas import ChartConfig, HeatmapChartConfig + + +class TestHeatmapChartConfigSchema: + """HeatmapChartConfig schema validation.""" + + def test_basic_heatmap_config(self) -> None: Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing test docstrings</b></div> <div id="fix"> BITO adaptive rule 12148 requires docstrings on all new test functions. 20 of 22 test methods here (e.g. `test_basic_heatmap_config`, `test_heatmap_missing_x_axis`, `test_x_axis_reaches_group_by`) lack them; only `test_heatmap_axis_rejects_aggregate` and `test_groupby_alias_for_y_axis` comply. Sibling test files share the gap, but the rule is org-mandated. Add one-line docstrings stating scenario and expected outcome. </div> </div> <small><i>Code Review Run #15921f</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]
