bito-code-review[bot] commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4121423551
##########
tests/unit_tests/mcp_service/chart/test_histogram_boxplot_charts.py:
##########
@@ -463,3 +463,65 @@ def
test_whisker_options_alongside_whisker_type_is_consumed(self) -> None:
}
)
assert config.whisker_type == "min_max"
+
+
[email protected](
+ ("adhoc_filters", "expected_metrics"),
+ [
+ ([], []),
+ (
+ [
+ {
+ "expressionType": "SQL",
+ "clause": "HAVING",
+ "sqlExpression": "COUNT(*) > 1",
+ }
+ ],
+ [
+ {
+ "expressionType": "SQL",
+ "sqlExpression": "COUNT(*)",
+ "label": "COUNT(*)",
+ }
+ ],
+ ),
+ ],
+)
+def test_histogram_query_matches_frontend_build_query(
+ adhoc_filters: list[dict[str, str]], expected_metrics: list[dict[str, str]]
+) -> None:
+ """Histogram queries select the binned column and apply
histogramOperator."""
+ from unittest.mock import patch
+
+ from superset.mcp_service.chart import chart_helpers
Review Comment:
<!-- Bito Reply -->
The suggestion to hoist imports to the module level and use the
string-target patch is appropriate. This aligns with the project's established
patterns in other chart tests and improves consistency and maintainability by
avoiding inline imports.
**tests/unit_tests/mcp_service/chart/test_histogram_boxplot_charts.py**
```
from unittest.mock import patch
from superset.mcp_service.chart import chart_helpers
@patch("superset.mcp_service.chart.chart_helpers.resolve_datasource_engine")
def test_histogram_query_matches_frontend_build_query(
mock_resolve_engine, adhoc_filters: list[dict[str, str]],
expected_metrics: list[dict[str, str]]
) -> None:
```
##########
superset/mcp_service/chart/tool/update_chart_preview.py:
##########
@@ -292,7 +298,7 @@ def update_chart_preview( # noqa: C901
config,
new_form_data,
dataset,
- run_compile_check=config.chart_type in ("gauge", "treemap_v2"),
+ run_compile_check=bool(plugin and
plugin.requires_compile_check),
Review Comment:
<!-- Bito Reply -->
The suggestion to derive `run_compile_check` from the plugin owning the
final configuration is correct and addresses the issue where the flag could be
incorrectly skipped when the original plugin is disabled. The implementation in
the provided diff hunk correctly uses `bool(plugin and
plugin.requires_compile_check)` to ensure the flag is derived from the active
plugin configuration, which is the appropriate approach for maintaining
consistency in chart preview updates.
**superset/mcp_service/chart/tool/update_chart_preview.py**
```
- run_compile_check=config.chart_type in ("gauge",
"treemap_v2"),
+ run_compile_check=bool(plugin and
plugin.requires_compile_check),
```
--
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]
