RHIDP-14000: update unit tests for shields#1872
Conversation
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
WalkthroughUnit tests for the ChangesShields Endpoint Test Coverage
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/app/endpoints/test_shields.py (1)
243-309: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider patching the configuration for test isolation.
Unlike other tests in this file (e.g., lines 27, 88, 161), this test creates an
AppConfiginstance (lines 291-292) but doesn't patch it into the endpoint's configuration module. While the test may work if the module already has a valid configuration, this makes the test dependent on external state.Recommended fix to match other test patterns
Add after line 292:
cfg = AppConfig() cfg.init_from_dict(config_dict) + +mocker.patch("app.endpoints.shields.configuration", cfg)Then move the client holder patching (lines 286-289) after the configuration patch to maintain a consistent setup order.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/app/endpoints/test_shields.py` around lines 243 - 309, The test creates an AppConfig instance (cfg.init_from_dict) but doesn't inject it into the endpoint module, leaving the test dependent on external module state; set the endpoint module's config to the new cfg (e.g. assign cfg to app.endpoints.shields.cfg or whatever module-level config variable is used by shields_endpoint_handler) immediately after cfg.init_from_dict, and then move the AsyncLlamaStackClientHolder patching (mock_client_holder) to after that assignment so the mocked client uses the test config; ensure you reference AppConfig, cfg, AsyncLlamaStackClientHolder and shields_endpoint_handler when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/app/endpoints/test_shields.py`:
- Around line 440-496: The test
test_shields_endpoint_handler_malformed_shield_objects uses
mocker.patch("client.AsyncLlamaStackClientHolder.get_client") but should follow
the file's consistent pattern; change the patch to
mocker.patch("app.endpoints.shields.AsyncLlamaStackClientHolder") (matching the
earlier tests) and set its return_value so that .get_client() returns
mock_client, and replace the generic mock_config patch of
app.endpoints.shields.configuration with the real cfg object already created, so
the test uses AsyncLlamaStackClientHolder -> get_client -> mock_client and the
actual cfg.
---
Outside diff comments:
In `@tests/unit/app/endpoints/test_shields.py`:
- Around line 243-309: The test creates an AppConfig instance
(cfg.init_from_dict) but doesn't inject it into the endpoint module, leaving the
test dependent on external module state; set the endpoint module's config to the
new cfg (e.g. assign cfg to app.endpoints.shields.cfg or whatever module-level
config variable is used by shields_endpoint_handler) immediately after
cfg.init_from_dict, and then move the AsyncLlamaStackClientHolder patching
(mock_client_holder) to after that assignment so the mocked client uses the test
config; ensure you reference AppConfig, cfg, AsyncLlamaStackClientHolder and
shields_endpoint_handler when making the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3c03fb8-dcdb-484b-ad2d-20e9514fb9c4
📒 Files selected for processing (1)
tests/unit/app/endpoints/test_shields.py
📜 Review details
⏰ 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). (17)
- GitHub Check: check_dependencies
- GitHub Check: spectral
- GitHub Check: Pyright
- GitHub Check: bandit
- GitHub Check: integration_tests (3.12)
- GitHub Check: unit_tests (3.12)
- GitHub Check: build-pr
- GitHub Check: Pylinter
- GitHub Check: mypy
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🧰 Additional context used
📓 Path-based instructions (1)
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/app/endpoints/test_shields.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: max-svistunov
Repo: lightspeed-core/lightspeed-stack PR: 1580
File: docs/design/llama-stack-config-merge/poc-results/library-mode/synthesized-run.yaml:107-110
Timestamp: 2026-05-20T08:09:34.319Z
Learning: In the lightspeed-stack project (LCORE-836 unified config), the `provider_shield_id` for a Llama Guard safety shield entry must be a guard model identifier (e.g., `meta-llama/Llama-Guard-3-8B`), not a chat model id like `openai/gpt-4o-mini`. Using a chat model id there only means the native_override key landed — it does not mean the safety shield is actually gating queries. E2E tests for the implementation JIRA must exercise a real Llama Guard model to validate the shield.
📚 Learning: 2026-05-20T08:09:34.319Z
Learnt from: max-svistunov
Repo: lightspeed-core/lightspeed-stack PR: 1580
File: docs/design/llama-stack-config-merge/poc-results/library-mode/synthesized-run.yaml:107-110
Timestamp: 2026-05-20T08:09:34.319Z
Learning: In the lightspeed-stack project (LCORE-836 unified config), the `provider_shield_id` for a Llama Guard safety shield entry must be a guard model identifier (e.g., `meta-llama/Llama-Guard-3-8B`), not a chat model id like `openai/gpt-4o-mini`. Using a chat model id there only means the native_override key landed — it does not mean the safety shield is actually gating queries. E2E tests for the implementation JIRA must exercise a real Llama Guard model to validate the shield.
Applied to files:
tests/unit/app/endpoints/test_shields.py
📚 Learning: 2026-05-06T06:57:52.173Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-06T06:57:52.173Z
Learning: Applies to src/app/**/*.py : Use FastAPI `HTTPException` with appropriate status codes for API endpoints and handle `APIConnectionError` from Llama Stack
Applied to files:
tests/unit/app/endpoints/test_shields.py
🔇 Additional comments (5)
tests/unit/app/endpoints/test_shields.py (5)
41-42: LGTM!
238-239: LGTM!
388-437: LGTM!
440-496: LGTM!
306-308: Don’t assume"Connection error"is indetail["cause"]without checking the localAPIConnectionErrorstring formatting.
The attempted verification failed becausellama_stack_clientisn’t importable in this environment, sostr(APIConnectionError(request=None))can’t be validated from the provided script; inspect the repo’sAPIConnectionErrorimplementation/__str__(the exact type used by the endpoint) before relying on that substring.
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Jdubrick
changed the title
2 participants
Description
assertsthat were not firing because an exception would be raised before they executedType of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
/shieldsREST API endpoint, enhancing error handling validation across multiple failure scenarios including configuration issues, connection failures, and unexpected exceptions.