bito-code-review[bot] commented on code in PR #37639: URL: https://github.com/apache/superset/pull/37639#discussion_r2759840096
########## tests/unit_tests/mcp_service/chart/test_preview_utils.py: ########## @@ -0,0 +1,146 @@ +# 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 preview_utils query context column building. +""" + + +class TestPreviewUtilsColumnBuilding: + """Tests for x_axis + groupby column building in generate_preview_from_form_data. + + The function must build the columns list from both x_axis and groupby for + XY charts, and fall back to form_data["columns"] for table charts. + """ + + def test_xy_chart_uses_x_axis_and_groupby(self): + """Test XY chart form_data builds columns from x_axis + groupby.""" + form_data = { + "x_axis": "territory", + "groupby": ["year"], + "metrics": [{"label": "SUM(sales)"}], + } + + x_axis_config = form_data.get("x_axis") + groupby_columns = form_data.get("groupby", []) + raw_columns = form_data.get("columns", []) + + columns = raw_columns.copy() if raw_columns else groupby_columns.copy() + if x_axis_config and isinstance(x_axis_config, str): + if x_axis_config not in columns: + columns.insert(0, x_axis_config) + elif x_axis_config and isinstance(x_axis_config, dict): + col_name = x_axis_config.get("column_name") + if col_name and col_name not in columns: + columns.insert(0, col_name) + + assert columns == ["territory", "year"] + + def test_table_chart_uses_columns_field(self): + """Test table chart form_data uses 'columns' field directly.""" + form_data = { + "columns": ["name", "region", "sales"], + "metrics": [], + } + + x_axis_config = form_data.get("x_axis") + groupby_columns = form_data.get("groupby", []) + raw_columns = form_data.get("columns", []) + + columns = raw_columns.copy() if raw_columns else groupby_columns.copy() + if x_axis_config and isinstance(x_axis_config, str): + if x_axis_config not in columns: + columns.insert(0, x_axis_config) + + assert columns == ["name", "region", "sales"] + + def test_xy_chart_x_axis_dict_format(self): + """Test XY chart with x_axis as dict (column_name key).""" + form_data = { + "x_axis": {"column_name": "order_date"}, + "groupby": ["product_type"], + "metrics": [{"label": "SUM(revenue)"}], + } + + x_axis_config = form_data.get("x_axis") + groupby_columns = form_data.get("groupby", []) + raw_columns = form_data.get("columns", []) + + columns = raw_columns.copy() if raw_columns else groupby_columns.copy() + if x_axis_config and isinstance(x_axis_config, str): + if x_axis_config not in columns: + columns.insert(0, x_axis_config) + elif x_axis_config and isinstance(x_axis_config, dict): + col_name = x_axis_config.get("column_name") + if col_name and col_name not in columns: + columns.insert(0, col_name) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incorrect Column Logic in Tests</b></div> <div id="fix"> The column building logic in the test methods incorrectly treats empty 'columns' lists the same as missing 'columns', which can lead to wrong column selection for table charts with empty columns. The logic should distinguish table charts (use 'columns' if present) from XY charts (use groupby + x_axis). This matches the function's docstring and prevents bugs like using groupby for table charts with empty columns. The implementation in preview_utils.py has the same issue and should be fixed similarly. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion if "columns" in form_data: columns = raw_columns.copy() else: columns = groupby_columns.copy() if x_axis_config and isinstance(x_axis_config, str): if x_axis_config not in columns: columns.insert(0, x_axis_config) elif x_axis_config and isinstance(x_axis_config, dict): col_name = x_axis_config.get("column_name") if col_name and col_name not in columns: columns.insert(0, col_name) ```` </div> </details> </div> <small><i>Code Review Run #bc15b0</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]
