feat(sdk): Add TestLLM class for testing without mocking LiteLLM#2016
feat(sdk): Add TestLLM class for testing without mocking LiteLLM#2016VascoSch92 wants to merge 4 commits intomainfrom
Conversation
This commit introduces a new TestLLM class in openhands.sdk.testing that provides a clean way to write tests without needing to mock LiteLLM internals. Key features: - TestLLM is a real LLM subclass that works anywhere an LLM is accepted - No @patch decorators needed - just pass TestLLM as the llm= argument - Tests speak in SDK types (Message, TextContent, MessageToolCall) - Clear error when scripted responses are exhausted - Zero-cost metrics by default - Always uses completion() path (uses_responses_api returns False) Also refactors test_agent_status_transition.py to demonstrate the new TestLLM usage, replacing ~20 lines of mock setup with ~3 lines. Closes #2005
Coverage Report •
|
||||||||||||||||||||
|
@OpenHands WDYT about this draft PR? Feel free to read AGENTS.md and make the project, and find litellm codebase in .venv. Investigate and understand all that it's doing and how would we use this. Look at the linked issue too. Post your feedback as a comment to this PR, so I can read it. Note that it will be rendered as markdown. Use smolpaws. |
|
I'm on it! enyst can track my progress at all-hands.dev |
Feedback on PR #2016:
|
This comment was marked as duplicate.
This comment was marked as duplicate.
|
Ooh this is cool! Super helpful that we can compose the responses with typed objects rather than mocking json strings for llm responses Is there a way to support multi turn llm responses? (emit all messages in the queue until a "finish" response?) |
I think it is already supporting that. For example: from openhands.sdk import Agent
from openhands.sdk.llm.message import Message, TextContent
from openhands.sdk import LLM, Agent, Conversation
from pathlib import Path
llm = TestLLM.from_messages([
Message(role="assistant", content=[TextContent(text="Hello!")]),
Message(role="assistant", content=[TextContent(text="Goodbye!")]),
])
agent = Agent(llm=llm)
conversation = Conversation(agent=agent, workspace=Path.cwd())
conversation.send_message("Hey")
conversation.run()
conversation.send_message("Goodbye")
conversation.run()it is faking the conversation |
|
@malhotra5 Actually integration with tools is also working. For example if __name__ == "__main__":
from openhands.sdk import Agent, Conversation
from openhands.sdk.llm.message import Message, MessageToolCall, TextContent
from pathlib import Path
llm = TestLLM.from_messages([
Message(
role="assistant",
content=[TextContent(text="Oh yeah. Let's call the finish tool")],
tool_calls=[
MessageToolCall(
id="call_1",
name="finish",
arguments='{"message": "I like trains"}',
origin="completion",
)
],
),
Message(role="assistant", content=[TextContent(text="This message will never been displayed")]),
])
agent = Agent(llm=llm)
conversation = Conversation(agent=agent, workspace=Path.cwd())
conversation.send_message("Let's see what happens with tools :-)")
conversation.run()is faking the conversation This is pretty nice... and also show how well structured is the sdk :-) I think this class can be really helpfull for the sdk test suite and the end to end CLI tests.... |
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
VascoSch92
changed the title
VascoSch92
changed the title
|
This is great! Thanks for clarifying the behavior this seems super powerful Do we want the ability to raise known llm error types as well? Could be great if we raise a budget error, or context window error for tests (doesn't have to be in this PR) |
I made a couple of changes and now you can pass excpetion which will be raises. In this way you can also tests behaviours when errors happen. from pathlib import Path
from openhands.sdk import Agent, Conversation
from openhands.sdk.llm.message import Message, TextContent
llm = TestLLM.from_messages(
[
Message(role="assistant", content=[TextContent(text="Hello!")]),
Message(role="assistant", content=[TextContent(text="Goodbye!")]),
LLMContextWindowExceedError("context too long"),
]
)
agent = Agent(llm=llm)
conversation = Conversation(agent=agent, workspace=Path.cwd())
conversation.send_message("Hey")
conversation.run()
conversation.send_message("Goodbye")
conversation.run()
conversation.send_message("I forgot something!")
conversation.run()This will raise |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Elegant solution that dramatically simplifies test writing (20 lines → 3 lines). Core idea is sound, but found a few important issues around silent data loss and missing test coverage for the testing utility itself.
| """Convert message content to a string.""" | ||
| from openhands.sdk.llm.message import TextContent | ||
|
|
||
| parts = [] |
There was a problem hiding this comment.
🟠 Important: _content_to_string only handles TextContent and silently ignores other content types like ImageContent. This could mask bugs in tests where messages accidentally contain unsupported content types.
Suggest either:
- Handle all content types appropriately, or
- Raise an error for unsupported types with a clear message
| parts = [] | |
| def _content_to_string(self, message: Message) -> str: | |
| """Convert message content to a string.""" | |
| from openhands.sdk.llm.message import TextContent | |
| parts = [] | |
| for item in message.content: | |
| if isinstance(item, TextContent): | |
| parts.append(item.text) | |
| else: | |
| raise ValueError( | |
| f"TestLLM only supports TextContent, got {type(item).__name__}" | |
| ) | |
| return "\n".join(parts) |
| >>> llm.completion([...]) # returns "OK" | ||
| >>> llm.completion([...]) # raises LLMContextWindowExceedError | ||
| """ | ||
|
|
There was a problem hiding this comment.
🟠 Important: No unit tests for TestLLM itself. A testing utility should have its own tests to ensure:
_create_model_responseproduces valid LiteLLM responses_content_to_stringhandles edge cases correctlyTestLLMExhaustedErroris raised when responses are exhausted- Exception responses in the queue are raised correctly
remaining_responsesandcall_countproperties work
Without these tests, bugs in TestLLM could cause confusing test failures.
| tools=tools, | ||
| _return_metrics=_return_metrics, | ||
| add_security_risk_prediction=add_security_risk_prediction, | ||
| on_token=on_token, |
There was a problem hiding this comment.
🟡 Suggestion: uses_responses_api() always returning False means TestLLM cannot be used to test code paths that specifically use the responses API.
This is a significant limitation that should be documented in the class docstring, or consider adding a parameter to control this behavior:
def __init__(self, *, use_responses_api: bool = False, **data: Any) -> None:
self._use_responses_api = use_responses_api
# ...
def uses_responses_api(self) -> bool:
return self._use_responses_api| prompt_tokens=0, | ||
| completion_tokens=0, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🟡 Suggestion: _create_model_response creates a minimal ModelResponse that might not match real LLM behavior. Real LLMs may include additional fields that downstream code depends on (e.g., usage stats, model-specific metadata).
Consider adding integration tests that verify TestLLM produces responses processed identically to real LLM responses by the Agent/Conversation machinery. This would catch cases where the minimal response causes different behavior than production.
| class SignalingExecutor( | ||
| ToolExecutor[StatusTransitionMockAction, StatusTransitionMockObservation] | ||
| ): | ||
| """Executor that signals when execution starts and captures status.""" |
There was a problem hiding this comment.
🟡 Suggestion: SignalingExecutor is defined inline here, but StatusCheckingExecutor is still referenced in the module (imported but not shown in diff). Consider consolidating these test executors:
- Move both to
conftest.pyas reusable fixtures, or - Use
SignalingExecutorconsistently throughout the file
This follows the repo guideline: "If you find yourself duplicating logics in preparing mocks, loading data etc, these logic should be fixtures in conftest.py!"
enyst
added
behavior-initiative
5 participants
Summary
This PR introduces a new
TestLLMclass inopenhands.sdk.testingthat provides a clean way to write tests without needing to mock LiteLLM internals.Key Features
Agent(llm=...), infallback_llms, incondensers, inrouters, etc.TestLLMas thellm=argumentMessage,TextContent,MessageToolCall) instead of LiteLLM typesTestLLMExhaustedErrorwith a helpful messageuses_responses_apireturnsFalse)Example Usage
What This Replaces
Today a typical test looks like ~20 lines of setup (
@patchdecorator + mock response factory +side_effectlist). WithTestLLM.from_messages(...)it's ~3 lines. The maintenance burden drops because tests speak in SDK types instead of LiteLLM types.This PR also refactors
tests/sdk/conversation/local/test_agent_status_transition.pyto demonstrate the newTestLLMusage.Closes #2005
Checklist
@VascoSch92 can click here to continue refining the PR
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:9895a77-pythonRun
All tags pushed for this build
About Multi-Architecture Support
9895a77-python) is a multi-arch manifest supporting both amd64 and arm649895a77-python-amd64) are also available if needed