test: coverage for q search of mcp#1234
Conversation
Signed-off-by: fege <fmosca@redhat.com>
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/wip', '/build-push-pr-image', '/hold', '/lgtm', '/verified', '/cherry-pick'} |
📝 WalkthroughWalkthroughA new test module is introduced for MCP server keyword search functionality, containing a parameterized test that validates keyword search behavior when combined with filter or named queries using GET requests to the mcp_servers endpoint. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/model_registry/mcp_servers/search/test_keyword_search.py (1)
44-46: Assert response schema before dereferencingserver["name"].Line 45 assumes every item is a dict with
name; malformed/error payloads produce opaque failures. Add explicit payload contract checks first.Proposed hardening
- items = response.get("items", []) - actual_names = {server["name"] for server in items} + items = response.get("items") + assert isinstance(items, list), f"Unexpected response payload: {response}" + assert all(isinstance(server, dict) and "name" in server for server in items), ( + f"One or more items are missing `name`: {items}" + ) + actual_names = {server["name"] for server in items}As per coding guidelines, “REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/mcp_servers/search/test_keyword_search.py` around lines 44 - 46, Before dereferencing server["name"], validate the response/item schema: ensure response.get("items") is a list and each item is a dict containing a "name" key. Update the assertion block around items/actual_names (variables: response, items, actual_names, expected_names) to first assert isinstance(items, list) and for each server assert isinstance(server, dict) and "name" in server, then build actual_names from server["name"] and compare to expected_names; fail with clear messages if the contract is violated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/model_registry/mcp_servers/search/test_keyword_search.py`:
- Around line 44-46: Before dereferencing server["name"], validate the
response/item schema: ensure response.get("items") is a list and each item is a
dict containing a "name" key. Update the assertion block around
items/actual_names (variables: response, items, actual_names, expected_names) to
first assert isinstance(items, list) and for each server assert
isinstance(server, dict) and "name" in server, then build actual_names from
server["name"] and compare to expected_names; fail with clear messages if the
contract is violated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 02f0bbf4-8332-426e-8726-3fd8dfba8ab6
📒 Files selected for processing (1)
tests/model_registry/mcp_servers/search/test_keyword_search.py
|
Status of building tag latest: success. |
Signed-off-by: fege <fmosca@redhat.com> Co-authored-by: Debarati Basu-Nag <dbasunag@redhat.com> Signed-off-by: Shehan Saleem <ssaleem@redhat.com>
Pull Request
Summary
Related Issues
How it has been tested
Additional Requirements
Summary by CodeRabbit