bito-code-review[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4174970241
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py:
##########
@@ -556,6 +556,90 @@ def run(self) -> dict[str, Any]:
assert query["filters"] == [{"col": "gender", "op": "==", "val":
"boy"}]
assert "adhoc_filters" not in query
+ def test_table_preview_exposes_jinja_context(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ """Chart preview queries expose the same Jinja inputs as
get_chart_data."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
+
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ query = QueryObject(
+ filters=[{"col": "region", "op": "IN", "val": ["North"]}],
+ time_range="Last week",
+ columns=["region"],
+ metrics=["count"],
+ )
+ query_context = SimpleNamespace(
+ queries=[query],
+ form_data={"url_params": {"tenant": "acme"}},
+ )
+ observed: dict[str, bool] = {}
+
+ class ChartDataCommand:
+ def __init__(self, qc: object) -> None:
+ self.query_context = qc
+
+ def validate(self) -> None:
+ pass
+
+ def run(self) -> dict[str, Any]:
+ assert_request_dependent_jinja_macros()
+ observed["ran"] = True
+ return {
+ "queries": [
+ {
+ "data": [{"region": "North"}],
+ "colnames": ["region"],
+ "rowcount": 1,
+ }
+ ]
+ }
+
+ preview_module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_preview"
+ )
+ monkeypatch.setattr(
+ preview_module,
+ "build_query_context_from_form_data",
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Untyped lambda in new test</b></div>
<div id="fix">
BITO.md adaptive rule 13350 requires typed helper functions instead of
untyped lambdas. The new test adds `lambda *args, **kwargs: query_context`
(line 612) patching `build_query_context_from_form_data`; define a nested `def
_fake_build_query_context(*args: Any, **kwargs: Any) -> SimpleNamespace` above
the `monkeypatch.setattr` call and pass its name. The file's 10 pre-existing
lambdas are unchanged and out of scope.
</div>
</div>
<small><i>Code Review Run #4f788a</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
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py:
##########
@@ -228,6 +231,133 @@ def test_null_chart_metadata(self):
assert output.datasource_name is None
+class TestChartSqlJinjaContext:
+ """SQL generation must expose the same Jinja inputs as chart data
execution."""
+
+ def _command_observing_jinja(self, observed: dict[str, bool]) -> type:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing docstring on test helper</b></div>
<div id="fix">
`_command_observing_jinja` (and the nested `ChartDataCommand` double) has no
docstring, unlike the two test methods in this class. BITO rule 12147 requires
docstrings on every new function in test files, explicitly including helper
functions. Add a one-liner such as: "Build a ChartDataCommand double that
asserts request-dependent Jinja inputs during run()."
</div>
</div>
<small><i>Code Review Run #4f788a</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
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py:
##########
@@ -228,6 +231,133 @@ def test_null_chart_metadata(self):
assert output.datasource_name is None
+class TestChartSqlJinjaContext:
+ """SQL generation must expose the same Jinja inputs as chart data
execution."""
+
+ def _command_observing_jinja(self, observed: dict[str, bool]) -> type:
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
+
+ class ChartDataCommand:
+ def __init__(self, query_context: object) -> None:
+ self.query_context = query_context
+
+ def validate(self) -> None:
+ pass
+
+ def run(self) -> dict[str, Any]:
+ assert_request_dependent_jinja_macros()
+ observed["ran"] = True
+ return {"queries": [{"query": "SELECT 1", "language": "sql"}]}
+
+ return ChartDataCommand
+
+ def test_sql_from_form_data_exposes_jinja_context(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ """The form_data SQL path publishes filters, time_range, and
url_params."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ query = QueryObject(
+ filters=[{"col": "region", "op": "IN", "val": ["North"]}],
+ time_range="Last week",
+ )
+ query_context = SimpleNamespace(
+ queries=[query],
+ form_data={"url_params": {"tenant": "acme"}},
+ datasource=SimpleNamespace(id=7, type="table"),
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unannotated locals in tests</b></div>
<div id="fix">
Locals `get_data_command_module`, `query`, `query_context`, `chart`, and
`result` lack explicit type annotations; the second test mirrors this at lines
319-329 and 346-355. BITO rule 13153 requires annotations on all test-file
locals even when inferable. Annotate e.g. `query: QueryObject`, `query_context:
SimpleNamespace`, `result: ChartSql | ChartError`.
</div>
</div>
<small><i>Code Review Run #4f788a</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]
