test: Add unit tests for utils/client_info module #379
Conversation
Add comprehensive unit tests for the client_info utility module: - TestGetFriendlyName: client name to friendly name mapping - TestFormatClientInfo: client info formatting - TestGetClientInfoFromContext: context extraction with edge cases - TestGetCachedClientInfo: caching behavior - TestGetClientFriendlyName: convenience function - TestLogClientInfo: logging functionality This module previously had no test coverage.
Summary of ChangesHello @kou-123, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the test coverage for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of unit tests for the utils/client_info.py module, which is a great addition for improving code quality and reliability. The tests cover many edge cases and scenarios. My review includes suggestions to further improve the tests by using pytest.mark.parametrize for better test reporting, refactoring duplicated and brittle test setup code into a more robust pytest fixture, and adding a missing test case to ensure full coverage of the logic.
tests/test_client_info.py
Outdated
| def setup_method(self): | ||
| """Reset the cache before each test.""" | ||
| import utils.client_info as client_info_module | ||
|
|
||
| client_info_module._client_info_cache = None |
There was a problem hiding this comment.
Directly modifying the private _client_info_cache variable from another module makes these tests brittle, as they are tightly coupled to the implementation details of utils.client_info. A more robust approach is to use pytest's monkeypatch fixture to control the module's state.
This setup_method is also duplicated across four test classes. This can be consolidated into a single autouse fixture at the module level to reduce code duplication.
Consider adding this fixture to the top of your file and removing the duplicated setup_method from each class:
@pytest.fixture(autouse=True)
def _reset_cache(monkeypatch):
"""Fixture to reset the client info cache before each test."""
monkeypatch.setattr("utils.client_info._client_info_cache", None)
tests/test_client_info.py
Outdated
| def test_claude_variants(self): | ||
| """Various Claude client names should return 'Claude'.""" | ||
| claude_names = [ | ||
| "claude-ai", | ||
| "Claude-AI", | ||
| "CLAUDE-AI", | ||
| "claude", | ||
| "Claude", | ||
| "claude-desktop", | ||
| "Claude-Desktop", | ||
| "claude-code", | ||
| "anthropic", | ||
| "Anthropic-Client", | ||
| ] | ||
| for name in claude_names: | ||
| assert get_friendly_name(name) == "Claude", f"Failed for: {name}" |
There was a problem hiding this comment.
This test and others like test_gemini_variants and test_unknown_client_returns_default use a for loop over a list of test cases. While this works, it's better to use pytest.mark.parametrize. This decorator turns each item in your list into a separate test case, which provides clearer test reports. If one case fails, pytest will tell you exactly which input caused the failure, rather than failing the entire test method.
| def test_claude_variants(self): | |
| """Various Claude client names should return 'Claude'.""" | |
| claude_names = [ | |
| "claude-ai", | |
| "Claude-AI", | |
| "CLAUDE-AI", | |
| "claude", | |
| "Claude", | |
| "claude-desktop", | |
| "Claude-Desktop", | |
| "claude-code", | |
| "anthropic", | |
| "Anthropic-Client", | |
| ] | |
| for name in claude_names: | |
| assert get_friendly_name(name) == "Claude", f"Failed for: {name}" | |
| @pytest.mark.parametrize( | |
| "name", | |
| [ | |
| "claude-ai", | |
| "Claude-AI", | |
| "CLAUDE-AI", | |
| "claude", | |
| "Claude", | |
| "claude-desktop", | |
| "Claude-Desktop", | |
| "claude-code", | |
| "anthropic", | |
| "Anthropic-Client", | |
| ], | |
| ) | |
| def test_claude_variants(self, name): | |
| """Various Claude client names should return 'Claude'.""" | |
| assert get_friendly_name(name) == "Claude" |
tests/test_client_info.py
Outdated
|
|
||
|
|
||
| class TestGetClientInfoFromContext: | ||
| """Tests for get_client_info_from_context function.""" |
There was a problem hiding this comment.
The test coverage for get_client_info_from_context is good, but it appears to be missing a case. You test for _client_params being missing, but not for the case where _client_params exists but does not have the clientInfo attribute. Adding a test for this scenario would make the test suite more complete.
You could add a test like this to the TestGetClientInfoFromContext class:
def test_client_params_without_client_info(self):
"""Context with _client_params but no clientInfo should return None."""
server = MagicMock()
# Configure mock up to _client_params
server.request_context.session._client_params = MagicMock(spec=[]) # No clientInfo attribute
assert get_client_info_from_context(server) is None
tests/test_client_info.py
Outdated
|
|
||
| def test_caching_behavior(self): | ||
| """Should cache the result and return cached value on subsequent calls.""" | ||
| import utils.client_info as client_info_module |
There was a problem hiding this comment.
The import utils.client_info as client_info_module statement is repeated inside several methods (here and in setup_method, test_extraction_with_missing_version, etc.). Per PEP 8, imports should be placed at the top of the file. This improves code readability and avoids re-importing the same module multiple times. Please move this import to the top-level of the module.
tests/test_client_info.py
Outdated
| """Should handle missing version attribute.""" | ||
| import utils.client_info as client_info_module | ||
|
|
||
| client_info_module._client_info_cache = None |
There was a problem hiding this comment.
The setup_method for this test class already resets the _client_info_cache to None before each test runs. This line is therefore redundant and can be removed. Keeping it might cause confusion about whether the setup method is working as expected.
|
Thanks for the review! I've addressed all the feedback:
|
|
Thanks for the review! I've addressed all the feedback:
|
1 participant
Description
Add unit tests for
utils/client_info.pywhich previously had no test coverage.Changes
tests/test_client_info.pywith comprehensive tests for:get_friendly_name()- client name mappingformat_client_info()- info formattingget_client_info_from_context()- context extractionget_cached_client_info()- cachingget_client_friendly_name()- convenience functionlog_client_info()- loggingType of Change
Checklist