LCORE-740: type hints for endpoint handlers #752

tisnik · 2025-11-03T07:13:46Z

Description

LCORE-740: type hints for endpoint handlers

Type of change

Related Tickets & Documents

Related Issue #LCORE-740

Summary by CodeRabbit

Tests
- Enhanced type safety across test suite with comprehensive type annotations for fixtures, mocks, and test functions in endpoint tests.

coderabbitai · 2025-11-03T07:14:05Z

Walkthrough

This PR adds comprehensive type hints across eight endpoint test files. Changes include annotating test functions with MockerFixture, explicit return types (-> None), typed fixture return values, AuthTuple for authorization variables, and normalizing exception detail assertions to treat detail as a dict. No production logic modifications.

Changes

Cohort / File(s)	Summary
Endpoint test typing enhancements `tests/unit/app/endpoints/test_conversations.py`, `test_conversations_v2.py`, `test_metrics.py`, `test_models.py`, `test_providers.py`, `test_shields.py`, `test_streaming_query.py`, `test_tools.py`	Added explicit type hints throughout: `MockerFixture` parameter annotations on test methods, explicit `-> None` return types, typed fixture returns (`AppConfig`, `MockType`, `UserConversation`, `dict[str, Any]`, `list[dict[str, Any]]`), `AuthTuple` for authorization variables, `dict[str, Any]` for config dictionaries, and updated exception detail assertions to treat detail as dict. Imports added for `MockerFixture`, `MockType`, `AuthTuple`, and typing utilities. No behavioral changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Multiple files affected (8 test modules) with consistent but wide-spanning changes
Pattern is homogeneous and repetitive across files, reducing individual reasoning overhead
Requires verification that type annotations are consistently applied and assertions properly updated across all fixtures and test methods
No logic changes to verify, but breadth of files warrants careful verification of completeness

Possibly related PRs

#691: Applies identical MockerFixture type-hint patterns across pytest-mock usage in tests
#730: Normalizes AuthTuple typing and test function signatures in endpoint tests
#659: Widespread type-annotation and explicit return-type additions across unit test files

Suggested reviewers

manstis
umago

Poem

🐰 Hops with glee through types so clear,
MockerFixture and AuthTuple here!
Fixtures typed from end to start,
None returns do their part,
Test code blooms with hints so bright,
Every parameter feels just right! ✨

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "LCORE-740: type hints for endpoint handlers" is related to the changeset but lacks specificity about scope. All changes are confined to test files within `tests/unit/app/endpoints/`, where type hints are being added to test fixtures, helper functions, and test methods. While the title correctly references "endpoint handlers," a developer scanning commit history might reasonably expect modifications to production endpoint handler code itself, when the actual changes are exclusively test-related improvements. The PR description clarifies this is a "Unit tests improvement," but the title alone doesn't convey that the scope is limited to the test suite.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)

tests/unit/app/endpoints/test_metrics.py (1)
17-19: Awaited function must be patched with AsyncMock.

setup_model_metrics is awaited; patching with a regular Mock will raise “object MagicMock can’t be used in ‘await’ expression”. Use AsyncMock.
-    mock_setup_metrics = mocker.patch(
-        "app.endpoints.metrics.setup_model_metrics", return_value=None
-    )
+    mock_setup_metrics = mocker.patch(
+        "app.endpoints.metrics.setup_model_metrics",
+        new=mocker.AsyncMock(return_value=None),
+    )
tests/unit/app/endpoints/test_streaming_query.py (3)
236-309: Async iterator mock should return an iterator, not a list.

Setting aiter.return_value to a plain list can break async for. Return an iterator (or define an async generator).
-    mock_streaming_response.__aiter__.return_value = [
+    mock_streaming_response.__aiter__.return_value = iter([
         AgentTurnResponseStreamChunk(...),
         AgentTurnResponseStreamChunk(...),
         AgentTurnResponseStreamChunk(...),
         AgentTurnResponseStreamChunk(...),
-    ]
+    ])
Alternatively:
async def _agen(items):
    for i in items:
        yield i
mock_streaming_response.__aiter__.return_value = _agen([...])
351-359: Handle StreamingResponse chunks as bytes.

body_iterator yields bytes; joining with "" will fail. Join bytes and decode.
-    streaming_content = []
+    streaming_content: list[bytes] = []
@@
-    full_content = "".join(streaming_content)  # type: ignore
+    full_content = b"".join(streaming_content).decode()
369-371: Decode before json.loads.

streaming_content elements are bytes; slice then decode.
-    d = json.loads(streaming_content[6][5:])
+    d = json.loads(streaming_content[6][5:].decode())

🧹 Nitpick comments (5)

tests/unit/app/endpoints/test_tools.py (2)

50-103: Use plain dicts for tool fixtures; Mock mapping dunders are brittle.

dict(mock) is not guaranteed to work with instance‑level dunders on Mock. Returning real dicts is simpler and robust.

-@pytest.fixture
-def mock_tools_response(mocker: MockerFixture) -> list[MockType]:
+@pytest.fixture
+def mock_tools_response(mocker: MockerFixture) -> list[dict]:
     """Create mock tools response from LlamaStack client."""
