Test quality: 6 tests with no useful assertions, 1 dead test file

[strawgate]

Description

A test suite audit identified several tests that always pass regardless of behavior, plus a dead test file:

1. test_error_handling_middleware_transform_errors — asserts only exc_info.value is not None (always true after pytest.raises)
2. test_retry_middleware_with_transient_failures — zero assertions, try/except swallows all errors
3. test_error_handling_middleware_tracks_error_statistics — calls tools on a disconnected client (outside async with), all calls silently fail
4. test_pydantic_model_with_stringified_json_no_strict — try/except succeeds on both branches, test can never fail
5. test_path_traversal_blocked — assertions after pytest.raises are dead code (lines after the raising call never execute)
6. test_run_server.py — entire file (99 lines) is commented out, provides zero coverage

[marvin-context-protocol]

Claude finished @strawgate's task in 5m 58s —— View job

---

Recommendation

All 5 test issues and the dead test file are confirmed. Fix each test to make meaningful assertions, and delete test_run_server.py. The fixes are straightforward — each test has a clear intended behavior that just needs proper assertion wiring.

Findings

I verified each reported issue by reading the code, running the tests, and instrumenting behavior. All 6 items are real problems.

1. test_error_handling_middleware_transform_errors (test_error_handling.py:535-551) — Confirmed: tautological assertion

The only assertion is assert exc_info.value is not None (line 551), which is always true after pytest.raises succeeds. The test configures transform_errors=True but never checks that the error was actually transformed into an McpError with the correct error code (-32602 for ValueError per error_handling.py:94-97).

2. test_retry_middleware_with_transient_failures (test_error_handling.py:557-588) — Confirmed: zero assertions

The test computes success_count but never asserts on it. The try/except on line 582 swallows all exceptions. The trailing comment says "some operations should succeed due to retries" but never verifies this.

3. test_error_handling_middleware_tracks_error_statistics (test_error_handling.py:441-472) — Confirmed: dead code outside async with

Lines 460-465 call client.call_tool(...) after the async with Client(...) block exits (line 459). I verified independently that these calls raise RuntimeError: Client is not connected, which the try/except swallows. The middleware records zero stats from those 5 calls. The test still passes because the 5 calls inside the context manager (3 + 2) satisfy the >= 5 assertion.

4. test_pydantic_model_with_stringified_json_no_strict (test_input_validation.py:133-169) — Confirmed: test can never fail in practice

The try/except accepts both outcomes: success → pass, failure → asserts error message contains "validation" or "invalid" (which it does). I verified the test currently takes the failure branch with a ValidationError whose message always contains "validation". Neither branch would cause a test failure, so this test provides zero regression coverage for stringified JSON handling.

5. test_path_traversal_blocked (test_skills_provider.py:641-666) — Confirmed: security assertions are dead code

The client.read_resource(...) call at line 659 raises FileNotFoundError, so lines 662-666 (the assert "SECRET DATA" not in content.text check) never execute. The test passes solely because some exception is raised, not because the security property (no secret data leakage) is verified. I confirmed via test output that the path ../../../secret.txt gets normalized to secret.txt, which then correctly raises FileNotFoundError.

6. test_run_server.py (test_run_server.py:1-99) — Confirmed: 99 lines of commented-out code

Entire file has been commented out since it was first committed. It provides zero test coverage and should be deleted.

Verification

I ran all 5 tests — they all pass despite the issues described above:

$ uv run pytest tests/server/middleware/test_error_handling.py -k "test_error_handling_middleware_transform_errors" -xvs
PASSED

$ uv run pytest tests/server/middleware/test_error_handling.py -k "test_retry_middleware_with_transient_failures" -xvs
PASSED  # (zero assertions)

$ uv run pytest tests/server/middleware/test_error_handling.py -k "test_error_handling_middleware_tracks_error_statistics" -xvs
PASSED  # (5 client.call_tool calls outside async-with silently fail)

$ uv run pytest tests/server/test_input_validation.py -k "test_pydantic_model_with_stringified_json_no_strict" -xvs
PASSED  # (takes failure branch, passes via error msg check)

$ uv run pytest tests/server/providers/test_skills_provider.py -k "test_path_traversal_blocked" -xvs
PASSED  # (FileNotFoundError raised, security assertions never execute)

