bito-code-review[bot] commented on code in PR #38375: URL: https://github.com/apache/superset/pull/38375#discussion_r2892464209
########## tests/unit_tests/mcp_service/chart/test_new_chart_types.py: ########## @@ -0,0 +1,929 @@ +# 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. + +""" +Unit tests for new MCP chart types: pie, pivot_table, mixed_timeseries. + +Tests cover schema validation, form_data mapping, chart name generation, +and schema validator pre-validation for all three new chart types. +""" + +from unittest.mock import patch + +import pytest +from pydantic import ValidationError + +from superset.mcp_service.chart.chart_utils import ( + generate_chart_name, + map_config_to_form_data, + map_mixed_timeseries_config, + map_pie_config, + map_pivot_table_config, +) +from superset.mcp_service.chart.schemas import ( + AxisConfig, + ColumnRef, + FilterConfig, + MixedTimeseriesChartConfig, + PieChartConfig, + PivotTableChartConfig, +) +from superset.mcp_service.chart.validation.schema_validator import SchemaValidator + +# ============================================================ +# Pie Chart Schema Tests +# ============================================================ + + +class TestPieChartConfigSchema: + """Test PieChartConfig Pydantic schema validation.""" + + def test_basic_pie_config(self) -> None: + config = PieChartConfig( + chart_type="pie", + dimension=ColumnRef(name="product"), + metric=ColumnRef(name="revenue", aggregate="SUM"), + ) + assert config.chart_type == "pie" + assert config.dimension.name == "product" + assert config.metric.aggregate == "SUM" + assert config.donut is False + assert config.show_labels is True + assert config.label_type == "key_value_percent" + + def test_donut_chart_config(self) -> None: + config = PieChartConfig( + chart_type="pie", + dimension=ColumnRef(name="category"), + metric=ColumnRef(name="count", aggregate="COUNT"), + donut=True, + inner_radius=40, + outer_radius=80, + ) + assert config.donut is True + assert config.inner_radius == 40 + assert config.outer_radius == 80 + + def test_pie_config_with_all_options(self) -> None: + config = PieChartConfig( + chart_type="pie", + dimension=ColumnRef(name="region"), + metric=ColumnRef(name="sales", aggregate="SUM", label="Total Sales"), + donut=True, + show_labels=False, + label_type="percent", + sort_by_metric=False, + show_legend=False, + row_limit=50, + number_format="$,.2f", + show_total=True, + labels_outside=False, + outer_radius=90, + inner_radius=50, + filters=[FilterConfig(column="status", op="=", value="active")], + ) + assert config.show_labels is False + assert config.label_type == "percent" + assert config.row_limit == 50 + assert config.show_total is True + assert config.filters is not None + assert len(config.filters) == 1 + + def test_pie_config_rejects_extra_fields(self) -> None: + with pytest.raises(ValidationError): + PieChartConfig( + chart_type="pie", + dimension=ColumnRef(name="product"), + metric=ColumnRef(name="revenue", aggregate="SUM"), + unknown_field="bad", + ) + + def test_pie_config_missing_dimension(self) -> None: + with pytest.raises(ValidationError): + PieChartConfig( + chart_type="pie", + metric=ColumnRef(name="revenue", aggregate="SUM"), + ) + + def test_pie_config_missing_metric(self) -> None: + with pytest.raises(ValidationError): + PieChartConfig( + chart_type="pie", + dimension=ColumnRef(name="product"), + ) + + def test_pie_config_row_limit_bounds(self) -> None: + with pytest.raises(ValidationError): + PieChartConfig( + chart_type="pie", + dimension=ColumnRef(name="product"), + metric=ColumnRef(name="revenue", aggregate="SUM"), + row_limit=0, + ) + + def test_pie_config_valid_label_types(self) -> None: + for label_type in [ + "key", + "value", + "percent", + "key_value", + "key_percent", + "key_value_percent", + "value_percent", + ]: + config = PieChartConfig( + chart_type="pie", + dimension=ColumnRef(name="product"), + metric=ColumnRef(name="revenue", aggregate="SUM"), + label_type=label_type, + ) + assert config.label_type == label_type + + +# ============================================================ +# Pie Chart Form Data Mapping Tests +# ============================================================ + + +class TestMapPieConfig: + """Test map_pie_config form_data generation.""" + + def test_basic_pie_form_data(self) -> None: + config = PieChartConfig( + chart_type="pie", + dimension=ColumnRef(name="product"), + metric=ColumnRef(name="revenue", aggregate="SUM"), + ) + result = map_pie_config(config) + + assert result["viz_type"] == "pie" + assert result["groupby"] == ["product"] + assert result["metric"]["aggregate"] == "SUM" + assert result["metric"]["column"]["column_name"] == "revenue" + assert result["show_labels"] is True + assert result["show_legend"] is True + assert result["label_type"] == "key_value_percent" + assert result["sort_by_metric"] is True + assert result["row_limit"] == 100 + assert result["donut"] is False + assert result["color_scheme"] == "supersetColors" + + def test_donut_form_data(self) -> None: + config = PieChartConfig( + chart_type="pie", + dimension=ColumnRef(name="category"), + metric=ColumnRef(name="count", aggregate="COUNT"), + donut=True, + inner_radius=40, + outer_radius=80, + ) + result = map_pie_config(config) + + assert result["donut"] is True + assert result["innerRadius"] == 40 + assert result["outerRadius"] == 80 + + def test_pie_form_data_with_filters(self) -> None: + config = PieChartConfig( + chart_type="pie", + dimension=ColumnRef(name="product"), + metric=ColumnRef(name="revenue", aggregate="SUM"), + filters=[FilterConfig(column="region", op="=", value="US")], + ) + result = map_pie_config(config) + + assert "adhoc_filters" in result + assert len(result["adhoc_filters"]) == 1 + assert result["adhoc_filters"][0]["subject"] == "region" + assert result["adhoc_filters"][0]["operator"] == "==" + assert result["adhoc_filters"][0]["comparator"] == "US" + + def test_pie_form_data_custom_options(self) -> None: + config = PieChartConfig( + chart_type="pie", + dimension=ColumnRef(name="status"), + metric=ColumnRef(name="count", aggregate="COUNT"), + show_labels=False, + label_type="percent", + show_legend=False, + number_format="$,.2f", + show_total=True, + labels_outside=False, + ) + result = map_pie_config(config) + + assert result["show_labels"] is False + assert result["label_type"] == "percent" + assert result["show_legend"] is False + assert result["number_format"] == "$,.2f" + assert result["show_total"] is True + assert result["labels_outside"] is False + + def test_pie_form_data_custom_metric_label(self) -> None: + config = PieChartConfig( + chart_type="pie", + dimension=ColumnRef(name="product"), + metric=ColumnRef(name="revenue", aggregate="SUM", label="Total Revenue"), + ) + result = map_pie_config(config) + + assert result["metric"]["label"] == "Total Revenue" + assert result["metric"]["hasCustomLabel"] is True + + +# ============================================================ +# Pivot Table Schema Tests +# ============================================================ + + +class TestPivotTableChartConfigSchema: + """Test PivotTableChartConfig Pydantic schema validation.""" + + def test_basic_pivot_table_config(self) -> None: + config = PivotTableChartConfig( + chart_type="pivot_table", + rows=[ColumnRef(name="product")], + metrics=[ColumnRef(name="revenue", aggregate="SUM")], + ) + assert config.chart_type == "pivot_table" + assert len(config.rows) == 1 + assert len(config.metrics) == 1 + assert config.aggregate_function == "Sum" + assert config.show_row_totals is True + assert config.show_column_totals is True + + def test_pivot_table_with_columns(self) -> None: + config = PivotTableChartConfig( + chart_type="pivot_table", + rows=[ColumnRef(name="product")], + columns=[ColumnRef(name="region")], + metrics=[ColumnRef(name="revenue", aggregate="SUM")], + ) + assert config.columns is not None + assert len(config.columns) == 1 + assert config.columns[0].name == "region" + + def test_pivot_table_with_all_options(self) -> None: + config = PivotTableChartConfig( + chart_type="pivot_table", + rows=[ColumnRef(name="product"), ColumnRef(name="category")], + columns=[ColumnRef(name="region")], + metrics=[ + ColumnRef(name="revenue", aggregate="SUM"), + ColumnRef(name="orders", aggregate="COUNT"), + ], + aggregate_function="Average", + show_row_totals=False, + show_column_totals=False, + transpose=True, + combine_metric=True, + row_limit=5000, + value_format="$,.2f", + filters=[FilterConfig(column="year", op="=", value=2024)], + ) + assert config.aggregate_function == "Average" + assert config.show_row_totals is False + assert config.transpose is True + assert config.combine_metric is True + assert config.row_limit == 5000 + + def test_pivot_table_missing_rows(self) -> None: + with pytest.raises(ValidationError): + PivotTableChartConfig( + chart_type="pivot_table", + metrics=[ColumnRef(name="revenue", aggregate="SUM")], + ) + + def test_pivot_table_missing_metrics(self) -> None: + with pytest.raises(ValidationError): + PivotTableChartConfig( + chart_type="pivot_table", + rows=[ColumnRef(name="product")], + ) + + def test_pivot_table_empty_rows(self) -> None: + with pytest.raises(ValidationError): + PivotTableChartConfig( + chart_type="pivot_table", + rows=[], + metrics=[ColumnRef(name="revenue", aggregate="SUM")], + ) + + def test_pivot_table_rejects_extra_fields(self) -> None: + with pytest.raises(ValidationError): + PivotTableChartConfig( + chart_type="pivot_table", + rows=[ColumnRef(name="product")], + metrics=[ColumnRef(name="revenue", aggregate="SUM")], + unknown_field="bad", + ) + + def test_pivot_table_valid_aggregate_functions(self) -> None: + for agg in ["Sum", "Average", "Median", "Count", "Minimum", "Maximum"]: + config = PivotTableChartConfig( + chart_type="pivot_table", + rows=[ColumnRef(name="product")], + metrics=[ColumnRef(name="revenue", aggregate="SUM")], + aggregate_function=agg, + ) + assert config.aggregate_function == agg + + +# ============================================================ +# Pivot Table Form Data Mapping Tests +# ============================================================ + + +class TestMapPivotTableConfig: + """Test map_pivot_table_config form_data generation.""" + + def test_basic_pivot_form_data(self) -> None: + config = PivotTableChartConfig( + chart_type="pivot_table", + rows=[ColumnRef(name="product")], + metrics=[ColumnRef(name="revenue", aggregate="SUM")], + ) + result = map_pivot_table_config(config) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete Test Coverage</b></div> <div id="fix"> The test file's docstring indicates coverage for mixed_timeseries charts, chart name generation, and schema validator pre-validation, but these are not implemented in the tests. This could allow bugs in untested code paths to go undetected. </div> </div> <small><i>Code Review Run #bc324c</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]
