bito-code-review[bot] commented on code in PR #44082: URL: https://github.com/apache/superset/pull/44082#discussion_r4093779581
########## 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: <div> <div id="suggestion"> <div id="issue"><b>Missing test docstring</b></div> <div id="fix"> New test functions need a docstring per repo rule (BITO.md adaptive rule 12148): docstrings document test intent and are enforced by linting, and an inline comment alone does not satisfy the rule. Add a short docstring above the existing comment in `test_skipped_charts_are_listed_by_reason_without_running`; no behavior change. </div> </div> <small><i>Code Review Run #ba6917</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]
