Python: Improve test coverage #10681
Conversation
markwallace-microsoft
added
the
python
github-actions
bot
changed the title
Python Test Coverage Report •
Python Unit Test Overview
|
| assert "lr=lang_en" in query_str | ||
|
|
||
|
|
||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
Not needed, we have default mark for this
|
|
||
| a = channel.messages | ||
|
|
||
| print(a) |
There was a problem hiding this comment.
| print(a) |
| mock_agent.agent_model.foundation_model = "mock-foundation-model" | ||
|
|
||
| # Mock create_session_id to return a fixed session id | ||
| mock_agent.create_session_id.return_value = "test-session-id" |
There was a problem hiding this comment.
nit: why is this needed?
There was a problem hiding this comment.
just removed this :)
| assert "No chat history available" in str(exc_info.value) | ||
|
|
||
|
|
||
| @patch("semantic_kernel.agents.bedrock.bedrock_agent.BedrockAgent.create_session_id", return_value="session-id") |
There was a problem hiding this comment.
nit: probably don't need to mock this
There was a problem hiding this comment.
removed this one as well :)
| assert "No chat history available." in str(exc_info.value) | ||
|
|
||
|
|
||
| @patch("semantic_kernel.agents.bedrock.bedrock_agent.BedrockAgent.create_session_id", return_value="session-id") |
| """Test get_history yields messages in reverse order.""" | ||
| channel, _ = channel_and_agent | ||
|
|
||
| channel.messages.append(ChatMessageContent(role=AuthorRole.USER, content="User1")) |
There was a problem hiding this comment.
The chat_history fixture can be used here
| def test_format_system_message() -> None: | ||
| """Test that system message is formatted correctly.""" | ||
| content = ChatMessageContent(role=AuthorRole.SYSTEM, content="System message") | ||
| formatted = _format_system_message(content) |
There was a problem hiding this comment.
We probably should not test any private method.
There was a problem hiding this comment.
This is the main idea: https://stackoverflow.com/a/250719. We generally make sure these methods behave correctly via integration tests.
There was a problem hiding this comment.
Updating this to test through the public MESSAGE_CONVERTERS as in other unit tests.
| @@ -372,3 +385,365 @@ async def test_with_different_execution_settings_stream( | |||
| continue | |||
| assert mock_mistral_ai_client_completion_stream.chat.stream_async.call_args.kwargs["temperature"] == 0.2 | |||
| assert mock_mistral_ai_client_completion_stream.chat.stream_async.call_args.kwargs["seed"] == 2 | |||
|
|
|||
|
|
|||
| async def test_mistral_ai_chat_completion_initialization_valid(): | |||
There was a problem hiding this comment.
We can set the environment variables in a fixture so that the setting object can be instantiated. Please refer to the sample here: https://github.com/microsoft/semantic-kernel/blob/main/python/tests/unit/agents/bedrock_agent/conftest.py#L14
There was a problem hiding this comment.
I believe the goal of this test would be to actually validate both the settings creation and mistral base are being called. Which is why they are being patched, I am happy to update otherwise though.
There was a problem hiding this comment.
Removing this one actually, this seems to be redundant with other test.
| assert "The MistralAI chat model ID is required." in str(exc_info.value) | ||
|
|
||
|
|
||
| async def test_mistral_ai_chat_completion_initialization_validation_error(): |
There was a problem hiding this comment.
| assert "Failed to create MistralAI settings." in str(exc_info.value) | ||
|
|
||
|
|
||
| async def test_mistral_ai_chat_completion_inner_get_chat_message_contents_success(): |
There was a problem hiding this comment.
Again, we shouldn't be testing the _inner_xxx method in unit tests.
There was a problem hiding this comment.
Update those to use the proper public methods.
| async def test_bing_search_init_success(): | ||
| """Test that BingSearch initializes successfully with valid parameters.""" | ||
| # Arrange/Act | ||
| with patch.object(BingSettings, "create", return_value=MagicMock(spec=BingSettings)) as mock_settings_create: |
There was a problem hiding this comment.
Use a fixture for the environment variable to instantiate the settings.
There was a problem hiding this comment.
Indeed, we have bing_unit_test_env in the base level conftest
There was a problem hiding this comment.
Tests updated to use bing_unit_test_env :). Thank you
| mock_channel.messages.append(ChatMessageContent(role=AuthorRole.USER, content="User message")) | ||
|
|
||
| # Create a dummy agent that is not BedrockAgent | ||
| non_bedrock_agent = Agent() |
There was a problem hiding this comment.
This will now fail because in the last few days we updated the Agent to be an ABC.
There was a problem hiding this comment.
Tests have been updated accordingly :)
markwallace-microsoft
added
.NET
github-actions
bot
changed the title
rracanicci
force-pushed
the
feature/improve-test-coverage
branch
from
50cd0c6 to
4fb3101
Compare
markwallace-microsoft
removed
.NET
rracanicci
changed the title
rracanicci
force-pushed
the
feature/improve-test-coverage
branch
2 times, most recently
from
19bb678 to
a815b23
Compare
rracanicci
force-pushed
the
feature/improve-test-coverage
branch
from
4476f53 to
8f62a96
Compare
Motivation and Context
Adding more tests to improve overall coverage.
Description
Contribution Checklist