Skip to content

Commit dfca0a0

Browse files
kingpanther13claude
andcommitted
docs(tests): scope the failure-helper claim and fix safe_call_tool's docstring
Two corrections from Codex review: call_tool_failure() only proves the call failed when expected_error is given. assert_mcp_failure()'s check is `if data.get("success")`, so a result dict that omits the key entirely is accepted as a failure. The guidance now says to pass expected_error and states the limitation rather than claiming the helper always proves failure. safe_call_tool()'s own docstring still advertised it as the helper for tests that expect a failure, contradicting the new rule in a second place and recreating the ambiguity this branch removes. It now names its real roles -- finally-block cleanup and availability probing -- and points at call_tool_failure() for asserted failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015c4gkE9mcKu4RiuQzR5eff
1 parent 3f229a2 commit dfca0a0

2 files changed

Lines changed: 10 additions & 6 deletions

File tree

tests/AGENTS.md

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,11 @@ rather than assume the e2e's always-present case.
4949
## Test Patterns
5050

5151
- Tests expecting tool **success**: use `mcp.call_tool_success()` inside `MCPAssertions` context
52-
- Tests expecting tool **failure**: use `mcp.call_tool_failure()` inside `MCPAssertions` context.
53-
It catches the `ToolError`, asserts the call actually failed rather than silently
54-
succeeding, and — when `expected_error` is given — matches it against the extracted
55-
error message.
52+
- Tests expecting tool **failure**: use `mcp.call_tool_failure()` inside `MCPAssertions`
53+
context, and **pass `expected_error`**. It catches the `ToolError` and matches
54+
`expected_error` against the extracted message. Without `expected_error` it is a weak
55+
assertion: the success check is `if data.get("success")`, so a result dict that omits
56+
the key entirely is accepted as a failure.
5657
- `safe_call_tool()` is for calls whose outcome the test does **not** assert: `finally`
5758
cleanup (so a cleanup failure cannot mask the real assertion) and service-availability
5859
probes. It swallows `ToolError` and returns a parsed dict, so using it for an expected

tests/src/e2e/utilities/assertions.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,11 @@ async def safe_call_tool(
113113
) -> dict[str, Any]:
114114
"""Call an MCP tool and return parsed result, handling ToolError exceptions.
115115
116-
This is useful for tests that expect tools to fail and want to inspect
117-
the error response without catching exceptions manually.
116+
For a call whose outcome the test does NOT assert: ``finally``-block cleanup
117+
(so a cleanup failure cannot mask the real assertion) and service-availability
118+
probes. It swallows ``ToolError`` and returns a parsed dict either way, so a
119+
test that asserts a failure should use ``MCPAssertions.call_tool_failure()``
120+
with ``expected_error`` instead -- see tests/AGENTS.md "Test Patterns".
118121
119122
Args:
120123
mcp_client: The MCP client instance

0 commit comments

Comments
 (0)