bito-code-review[bot] commented on code in PR #43129:
URL: https://github.com/apache/superset/pull/43129#discussion_r3777035858


##########
tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py:
##########
@@ -2078,6 +2078,37 @@ async def 
test_create_virtual_dataset_create_failed(mcp_server: object) -> None:
     assert "Failed to create dataset" in data["error"]
 
 
[email protected]
+async def test_create_virtual_dataset_sql_error_is_actionable(
+    mcp_server: object,
+) -> None:
+    """Warehouse SQL errors are recoverable tool results, not adapter 
crashes."""
+    from superset.exceptions import SupersetGenericDBErrorException
+
+    mock_command = MagicMock()
+    mock_command.run.side_effect = SupersetGenericDBErrorException(
+        "Invalid column name 'missing_value'"
+    )
+
+    with patch(
+        "superset.commands.dataset.create.CreateDatasetCommand",
+        return_value=mock_command,
+    ):
+        async with Client(mcp_server) as client:
+            request = CreateVirtualDatasetRequest(
+                database_id=1,
+                sql="SELECT missing_value FROM sample_events",
+                dataset_name="Test",
+            )
+            result = await client.call_tool(
+                "create_virtual_dataset", {"request": request.model_dump()}
+            )
+            data = json.loads(result.content[0].text)
+
+    assert data["id"] is None
+    assert "Invalid column name" in data["error"]

Review Comment:
   <div>
   
   
   <div id="suggestion">
   <div id="issue"><b>Incomplete test assertions</b></div>
   <div id="fix">
   
   Test assertions are incomplete compared to adjacent error-handling tests 
(lines 2076, 2141). The test verifies `id=None` and error message presence but 
omits `assert data["columns"] == []` and `assert data["error"] is not None`, 
which would catch implementation bugs where the 
`SupersetGenericDBErrorException` handler incorrectly populates these fields.
   </div>
   
   
   </div>
   
   
   
   
   <small><i>Code Review Run #522331</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]

Reply via email to