feat: Separate out cluster, operator and component health checks#1133
Merged
dbasunag merged 4 commits intoopendatahub-io:mainfrom Feb 24, 2026
Merged
feat: Separate out cluster, operator and component health checks#1133dbasunag 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', '/wip', '/verified', '/cherry-pick', '/build-push-pr-image', '/lgtm'} |
Contributor
📝 WalkthroughWalkthroughTests are reorganized to separate cluster health checks from operator-specific health checks. Two new pytest markers ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/model_registry/ai_hub_health/test_mr_health_check.py (1)
44-48:⚠️ Potential issue | 🟡 Minor
test_mr_pods_healthis missing a mandatory docstring.Per coding guidelines, every test MUST have a docstring explaining what it tests, using Given-When-Then format. Also, the
-> Nonereturn type annotation is missing (mypy strict).📝 Proposed fix
`@pytest.mark.component_health` - def test_mr_pods_health(self, admin_client: DynamicClient): + def test_mr_pods_health(self, admin_client: DynamicClient) -> None: + """Verify Model Registry pods are healthy. + + Given: A running OpenDataHub/RHOAI instance with Model Registry enabled. + When: Querying all pods in the model registry namespace. + Then: All pods should be in Running/Completed state. + """ namespace = py_config["model_registry_namespace"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/ai_hub_health/test_mr_health_check.py` around lines 44 - 48, Add a mandatory docstring and return type annotation to the test_mr_pods_health function: update the function signature to include "-> None" and add a triple-quoted docstring above the body following Given-When-Then that states the initial state (Given the model registry namespace from py_config), the action (When we check pods via wait_for_pods_running with admin_client), and the expected outcome (Then all pods are in Running state), keeping references to test_mr_pods_health, admin_client, py_config, and wait_for_pods_running so the purpose and intent are clear.
🧹 Nitpick comments (1)
tests/model_registry/ai_hub_health/test_mr_health_check.py (1)
44-44: Redundant@pytest.mark.component_healthontest_mr_pods_health.The class
TestMrDefaultis already decorated with@pytest.mark.component_healthat Line 16, which propagates to all methods. The method-level marker at Line 44 is a no-op.♻️ Proposed fix
- `@pytest.mark.component_health` def test_mr_pods_health(self, admin_client: DynamicClient):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/ai_hub_health/test_mr_health_check.py` at line 44, The method-level pytest.mark.component_health on test_mr_pods_health is redundant because the TestMrDefault class is already decorated with `@pytest.mark.component_health`; remove the duplicate marker on the test method `test_mr_pods_health` so the class-level marker applies to all methods and avoids noisy/no-op decorators.
🤖 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/cluster_health/test_operator_health.py`:
- Around line 13-20: Add missing docstrings and explicit return type annotations
to the test functions (e.g., test_data_science_cluster_initialization_healthy
and test_data_science_cluster_healthy and any other test_* functions in this
file); each test must have a triple-quoted docstring in Given-When-Then format
describing the preconditions, action, and expected outcome, and the function
signature must include "-> None". Update the function definitions and ensure the
docstrings are placed immediately below the def line for linters and mypy strict
compliance.
- Around line 37-39: The test currently ignores the return value of
wait_for_pods_running (typed bool | None) so a None (e.g., from swallowed
TimeoutExpiredError) will silently pass; update test_pods_cluster_healthy to
capture the result from wait_for_pods_running and assert it is True (or
explicitly fail on None) so the test fails when pods aren't confirmed running —
reference wait_for_pods_running and test_pods_cluster_healthy and ensure the
assertion includes a clear message (e.g., "Pods not running" or
"wait_for_pods_running returned None/False").
---
Outside diff comments:
In `@tests/model_registry/ai_hub_health/test_mr_health_check.py`:
- Around line 44-48: Add a mandatory docstring and return type annotation to the
test_mr_pods_health function: update the function signature to include "-> None"
and add a triple-quoted docstring above the body following Given-When-Then that
states the initial state (Given the model registry namespace from py_config),
the action (When we check pods via wait_for_pods_running with admin_client), and
the expected outcome (Then all pods are in Running state), keeping references to
test_mr_pods_health, admin_client, py_config, and wait_for_pods_running so the
purpose and intent are clear.
---
Nitpick comments:
In `@tests/model_registry/ai_hub_health/test_mr_health_check.py`:
- Line 44: The method-level pytest.mark.component_health on test_mr_pods_health
is redundant because the TestMrDefault class is already decorated with
`@pytest.mark.component_health`; remove the duplicate marker on the test method
`test_mr_pods_health` so the class-level marker applies to all methods and
avoids noisy/no-op decorators.
fege
reviewed
Feb 23, 2026
fege
requested changes
Feb 23, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/model_registry/component_health/test_mr_health_check.py (1)
46-50:⚠️ Potential issue | 🟡 Minor
test_mr_pods_healthhas four issues introduced or exposed by this change
- Redundant marker (line 46):
@pytest.mark.component_healthis already applied at the class level (line 16); the method-level decorator is a no-op and should be removed.- Missing
-> None(line 47): Every other test method in this class was updated to include an explicit return type in this PR;test_mr_pods_healthwas missed.- Missing docstring: The coding guideline requires every test to have a docstring; this method has none.
- Stale log message (line 49):
"for cluster health"should read"for component health"after the rename.🔧 Proposed fix
- `@pytest.mark.component_health` - def test_mr_pods_health(self, admin_client: DynamicClient): + def test_mr_pods_health(self, admin_client: DynamicClient) -> None: + """ + Given a DynamicClient, + When the ModelRegistry pods are retrieved from the configured namespace, + Then all pods should be in Running state. + """ namespace = py_config["model_registry_namespace"] - LOGGER.info(f"Testing Pods in namespace {namespace} for cluster health") + LOGGER.info(f"Testing Pods in namespace {namespace} for component health") wait_for_pods_running(admin_client=admin_client, namespace_name=namespace)As per coding guidelines: "
tests/**/*.py: Every test MUST have a docstring explaining what it tests" and "Add type annotations to test code and fixtures (mypy strict enforced)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/component_health/test_mr_health_check.py` around lines 46 - 50, Remove the redundant method-level pytest marker by deleting the `@pytest.mark.component_health` decorator above test_mr_pods_health, add an explicit return type annotation `-> None` to the `def test_mr_pods_health` signature, add a short docstring at the start of `test_mr_pods_health` describing what this test verifies (e.g., that pods in the model registry namespace reach Running), and update the LOGGER.info message (the string passed to LOGGER.info) to say "for component health" instead of "for cluster health"; the relevant symbols are the `test_mr_pods_health` function, the LOGGER.info call, and the existing call to `wait_for_pods_running`.
🧹 Nitpick comments (1)
tests/model_registry/component_health/test_mr_health_check.py (1)
19-19: Docstrings don't follow Given-When-Then formatAll three method docstrings (
test_mr_management_state,test_mr_namespace_exists_and_active,test_mr_condition_in_dsc) are plain one-line descriptions. Since the signatures were updated in this PR, updating the docstrings to the required format is a natural fit.Example for
test_mr_management_state:""" Given a DataScienceCluster resource, When the ModelRegistry component is configured, Then its managementState should be MANAGED. """As per coding guidelines: "
tests/**/*.py: Use Given-When-Then format in test docstrings for behavioral clarity" and "Write Google-format docstrings for tests and fixtures."Also applies to: 28-28, 38-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/component_health/test_mr_health_check.py` at line 19, Update the one-line docstrings for the test functions test_mr_management_state, test_mr_namespace_exists_and_active, and test_mr_condition_in_dsc to use Given-When-Then Google-style test docstrings: for each function replace the single-line description with a three-line docstring that starts with "Given ..." describing the precondition (DataScienceCluster resource or component state), "When ..." describing the action/trigger (ModelRegistry configured or inspected), and "Then ..." describing the expected outcome (managementState is MANAGED, namespace exists and is active, specific condition present in DSC), ensuring each docstring follows the Google-format multi-line convention used across tests/**/*.py.
🤖 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/cluster_health/test_cluster_health.py`:
- Around line 10-13: The test_cluster_node_healthy docstring is a plain
description; update it to a Given-When-Then formatted docstring describing the
precondition (Given a list of cluster nodes), the action (When the health check
is performed), and the expectation (Then all nodes should be reported healthy
and any failing assertions made). Edit the docstring in the function
test_cluster_node_healthy to follow that structure and mention the behavior
being asserted so reviewers and readers can quickly see the test's intent.
---
Outside diff comments:
In `@tests/model_registry/component_health/test_mr_health_check.py`:
- Around line 46-50: Remove the redundant method-level pytest marker by deleting
the `@pytest.mark.component_health` decorator above test_mr_pods_health, add an
explicit return type annotation `-> None` to the `def test_mr_pods_health`
signature, add a short docstring at the start of `test_mr_pods_health`
describing what this test verifies (e.g., that pods in the model registry
namespace reach Running), and update the LOGGER.info message (the string passed
to LOGGER.info) to say "for component health" instead of "for cluster health";
the relevant symbols are the `test_mr_pods_health` function, the LOGGER.info
call, and the existing call to `wait_for_pods_running`.
---
Nitpick comments:
In `@tests/model_registry/component_health/test_mr_health_check.py`:
- Line 19: Update the one-line docstrings for the test functions
test_mr_management_state, test_mr_namespace_exists_and_active, and
test_mr_condition_in_dsc to use Given-When-Then Google-style test docstrings:
for each function replace the single-line description with a three-line
docstring that starts with "Given ..." describing the precondition
(DataScienceCluster resource or component state), "When ..." describing the
action/trigger (ModelRegistry configured or inspected), and "Then ..."
describing the expected outcome (managementState is MANAGED, namespace exists
and is active, specific condition present in DSC), ensuring each docstring
follows the Google-format multi-line convention used across tests/**/*.py.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/cluster_health/test_cluster_health.pytests/cluster_health/test_operator_health.pytests/model_registry/component_health/__init__.pytests/model_registry/component_health/conftest.pytests/model_registry/component_health/test_mr_health_check.pytests/model_registry/component_health/test_mr_operator_health.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cluster_health/test_operator_health.py
fege
approved these changes
Feb 23, 2026
aloganat
reviewed
Feb 23, 2026
aloganat
approved these changes
Feb 23, 2026
kpunwatk
approved these changes
Feb 24, 2026
|
Status of building tag latest: success. |
Collaborator
Author
|
/cherry-pick 3.3 |
Contributor
|
Error cherry-picking. |
Contributor
|
|
dbasunag
added a commit
to dbasunag/opendatahub-tests
that referenced
this pull request
Feb 25, 2026
…ndatahub-io#1133) * feat: Separate out cluster, operator and component health checks * fix: Addressed review comments
dbasunag
added a commit
that referenced
this pull request
Feb 25, 2026
…) (#1142) * feat: Separate out cluster, operator and component health checks * fix: Addressed review comments
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
5 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.
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
Tests
Chores