LCORE-657: Unit tests for configuration#1139
Conversation
WalkthroughAdds and expands unit tests for AppConfig to cover additional public configuration subsections: a2a_state, token_usage_history, azure_entra_id, splunk, and deployment_environment; updates test YAML and init data to assert presence and defaults for these sections. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/unit/test_configuration.py`:
- Around line 96-114: Update the five raises in tests/unit/test_configuration.py
to assert the specific LogicError instead of the broad Exception: replace
pytest.raises(Exception, match="logic error: configuration is not loaded") with
pytest.raises(LogicError, match="logic error: configuration is not loaded") for
the checks that access cfg.a2a_state, cfg.token_usage_history,
cfg.azure_entra_id, cfg.splunk, and cfg.deployment_environment; also ensure
LogicError is imported into the test module (or referenced from its defining
module) so the test file references the correct exception class.
d6ed09e to
ce7dc89
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/unit/test_configuration.py`:
- Around line 222-223: The test currently only checks cfg.deployment_environment
is not None; change it to assert the concrete expected value by replacing that
check with an equality assertion (e.g., assert cfg.deployment_environment ==
"foo") so the test verifies the value set earlier in the fixture; locate the
assertion referencing cfg.deployment_environment in the test_configuration.py
test block and update it accordingly.
- Around line 780-781: The docstring for
test_configuration_with_token_history_no_storage is incorrect (it references
"quota handlers configuration"); update the function's docstring to accurately
describe that the test verifies loading configuration from YAML for token
history settings when no storage backend is configured, e.g., mention "token
history when no storage backend is configured" and keep it concise to match the
test purpose.
| # check deployment_environment | ||
| assert cfg.deployment_environment is not None |
There was a problem hiding this comment.
Assert the actual value of deployment_environment, not just non-None.
The dict on line 161 sets "deployment_environment": "foo", but the assertion here only checks is not None. Every other field in this test block asserts the concrete value. This should do the same to avoid silently passing when the value is wrong.
Proposed fix
# check deployment_environment
- assert cfg.deployment_environment is not None
+ assert cfg.deployment_environment == "foo"📝 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.
| # check deployment_environment | |
| assert cfg.deployment_environment is not None | |
| # check deployment_environment | |
| assert cfg.deployment_environment == "foo" |
🤖 Prompt for AI Agents
In `@tests/unit/test_configuration.py` around lines 222 - 223, The test currently
only checks cfg.deployment_environment is not None; change it to assert the
concrete expected value by replacing that check with an equality assertion
(e.g., assert cfg.deployment_environment == "foo") so the test verifies the
value set earlier in the fixture; locate the assertion referencing
cfg.deployment_environment in the test_configuration.py test block and update it
accordingly.
| def test_configuration_with_token_history_no_storage(tmpdir: Path) -> None: | ||
| """Test loading configuration from YAML file with quota handlers configuration.""" |
There was a problem hiding this comment.
Docstring is a copy-paste from test_configuration_with_quota_handlers_no_storage.
The docstring says "quota handlers configuration" but the test is specifically about token history when no storage backend is configured.
Proposed fix
def test_configuration_with_token_history_no_storage(tmpdir: Path) -> None:
- """Test loading configuration from YAML file with quota handlers configuration."""
+ """Test token usage history is created when enabled without storage backend."""📝 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.
| def test_configuration_with_token_history_no_storage(tmpdir: Path) -> None: | |
| """Test loading configuration from YAML file with quota handlers configuration.""" | |
| def test_configuration_with_token_history_no_storage(tmpdir: Path) -> None: | |
| """Test token usage history is created when enabled without storage backend.""" |
🤖 Prompt for AI Agents
In `@tests/unit/test_configuration.py` around lines 780 - 781, The docstring for
test_configuration_with_token_history_no_storage is incorrect (it references
"quota handlers configuration"); update the function's docstring to accurately
describe that the test verifies loading configuration from YAML for token
history settings when no storage backend is configured, e.g., mention "token
history when no storage backend is configured" and keep it concise to match the
test purpose.
Description
LCORE-657: Unit tests for configuration
Type of change
Tools used to create PR
Related Tickets & Documents
Summary by CodeRabbit