LCORE-657: unit tests for configuration.py #527
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 8 minutes and 21 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 (1)
WalkthroughRemoved the explicit None-guard from AppConfig.authentication_configuration so the accessor returns the stored authentication value after verifying the overall configuration is loaded. Tests were expanded to reset AppConfig between tests, check "not loaded" behavior for more subsections, and assert presence of authentication/authorization/database/inference after loading. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant AppConfig
rect rgba(230,245,255,0.6)
note right of AppConfig: Caller requests authentication_configuration
Caller->>AppConfig: get authentication_configuration()
alt Config not loaded
AppConfig-->>Caller: raise LogicError("configuration is not loaded")
else Config loaded
par Old behavior (previous)
AppConfig-->>Caller: raise LogicError if authentication is None
and New behavior (current)
AppConfig-->>Caller: return authentication (may be None)
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
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 |
tisnik
force-pushed
the
lcore-657-unit-tests-for-configuration.py
branch
from
61a0c3c to
527beb4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/unit/test_configuration.py (4)
16-16: Tighten exception type: use LogicError instead of broad Exception in pytest.raises.This makes tests stricter and consistent with the later tests that already expect LogicError.
- with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.configuration # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.service_configuration # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.llama_stack_configuration # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = ( cfg.user_data_collection_configuration ) # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.mcp_servers # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.authentication_configuration # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.customization # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.authorization_configuration # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.inference # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.database_configuration # pylint: disable=pointless-statementAlso applies to: 20-20, 24-24, 28-30, 34-34, 38-38, 42-42, 44-46, 48-50, 52-54
86-88: Add negative case for authentication omitted (should return None after load).Since authentication_configuration now returns the stored value (possibly None) post-load, cover that explicitly.
def test_authentication_configuration_absent_returns_none() -> None: cfg = AppConfig() cfg.init_from_dict({ "name": "foo", "service": {"host": "localhost", "port": 8080, "auth_enabled": False, "workers": 1, "color_log": True, "access_log": True}, "llama_stack": {"api_key": "xyzzy", "url": "http://x.y.com:1234", "use_as_library_client": False}, "user_data_collection": {"feedback_enabled": False}, # no "authentication" key "mcp_servers": [], "customization": None, }) assert cfg.authentication_configuration is None
118-130: Broaden post-load assertions to check types/defaults, not just non-None.Optional, but asserting types or representative defaults gives stronger guarantees than existence checks.
- For authentication: also assert that module == "noop" is the expected default behavior for the provided dict (already done).
- For authorization/database/inference: assert a type or a sentinel field if available (e.g., isinstance(..., AuthorizationConfiguration)).
Example:
# if available to import from configuration import AuthorizationConfiguration, DatabaseConfiguration, InferenceConfiguration assert isinstance(cfg.authorization_configuration, AuthorizationConfiguration) assert isinstance(cfg.database_configuration, DatabaseConfiguration) assert isinstance(cfg.inference, InferenceConfiguration)
177-204: Fix docstring wording and consider asserting defaults for explicit empty authorization.Minor wording nit and optional stronger check that {} maps to a valid default object.
-def test_init_from_dict_with_authorization_configuration() -> None: - """Test initialization with authorization configuration configuration.""" +def test_init_from_dict_with_authorization_configuration() -> None: + """Test initialization with explicit empty authorization configuration."""Optionally, add:
# if type is importable from configuration import AuthorizationConfiguration assert isinstance(cfg.authorization_configuration, AuthorizationConfiguration)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/configuration.py(0 hunks)tests/unit/test_configuration.py(4 hunks)
💤 Files with no reviewable changes (1)
- src/configuration.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/test_configuration.py (1)
src/configuration.py (6)
configuration(60-64)authentication_configuration(95-100)authorization_configuration(103-111)inference(121-125)database_configuration(128-132)init_from_dict(55-57)
⏰ 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). (1)
- GitHub Check: build-pr
tisnik
force-pushed
the
lcore-657-unit-tests-for-configuration.py
branch
from
527beb4 to
bd708c9
Compare
tisnik
force-pushed
the
lcore-657-unit-tests-for-configuration.py
branch
from
bd708c9 to
d30ed3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/unit/test_configuration.py (5)
8-20: Tighten the autouse reset fixture (fix linter warnings, narrow exception scope).Good call adding the isolation fixture. Two polish items:
- Remove broad Exception catching (W0718) and use hasattr instead.
- Silence protected-access locally to avoid W0212 without affecting the module.
Apply:
@pytest.fixture(autouse=True) def _reset_app_config_between_tests(): - # ensure clean state before each test - try: - AppConfig()._configuration = None # type: ignore[attr-defined] - except Exception: - pass - yield - # ensure clean state after each test - try: - AppConfig()._configuration = None # type: ignore[attr-defined] - except Exception: - pass + # ensure clean state before each test + cfg = AppConfig() + # pylint: disable=protected-access + if hasattr(cfg, "_configuration"): + cfg._configuration = None # type: ignore[attr-defined] + yield + # ensure clean state after each test + # pylint: disable=protected-access + if hasattr(cfg, "_configuration"): + cfg._configuration = None # type: ignore[attr-defined]
29-70: Assert the precise exception type (LogicError) for “not loaded” checks.These guards currently expect Exception; tighten to LogicError for accuracy and stronger guarantees.
- with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.configuration # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.service_configuration # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.llama_stack_configuration # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = ( cfg.user_data_collection_configuration ) # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.mcp_servers # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.authentication_configuration # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.customization # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.authorization_configuration # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.inference # pylint: disable=pointless-statement - with pytest.raises(Exception, match="logic error: configuration is not loaded"): + with pytest.raises(LogicError, match="logic error: configuration is not loaded"): _ = cfg.database_configuration # pylint: disable=pointless-statementOptional: parametrize the properties to de-duplicate the repetitive blocks. I can draft that if you want.
133-145: Add type assertions for stronger guarantees on new subsections.Great to check presence. Assert expected types too for clearer failure messages and future refactors.
# check authentication_configuration assert cfg.authentication_configuration is not None assert cfg.authentication_configuration.module == "noop" # check authorization configuration - default value assert cfg.authorization_configuration is not None # check database configuration - assert cfg.database_configuration is not None + assert cfg.database_configuration is not None + # optional: validate type if exported in public API + # from models.config import DatabaseConfiguration + # assert isinstance(cfg.database_configuration, DatabaseConfiguration) # check inference configuration - assert cfg.inference is not None + assert cfg.inference is not None + # optional: validate type if exported in public API + # from models.config import InferenceConfiguration + # assert isinstance(cfg.inference, InferenceConfiguration)
192-219: Fix docstring typo and add a minimal behavior assertion.Minor wording nit; also consider a minimal semantic check to ensure an object is actually returned.
-def test_init_from_dict_with_authorization_configuration() -> None: - """Test initialization with authorization configuration configuration.""" +def test_init_from_dict_with_authorization_configuration() -> None: + """Test initialization with authorization configuration.""" @@ cfg.init_from_dict(config_dict) - assert cfg.authorization_configuration is not None + assert cfg.authorization_configuration is not None + # optional: check that defaults are applied when empty dict provided + # e.g., assert getattr(cfg.authorization_configuration, "rules", None) is not None
1-498: Potential singleton pitfall: re-instantiation may reset configuration.Unrelated to test logic but worth verifying: if AppConfig.init sets self._configuration = None unconditionally, calling AppConfig() elsewhere could wipe loaded state. Add a regression test ensuring a second instantiation does not reset configuration, or guard init in src with hasattr checks.
I can open a follow-up issue and supply the test if desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/test_configuration.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/test_configuration.py (1)
src/configuration.py (12)
AppConfig(32-132)configuration(60-64)service_configuration(67-71)llama_stack_configuration(74-78)user_data_collection_configuration(81-85)mcp_servers(88-92)authentication_configuration(95-100)customization(114-118)authorization_configuration(103-111)inference(121-125)database_configuration(128-132)init_from_dict(55-57)
🪛 GitHub Actions: Python linter
tests/unit/test_configuration.py
[warning] 12-12: W0212: Access to a protected member '_configuration' of a client class (protected-access).
[warning] 13-13: W0718: Catching too general exception 'Exception' (broad-exception-caught).
[warning] 18-18: W0212: Access to a protected member '_configuration' of a client class (protected-access).
[warning] 19-19: W0718: Catching too general exception 'Exception' (broad-exception-caught).
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
1 participant
Description
LCORE-657: unit tests for
configuration.pyRemoved dead code
Type of change
Related Tickets & Documents
Summary by CodeRabbit
Bug Fixes
Tests