feat: add fixture to skip test according to rhoai version#392
feat: add fixture to skip test according to rhoai version#392mwaykole wants to merge 10 commits intoopendatahub-io:mainfrom
Conversation
Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
This comment was marked as off-topic.
This comment was marked as off-topic.
for more information, see https://pre-commit.ci
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/verified', '/build-push-pr-image', '/wip', '/lgtm', '/hold', '/cherry-pick'} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/conftest.py(2 hunks)utilities/version_utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/conftest.py:200-216
Timestamp: 2025-06-05T14:32:40.247Z
Learning: In the opendatahub-tests repository, the test fixtures should raise exceptions on cleanup failures rather than just logging warnings. The user fege prefers strict cleanup behavior where tests fail if cleanup doesn't work properly, rather than silently continuing.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: israel-hdez
PR: opendatahub-io/opendatahub-tests#346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper `create_isvc` (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.
tests/conftest.py (2)
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.
🧬 Code Graph Analysis (1)
tests/conftest.py (1)
utilities/operator_utils.py (2)
get_csv_related_images(32-54)get_cluster_service_version(14-29)
🪛 Pylint (3.3.7)
utilities/version_utils.py
[refactor] 4-4: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[refactor] 38-38: Either all return statements in a function should return an expression, or none of them should.
(R1710)
🔇 Additional comments (5)
tests/conftest.py (5)
49-49: LGTM: Clean import addition.The import of
get_cluster_service_versionis correctly added and will be used by the newget_rhoai_csv_versionfixture.
75-91: Well-designed fixture with proper dependency ordering.The fixture correctly:
- Uses session scope and autouse for efficiency
- Depends on
dsci_resourceto ensure proper initialization order- Handles distribution-specific CSV prefix selection
- Stores the version in global config for reuse
The implementation properly leverages the existing
get_cluster_service_versionutility function and follows established patterns in the codebase.
93-107: Robust version comparison implementation.The
_compare_versionsfunction is well-implemented with:
- Proper use of the
packaging.versionmodule for semantic version comparison- Comprehensive error handling with logging
- Clear return value semantics (-1, 0, 1)
- Informative logging for debugging
109-129: Well-designed fixture following pytest best practices.The fixture properly:
- Validates required parameters with clear error messages
- Uses appropriate scope (function-level)
- Implements clear conditional logic for downstream-only skipping
- Provides comprehensive docstring with usage examples
The parameter validation aligns with the learnings about strict error handling preferences in this repository.
131-152: Consistent implementation with proper error handling.This fixture mirrors the design of
skip_if_downstream_version_greater_thanwith consistent:
- Parameter validation
- Distribution checking
- Version comparison logic
- Documentation style
The implementation correctly handles the version comparison direction and maintains consistency with the companion fixture.
Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
utilities/version_utils.py (2)
9-11: Missing upstream implementation as documented.The docstring claims support for "both UPSTREAM and DOWNSTREAM distributions" with "UPSTREAM: Uses environment variable control", but the implementation only handles downstream. This is the same issue flagged in previous reviews.
44-46: Fix incomplete docstring.The docstring is malformed - line 46 starts with "- DOWNSTREAM: Uses RHODS version checking" without proper context or introduction.
Apply this fix to complete the docstring:
def skip_if_version_greater_than(max_version: str) -> tuple[Any, ...]: """ Skip tests if version is greater than specified maximum. - - DOWNSTREAM: Uses RHODS version checking + + Works for DOWNSTREAM distribution: + - DOWNSTREAM: Uses RHODS version checking
🧹 Nitpick comments (1)
utilities/version_utils.py (1)
29-37: Remove unnecessary else clauses after return statements.The
elseclauses afterreturnstatements are unnecessary and can be simplified for better readability.Apply this refactor to both functions:
if distribution == "downstream": # Use existing RHODS version checking fixtures return ( pytest.mark.parametrize("skip_if_downstream_version_less_than", [min_version], indirect=True), pytest.mark.usefixtures("skip_if_downstream_version_less_than"), ) - else: - # For non-downstream distributions, return empty tuple - return () + # For non-downstream distributions, return empty tuple + return ()Also applies to: 65-73
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utilities/version_utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/conftest.py:200-216
Timestamp: 2025-06-05T14:32:40.247Z
Learning: In the opendatahub-tests repository, the test fixtures should raise exceptions on cleanup failures rather than just logging warnings. The user fege prefers strict cleanup behavior where tests fail if cleanup doesn't work properly, rather than silently continuing.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: israel-hdez
PR: opendatahub-io/opendatahub-tests#346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper `create_isvc` (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.
🪛 Pylint (3.3.7)
utilities/version_utils.py
[refactor] 29-37: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 65-73: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
utilities/version_utils.py (1)
23-26: Update docstring to reflect actual implementation.The docstring claims upstream support that isn't implemented, which was previously flagged in review comments.
- Works for both UPSTREAM and DOWNSTREAM distributions: - - DOWNSTREAM: Uses RHODS version checking - - UPSTREAM: Uses environment variable control + Works for DOWNSTREAM distribution: + - DOWNSTREAM: Uses RHODS version checking
🧹 Nitpick comments (2)
utilities/version_utils.py (2)
49-51: Remove unnecessary else clause for cleaner code.- else: - # For non-downstream distributions, return empty tuple - return () + # For non-downstream distributions, return empty tuple + return ()
84-86: Remove unnecessary else clause for cleaner code.- else: - # For non-downstream distributions, return empty tuple - return () + # For non-downstream distributions, return empty tuple + return ()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/conftest.py(2 hunks)utilities/version_utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/conftest.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/conftest.py:200-216
Timestamp: 2025-06-05T14:32:40.247Z
Learning: In the opendatahub-tests repository, the test fixtures should raise exceptions on cleanup failures rather than just logging warnings. The user fege prefers strict cleanup behavior where tests fail if cleanup doesn't work properly, rather than silently continuing.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: israel-hdez
PR: opendatahub-io/opendatahub-tests#346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper `create_isvc` (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.
🪛 Ruff (0.11.9)
utilities/version_utils.py
15-15: Undefined name LOGGER
(F821)
🪛 Flake8 (7.2.0)
utilities/version_utils.py
[error] 15-15: undefined name 'LOGGER'
(F821)
🪛 Pylint (3.3.7)
utilities/version_utils.py
[error] 15-15: Undefined variable 'LOGGER'
(E0602)
[refactor] 43-51: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 78-86: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
utilities/version_utils.py (1)
25-27: Update documentation to match implementation.The docstring claims upstream support that isn't implemented. The current implementation only handles downstream distributions.
- Works for both UPSTREAM and DOWNSTREAM distributions: - - DOWNSTREAM: Uses RHODS version checking - - UPSTREAM: Uses environment variable control + Works for DOWNSTREAM distribution: + - DOWNSTREAM: Uses RHODS version checking- Tuple of pytest marks for downstream OR skipif mark for upstream + Tuple of pytest marks for downstream or empty tupleAlso applies to: 37-38
🧹 Nitpick comments (2)
utilities/version_utils.py (2)
45-53: Remove unnecessary else clause after return.The else clause is unnecessary since the preceding if block contains a return statement.
if distribution == "downstream": # Use existing RHODS version checking fixtures return ( pytest.mark.parametrize("skip_if_downstream_version_less_than", [min_version], indirect=True), pytest.mark.usefixtures("skip_if_downstream_version_less_than"), ) - else: - # For non-downstream distributions, return empty tuple - return () + # For non-downstream distributions, return empty tuple + return ()
80-88: Remove unnecessary else clause after return.Same issue as the previous function - the else clause is unnecessary after a return statement.
if distribution == "downstream": # Use existing RHODS version checking fixtures return ( pytest.mark.parametrize("skip_if_downstream_version_greater_than", [max_version], indirect=True), pytest.mark.usefixtures("skip_if_downstream_version_greater_than"), ) - else: - # For non-downstream distributions, return empty tuple - return () + # For non-downstream distributions, return empty tuple + return ()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utilities/version_utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/conftest.py:200-216
Timestamp: 2025-06-05T14:32:40.247Z
Learning: In the opendatahub-tests repository, the test fixtures should raise exceptions on cleanup failures rather than just logging warnings. The user fege prefers strict cleanup behavior where tests fail if cleanup doesn't work properly, rather than silently continuing.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: israel-hdez
PR: opendatahub-io/opendatahub-tests#346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper `create_isvc` (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.
🪛 Ruff (0.11.9)
utilities/version_utils.py
4-4: Undefined name get_logger
(F821)
🪛 Flake8 (7.2.0)
utilities/version_utils.py
[error] 4-4: undefined name 'get_logger'
(F821)
🪛 Pylint (3.3.7)
utilities/version_utils.py
[error] 4-4: Undefined variable 'get_logger'
(E0602)
[refactor] 45-53: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 80-88: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🪛 GitHub Actions: Tox Tests
utilities/version_utils.py
[error] 4-4: NameError: name 'get_logger' is not defined. Import or define 'get_logger' before using it.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
utilities/version_utils.py (1)
22-58: Update docstring to reflect actual implementation.The docstring claims upstream support but the implementation only handles downstream cases. Update the documentation to accurately reflect the current functionality.
- Works for both UPSTREAM and DOWNSTREAM distributions: - - DOWNSTREAM: Uses RHODS version checking - - UPSTREAM: Uses environment variable control + Works for DOWNSTREAM distribution: + - DOWNSTREAM: Uses RHODS version checking - Returns: - Tuple of pytest marks for downstream OR skipif mark for upstream + Returns: + Tuple of pytest marks for downstream or empty tuple for other distributionsOptional: Simplify control flow by removing unnecessary else.
if distribution == "downstream": # Use existing RHODS version checking fixtures return ( pytest.mark.parametrize("skip_if_downstream_version_less_than", [min_version], indirect=True), pytest.mark.usefixtures("skip_if_downstream_version_less_than"), ) - else: - # For non-downstream distributions, return empty tuple - return () + # For non-downstream distributions, return empty tuple + return ()
🧹 Nitpick comments (1)
utilities/version_utils.py (1)
60-92: LGTM! Consistent implementation.The function correctly implements version-based test skipping for downstream distributions with appropriate error handling.
Optional: Simplify control flow for consistency.
if distribution == "downstream": # Use existing RHODS version checking fixtures return ( pytest.mark.parametrize("skip_if_downstream_version_greater_than", [max_version], indirect=True), pytest.mark.usefixtures("skip_if_downstream_version_greater_than"), ) - else: - # For non-downstream distributions, return empty tuple - return () + # For non-downstream distributions, return empty tuple + return ()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utilities/version_utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/conftest.py:200-216
Timestamp: 2025-06-05T14:32:40.247Z
Learning: In the opendatahub-tests repository, the test fixtures should raise exceptions on cleanup failures rather than just logging warnings. The user fege prefers strict cleanup behavior where tests fail if cleanup doesn't work properly, rather than silently continuing.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: israel-hdez
PR: opendatahub-io/opendatahub-tests#346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper `create_isvc` (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.
🪛 Pylint (3.3.7)
utilities/version_utils.py
[refactor] 46-54: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 81-89: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🔇 Additional comments (2)
utilities/version_utils.py (2)
1-5: LGTM! Import issue resolved.The undefined
get_loggerissue from previous reviews has been properly fixed with the correct import fromsimple_logger.logger.
8-20: LGTM! Clean version comparison implementation.The function correctly implements semantic version comparison with proper error handling and logging.
|
/verified |
| return RedactedString(value=get_openshift_token()) | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session", autouse=True) |
There was a problem hiding this comment.
I don't understand this PR. We branch based on version. Why not remove not needed tests directly from the branches?
There was a problem hiding this comment.
Yes, but sometimes we submit a PR for an RHOAI N+2 branch feature that is expected to fail and create noise, since the dependent code from other components hasn't been merged yet. In such cases, this approach would be more efficient than manually adding or removing skip flags, IMO.
There was a problem hiding this comment.
This is not a good idea. Your test branch should reflect associated product branch behavior. Adding programmatic skips is not a good idea. Please use jira marker or xfail.
| return RedactedString(value=get_openshift_token()) | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session", autouse=True) |
There was a problem hiding this comment.
This is not a good idea. Your test branch should reflect associated product branch behavior. Adding programmatic skips is not a good idea. Please use jira marker or xfail.
|
after discussing with @dbasunag closing the pr seems like we will have some dead code |
3 participants
No description provided.