Conversation
Co-authored-by: frostmute <989225+frostmute@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Code Review
This pull request introduces unit tests for the on_unresolved_tool_cli function in tests/test_cli.py. The feedback suggests refactoring the individual test cases into a single parameterized test to reduce code duplication and improving test coverage by verifying the console output and input prompts.
| def test_on_unresolved_tool_cli_custom_input(): | ||
| with patch('builtins.input', return_value="Use my custom tool"): | ||
| result = on_unresolved_tool_cli("unknown_tool", "Use default tool") | ||
| assert result == "Use my custom tool" | ||
|
|
||
| def test_on_unresolved_tool_cli_empty_input(): | ||
| with patch('builtins.input', return_value=""): | ||
| result = on_unresolved_tool_cli("unknown_tool", "Use default tool") | ||
| assert result == "Use default tool" | ||
|
|
||
| def test_on_unresolved_tool_cli_whitespace_input(): | ||
| with patch('builtins.input', return_value=" Use my custom tool "): | ||
| result = on_unresolved_tool_cli("unknown_tool", "Use default tool") | ||
| assert result == "Use my custom tool" | ||
|
|
||
| def test_on_unresolved_tool_cli_only_whitespace_input(): | ||
| with patch('builtins.input', return_value=" "): | ||
| result = on_unresolved_tool_cli("unknown_tool", "Use default tool") | ||
| assert result == "Use default tool" |
There was a problem hiding this comment.
The current test suite for on_unresolved_tool_cli contains significant code duplication. Refactoring these into a single parameterized test using @pytest.mark.parametrize improves maintainability and leverages the already imported pytest module. Additionally, the tests should verify the console output and the prompt message to ensure the CLI interaction is fully covered, rather than just the return value.
| def test_on_unresolved_tool_cli_custom_input(): | |
| with patch('builtins.input', return_value="Use my custom tool"): | |
| result = on_unresolved_tool_cli("unknown_tool", "Use default tool") | |
| assert result == "Use my custom tool" | |
| def test_on_unresolved_tool_cli_empty_input(): | |
| with patch('builtins.input', return_value=""): | |
| result = on_unresolved_tool_cli("unknown_tool", "Use default tool") | |
| assert result == "Use default tool" | |
| def test_on_unresolved_tool_cli_whitespace_input(): | |
| with patch('builtins.input', return_value=" Use my custom tool "): | |
| result = on_unresolved_tool_cli("unknown_tool", "Use default tool") | |
| assert result == "Use my custom tool" | |
| def test_on_unresolved_tool_cli_only_whitespace_input(): | |
| with patch('builtins.input', return_value=" "): | |
| result = on_unresolved_tool_cli("unknown_tool", "Use default tool") | |
| assert result == "Use default tool" | |
| @pytest.mark.parametrize("user_input, expected_result", [ | |
| ("Use my custom tool", "Use my custom tool"), | |
| ("", "Use default tool"), | |
| (" Use my custom tool ", "Use my custom tool"), | |
| (" ", "Use default tool"), | |
| ]) | |
| def test_on_unresolved_tool_cli(user_input, expected_result, capsys): | |
| with patch('builtins.input', return_value=user_input) as mocked_input: | |
| result = on_unresolved_tool_cli("unknown_tool", "Use default tool") | |
| assert result == expected_result | |
| # Verify the prompt and printed output | |
| mocked_input.assert_called_once_with("Enter custom instruction (or press Enter to use default): ") | |
| captured = capsys.readouterr() | |
| assert "Unresolved tool mapping found: 'unknown_tool'" in captured.out | |
| assert "Default instruction: Use default tool" in captured.out |
🎯 What: Adds unit tests for the
on_unresolved_tool_clifunction inclaw2manus/cli.pywhich was previously untested.📊 Coverage: Covers custom inputs, empty inputs (fallback to default instruction), and whitespace stripping scenarios.
✨ Result: Increased test coverage and confidence in CLI tool mapping behavior.
PR created automatically by Jules for task 17637958629206717964 started by @frostmute