-    # Create mock tools that behave like dict when converted
-    tool1 = mocker.Mock()
-    tool1.__dict__.update(
-        {
-            "identifier": "filesystem_read",
-            "description": "Read contents of a file from the filesystem",
-            "parameters": [
-                {"name": "path", "description": "Path to the file to read", "parameter_type": "string", "required": True, "default": None}
-            ],
-            "provider_id": "model-context-protocol",
-            "toolgroup_id": "filesystem-tools",
-            "type": "tool",
-            "metadata": {},
-        }
-    )
-    # Make dict() work on the mock
-    tool1.keys.return_value = tool1.__dict__.keys()
-    tool1.__getitem__ = lambda self, key: self.__dict__[key]
-    tool1.__iter__ = lambda self: iter(self.__dict__)
-
-    tool2 = mocker.Mock()
-    tool2.__dict__.update(
-        {
-            "identifier": "git_status",
-            "description": "Get the status of a git repository",
-            "parameters": [
-                {"name": "repository_path", "description": "Path to the git repository", "parameter_type": "string", "required": True, "default": None}
-            ],
-            "provider_id": "model-context-protocol",
-            "toolgroup_id": "git-tools",
-            "type": "tool",
-            "metadata": {},
-        }
-    )
-    # Make dict() work on the mock
-    tool2.keys.return_value = tool2.__dict__.keys()
-    tool2.__getitem__ = lambda self, key: self.__dict__[key]
-    tool2.__iter__ = lambda self: iter(self.__dict__)
-
-    return [tool1, tool2]
+    tool1 = {
+        "identifier": "filesystem_read",
+        "description": "Read contents of a file from the filesystem",
+        "parameters": [
+            {"name": "path", "description": "Path to the file to read", "parameter_type": "string", "required": True, "default": None}
+        ],
+        "provider_id": "model-context-protocol",
+        "toolgroup_id": "filesystem-tools",
+        "type": "tool",
+        "metadata": {},
+    }
+    tool2 = {
+        "identifier": "git_status",
+        "description": "Get the status of a git repository",
+        "parameters": [
+            {"name": "repository_path", "description": "Path to the git repository", "parameter_type": "string", "required": True, "default": None}
+        ],
+        "provider_id": "model-context-protocol",
+        "toolgroup_id": "git-tools",
+        "type": "tool",
+        "metadata": {},
+    }
+    return [tool1, tool2]

116-121: Minor typos in comments.

Fix “bypass i”, “clien”, and “endpointt” for readability.

Also applies to: 236-238

tests/unit/app/endpoints/test_models.py (2)

25-30: Redundant configuration patching.

Patching configuration to a Mock and then immediately to None is unnecessary; keep only the None patch.

180-181: Prefer patching in the module under test.

For reliability, patch AsyncLlamaStackClientHolder on app.endpoints.models rather than client.
-    mock_lsc = mocker.patch("client.AsyncLlamaStackClientHolder.get_client")
+    mock_lsc = mocker.patch("app.endpoints.models.AsyncLlamaStackClientHolder.get_client")
Also applies to: 231-233

tests/unit/app/endpoints/test_shields.py (1)

25-30: Remove redundant configuration patch.

Patching configuration to a Mock and then to None can be reduced to a single None patch.

Also applies to: 81-85

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6e2c199 and d62afc2.

📒 Files selected for processing (8)

tests/unit/app/endpoints/test_conversations.py (35 hunks)
tests/unit/app/endpoints/test_conversations_v2.py (9 hunks)
tests/unit/app/endpoints/test_metrics.py (2 hunks)
tests/unit/app/endpoints/test_models.py (9 hunks)
tests/unit/app/endpoints/test_providers.py (7 hunks)
tests/unit/app/endpoints/test_shields.py (13 hunks)
tests/unit/app/endpoints/test_streaming_query.py (51 hunks)
tests/unit/app/endpoints/test_tools.py (14 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/app/endpoints/test_models.py
tests/unit/app/endpoints/test_tools.py
tests/unit/app/endpoints/test_shields.py
tests/unit/app/endpoints/test_conversations_v2.py
tests/unit/app/endpoints/test_metrics.py
tests/unit/app/endpoints/test_streaming_query.py
tests/unit/app/endpoints/test_providers.py
tests/unit/app/endpoints/test_conversations.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/app/endpoints/test_models.py
tests/unit/app/endpoints/test_tools.py
tests/unit/app/endpoints/test_shields.py
tests/unit/app/endpoints/test_conversations_v2.py
tests/unit/app/endpoints/test_metrics.py
tests/unit/app/endpoints/test_streaming_query.py
tests/unit/app/endpoints/test_providers.py
tests/unit/app/endpoints/test_conversations.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/app/endpoints/test_models.py
tests/unit/app/endpoints/test_tools.py
tests/unit/app/endpoints/test_shields.py
tests/unit/app/endpoints/test_conversations_v2.py
tests/unit/app/endpoints/test_metrics.py
tests/unit/app/endpoints/test_streaming_query.py
tests/unit/app/endpoints/test_providers.py
tests/unit/app/endpoints/test_conversations.py

🧠 Learnings (7)

📓 Common learnings

Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to **/*.py : Provide complete type annotations for all function parameters and return types

Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to src/{models/**/*.py,configuration.py} : Use precise type hints in configuration (e.g., Optional[FilePath], PositiveInt, SecretStr)

Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to **/*.py : Provide complete type annotations for all class attributes