LCORE-739: type hint for mocker fixture #691
Conversation
|
Warning Rate limit exceeded@tisnik has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (15)
WalkthroughAdds static type annotations for the pytest-mock fixture by importing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Large, homogeneous changes (same annotation pattern) across many files; low logic density and no behavioral changes. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/unit/app/endpoints/test_feedback.py (3)
69-69: Add missing type hint for consistency.The
mockerparameter is missing theMockerFixturetype annotation, which is inconsistent with the PR objective and other test functions in this file.Apply this diff to add the type hint:
-async def test_feedback_endpoint_handler(mocker, feedback_request_data): +async def test_feedback_endpoint_handler( + mocker: MockerFixture, feedback_request_data +):
140-140: Add missing type hint for consistency.The
mockerparameter is missing theMockerFixturetype annotation, which is inconsistent with the PR objective and other test functions in this file.Apply this diff to add the type hint:
-def test_store_feedback(mocker, feedback_request_data): +def test_store_feedback(mocker: MockerFixture, feedback_request_data):
186-186: Add missing type hint for consistency.The
mockerparameter is missing theMockerFixturetype annotation, which is inconsistent with the PR objective and other test functions in this file.Apply this diff to add the type hint:
-def test_store_feedback_on_io_error(mocker, feedback_request_data): +def test_store_feedback_on_io_error(mocker: MockerFixture, feedback_request_data):tests/unit/app/endpoints/test_conversations_v2.py (2)
95-95: Add missing type hint for consistency.The
mockerparameter is missing theMockerFixturetype annotation, which is inconsistent with the PR objective and other test methods in this file.Apply this diff to add the type hint:
- def test_conversation_exists(self, mocker, mock_configuration): + def test_conversation_exists( + self, mocker: MockerFixture, mock_configuration + ):
105-105: Add missing type hint for consistency.The
mockerparameter is missing theMockerFixturetype annotation, which is inconsistent with the PR objective and other test methods in this file.Apply this diff to add the type hint:
- def test_conversation_not_exists(self, mocker, mock_configuration): + def test_conversation_not_exists( + self, mocker: MockerFixture, mock_configuration + ):
🧹 Nitpick comments (1)
tests/unit/utils/auth_helpers.py (1)
3-3: Remove unused import.The
Anyimport fromtypingis no longer used after replacing the generic type withMockerFixture.Apply this diff to remove the unused import:
-from typing import Any from unittest.mock import AsyncMock, Mock from pytest_mock import MockerFixture
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
tests/unit/app/endpoints/test_config.py(3 hunks)tests/unit/app/endpoints/test_conversations.py(27 hunks)tests/unit/app/endpoints/test_conversations_v2.py(7 hunks)tests/unit/app/endpoints/test_feedback.py(6 hunks)tests/unit/app/endpoints/test_health.py(6 hunks)tests/unit/utils/auth_helpers.py(1 hunks)tests/unit/utils/test_common.py(7 hunks)tests/unit/utils/test_transcripts.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/unit/utils/auth_helpers.pytests/unit/utils/test_common.pytests/unit/utils/test_transcripts.pytests/unit/app/endpoints/test_feedback.pytests/unit/app/endpoints/test_health.pytests/unit/app/endpoints/test_config.pytests/unit/app/endpoints/test_conversations.pytests/unit/app/endpoints/test_conversations_v2.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/utils/auth_helpers.pytests/unit/utils/test_common.pytests/unit/utils/test_transcripts.pytests/unit/app/endpoints/test_feedback.pytests/unit/app/endpoints/test_health.pytests/unit/app/endpoints/test_config.pytests/unit/app/endpoints/test_conversations.pytests/unit/app/endpoints/test_conversations_v2.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/unit/utils/auth_helpers.pytests/unit/utils/test_common.pytests/unit/utils/test_transcripts.pytests/unit/app/endpoints/test_feedback.pytests/unit/app/endpoints/test_health.pytests/unit/app/endpoints/test_config.pytests/unit/app/endpoints/test_conversations.pytests/unit/app/endpoints/test_conversations_v2.py
🧠 Learnings (1)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to tests/**/*.py : Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Applied to files:
tests/unit/utils/auth_helpers.py
🧬 Code graph analysis (3)
tests/unit/utils/auth_helpers.py (1)
src/models/config.py (2)
config(140-146)Action(329-375)
tests/unit/app/endpoints/test_conversations.py (2)
tests/unit/app/endpoints/test_conversations_v2.py (1)
test_configuration_not_loaded(121-135)tests/unit/test_configuration.py (1)
test_configuration_not_loaded(376-380)
tests/unit/app/endpoints/test_conversations_v2.py (2)
src/app/endpoints/conversations_v2.py (1)
check_valid_conversation_id(284-294)tests/unit/app/endpoints/test_conversations.py (5)
test_configuration_not_loaded(270-283)test_configuration_not_loaded(539-552)test_configuration_not_loaded(810-821)test_invalid_conversation_id_format(286-303)test_invalid_conversation_id_format(555-572)
🔇 Additional comments (8)
tests/unit/utils/test_transcripts.py (1)
4-5: LGTM! Type annotations applied correctly.The MockerFixture type hints are properly applied to all test functions using the mocker fixture in this file.
Also applies to: 16-16, 55-55
tests/unit/app/endpoints/test_health.py (1)
6-6: LGTM! Type annotations applied correctly.The MockerFixture type hints are properly applied to all test functions and methods using the mocker fixture.
Also applies to: 18-18, 47-49, 84-84, 117-117, 168-170
tests/unit/utils/auth_helpers.py (1)
10-10: LGTM! Type annotation upgraded correctly.The change from a generic
Anytype to the specificMockerFixturetype improves type safety.tests/unit/app/endpoints/test_config.py (1)
4-4: LGTM! Type annotations applied correctly.The MockerFixture type hints are properly applied to all test functions using the mocker fixture.
Also applies to: 13-13, 38-38
tests/unit/app/endpoints/test_feedback.py (1)
31-31: LGTM! Type annotations applied correctly.These test functions have the MockerFixture type hint properly applied.
Also applies to: 44-44, 94-94, 201-201, 218-218
tests/unit/utils/test_common.py (1)
4-4: LGTM! Type annotations applied correctly.The MockerFixture type hints are properly applied to all test functions using the mocker fixture, including appropriate multi-line formatting where needed.
Also applies to: 22-22, 53-55, 100-102, 140-142, 204-204, 245-247
tests/unit/app/endpoints/test_conversations_v2.py (1)
75-75: LGTM! Type annotations applied correctly.These test methods have the MockerFixture type hint properly applied.
Also applies to: 81-81, 121-121, 138-140, 159-159, 182-184, 204-204
tests/unit/app/endpoints/test_conversations.py (1)
7-7: LGTM! Type annotations applied correctly.The MockerFixture type hints are properly applied throughout this file to all helper functions and test methods using the mocker fixture, with appropriate multi-line formatting where needed.
Also applies to: 46-46, 69-69, 270-270, 286-288, 306-308, 336-338, 369-371, 401-407, 449-456, 492-499, 539-539, 555-557, 575-577, 604-606, 637-639, 672-678, 720-726, 767-769, 810-810, 824-826, 884-886, 903-905, 923-925, 957-959, 1022-1024, 1056-1058
tisnik
force-pushed
the
lcore-739-mocker-type
branch
2 times, most recently
from
edfbb37 to
76a2f33
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/utils/test_llama_stack_version.py (2)
80-80: Missing type annotation for mocker parameter.The function
test_check_llama_stack_version_too_big_versionis missing theMockerFixturetype annotation on itsmockerparameter, while all other test functions in this file have been annotated. For consistency, please add the type annotation.Apply this diff:
-async def test_check_llama_stack_version_too_big_version(mocker, subtests): +async def test_check_llama_stack_version_too_big_version(mocker: MockerFixture, subtests):
1-102: Fix Black formatting.The pipeline reports a Black formatting check failure for this file. Please run
black tests/unit/utils/test_llama_stack_version.pyto automatically fix the formatting issues.
🧹 Nitpick comments (1)
tests/unit/utils/test_endpoints.py (1)
1-1006: Note: File length exceeds pylint limit.The pipeline reports this file has 1006 lines, exceeding the 1000-line limit. While this is a pre-existing issue and not introduced by this PR, consider splitting this test file into smaller, more focused modules in a future refactor to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/unit/metrics/test_utis.py(2 hunks)tests/unit/utils/auth_helpers.py(1 hunks)tests/unit/utils/test_common.py(7 hunks)tests/unit/utils/test_endpoints.py(6 hunks)tests/unit/utils/test_llama_stack_version.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/utils/auth_helpers.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/unit/utils/test_endpoints.pytests/unit/utils/test_llama_stack_version.pytests/unit/utils/test_common.pytests/unit/metrics/test_utis.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/utils/test_endpoints.pytests/unit/utils/test_llama_stack_version.pytests/unit/utils/test_common.pytests/unit/metrics/test_utis.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/unit/utils/test_endpoints.pytests/unit/utils/test_llama_stack_version.pytests/unit/utils/test_common.pytests/unit/metrics/test_utis.py
🧠 Learnings (1)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to tests/**/*.py : Use pytest-mock to create AsyncMock objects for async interactions in tests
Applied to files:
tests/unit/utils/test_common.py
🧬 Code graph analysis (1)
tests/unit/metrics/test_utis.py (1)
src/metrics/utils.py (2)
setup_model_metrics(20-57)update_llm_token_count_from_turn(60-77)
🪛 GitHub Actions: Black
tests/unit/utils/test_llama_stack_version.py
[error] 1-1: Black formatting check failed: 1 file would be reformatted. Run 'black' to fix formatting. Command: 'uv tool run black --check .'
🪛 GitHub Actions: Python linter
tests/unit/utils/test_endpoints.py
[error] 1-1006: Pylint C0302: Too many lines in module (1006/1000). Total lines exceed pylint's limit. Command failed: 'uv run pylint src tests'. Exit code: 16.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (3)
tests/unit/utils/test_endpoints.py (1)
5-5: LGTM! Correct import for type annotation.The
MockerFixtureimport is appropriate for typing the mocker fixture parameters.tests/unit/metrics/test_utis.py (1)
3-3: LGTM! Complete and consistent type annotations.All test functions using the
mockerfixture are properly annotated withMockerFixture. This aligns well with the PR objective.Also applies to: 7-7, 80-80
tests/unit/utils/test_common.py (1)
5-5: LGTM! Complete and consistent type annotations.All test functions using the
mockerfixture are properly annotated withMockerFixture. The annotations are consistently applied throughout the file, aligning well with the PR objective.Also applies to: 22-22, 53-55, 100-102, 140-142, 204-204, 245-247
tisnik
force-pushed
the
lcore-739-mocker-type
branch
from
76a2f33 to
c1dacfe
Compare
More refactoring
tisnik
force-pushed
the
lcore-739-mocker-type
branch
2 times, most recently
from
7d1ac73 to
07d3cb1
Compare
tisnik
force-pushed
the
lcore-739-mocker-type
branch
from
07d3cb1 to
bfc6787
Compare
1 participant
Description
LCORE-739: type hint for mocker fixture
Type of change
Related Tickets & Documents
Summary by CodeRabbit