-
Notifications
You must be signed in to change notification settings - Fork 6
test(reasoning): Add tests for Reasoning base class #21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test(reasoning): Add tests for Reasoning base class #21
Conversation
WalkthroughAdds a new test in tests/test_reasoning/test_reasoning.py to validate Reasoning.execute_tool_call behavior using a minimal ConcreteReasoning subclass defined within the test. Updates imports to include Reasoning. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21 +/- ##
==========================================
+ Coverage 85.81% 86.30% +0.48%
==========================================
Files 17 17
Lines 1234 1234
==========================================
+ Hits 1059 1065 +6
+ Misses 175 169 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_reasoning/test_reasoning.py (1)
68-90
: Test logic is sound; consider additional edge case coverage.The test correctly validates the happy path for
execute_tool_call
:
- Mock interactions are properly verified
- Plan object structure is correctly asserted
- Test isolation is maintained
However, consider adding tests for:
- Error handling (e.g., LLM generation failure)
- Edge cases for
selected_tools
(None, empty list, multiple tools)- Behavior when
tool_manager.get_all_tools_schema
returns empty/malformed dataExample test for error handling:
def test_execute_tool_call_handles_llm_failure(self): """Test that execute_tool_call handles LLM generation failures gracefully.""" mock_agent = Mock() mock_agent.model.steps = 5 mock_agent.llm.generate.side_effect = Exception("LLM failure") mock_agent.tool_manager.get_all_tools_schema.return_value = [{"schema": "example"}] class ConcreteReasoning(Reasoning): def plan(self, prompt, obs=None, ttl=1, selected_tools=None): pass reasoning = ConcreteReasoning(agent=mock_agent) # Assert appropriate error handling or exception propagation with pytest.raises(Exception, match="LLM failure"): reasoning.execute_tool_call("Execute the plan.", selected_tools=["tool1"])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_reasoning/test_reasoning.py
(2 hunks)
🔇 Additional comments (3)
tests/test_reasoning/test_reasoning.py (3)
6-6
: LGTM!The import addition is necessary for testing the base
Reasoning
class.
64-66
: LGTM!The minimal
ConcreteReasoning
subclass appropriately satisfies the abstract base class requirement without introducing unnecessary test complexity.
53-56
: No changes needed to LLM response mock
Theexecute_tool_call
method readsrsp.choices[0].message
directly, so the test’s mock setup correctly matches the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noticing that execute_tool_call
hadn’t been covered in the tests, the code looks good to me!
Description
This Pull Request addresses a gap in test coverage for the abstract
Reasoning
base class, located inmesa_llm/reasoning/reasoning.py
. By increasing the test coverage for this foundational module, we improve the overall robustness of the library and ensure that core methods shared by all reasoning strategies (likeCoT
,ReAct
, etc.) are reliable.Key Changes
TestReasoningBase
, totests/test_reasoning/test_reasoning.py
.test_execute_tool_call_generates_plan
, verifies the logic of theexecute_tool_call
method on the base class.Reasoning
class, a minimalConcreteReasoning
subclass was created within the test scope. The agent, itsllm.generate
method, andtool_manager.get_all_tools_schema
were mocked to isolate the function's behavior and assert its correctness.Impact: Before and After
mesa_llm/reasoning/reasoning.py
was 63%.This is a significant improvement that makes the core reasoning logic more resilient to future changes.
How to Verify
pip install -e ".[dev]"
.mesa_llm/reasoning/reasoning.py
to 78%.Summary by CodeRabbit