bito-code-review[bot] commented on code in PR #36909:
URL: https://github.com/apache/superset/pull/36909#discussion_r2667218502
##########
tests/unit_tests/db_engine_specs/test_trino.py:
##########
@@ -1214,3 +1214,174 @@ def update_stats(*args, **kwargs):
# So we expect: 1 (initial) + 1 (0.5) + 1 (1.0) = 3 commits total
commit_calls = mock_db.session.commit.call_count
assert commit_calls >= 2 # At least initial commit and one progress update
+
+
+@patch("superset.db_engine_specs.presto.PrestoBaseEngineSpec.handle_cursor")
+@patch("superset.db_engine_specs.trino.TrinoEngineSpec.cancel_query")
+@patch("superset.db_engine_specs.trino.db")
+@patch("superset.db_engine_specs.trino.app")
+def test_handle_cursor_sets_progress_text_for_planning_state(
+ mock_app: Mock,
+ mock_db: Mock,
+ mock_cancel_query: Mock,
+ mock_presto_handle_cursor: Mock,
+ mocker: MockerFixture,
+) -> None:
+ """Test that handle_cursor sets progress_text to 'Scheduled' for PLANNING
state."""
+ from superset.db_engine_specs.trino import TrinoEngineSpec
+ from superset.models.sql_lab import Query
+
+ mock_app.config = {"DB_POLL_INTERVAL_SECONDS": {"trino": 0}}
+
+ cursor_mock = mocker.MagicMock(spec=["query_id", "stats", "info_uri"])
+ cursor_mock.query_id = "test-query-id"
+
+ call_count = 0
+
+ def update_stats(*args, **kwargs):
+ nonlocal call_count
+ call_count += 1
+ if call_count >= 1:
+ cursor_mock.stats = {
+ "state": "FINISHED",
+ "completedSplits": 10,
+ "totalSplits": 10,
+ }
+
+ # Start with PLANNING state
+ cursor_mock.stats = {"state": "PLANNING", "completedSplits": 0,
"totalSplits": 10}
+
+ with patch("superset.db_engine_specs.trino.time.sleep",
side_effect=update_stats):
+ query = Query()
+ query.status = "running"
+ TrinoEngineSpec.handle_cursor(cursor=cursor_mock, query=query)
+
+ # Check that progress_text was set to "Scheduled" for PLANNING state
+ assert query.extra.get("progress_text") is not None
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Weak test assertion for progress_text mapping</b></div>
<div id="fix">
The assertion here is too weak—it only checks that progress_text is set, but
doesn't verify the correct mapped value for PLANNING state. Since the code sets
it to 'Scheduled' for PLANNING, the test should assert equality to catch
mapping errors.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/tests/unit_tests/db_engine_specs/test_trino.py
+++ b/tests/unit_tests/db_engine_specs/test_trino.py
@@ -1259,7 +1259,7 @@ def
test_handle_cursor_sets_progress_text_for_planning_state(
# Check that progress_text was set to "Scheduled" for PLANNING state
- assert query.extra.get("progress_text") is not None
+ assert query.extra.get("progress_text") == "Scheduled"
```
</div>
</details>
</div>
<small><i>Code Review Run #bc3755</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/db_engine_specs/test_trino.py:
##########
@@ -1214,3 +1214,174 @@ def update_stats(*args, **kwargs):
# So we expect: 1 (initial) + 1 (0.5) + 1 (1.0) = 3 commits total
commit_calls = mock_db.session.commit.call_count
assert commit_calls >= 2 # At least initial commit and one progress update
+
+
+@patch("superset.db_engine_specs.presto.PrestoBaseEngineSpec.handle_cursor")
+@patch("superset.db_engine_specs.trino.TrinoEngineSpec.cancel_query")
+@patch("superset.db_engine_specs.trino.db")
+@patch("superset.db_engine_specs.trino.app")
+def test_handle_cursor_sets_progress_text_for_planning_state(
+ mock_app: Mock,
+ mock_db: Mock,
+ mock_cancel_query: Mock,
+ mock_presto_handle_cursor: Mock,
+ mocker: MockerFixture,
+) -> None:
+ """Test that handle_cursor sets progress_text to 'Scheduled' for PLANNING
state."""
+ from superset.db_engine_specs.trino import TrinoEngineSpec
+ from superset.models.sql_lab import Query
+
+ mock_app.config = {"DB_POLL_INTERVAL_SECONDS": {"trino": 0}}
+
+ cursor_mock = mocker.MagicMock(spec=["query_id", "stats", "info_uri"])
+ cursor_mock.query_id = "test-query-id"
+
+ call_count = 0
+
+ def update_stats(*args, **kwargs):
+ nonlocal call_count
+ call_count += 1
+ if call_count >= 1:
+ cursor_mock.stats = {
+ "state": "FINISHED",
+ "completedSplits": 10,
+ "totalSplits": 10,
+ }
+
+ # Start with PLANNING state
+ cursor_mock.stats = {"state": "PLANNING", "completedSplits": 0,
"totalSplits": 10}
+
+ with patch("superset.db_engine_specs.trino.time.sleep",
side_effect=update_stats):
+ query = Query()
+ query.status = "running"
+ TrinoEngineSpec.handle_cursor(cursor=cursor_mock, query=query)
+
+ # Check that progress_text was set to "Scheduled" for PLANNING state
+ assert query.extra.get("progress_text") is not None
+
+
+@patch("superset.db_engine_specs.presto.PrestoBaseEngineSpec.handle_cursor")
+@patch("superset.db_engine_specs.trino.TrinoEngineSpec.cancel_query")
+@patch("superset.db_engine_specs.trino.db")
+@patch("superset.db_engine_specs.trino.app")
+def test_handle_cursor_sets_progress_text_for_queued_state(
+ mock_app: Mock,
+ mock_db: Mock,
+ mock_cancel_query: Mock,
+ mock_presto_handle_cursor: Mock,
+ mocker: MockerFixture,
+) -> None:
+ """Test that handle_cursor sets progress_text to 'Queued' for QUEUED
state."""
+ from superset.db_engine_specs.trino import TrinoEngineSpec
+ from superset.models.sql_lab import Query
+
+ mock_app.config = {"DB_POLL_INTERVAL_SECONDS": {"trino": 0}}
+
+ cursor_mock = mocker.MagicMock(spec=["query_id", "stats", "info_uri"])
+ cursor_mock.query_id = "test-query-id"
+
+ call_count = 0
+
+ def update_stats(*args, **kwargs):
+ nonlocal call_count
+ call_count += 1
+ if call_count >= 1:
+ cursor_mock.stats = {
+ "state": "FINISHED",
+ "completedSplits": 10,
+ "totalSplits": 10,
+ }
+
+ # Start with QUEUED state
+ cursor_mock.stats = {"state": "QUEUED", "completedSplits": 0,
"totalSplits": 0}
+
+ with patch("superset.db_engine_specs.trino.time.sleep",
side_effect=update_stats):
+ query = Query()
+ query.status = "running"
+ TrinoEngineSpec.handle_cursor(cursor=cursor_mock, query=query)
+
+ # Check that progress_text was set
+ assert query.extra.get("progress_text") is not None
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Weak test assertion for progress_text mapping</b></div>
<div id="fix">
Similar to the PLANNING test, this assertion is too weak—it doesn't verify
the correct 'Queued' value for QUEUED state, potentially missing mapping bugs.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/tests/unit_tests/db_engine_specs/test_trino.py
+++ b/tests/unit_tests/db_engine_specs/test_trino.py
@@ -1303,7 +1303,7 @@ def
test_handle_cursor_sets_progress_text_for_queued_state(
# Check that progress_text was set
- assert query.extra.get("progress_text") is not None
+ assert query.extra.get("progress_text") == "Queued"
```
</div>
</details>
</div>
<small><i>Code Review Run #bc3755</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]
