LCORE-381: added missing unit tests #336
Conversation
WalkthroughThis update introduces new unit tests for the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Unit Test
participant AuthConfig as AuthenticationConfiguration
Test->>AuthConfig: Create with valid JwkConfiguration
AuthConfig-->>Test: Success
Test->>AuthConfig: Create with empty JwkConfiguration
AuthConfig-->>Test: Raise ValidationError
Test->>AuthConfig: Create with AUTH_MOD_JWK_TOKEN but no jwk_config
AuthConfig-->>Test: Raise ValidationError
sequenceDiagram
participant Test as Unit Test
participant Client as LlamaStackClient
Test->>Client: Obtain client instance
Test->>Client: Assert not closed
Test->>Client: Call close()/await close()
Test->>Client: Assert closed
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (1)
tests/unit/test_client.py (1)
104-108: Consider the redundancy of explicit close() in async context manager.The async context manager usage is excellent practice for resource management. However, explicitly calling
await ls_client.close()inside the context may be redundant since the context manager should handle cleanup automatically when exiting.Consider whether the explicit
close()call is necessary:async with client.get_client() as ls_client: assert ls_client is not None assert not ls_client.is_closed() - await ls_client.close() - assert ls_client.is_closed()Or if you want to test explicit closure within the context, add a comment explaining the intention.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/models/test_config.py(2 hunks)tests/unit/test_client.py(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Black
tests/unit/test_client.py
[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black --write' to fix code style issues.
⏰ 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: e2e_tests
🔇 Additional comments (6)
tests/unit/test_client.py (2)
49-51: LGTM! Good addition of client lifecycle verification.These assertions properly test the client's closed state before and after calling
close(), which improves test coverage for client resource management.
68-70: LGTM! Consistent client lifecycle testing.The same client state verification pattern is applied here, maintaining consistency with the library client test and ensuring both client types are properly tested.
tests/unit/models/test_config.py (4)
13-13: LGTM! Necessary imports for JWK token testing.The added imports
AUTH_MOD_JWK_TOKENandJwkConfigurationare correctly placed and required for the new authentication tests.Also applies to: 21-21
687-702: LGTM! Comprehensive positive test case for JWK token authentication.This test properly verifies the successful creation and configuration of
AuthenticationConfigurationwith the JWK token module, including all relevant assertions for the configuration properties.
704-715: LGTM! Good validation test for insufficient JWK configuration.This test properly verifies that a
ValidationErroris raised whenJwkConfigurationis provided but lacks required fields, ensuring proper validation behavior.
717-731: LGTM! Thorough validation test for missing JWK configuration.This test ensures that a
ValidationErrorwith a specific message is raised when the JWK token authentication module is specified but noJwkConfigurationis provided, completing the validation test coverage.
tisnik
force-pushed
the
lcore-381-added-unit-tests
branch
from
0ac8fc4 to
6b08230
Compare
1 participant
Description
LCORE-381: added missing unit tests
Type of change
Related Tickets & Documents
Summary by CodeRabbit