Replace type: ignore[attr-defined] with isinstance assertions in tests#2665
Replace type: ignore[attr-defined] with isinstance assertions in tests#2665
Conversation
marvin-context-protocol
Bot
added
enhancement
Test Failure AnalysisUpdated: Added test failure analysis from latest workflow run. ❌ Test Failures (4 tests failing)Summary: The PR replaced Root Cause: The Failed Tests:
Solution: Revert the
Files to fix:
Detailed Test Failure Analysis1. test_elicitation_enum_response (test_elicitation.py:314)What the test does: Calls a tool that elicits an enum response and returns it. Assertion: Actual type: Error: Why it fails: Enums are not strings. While 2. test_resource_content_binary_with_meta (test_server.py:660)What the test does: Creates a resource with binary content and meta, verifies it returns with blob. Assertion: Actual type: Error: Why it fails: Binary content ( 3. test_output_schema_explicit_object_full_handshake (test_server_interactions.py:1053)What the test does: Tests tool with explicit output schema, verifies client deserializes structured content. Assertion: Actual type: Error: Why it fails: The client deserializes the JSON response into a Pydantic 4. test_output_schema_dataclass_full_handshake (test_server_interactions.py:1141)What the test does: Tests tool with dataclass output, verifies client deserializes to the dataclass. Assertion: Actual type: Error: Why it fails: The client creates a new Pydantic model ( ❌ Type Check FailuresSummary: The Root Cause: The type checker (
Suggested Solution: For cases where
For tests that intentionally pass wrong types to verify error handling, use Files to modify:
Type Check Detailed AnalysisType Narrowing Issue in test_elicitation.pyThe async def elicit(
self,
message: str,
response_type: None,
) -> (
AcceptedElicitation[dict[str, Any]] | DeclinedElicitation | CancelledElicitation
)When calling with result = await context.elicit(message="", response_type=None)
assert result.action == "accept"
assert isinstance(result.data, dict)
return result.dataWhile checking Type checker warnings: Intentional Type Errors in Teststest_tool_manager.py:141: with pytest.raises(TypeError, match="not a callable object"):
assert isinstance(1, int) # Intentionally passing invalid type
tool = Tool.from_function(1) # This SHOULD fail at runtimeThe Type checker error: test_tool_manager.py:1055, 1070: assert isinstance(parent_mcp._providers, list)
parent_mcp._providers.append("invalid") # Intentionally corrupt for testingType checker error: test_tool_transform.py:955: with pytest.raises(ValueError, match="Cannot specify 'required=False'"):
Tool.from_tool(
base_tool,
transform_args={"required_param": ArgTransform(required=False, default=99)},
)Type checker error: All of these are tests that verify error handling works correctly - the type checker is correctly identifying that the code is wrong, which is exactly what we want to happen at runtime. Related FilesTest files with failures:
Source files referenced:
|
- Fix enum test to check for ResponseEnum instead of str - Fix binary resource test to check for BlobResourceContents instead of TextResourceContents - Fix Root type tests to check attributes directly instead of isinstance checks
- Remove execution methods from TransformingProvider (only handles transformations) - Add execution methods to base Provider class with default implementations - Fix type narrowing in tests using cast() instead of type: ignore - Fix PromptResult type handling in prompt render tests - Fix type narrowing in middleware test for arguments and structured_content
WalkthroughThis pull request adds four execution helper methods to the Provider base class (call_tool, read_resource, read_resource_template, render_prompt) that delegate to existing component lookups and operations. These are documented as overridable hooks with graceful None handling when components are absent. The corresponding public methods are removed from TransformingProvider, consolidating the implementation in the base class. Supporting public types (ResourceContent, PromptResult, ToolResult) are introduced via imports. Additionally, TransformingProvider receives internal validation for tool_renames to prevent duplicate target names and includes a reverse mapping for lookups. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (41)
📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)src/fastmcp/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (1)src/fastmcp/server/providers/transforming.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Replaced
type: ignore[attr-defined]annotations with properisinstancetype assertions in test files to improve type safety and code clarity.isinstancechecks before accessing attributes like.text,.content,.data, etc.type: ignoreannotations only for:_injected_values,_subscription_task_group, etc.)__name__on command objects)This improves type checking accuracy while maintaining test functionality.