bito-code-review[bot] commented on code in PR #44082: URL: https://github.com/apache/superset/pull/44082#discussion_r4046533712
########## tests/unit_tests/dashboards/test_excel_export_sync_budget.py: ########## @@ -0,0 +1,372 @@ +# 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. +from __future__ import annotations + +from collections.abc import Iterator +from typing import Any +from unittest import mock + +import pytest +from celery.exceptions import SoftTimeLimitExceeded +from flask import current_app + +from superset.dashboards.excel_export.sync_budget import plan_inline_export +from superset.utils import json + +MODULE = "superset.dashboards.excel_export.sync_budget" + + +def _saved_metric(name: str, expression: str) -> mock.MagicMock: + """A metric saved on a dataset.""" + metric = mock.MagicMock() + metric.metric_name = name + metric.expression = expression + return metric + + +def _chart( + chart_id: int, + *queries: dict[str, Any], + saved_metrics: tuple[tuple[str, str], ...] = (), + engine: str = "base", +) -> mock.MagicMock: + """A chart whose saved query context holds ``queries``.""" + chart = mock.MagicMock() + chart.id = chart_id + chart.slice_name = f"Chart {chart_id}" + chart.viz_type = "table" + chart.query_context = json.dumps({"queries": list(queries)}) + chart.datasource.metrics = [_saved_metric(*metric) for metric in saved_metrics] + chart.datasource.database.backend = engine + return chart + + +def _unexportable_chart(chart_id: int) -> mock.MagicMock: + """A chart the export has to skip: no context, and no rebuilding it.""" + chart = _chart(chart_id) + chart.query_context = None + chart.viz_type = "mixed_timeseries" # outside the rebuild allowlist + return chart + + [email protected] +def charts() -> Iterator[mock.MagicMock]: + """Patch the layout walk so tests supply the dashboard's charts directly.""" + with mock.patch(f"{MODULE}.get_charts_in_layout_order") as ordered: + yield ordered + + [email protected](autouse=True) +def restore_config() -> Iterator[None]: + """Undo config edits: the app fixture is shared by every test in the module.""" + original_sync_max_rows = current_app.config["EXCEL_EXPORT_SYNC_MAX_ROWS"] + original_row_limit = current_app.config["ROW_LIMIT"] + yield + current_app.config["EXCEL_EXPORT_SYNC_MAX_ROWS"] = original_sync_max_rows + current_app.config["ROW_LIMIT"] = original_row_limit + + +def test_plan_sums_the_row_limit_of_every_chart(charts: mock.MagicMock) -> None: + charts.return_value = [ + _chart(10, {"row_limit": 1000}), + _chart(20, {"row_limit": 250}), + ] + + assert plan_inline_export(mock.MagicMock()).requested_rows == 1250 + + +def test_plan_counts_every_query_of_a_multi_query_chart( + charts: mock.MagicMock, +) -> None: + # Count every query in a multi-query chart. + charts.return_value = [_chart(10, {"row_limit": 100}, {"row_limit": 400})] + + assert plan_inline_export(mock.MagicMock()).requested_rows == 500 + + [email protected]( + "query", + [ + {"row_limit": -5}, # below the schema's minimum + {"row_limit": "many"}, # not a number + {"row_limit": [100]}, # not a scalar + ], +) +def test_plan_row_total_is_indeterminate_without_a_finite_row_limit( + charts: mock.MagicMock, query: dict[str, Any] +) -> None: + # Every query needs a finite limit. + charts.return_value = [_chart(10, {"row_limit": 100}), _chart(20, query)] + + plan = plan_inline_export(mock.MagicMock()) + + assert plan.requested_rows is None + assert plan.fits_row_budget is False + + [email protected]("row_limit", ["1000", 1000.0]) +def test_plan_reads_a_row_limit_the_query_schema_accepts( + charts: mock.MagicMock, row_limit: Any +) -> None: + # `ChartDataQueryContextSchema` coerces these to 1000 and runs the export, so + # planning has to size them instead of refusing the whole dashboard. + charts.return_value = [_chart(10, {"row_limit": row_limit})] + + assert plan_inline_export(mock.MagicMock()).requested_rows == 1000 + + [email protected]("query", [{}, {"row_limit": 0}, {"row_limit": None}]) +def test_plan_uses_default_when_row_limit_is_omitted( + charts: mock.MagicMock, query: dict[str, Any] +) -> None: + current_app.config["ROW_LIMIT"] = 250 + charts.return_value = [_chart(10, {"row_limit": 100}), _chart(20, query)] + + assert plan_inline_export(mock.MagicMock()).requested_rows == 350 + + [email protected]( + "metric", + [ + pytest.param("count", id="saved metric"), + pytest.param( + { + "expressionType": "SIMPLE", + "aggregate": "SUM", + "column": {"column_name": "amount"}, + }, + id="simple adhoc metric", + ), + pytest.param( + {"expressionType": "SQL", "sqlExpression": "SUM(amount)"}, + id="custom SQL metric that aggregates", + ), + ], +) +def test_plan_counts_aggregate_only_queries_as_one_row( + charts: mock.MagicMock, metric: Any +) -> None: + charts.return_value = [ + _chart( + chart_id, + { + "columns": [], + "metrics": [metric], + "granularity": "order_date", + }, + saved_metrics=(("count", "COUNT(*)"),), Review Comment: <div> <div id="suggestion"> <div id="issue"><b>SyntaxError from escaped quotes</b></div> <div id="fix"> This line contains literal backslash characters: `((\"count\", \"COUNT(*)\"),)`. Python parses `\"` as an escaped quote inside a double-quoted string, so the element becomes the string `\"count\"` (with literal quotes) instead of the tuple `("count", "COUNT(*)")`, and the unbalanced closing parens then raise `SyntaxError: unmatched ')'` at collection time — the whole module fails to import and every test in it errors. The identical literal on lines 206 and 227 is written without backslashes; this line should match them. </div> </div> <small><i>Code Review Run #afc86f</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]
