test: add MCP server catalog integration tests#1168
Merged
dbasunag merged 4 commits intoopendatahub-io:mainfrom Mar 5, 2026
Merged
test: add MCP server catalog integration tests#1168dbasunag merged 4 commits intoopendatahub-io:mainfrom
dbasunag merged 4 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/hold', '/verified', '/cherry-pick', '/build-push-pr-image', '/wip', '/lgtm'} |
Contributor
📝 WalkthroughWalkthroughAdds MCP server test resources and fixtures, new MCP server constants and tests (single/multi/invalid sources), introduces shared HTTP and pod utilities in tests/model_registry/utils.py, removes duplicate helpers from model_catalog/utils.py, and updates import paths across many model_registry tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/model_registry/mcp_servers/test_multi_source.py (1)
42-47: Prefer explicit assertion overKeyErrorfor unknown server names.At Line 44, direct indexing can fail with an opaque traceback. A guarded lookup keeps failure output clearer.
Proposed refinement
for server in response.get("items", []): name = server["name"] - expected_source = EXPECTED_MCP_SOURCE_ID_MAP[name] + expected_source = EXPECTED_MCP_SOURCE_ID_MAP.get(name) + assert expected_source is not None, f"Unexpected MCP server returned: {name}" assert server.get("source_id") == expected_source, ( f"Server '{name}' has source_id '{server.get('source_id')}', expected '{expected_source}'" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/mcp_servers/test_multi_source.py` around lines 42 - 47, The test currently does a direct lookup EXPECTED_MCP_SOURCE_ID_MAP[name] which will raise a KeyError for unknown server names; change this to a guarded lookup and explicit assertion: first assert that name is present in EXPECTED_MCP_SOURCE_ID_MAP (use "name in EXPECTED_MCP_SOURCE_ID_MAP") with a clear failure message, then retrieve expected_source via EXPECTED_MCP_SOURCE_ID_MAP.get(name) and assert server.get("source_id") == expected_source with the existing message; update the block using the variables response, server, name, and EXPECTED_MCP_SOURCE_ID_MAP to make failures explicit and readable.tests/model_registry/mcp_servers/conftest.py (1)
75-78: De-duplicatemcp_catalogsentries before patching.Current append/extend logic can duplicate source IDs when they already exist in
sources.yaml, which can make assertions non-deterministic.Proposed de-duplication helper
+def _merge_mcp_sources(current_data: dict, new_sources: list[dict]) -> None: + existing = { + source.get("id"): source + for source in current_data.get("mcp_catalogs", []) + if isinstance(source, dict) and source.get("id") + } + for source in new_sources: + existing[source["id"]] = source + current_data["mcp_catalogs"] = list(existing.values())- if "mcp_catalogs" not in current_data: - current_data["mcp_catalogs"] = [] - current_data["mcp_catalogs"].append(MCP_CATALOG_SOURCE) + _merge_mcp_sources(current_data=current_data, new_sources=[MCP_CATALOG_SOURCE])- if "mcp_catalogs" not in current_data: - current_data["mcp_catalogs"] = [] - current_data["mcp_catalogs"].extend([MCP_CATALOG_SOURCE, MCP_CATALOG_SOURCE2]) + _merge_mcp_sources(current_data=current_data, new_sources=[MCP_CATALOG_SOURCE, MCP_CATALOG_SOURCE2])- if "mcp_catalogs" not in current_data: - current_data["mcp_catalogs"] = [] - current_data["mcp_catalogs"].extend([MCP_CATALOG_SOURCE, MCP_CATALOG_INVALID_SOURCE]) + _merge_mcp_sources(current_data=current_data, new_sources=[MCP_CATALOG_SOURCE, MCP_CATALOG_INVALID_SOURCE])Also applies to: 116-119, 159-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/mcp_servers/conftest.py` around lines 75 - 78, The test setup unconditionally appends MCP_CATALOG_SOURCE to current_data["mcp_catalogs"], causing duplicate entries; change the logic so you deduplicate before patching—e.g., compute a unique list (by source id or full entry) for current_data["mcp_catalogs"] and only add MCP_CATALOG_SOURCE if it isn't already present, or replace the list with a deduped union (use a helper like dedupe_sources(current_data["mcp_catalogs"], MCP_CATALOG_SOURCE)); apply the same deduplication change to the other similar blocks that manipulate current_data["mcp_catalogs"] (the sections around the other append/extend usages).
🤖 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/conftest.py`:
- Around line 44-51: The readiness check in wait_for_mcp_catalog_api only
ensures items is non-empty and can pass before all fixtures are indexed; change
the function signature to accept readiness criteria (e.g., expected_source_ids:
list[str] and/or min_server_count: int), and update the body to validate that
every expected_source_id appears in response.json()["items"] (or that len(items)
>= min_server_count) before returning, raising ResourceNotFoundError if criteria
are not yet met; update all callers to pass the appropriate expected_source_ids
or min_server_count for their fixtures and improve the error message to include
which IDs/count are still missing.
- Around line 48-50: The call to response.json() can raise
json.JSONDecodeError/ValueError and currently escapes the `@retry` handling; wrap
the response.json() call in a try/except that catches json.JSONDecodeError and
ValueError (import JSONDecodeError from json if needed) and either re-raise a
known retryable exception used by the existing `@retry` decorator or add
JSONDecodeError/ValueError to the decorator's exceptions_dict so the fixture
(the code around response.json() and the ResourceNotFoundError) will be retried
on transient non-JSON responses.
In `@tests/model_registry/mcp_servers/test_invalid_yaml.py`:
- Around line 37-39: The test currently only asserts EXPECTED_MCP_SERVER_NAMES
<= server_names which allows bad entries to slip in; update the test to also
assert negative conditions that invalid names are not present in server_names
(e.g., assert "" not in server_names, assert None not in server_names, and
assert any known malformed sentinel name used in fixtures/configs—such as a
MALFORMED_SOURCE_NAME or similar sentinel— not in server_names). Apply the same
negative-path assertions at the other check block that mirrors lines 63-65 so
both positive and explicit rejection checks run.
---
Nitpick comments:
In `@tests/model_registry/mcp_servers/conftest.py`:
- Around line 75-78: The test setup unconditionally appends MCP_CATALOG_SOURCE
to current_data["mcp_catalogs"], causing duplicate entries; change the logic so
you deduplicate before patching—e.g., compute a unique list (by source id or
full entry) for current_data["mcp_catalogs"] and only add MCP_CATALOG_SOURCE if
it isn't already present, or replace the list with a deduped union (use a helper
like dedupe_sources(current_data["mcp_catalogs"], MCP_CATALOG_SOURCE)); apply
the same deduplication change to the other similar blocks that manipulate
current_data["mcp_catalogs"] (the sections around the other append/extend
usages).
In `@tests/model_registry/mcp_servers/test_multi_source.py`:
- Around line 42-47: The test currently does a direct lookup
EXPECTED_MCP_SOURCE_ID_MAP[name] which will raise a KeyError for unknown server
names; change this to a guarded lookup and explicit assertion: first assert that
name is present in EXPECTED_MCP_SOURCE_ID_MAP (use "name in
EXPECTED_MCP_SOURCE_ID_MAP") with a clear failure message, then retrieve
expected_source via EXPECTED_MCP_SOURCE_ID_MAP.get(name) and assert
server.get("source_id") == expected_source with the existing message; update the
block using the variables response, server, name, and EXPECTED_MCP_SOURCE_ID_MAP
to make failures explicit and readable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46d81832-e7f4-4e29-acf2-8aa729a24380
📒 Files selected for processing (24)
tests/model_registry/conftest.pytests/model_registry/mcp_servers/__init__.pytests/model_registry/mcp_servers/conftest.pytests/model_registry/mcp_servers/constants.pytests/model_registry/mcp_servers/test_data_integrity.pytests/model_registry/mcp_servers/test_invalid_yaml.pytests/model_registry/mcp_servers/test_multi_source.pytests/model_registry/model_catalog/catalog_config/conftest.pytests/model_registry/model_catalog/catalog_config/test_catalog_source_merge.pytests/model_registry/model_catalog/catalog_config/test_custom_model_catalog.pytests/model_registry/model_catalog/catalog_config/test_default_model_catalog.pytests/model_registry/model_catalog/catalog_config/utils.pytests/model_registry/model_catalog/conftest.pytests/model_registry/model_catalog/huggingface/test_huggingface_source_error_validation.pytests/model_registry/model_catalog/huggingface/utils.pytests/model_registry/model_catalog/metadata/test_custom_properties.pytests/model_registry/model_catalog/metadata/test_filter_options_endpoint.pytests/model_registry/model_catalog/metadata/test_sources_endpoint.pytests/model_registry/model_catalog/metadata/utils.pytests/model_registry/model_catalog/search/utils.pytests/model_registry/model_catalog/sorting/utils.pytests/model_registry/model_catalog/upgrade/test_model_catalog_upgrade.pytests/model_registry/model_catalog/utils.pytests/model_registry/utils.py
dbasunag
reviewed
Mar 4, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/model_registry/mcp_servers/conftest.py (1)
87-90: Consider extracting the repeated YAML loading pattern.The same pattern for loading and initializing
mcp_catalogsappears in all three fixtures. This could be extracted to a helper function to reduce duplication.♻️ Optional helper extraction
def _load_mcp_catalogs_data(catalog_config_map: ConfigMap) -> dict: """Load existing mcp_catalogs data from ConfigMap, initializing if absent.""" current_data = yaml.safe_load(catalog_config_map.instance.data.get("sources.yaml", "{}") or "{}") if "mcp_catalogs" not in current_data: current_data["mcp_catalogs"] = [] return current_dataThen replace the repeated blocks with:
current_data = _load_mcp_catalogs_data(catalog_config_map) current_data["mcp_catalogs"].append(MCP_CATALOG_SOURCE) # or extend, as neededAlso applies to: 128-131, 171-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/mcp_servers/conftest.py` around lines 87 - 90, Extract the repeated YAML-loading/initialization logic into a helper function (e.g., _load_mcp_catalogs_data) that accepts catalog_config_map (the ConfigMap-like fixture) and returns the parsed dict with "mcp_catalogs" initialized if missing; then replace the duplicated blocks in the three fixtures by calling that helper and appending/ extending MCP_CATALOG_SOURCE to the returned dict before writing back to catalog_config_map.instance.data["sources.yaml"]. Reference: the repeated use of yaml.safe_load(...catalog_config_map.instance.data.get("sources.yaml", "{}") or "{}"), the MCP_CATALOG_SOURCE symbol, and the fixtures that manipulate catalog_config_map.
🤖 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/conftest.py`:
- Around line 87-90: Extract the repeated YAML-loading/initialization logic into
a helper function (e.g., _load_mcp_catalogs_data) that accepts
catalog_config_map (the ConfigMap-like fixture) and returns the parsed dict with
"mcp_catalogs" initialized if missing; then replace the duplicated blocks in the
three fixtures by calling that helper and appending/ extending
MCP_CATALOG_SOURCE to the returned dict before writing back to
catalog_config_map.instance.data["sources.yaml"]. Reference: the repeated use of
yaml.safe_load(...catalog_config_map.instance.data.get("sources.yaml", "{}") or
"{}"), the MCP_CATALOG_SOURCE symbol, and the fixtures that manipulate
catalog_config_map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 49303754-dcd3-4fe1-9120-b16896115d20
📒 Files selected for processing (4)
tests/model_registry/mcp_servers/conftest.pytests/model_registry/mcp_servers/test_data_integrity.pytests/model_registry/mcp_servers/test_invalid_yaml.pytests/model_registry/mcp_servers/test_multi_source.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_registry/mcp_servers/test_multi_source.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/model_registry/mcp_servers/constants.py (1)
75-83: Avoid mutable module “constants” to prevent cross-test state bleed.
dict/setconstants can be mutated by tests and cause order-dependent failures. Prefer immutable containers (MappingProxyType,frozenset, tuples).Suggested hardening diff
+from types import MappingProxyType +from typing import Final @@ -MCP_CATALOG_SOURCE: dict = { +MCP_CATALOG_SOURCE: Final = MappingProxyType({ "name": MCP_CATALOG_SOURCE_NAME, "id": MCP_CATALOG_SOURCE_ID, "type": "yaml", "enabled": True, - "properties": {"yamlCatalogPath": MCP_SERVERS_YAML_CATALOG_PATH}, - "labels": [MCP_CATALOG_SOURCE_NAME], -} + "properties": MappingProxyType({"yamlCatalogPath": MCP_SERVERS_YAML_CATALOG_PATH}), + "labels": (MCP_CATALOG_SOURCE_NAME,), +}) @@ -EXPECTED_MCP_SERVER_NAMES: set[str] = {"weather-api", "file-manager", "calculator"} +EXPECTED_MCP_SERVER_NAMES: Final[frozenset[str]] = frozenset({"weather-api", "file-manager", "calculator"})Also applies to: 106-113, 115-133, 157-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/mcp_servers/constants.py` around lines 75 - 83, MCP_CATALOG_SOURCE (and the other module-level dict/set constants in this file) are mutable and can be changed by tests; make them immutable by importing MappingProxyType from types and wrapping top-level dicts with MappingProxyType, convert nested mutable sequences/sets (e.g., "labels") to tuples or frozenset, and ensure "properties" maps are also wrapped with MappingProxyType; keep the original constant names (MCP_CATALOG_SOURCE, MCP_CATALOG_SOURCE_NAME, MCP_CATALOG_SOURCE_ID, MCP_SERVERS_YAML_CATALOG_PATH) but replace their literal dict/list/set values with immutable equivalents so tests cannot mutate module state.
🤖 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/constants.py`:
- Around line 75-83: MCP_CATALOG_SOURCE (and the other module-level dict/set
constants in this file) are mutable and can be changed by tests; make them
immutable by importing MappingProxyType from types and wrapping top-level dicts
with MappingProxyType, convert nested mutable sequences/sets (e.g., "labels") to
tuples or frozenset, and ensure "properties" maps are also wrapped with
MappingProxyType; keep the original constant names (MCP_CATALOG_SOURCE,
MCP_CATALOG_SOURCE_NAME, MCP_CATALOG_SOURCE_ID, MCP_SERVERS_YAML_CATALOG_PATH)
but replace their literal dict/list/set values with immutable equivalents so
tests cannot mutate module state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b07088c-942c-4957-ab9e-3ab655f30ec3
📒 Files selected for processing (2)
tests/model_registry/mcp_servers/constants.pytests/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
dbasunag
approved these changes
Mar 5, 2026
|
Status of building tag latest: success. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Summary
Tests for MCP server catalog functionality:
Related Issues
How it has been tested
Additional Requirements
Summary by CodeRabbit
Tests
Chores