Skip to content

test: add coverage for mcp tools#1314

Merged
fege merged 3 commits intoopendatahub-io:mainfrom
fege:tool_mcp
Mar 27, 2026
Merged

test: add coverage for mcp tools#1314
fege merged 3 commits intoopendatahub-io:mainfrom
fege:tool_mcp

Conversation

@fege
Copy link
Copy Markdown
Contributor

@fege fege commented Mar 27, 2026

Pull Request

Summary

Related Issues

Please review and indicate how it has been tested

  • Locally
  • Jenkins

Additional Requirements

  • If this PR introduces a new test image, did you create a PR to mirror it in disconnected environment?
  • If this PR introduces new marker(s)/adds a new component, was relevant ticket created to update relevant Jenkins job?

Summary by CodeRabbit

  • Tests
    • Extended test coverage for MCP server tool data integrity: validates default omission/null of tool fields and verifies toolCount accuracy for all servers.
    • Added tests for tool-listing behavior: enforces per-server tool limits (one test marked expected-failure) and verifies that requests exceeding the maximum limit produce the appropriate error response.

Signed-off-by: fege <fmosca@redhat.com>
@github-actions
Copy link
Copy Markdown

The following are automatically added/executed:

  • PR size label.
  • Run pre-commit
  • Run tox
  • Add PR author as the PR assignee
  • Build image based on the PR

Available user actions:

  • To mark a PR as WIP, add /wip in a comment. To remove it from the PR comment /wip cancel to the PR.
  • To block merging of a PR, add /hold in a comment. To un-block merging of PR comment /hold cancel.
  • To mark a PR as approved, add /lgtm in a comment. To remove, add /lgtm cancel.
    lgtm label removed on each new commit push.
  • To mark PR as verified comment /verified to the PR, to un-verify comment /verified cancel to the PR.
    verified label removed on each new commit push.
  • To Cherry-pick a merged PR /cherry-pick <target_branch_name> to the PR. If <target_branch_name> is valid,
    and the current PR is merged, a cherry-picked PR would be created and linked to the current PR.
  • To build and push image to quay, add /build-push-pr-image in a comment. This would create an image with tag
    pr-<pr_number> to quay repository. This image tag, however would be deleted on PR merge or close action.
Supported labels

{'/build-push-pr-image', '/hold', '/verified', '/wip', '/cherry-pick', '/lgtm'}

dbasunag
dbasunag previously approved these changes Mar 27, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 9e565e49-06de-4da0-a1be-b3689c77edcb

📥 Commits

Reviewing files that changed from the base of the PR and between 0edb53f and f755d91.

📒 Files selected for processing (1)
  • tests/model_registry/mcp_servers/test_data_integrity.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/model_registry/mcp_servers/test_data_integrity.py

📝 Walkthrough

Walkthrough

Test suite for MCP server data integrity extended: validates tools is absent/null by default and toolCount accuracy; adds test_mcp_server_tool_limit (marked xfail) to verify toolLimit caps returned tools and test_tool_limit_exceeding_maximum to assert toolLimit=101 raises ResourceNotFoundError.

Changes

Cohort / File(s) Summary
MCP Server Test Data Integrity
tests/model_registry/mcp_servers/test_data_integrity.py
Imported ResourceNotFoundError. Extended test_mcp_servers_loaded to assert tools is omitted or None by default and toolCount equals EXPECTED_MCP_SERVER_TOOL_COUNTS. Added test_mcp_server_tool_limit (marked xfail) calling mcp_servers?includeTools=true&toolLimit=1 and asserting each server's tools length ≤ 1. Added test_tool_limit_exceeding_maximum (tier3) asserting includeTools=true&toolLimit=101 causes the listing endpoint to raise ResourceNotFoundError. Existing test_mcp_server_tools_loaded remains focused on correctness when includeTools=true.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive The PR description uses the template structure but provides no concrete implementation details in the Summary section, leaves all issue links blank, and omits specifics about what testing was performed locally. Fill in the Summary section with concrete details about what was added (new test cases, validation assertions) and describe how testing was performed locally to demonstrate the changes work as intended.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'test: add coverage for mcp tools' is concise and clearly describes the main change—adding test coverage for MCP tools—which aligns with the changeset's focus on extending MCP server data integrity validation.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/model_registry/mcp_servers/test_data_integrity.py`:
- Around line 51-55: The test currently only checks each server's tool count and
can falsely pass when response.get("items") is empty; add an assertion before
the loop that response.get("items") is a non-empty iterable (e.g., assert
response.get("items"), or assert len(response.get("items", [])) > 0) to ensure
at least one server was returned, then proceed to iterate over
response.get("items", []) and validate each server's "tools" length against
tool_limit (symbols: response, items, tool_limit).
- Around line 64-69: Replace the use of execute_get_command and the
ResourceNotFoundError assertion with a direct call to execute_get_call to assert
the HTTP status code; specifically, call execute_get_call with
url=f"{mcp_catalog_rest_urls[0]}mcp_servers",
headers=model_registry_rest_headers,
params={"includeTools":"true","toolLimit":"101"} and assert the returned
response.status_code equals 400 (or the expected validation status) so the test
checks the API contract rather than the wrapper raising ResourceNotFoundError.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 472abebe-90d6-4e19-8654-0810c10c1099

📥 Commits

Reviewing files that changed from the base of the PR and between 2f23786 and 0edb53f.

📒 Files selected for processing (1)
  • tests/model_registry/mcp_servers/test_data_integrity.py

Signed-off-by: fege <fmosca@redhat.com>
@fege fege enabled auto-merge (squash) March 27, 2026 13:53
Copy link
Copy Markdown
Contributor

@sheltoncyril sheltoncyril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@fege fege merged commit 24d4f58 into opendatahub-io:main Mar 27, 2026
10 checks passed
@github-actions
Copy link
Copy Markdown

Status of building tag latest: success.
Status of pushing tag latest to image registry: success.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants