Skip to content

LCORE-740: type hints for test_configuration.py #608

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 1, 2025
Merged

LCORE-740: type hints for test_configuration.py #608

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 19 additions & 16 deletions tests/unit/test_configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
"""Unit tests for functions defined in src/configuration.py."""

from pathlib import Path
from typing import Any, Generator

import pytest
from configuration import AppConfig, LogicError
from models.config import CustomProfile, ModelContextProtocolServer


# pylint: disable=broad-exception-caught,protected-access
@pytest.fixture(autouse=True)
def _reset_app_config_between_tests():
def _reset_app_config_between_tests() -> Generator:
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Complete the Generator type annotation.

The fixture return type should fully specify the Generator type parameters. For a fixture that yields without a value, use Generator[None, None, None] from collections.abc or typing.

Apply this diff:

-def _reset_app_config_between_tests() -> Generator:
+def _reset_app_config_between_tests() -> Generator[None, None, None]:
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def _reset_app_config_between_tests() -> Generator:
def _reset_app_config_between_tests() -> Generator[None, None, None]:
# (fixture body unchanged)
🤖 Prompt for AI Agents 
In tests/unit/test_configuration.py around line 13, the return type for the
fixture is too generic; change the annotation to a fully specified Generator
type for a yield-only fixture by using Generator[None, None, None] and ensure
you import Generator from typing or collections.abc at the top of the file;
update the function signature to use Generator[None, None, None] and add or
adjust the import accordingly.

# ensure clean state before each test
try:
AppConfig()._configuration = None # type: ignore[attr-defined]
Expand Down Expand Up @@ -83,7 +86,7 @@ def test_configuration_is_singleton() -> None:

