bito-code-review[bot] commented on code in PR #44082: URL: https://github.com/apache/superset/pull/44082#discussion_r4094874736
########## tests/unit_tests/dashboards/test_excel_export_workbook.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 passing already-resolved query contexts into the workbook builder. + +The builder's own behavior (sheet naming, skipped charts, filter application) is +covered through the Celery task in +``tests/unit_tests/tasks/test_export_dashboard_excel.py``. +""" + +from __future__ import annotations + +import os +import tempfile +from collections.abc import Iterator +from typing import Any +from unittest import mock + +import pytest + +from superset.dashboards.excel_export import email +from superset.dashboards.excel_export.workbook import build_workbook +from superset.utils import json + +MODULE = "superset.dashboards.excel_export.workbook" + + +def _chart(chart_id: int, name: str) -> mock.MagicMock: + chart = mock.MagicMock() + chart.id = chart_id + chart.slice_name = name + chart.viz_type = "table" + chart.query_context = json.dumps({"queries": [{"row_limit": 100}]}) + return chart + + [email protected] +def mocks() -> Iterator[dict[str, Any]]: + """Patch the builder's collaborators; keep the real xlsx writer.""" + with mock.patch.multiple( + MODULE, + get_charts_in_layout_order=mock.DEFAULT, + get_dashboard_filter_context=mock.DEFAULT, + ChartDataQueryContextSchema=mock.DEFAULT, + ChartDataCommand=mock.DEFAULT, + resolve_query_context=mock.DEFAULT, + ) as patched: + patched["get_dashboard_filter_context"].return_value.extra_form_data = {} + patched["ChartDataCommand"].return_value.run.return_value = { + "queries": [{"colnames": ["a"], "data": [{"a": 1}]}] + } + yield patched + + [email protected] +def workbook_path() -> Iterator[str]: + file_descriptor, path = tempfile.mkstemp(suffix=".xlsx") + os.close(file_descriptor) + yield path + if os.path.exists(path): + os.remove(path) + + +def _build(path: str, **kwargs: Any) -> Any: + dashboard = mock.MagicMock() + dashboard.id = 1 + return build_workbook( + path, dashboard, {}, "job-1", "data", mock.MagicMock(), **kwargs + ) + + +def test_provided_query_context_is_used_without_resolving_again( + mocks: dict[str, Any], workbook_path: str +) -> None: + # Use the context measured by the row budget. + chart = _chart(10, "First") + mocks["get_charts_in_layout_order"].return_value = [chart] + provided = {"queries": [{"row_limit": 7, "metrics": ["count"]}]} + + _build(workbook_path, query_contexts={10: provided}) + + mocks["resolve_query_context"].assert_not_called() + loaded = mocks["ChartDataQueryContextSchema"].return_value.load.call_args.args[0] + assert loaded["queries"] == provided["queries"] + + +def test_a_chart_resolved_to_none_is_skipped_without_resolving_again( + mocks: dict[str, Any], workbook_path: str +) -> None: + # ``None`` marks a chart already resolved as unexportable. + chart = _chart(20, "Skipped") + mocks["get_charts_in_layout_order"].return_value = [chart] + + errored = _build(workbook_path, query_contexts={20: None}) + + mocks["resolve_query_context"].assert_not_called() + mocks["ChartDataCommand"].return_value.run.assert_not_called() + assert [label for labels in errored.values() for label in labels] == [ + "20 - Skipped" + ] + + +def test_skipped_charts_are_listed_by_reason_without_running( Review Comment: <!-- Bito Reply --> The suggestion to add a docstring to the test function is correct and follows the repository's linting requirements. Applying this change improves the code by documenting the test's intent, as requested by the reviewer. **tests/unit_tests/dashboards/test_excel_export_workbook.py** ``` def test_skipped_charts_are_listed_by_reason_without_running( mocks: dict[str, Any], workbook_path: str ) -> None: """Test that skipped charts are listed by reason without running.""" # Skipped charts are listed by reason without running. ``` ########## 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: <!-- Bito Reply --> The reviewer's suggestion was based on an interpretation of the rendered diff, which introduced backslashes that are not present in the actual source code. Since the code `saved_metrics=(("count", "COUNT(*)"),),` is syntactically correct and the tests pass as expected, the suggestion is not necessary to apply. -- 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]
