fix: label tests needed update to accomodate mcp labels#1362
Merged
dbasunag merged 2 commits intoopendatahub-io:mainfrom Apr 6, 2026
Merged
fix: label tests needed update to accomodate mcp labels#1362dbasunag merged 2 commits intoopendatahub-io:mainfrom
dbasunag merged 2 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/cherry-pick', '/verified', '/build-push-pr-image', '/hold', '/lgtm', '/wip'} |
Contributor
📝 WalkthroughWalkthroughParameterized the labels endpoint test over Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>
![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: 1
🤖 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/model_catalog/metadata/test_labels_endpoint.py`:
- Around line 29-33: The test signature includes a non-existent fixture
expected_labels_by_asset_type which breaks test collection; remove that fixture
parameter from test_labels_endpoint_default_data and instead derive the expected
labels inside the test using the existing fixtures and the asset_type parameter
(e.g., lookup expected labels from an available fixture or a small mapping keyed
by asset_type), and apply the same fix to the second parametrized case
referenced around line 42 so the test uses only valid fixtures
(model_catalog_rest_url, asset_type) and computes expected labels locally before
assertions.
🪄 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: d7ec1a97-fff2-4102-b0a2-ae84095e6317
📒 Files selected for processing (1)
tests/model_registry/model_catalog/metadata/test_labels_endpoint.py
rhods-ci-bot
added
commented-by-dbasunag
and removed
lgtm-by-sheltoncyril
lgtm-by-SB159
labels
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/model_registry/model_catalog/metadata/conftest.py (1)
16-17: O(n²) complexity and implicit logic in label partitioning.Line 17 uses
label not in mcp_labelswhich performs a linear scan for each label. More critically, it relies on object identity rather than the explicit condition. For consistency with line 16 and clarity:mcp_labels = [label for label in all_labels if label.get("assetType") == "mcp_servers"] - model_labels = [label for label in all_labels if label not in mcp_labels] + model_labels = [label for label in all_labels if label.get("assetType") != "mcp_servers"]This also correctly handles labels without an
assetTypefield (they'll be included inmodel_labels).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_catalog/metadata/conftest.py` around lines 16 - 17, The current partitioning creates mcp_labels and then builds model_labels using "label not in mcp_labels", causing O(n²) behavior and relying on object identity; change the model_labels comprehension to mirror the explicit condition used for mcp_labels by using label.get("assetType") != "mcp_servers" so you filter by the assetType key (which also correctly includes labels missing assetType) rather than membership testing against mcp_labels.
🤖 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/model_catalog/metadata/test_labels_endpoint.py`:
- Around line 29-34: The test function test_labels_endpoint_default_data is
missing a return type annotation; update its signature to include "-> None"
(i.e., def test_labels_endpoint_default_data(... ) -> None:) so it satisfies
mypy's disallow_untyped_defs rule and preserves the existing parameters
(model_catalog_rest_url, expected_labels_by_asset_type, asset_type) and
behavior.
---
Nitpick comments:
In `@tests/model_registry/model_catalog/metadata/conftest.py`:
- Around line 16-17: The current partitioning creates mcp_labels and then builds
model_labels using "label not in mcp_labels", causing O(n²) behavior and relying
on object identity; change the model_labels comprehension to mirror the explicit
condition used for mcp_labels by using label.get("assetType") != "mcp_servers"
so you filter by the assetType key (which also correctly includes labels missing
assetType) rather than membership testing against mcp_labels.
🪄 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: 4db57763-de0a-43c2-84a5-7ddbbf37c151
📒 Files selected for processing (2)
tests/model_registry/model_catalog/metadata/conftest.pytests/model_registry/model_catalog/metadata/test_labels_endpoint.py
mwaykole
previously approved these changes
Apr 6, 2026
kami619
reviewed
Apr 6, 2026
Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/model_registry/model_catalog/metadata/test_labels_endpoint.py (1)
30-35:⚠️ Potential issue | 🟡 MinorAdd explicit
-> Nonereturn annotation on the modified test method.Line 30 defines
test_labels_endpoint_default_datawithout a return type, which violates the repo’s mypy setting for untyped defs.Suggested patch
def test_labels_endpoint_default_data( self, model_catalog_rest_url: list[str], expected_labels_by_asset_type: list[dict[str, Any]], asset_type: Literal["models", "mcp_servers"], - ): + ) -> None:As per coding guidelines, "Mypy: strict-ish settings—add type annotations for new defs, disallow untyped defs/incomplete defs, disallow implicit Optional, and enable error codes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_catalog/metadata/test_labels_endpoint.py` around lines 30 - 35, The test function test_labels_endpoint_default_data is missing an explicit return annotation; update its signature to include "-> None" to satisfy the repository's mypy rules (untyped defs disallowed). Locate the def test_labels_endpoint_default_data(...) in tests/model_registry/model_catalog/metadata/test_labels_endpoint.py and add the "-> None" return type to the function signature while keeping parameters unchanged; no other logic changes are required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/model_registry/model_catalog/metadata/test_labels_endpoint.py`:
- Around line 30-35: The test function test_labels_endpoint_default_data is
missing an explicit return annotation; update its signature to include "-> None"
to satisfy the repository's mypy rules (untyped defs disallowed). Locate the def
test_labels_endpoint_default_data(...) in
tests/model_registry/model_catalog/metadata/test_labels_endpoint.py and add the
"-> None" return type to the function signature while keeping parameters
unchanged; no other logic changes are required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f05c869e-f05a-425d-a5e8-62efbb124d0e
📒 Files selected for processing (3)
tests/model_registry/model_catalog/conftest.pytests/model_registry/model_catalog/metadata/conftest.pytests/model_registry/model_catalog/metadata/test_labels_endpoint.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_registry/model_catalog/metadata/conftest.py
|
Status of building tag latest: success. |
fege
reviewed
Apr 7, 2026
Comment on lines
+22
to
+29
| @pytest.mark.parametrize( | ||
| "expected_labels_by_asset_type, asset_type", | ||
| [ | ||
| pytest.param("models", "models", id="test_models"), | ||
| pytest.param("mcp_servers", "mcp_servers", id="test_mcp_servers"), | ||
| ], | ||
| indirect=["expected_labels_by_asset_type"], | ||
| ) |
Contributor
There was a problem hiding this comment.
probably we should test the case when no asset type is passed. It should return model labels if we are consistent with the other endpoints that have asset_type
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
7 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
Fixes ODH nightly failures and failures against 3.4 smoke
Related Issues
Please review and indicate how it has been tested
Additional Requirements
Summary by CodeRabbit