From fb03e8559232b04c53f91ddbdaab24a6977d5b40 Mon Sep 17 00:00:00 2001 From: Bill Easton Date: Sun, 12 Apr 2026 12:14:52 -0500 Subject: [PATCH] Fix high-severity test quality issues (#3854) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Delete entirely commented-out test_run_server.py (99 lines dead code) - Fix test_pydantic_model_with_stringified_json_no_strict: replace try/except-both-branches-pass with clear pytest.raises assertion - Fix test_path_traversal_blocked: remove dead assertions after pytest.raises (lines after raise never execute) Error handling middleware test fixes are in a separate PR (#3858) which also fixes the underlying RetryMiddleware bug. 🤖 Generated with Claude Code Co-authored-by: Claude Opus 4.6 (1M context) --- .../server/providers/test_skills_provider.py | 10 +- tests/server/test_input_validation.py | 35 ++----- tests/server/test_run_server.py | 98 ------------------- 3 files changed, 12 insertions(+), 131 deletions(-) delete mode 100644 tests/server/test_run_server.py diff --git a/tests/server/providers/test_skills_provider.py b/tests/server/providers/test_skills_provider.py index 3caa96cf4..97732f308 100644 --- a/tests/server/providers/test_skills_provider.py +++ b/tests/server/providers/test_skills_provider.py @@ -653,14 +653,8 @@ class TestPathTraversalPrevention: mcp.add_provider(SkillsDirectoryProvider(roots=skills_dir)) async with Client(mcp) as client: - # Path traversal attempts should fail (either normalized away or blocked) - # The important thing is that SECRET DATA is never returned + # Path traversal attempts should fail — the secret must never be returned with pytest.raises(Exception): - result = await client.read_resource( + await client.read_resource( AnyUrl("skill://test-skill/../../../secret.txt") ) - # If we somehow got here, ensure we didn't get the secret - if result: - for content in result: - if hasattr(content, "text"): - assert "SECRET DATA" not in content.text diff --git a/tests/server/test_input_validation.py b/tests/server/test_input_validation.py index b9008f7f9..60456d0c3 100644 --- a/tests/server/test_input_validation.py +++ b/tests/server/test_input_validation.py @@ -13,6 +13,7 @@ from mcp.types import TextContent from pydantic import BaseModel from fastmcp import Client, FastMCP +from fastmcp.exceptions import ToolError class UserProfile(BaseModel): @@ -130,8 +131,13 @@ class TestPydanticModelArguments: assert "Alice" in result.content[0].text assert "30" in result.content[0].text - async def test_pydantic_model_with_stringified_json_no_strict(self): - """Test if stringified JSON is accepted for Pydantic models without strict validation.""" + async def test_stringified_json_not_auto_parsed_for_pydantic_models(self): + """Stringified JSON is rejected for Pydantic model parameters. + + Some LLM clients send stringified JSON (a JSON string containing a + JSON object) instead of a proper JSON object. FastMCP does not + auto-parse these; callers get a validation error. + """ mcp = FastMCP("TestServer", strict_input_validation=False) @mcp.tool @@ -140,33 +146,12 @@ class TestPydanticModelArguments: return f"Created user {profile.name}, age {profile.age}" async with Client(mcp) as client: - # Some LLM clients send stringified JSON instead of actual JSON stringified = json.dumps( {"name": "Bob", "age": 25, "email": "bob@example.com"} ) - # This test verifies whether we handle stringified JSON - error_msg = "" - try: - result = await client.call_tool("create_user", {"profile": stringified}) - # If this succeeds, we're handling stringified JSON - assert isinstance(result.content[0], TextContent) - assert "Bob" in result.content[0].text - stringified_json_works = True - except Exception as e: - # If this fails, we're not handling stringified JSON - stringified_json_works = False - error_msg = str(e) - - # Document the behavior - we want to know if this works or not - if stringified_json_works: - # This is the desired behavior - pass - else: - # This means stringified JSON doesn't work - document it - assert ( - "validation" in error_msg.lower() or "invalid" in error_msg.lower() - ) + with pytest.raises(ToolError, match="validation"): + await client.call_tool("create_user", {"profile": stringified}) async def test_pydantic_model_with_coercion(self): """Pydantic models should benefit from coercion without strict validation.""" diff --git a/tests/server/test_run_server.py b/tests/server/test_run_server.py deleted file mode 100644 index 65e3112ac..000000000 --- a/tests/server/test_run_server.py +++ /dev/null @@ -1,98 +0,0 @@ -# from pathlib import Path -# from typing import TYPE_CHECKING, Any - -# import pytest - -# import fastmcp -# from fastmcp import FastMCP - -# if TYPE_CHECKING: -# pass - -# USERS = [ -# {"id": "1", "name": "Alice", "active": True}, -# {"id": "2", "name": "Bob", "active": True}, -# {"id": "3", "name": "Charlie", "active": False}, -# ] - - -# @pytest.fixture -# def fastmcp_server(): -# server = FastMCP("TestServer") - -# # --- Tools --- - -# @server.tool -# def greet(name: str) -> str: -# """Greet someone by name.""" -# return f"Hello, {name}!" - -# @server.tool -# def add(a: int, b: int) -> int: -# """Add two numbers together.""" -# return a + b - -# @server.tool -# def error_tool(): -# """This tool always raises an error.""" -# raise ValueError("This is a test error") - -# # --- Resources --- - -# @server.resource(uri="resource://wave") -# def wave() -> str: -# return "👋" - -# @server.resource(uri="data://users") -# async def get_users() -> list[dict[str, Any]]: -# return USERS - -# @server.resource(uri="data://user/{user_id}") -# async def get_user(user_id: str) -> dict[str, Any] | None: -# return next((user for user in USERS if user["id"] == user_id), None) - -# # --- Prompts --- - -# @server.prompt -# def welcome(name: str) -> str: -# return f"Welcome to FastMCP, {name}!" - -# return server - - -# @pytest.fixture -# async def stdio_client(): -# # Find the stdio.py script path -# base_dir = Path(__file__).parent -# stdio_script = base_dir / "test_servers" / "stdio.py" - -# if not stdio_script.exists(): -# raise FileNotFoundError(f"Could not find stdio.py script at {stdio_script}") - -# client = fastmcp.Client( -# transport=fastmcp.client.transports.StdioTransport( -# command="python", -# args=[str(stdio_script)], -# ) -# ) - -# async with client: -# print("READY") -# yield client -# print("DONE") - - -# class TestRunServerStdio: -# async def test_run_server_stdio( -# self, fastmcp_server: FastMCP, stdio_client: fastmcp.Client -# ): -# print("TEST") -# tools = await stdio_client.list_tools() -# print("TEST 2") -# assert tools == 1 - - -# class TestRunServerSSE: -# -# async def test_run_server_sse(self, fastmcp_server: FastMCP): -# pass