Skip to content

test: assetType for source endpoint#1187

Merged
fege merged 2 commits intoopendatahub-io:mainfrom
fege:assetType
Mar 9, 2026
Merged

test: assetType for source endpoint#1187
fege merged 2 commits intoopendatahub-io:mainfrom
fege:assetType

Conversation

@fege
Copy link
Copy Markdown
Contributor

@fege fege commented Mar 9, 2026

Pull Request

Summary

  • Test source endpoint for assetType
  • Remove test that is covered in upstream
  • Remove xfail, bug was fixed RHOAIENG-51765

Related Issues

  • Fixes:
  • JIRA: RHOAIENG-51583

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
    • Added test coverage for filtering model catalog sources by asset type.
    • Reorganized test infrastructure to improve support for MCP (Model Catalog Provider) server configuration testing.
    • Enhanced test utilities for MCP catalog API readiness verification.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 9, 2026

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

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

Comment thread tests/model_registry/conftest.py
Copy link
Copy Markdown
Collaborator

@dbasunag dbasunag left a comment

Choose a reason for hiding this comment

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

Some minor nitpicking. Please feel free to address them later.

Comment thread tests/model_registry/conftest.py
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 9, 2026

📝 Walkthrough

Walkthrough

Fixtures for MCP catalog configuration migrated from mcp_servers/conftest.py to parent module conftest.py. New wait_for_mcp_catalog_api helper function added to utilities. Test coverage expanded for asset type filtering. Several MCP tool-related test cases removed.

Changes

Cohort / File(s) Summary
Fixture Consolidation
tests/model_registry/conftest.py, tests/model_registry/mcp_servers/conftest.py
mcp_catalog_rest_urls and mcp_servers_configmap_patch fixtures moved from mcp_servers module to parent conftest to share across test suites; ConfigMap patching logic now class-scoped at parent level.
Utility Functions
tests/model_registry/utils.py
Added wait_for_mcp_catalog_api() with retry decorator (90s timeout, 5s sleep) to poll MCP servers endpoint and validate non-empty response items list.
Test Coverage Changes
tests/model_registry/mcp_servers/test_data_integrity.py
Removed test_mcp_server_tool_count_without_include and xfail wrapper for test_mcp_server_tools_loaded; removed EXPECTED_MCP_SERVER_TOOL_COUNTS import.
Sources Endpoint Tests
tests/model_registry/model_catalog/metadata/test_sources_endpoint.py
Added TestAssetTypeFilter class with parameterized test_asset_type_filters_sources() validating source filtering by assetType; includes MCP configmap fixture and dynamic URL construction.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding a test for assetType filtering in the source endpoint.
Description check ✅ Passed The description covers all required sections with substantive content: summary explains the three key changes, related JIRA ticket is provided, and testing status is indicated.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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/conftest.py`:
- Around line 519-528: The teardown only calls
wait_for_model_catalog_pod_ready_after_deletion so tests can still hit a
pod-ready but not-fully-reloaded catalog; after the existing teardown call to
wait_for_model_catalog_pod_ready_after_deletion add the same API-level poll used
in setup by calling wait_for_mcp_catalog_api(url=mcp_catalog_rest_urls[0],
headers=model_registry_rest_headers) so the fixture waits for the MCP catalog
API to be healthy and reflect restored state before returning; locate this in
the ResourceEditor teardown block near ResourceEditor,
wait_for_model_catalog_pod_ready_after_deletion, and
mcp_catalog_rest_urls/model_registry_rest_headers usages.

In `@tests/model_registry/utils.py`:
- Around line 964-975: In wait_for_mcp_catalog_api, the direct call to
response.json() can raise ValueError/JSONDecodeError for partial/invalid JSON
and bypass the retry logic; catch ValueError around response.json() and re-raise
it as ResourceNotFoundError (or wrap with a descriptive message) so the retry
decorator (which handles ResourceNotFoundError and TransientUnauthorizedError)
will retry; update the block that calls execute_get_call and response.json() in
wait_for_mcp_catalog_api to perform this try/except conversion.

ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro

Run ID: 79cc5e0d-756f-4357-a41c-770b18fb571c

📥 Commits

Reviewing files that changed from the base of the PR and between b8e4bfe and 32b63f9.

📒 Files selected for processing (5)
  • tests/model_registry/conftest.py
  • tests/model_registry/mcp_servers/conftest.py
  • tests/model_registry/mcp_servers/test_data_integrity.py
  • tests/model_registry/model_catalog/metadata/test_sources_endpoint.py
  • tests/model_registry/utils.py
💤 Files with no reviewable changes (1)
  • tests/model_registry/mcp_servers/test_data_integrity.py

Comment thread tests/model_registry/conftest.py
Comment thread tests/model_registry/utils.py
@fege fege merged commit 7ac82f6 into opendatahub-io:main Mar 9, 2026
9 checks passed
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 9, 2026

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

@fege fege deleted the assetType branch March 9, 2026 17:03
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