I also wrote a standalone script confirming that Client.call_tool() outside async with raises RuntimeError: Client is not connected — confirming that lines 460-465 in test_error_handling_middleware_tracks_error_statistics are dead code.

Detailed Action Plan

Fix 1: test_error_handling_middleware_transform_errors (test_error_handling.py:535)

Replace the tautological assertion with a check that error transformation actually works:

async with Client(error_handling_server) as client:
    with pytest.raises(McpError) as exc_info:
        await client.call_tool("failing_operation", {"error_type": "value"})
    assert exc_info.value.error.code == -32602
    assert "Invalid params" in exc_info.value.error.message

This verifies the middleware transforms ValueError → McpError(-32602) per the _transform_error logic.

Fix 2: test_retry_middleware_with_transient_failures (test_error_handling.py:557)

Add an assertion on success_count:

# After the loop
assert success_count > 0, "Retry middleware should allow some intermittent operations to succeed"

Alternatively, use a fixed seed (random.seed(42)) and assert exact counts for determinism.

Fix 3: test_error_handling_middleware_tracks_error_statistics (test_error_handling.py:441)

Move lines 460-465 inside the async with block so the client is connected:

async with Client(error_handling_server) as client:
    for _ in range(3):
        with pytest.raises(Exception):
            await client.call_tool("failing_operation", {"error_type": "value"})
    for _ in range(2):
        with pytest.raises(Exception):
            await client.call_tool("failing_operation", {"error_type": "file"})
    # These must be INSIDE the async with block
    for _ in range(5):
        try:
            await client.call_tool("intermittent_operation", {"fail_rate": 0.8})
        except Exception:
            pass

Then tighten the assertion — the 5 deliberate failures should produce exactly 5 ToolError counts (plus whatever intermittent failures add).

Fix 4: test_pydantic_model_with_stringified_json_no_strict (test_input_validation.py:133)

Decide on the expected behavior and assert it. Currently, stringified JSON is rejected. The test should assert the actual expected behavior:

async with Client(mcp) as client:
    stringified = json.dumps({"name": "Bob", "age": 25, "email": "bob@example.com"})
    # Stringified JSON is NOT supported — should raise a validation error
    with pytest.raises(ToolError, match="validation"):
        await client.call_tool("create_user", {"profile": stringified})

If the intent is that stringified JSON should work in non-strict mode, flip the assertion to expect success and file a separate feature bug.

Fix 5: test_path_traversal_blocked (test_skills_provider.py:641)

Restructure to make the security assertion reachable:

async with Client(mcp) as client:
    with pytest.raises(Exception):
        result = await client.read_resource(
            AnyUrl("skill://test-skill/../../../secret.txt")
        )
        # This assertion runs only if read_resource doesn't raise,
        # which would mean the traversal bypassed protection
        assert False, f"Path traversal should have been blocked, got: {result}"

Or use a two-phase approach that handles both failure modes:

try:
    result = await client.read_resource(
        AnyUrl("skill://test-skill/../../../secret.txt")
    )
    # If we got here, verify no secret data leaked
    for content in result:
        if hasattr(content, "text"):
            assert "SECRET DATA" not in content.text
    pytest.fail("Expected path traversal to raise an exception")
except Exception:
    pass  # Expected: traversal blocked

Fix 6: Delete test_run_server.py

git rm tests/server/test_run_server.py

99 lines of commented-out code, dead since initial commit.

Related Files

Repository	File	Relevance	Sections
PrefectHQ/fastmcp	tests/server/middleware/test_error_handling.py	Contains 3 of the 5 problematic tests (issues 1, 2, 3)	441-472, 535-551, 557-588
PrefectHQ/fastmcp	tests/server/test_input_validation.py	Contains the stringified JSON test (issue 4)	133-169
PrefectHQ/fastmcp	tests/server/providers/test_skills_provider.py	Contains the path traversal test (issue 5)	641-666
PrefectHQ/fastmcp	tests/server/test_run_server.py	Dead file — 99 lines fully commented out (issue 6)	1-99
PrefectHQ/fastmcp	src/fastmcp/server/middleware/error_handling.py	Implementation of ErrorHandlingMiddleware and RetryMiddleware — needed to understand what the tests should assert	81-118

---

Marvin Context Protocol | Type /marvin to interact further

Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.