bito-code-review[bot] commented on code in PR #41127:
URL: https://github.com/apache/superset/pull/41127#discussion_r3565834114
##########
tests/unit_tests/sql_lab_test.py:
##########
@@ -193,6 +193,67 @@ def mock_serialize_payload(payload, use_msgpack):
)
+def test_execute_sql_statements_mutates_before_split_by_default(
+ mocker: MockerFixture, app
+) -> None:
+ """
+ With the default `MUTATE_AFTER_SPLIT=False`, `execute_sql_statements`
should
+ mutate the whole, un-split query once before splitting it into individual
+ statement blocks, for engines that execute statements individually rather
+ than as one. Regression guard for issue #30169.
+ """
+ query = mocker.MagicMock()
+ query.limit = 1
+ query.database = mocker.MagicMock()
+ query.database.cache_timeout = 100
+ query.status = "RUNNING"
+ query.select_as_cta = False
+ query.database.allow_run_async = True
+ query.database.db_engine_spec.engine = "sqlite"
+ query.database.db_engine_spec.run_multiple_statements_as_one = False
+ query.database.db_engine_spec.allows_sql_comments = True
+
+ mutate_mock = mocker.patch.object(
+ query.database,
+ "mutate_sql_based_on_config",
+ side_effect=lambda sql, **kw: sql,
+ )
+
+ mocker.patch("superset.sql_lab.get_query", return_value=query)
+ mocker.patch("sys.getsizeof", return_value=10000000)
+ mocker.patch(
+ "superset.sql_lab._serialize_payload",
+ side_effect=lambda payload, use_msgpack: "serialized_payload",
+ )
+ mocker.patch("superset.sql_lab.db.session.refresh", return_value=None)
+ mocker.patch("superset.sql_lab.results_backend", return_value=True)
+
+ execute_sql_statements(
+ query_id=1,
+ rendered_query="SELECT 1; SELECT 2;",
+ return_results=True,
+ store_results=True,
+ start_time=None,
+ expand_data=False,
+ log_params={},
+ )
+
+ is_split_values = [
+ call.kwargs.get("is_split") for call in mutate_mock.call_args_list
+ ]
+ # The mutator is called once on the whole, un-split query before
splitting...
+ assert is_split_values[0] is False
+ first_call_sql = mutate_mock.call_args_list[0].args[0]
+ assert "1" in first_call_sql
+ assert "2" in first_call_sql
+ # Both statements are present in a single, un-split call.
+ assert first_call_sql.count("SELECT") == 2
+ # ...and once again per already-split statement (a no-op when
+ # `MUTATE_AFTER_SPLIT=False`, since `is_split=True` won't match the
config).
+ assert all(value is True for value in is_split_values[1:])
+ assert len(is_split_values) == 3
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing execute_query mock</b></div>
<div id="fix">
The test's `execute_sql_statements` call at line 231 enters a loop (lines
515-559) that calls `execute_query` for each SQL block, but `execute_query` is
not mocked. Without this mock, the real `execute_query` runs and fails when
`db_engine_spec.fetch_data(cursor, increased_limit)` returns a MagicMock that
isn't callable, causing a TypeError that gets caught at line 547 and returns
early—hiding whether the test assertions would actually pass.
</div>
</div>
<small><i>Code Review Run #56d2d8</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]