def test_init_from_dict() -> None:
"""Test the configuration initialization from dictionary with config values."""
config_dict = {
config_dict: dict[str, Any] = {
"name": "foo",
"service": {
"host": "localhost",
Expand Down Expand Up @@ -226,7 +229,7 @@ def test_init_from_dict_with_authorization_configuration() -> None:
assert cfg.authorization_configuration is not None


def test_load_proper_configuration(tmpdir) -> None:
def test_load_proper_configuration(tmpdir: Path) -> None:
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Incorrect type annotation for pytest fixture.

The tmpdir fixture provides py.path.local objects, not pathlib.Path. The type annotation creates a mismatch that will cause type checker failures.

Recommended solution: Use pytest's tmp_path fixture instead, which provides pathlib.Path objects:

-def test_load_proper_configuration(tmpdir: Path) -> None:
+def test_load_proper_configuration(tmp_path: Path) -> None:
     """Test loading proper configuration from YAML file."""
-    cfg_filename = tmpdir / "config.yaml"
+    cfg_filename = tmp_path / "config.yaml"

Apply the same change to all test functions using tmpdir: Path (lines 264, 424, 468, 511, 550).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def test_load_proper_configuration(tmpdir: Path) -> None:
def test_load_proper_configuration(tmp_path: Path) -> None:
"""Test loading proper configuration from YAML file."""
cfg_filename = tmp_path / "config.yaml"
# ...rest of the test...
🤖 Prompt for AI Agents 
In tests/unit/test_configuration.py around line 232, the test function is
annotated with tmpdir: Path but pytest's tmpdir fixture yields py.path.local,
causing a type mismatch; change the parameter to use pytest's tmp_path fixture
(tmp_path: Path) instead and update the function signature accordingly; apply
the same replacement for all other test functions that currently declare tmpdir:
Path at lines 264, 424, 468, 511, and 550 so each uses tmp_path: Path to match
pathlib.Path objects.

"""Test loading proper configuration from YAML file."""
cfg_filename = tmpdir / "config.yaml"
with open(cfg_filename, "w", encoding="utf-8") as fout:
Expand Down Expand Up @@ -258,7 +261,7 @@ def test_load_proper_configuration(tmpdir) -> None:
assert cfg.user_data_collection_configuration is not None


def test_load_configuration_with_mcp_servers(tmpdir) -> None:
def test_load_configuration_with_mcp_servers(tmpdir: Path) -> None:
"""Test loading configuration from YAML file with MCP servers."""
cfg_filename = tmpdir / "config.yaml"
with open(cfg_filename, "w", encoding="utf-8") as fout:
Expand Down Expand Up @@ -301,7 +304,7 @@ def test_load_configuration_with_mcp_servers(tmpdir) -> None:

def test_mcp_servers_property_empty() -> None:
"""Test mcp_servers property returns empty list when no servers configured."""
config_dict = {
config_dict: dict[str, Any] = {
"name": "test",
"service": {
"host": "localhost",
Expand Down Expand Up @@ -369,56 +372,56 @@ def test_mcp_servers_property_with_servers() -> None:
assert servers[0].url == "http://localhost:8080"


def test_configuration_not_loaded():
def test_configuration_not_loaded() -> None:
"""Test that accessing configuration before loading raises an error."""
cfg = AppConfig()
with pytest.raises(LogicError, match="logic error: configuration is not loaded"):
cfg.configuration # pylint: disable=pointless-statement


def test_service_configuration_not_loaded():
def test_service_configuration_not_loaded() -> None:
"""Test that accessing service_configuration before loading raises an error."""
cfg = AppConfig()
with pytest.raises(LogicError, match="logic error: configuration is not loaded"):
cfg.service_configuration # pylint: disable=pointless-statement


def test_llama_stack_configuration_not_loaded():
def test_llama_stack_configuration_not_loaded() -> None:
"""Test that accessing llama_stack_configuration before loading raises an error."""
cfg = AppConfig()
with pytest.raises(LogicError, match="logic error: configuration is not loaded"):
cfg.llama_stack_configuration # pylint: disable=pointless-statement


def test_user_data_collection_configuration_not_loaded():
def test_user_data_collection_configuration_not_loaded() -> None:
"""Test that accessing user_data_collection_configuration before loading raises an error."""
cfg = AppConfig()
with pytest.raises(LogicError, match="logic error: configuration is not loaded"):
cfg.user_data_collection_configuration # pylint: disable=pointless-statement


def test_mcp_servers_not_loaded():
def test_mcp_servers_not_loaded() -> None:
"""Test that accessing mcp_servers before loading raises an error."""
cfg = AppConfig()
with pytest.raises(LogicError, match="logic error: configuration is not loaded"):
cfg.mcp_servers # pylint: disable=pointless-statement


def test_authentication_configuration_not_loaded():
def test_authentication_configuration_not_loaded() -> None:
"""Test that accessing authentication_configuration before loading raises an error."""
cfg = AppConfig()
with pytest.raises(LogicError, match="logic error: configuration is not loaded"):
cfg.authentication_configuration # pylint: disable=pointless-statement


def test_customization_not_loaded():
def test_customization_not_loaded() -> None:
"""Test that accessing customization before loading raises an error."""
cfg = AppConfig()
with pytest.raises(LogicError, match="logic error: configuration is not loaded"):
cfg.customization # pylint: disable=pointless-statement


def test_load_configuration_with_customization_system_prompt_path(tmpdir) -> None:
def test_load_configuration_with_customization_system_prompt_path(tmpdir: Path) -> None:
"""Test loading configuration from YAML file with customization."""
system_prompt_filename = tmpdir / "system_prompt.txt"
with open(system_prompt_filename, "w", encoding="utf-8") as fout:
Expand Down Expand Up @@ -462,7 +465,7 @@ def test_load_configuration_with_customization_system_prompt_path(tmpdir) -> Non
assert cfg.customization.system_prompt == "this is system prompt"


def test_load_configuration_with_customization_system_prompt(tmpdir) -> None:
def test_load_configuration_with_customization_system_prompt(tmpdir: Path) -> None:
"""Test loading configuration from YAML file with system_prompt in the customization."""
cfg_filename = tmpdir / "config.yaml"
with open(cfg_filename, "w", encoding="utf-8") as fout:
Expand Down Expand Up @@ -505,7 +508,7 @@ def test_load_configuration_with_customization_system_prompt(tmpdir) -> None:
)


def test_configuration_with_profile_customization(tmpdir) -> None:
def test_configuration_with_profile_customization(tmpdir: Path) -> None:
"""Test loading configuration from YAML file with a custom profile."""
expected_profile = CustomProfile(path="tests/profiles/test/profile.py")
expected_prompts = expected_profile.get_prompts()
Expand Down Expand Up @@ -544,7 +547,7 @@ def test_configuration_with_profile_customization(tmpdir) -> None:
) == expected_prompts.get("default")


def test_configuration_with_all_customizations(tmpdir) -> None:
def test_configuration_with_all_customizations(tmpdir: Path) -> None:
"""Test loading configuration from YAML file with a custom profile, prompt and prompt path."""
expected_profile = CustomProfile(path="tests/profiles/test/profile.py")
expected_prompts = expected_profile.get_prompts()
Expand Down
Loading