aminghadersohi commented on code in PR #38731:
URL: https://github.com/apache/superset/pull/38731#discussion_r2958653224
##########
tests/unit_tests/mcp_service/utils/test_schema_utils.py:
##########
@@ -758,19 +758,45 @@ async def my_tool(request, ctx=None):
for param in sig.parameters.values():
assert param.kind == inspect.Parameter.KEYWORD_ONLY
- def test_flattened_validation_error(self):
- """Pydantic validation errors should propagate when constructing
model."""
+ def test_flattened_validation_error_raises_tool_error(self):
+ """Pydantic validation errors should be converted to ToolError.
+
+ Raw ValidationError propagation causes LangGraph serialization failures
+ (TypeError: encoding without a string argument).
+ """
from unittest.mock import MagicMock, patch
+ from fastmcp.exceptions import ToolError
+
@parse_request(self.SimpleRequest)
def my_tool(request, ctx=None):
return "ok"
mock_ctx = MagicMock()
with patch("fastmcp.server.dependencies.get_context",
return_value=mock_ctx):
- with pytest.raises(ValidationError):
+ with pytest.raises(ToolError):
my_tool(name="test", count="not_a_number")
Review Comment:
Already addressed in the latest commit — async tests added:
`test_flattened_validation_error_raises_tool_error_async` and
`test_flattened_missing_required_field_raises_tool_error_async`.
##########
tests/unit_tests/mcp_service/utils/test_schema_utils.py:
##########
@@ -758,19 +758,45 @@ async def my_tool(request, ctx=None):
for param in sig.parameters.values():
assert param.kind == inspect.Parameter.KEYWORD_ONLY
- def test_flattened_validation_error(self):
- """Pydantic validation errors should propagate when constructing
model."""
+ def test_flattened_validation_error_raises_tool_error(self):
+ """Pydantic validation errors should be converted to ToolError.
+
+ Raw ValidationError propagation causes LangGraph serialization failures
+ (TypeError: encoding without a string argument).
+ """
from unittest.mock import MagicMock, patch
+ from fastmcp.exceptions import ToolError
+
@parse_request(self.SimpleRequest)
def my_tool(request, ctx=None):
return "ok"
mock_ctx = MagicMock()
with patch("fastmcp.server.dependencies.get_context",
return_value=mock_ctx):
- with pytest.raises(ValidationError):
+ with pytest.raises(ToolError):
my_tool(name="test", count="not_a_number")
+ def test_flattened_missing_required_field_raises_tool_error(self):
+ """Missing required fields should raise ToolError with helpful
message."""
+ from unittest.mock import MagicMock, patch
+
+ from fastmcp.exceptions import ToolError
+
+ class RequiredFieldRequest(BaseModel):
+ model_config = ConfigDict(extra="forbid")
+ name: str = Field(..., description="Required name")
+ count: int = Field(..., description="Required count")
+
+ @parse_request(RequiredFieldRequest)
+ def my_tool(request, ctx=None):
+ return "ok"
+
+ mock_ctx = MagicMock()
+ with patch("fastmcp.server.dependencies.get_context",
return_value=mock_ctx):
+ with pytest.raises(ToolError, match="Invalid request parameters"):
+ my_tool(name="test") # missing count
Review Comment:
Already addressed in the latest commit — async tests added:
`test_flattened_validation_error_raises_tool_error_async` and
`test_flattened_missing_required_field_raises_tool_error_async`.
--
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]
