Add tests to archive and unarchive models, versions#329
Add tests to archive and unarchive models, versions#329dbasunag merged 10 commits intoopendatahub-io:mainfrom
Conversation
WalkthroughThe changes introduce centralized resource attribute validation for model registry REST API tests by adding a utility function and custom exceptions. Test fixtures and constants are updated to support custom properties and more dynamic update scenarios. New tests are added for model version and registered model updates, and validation logic is refactored for consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant TestCase
participant Fixture
participant Utils
participant Exception
TestCase->>Fixture: Request updated resource (PATCH)
Fixture->>Utils: Call validate_resource_attributes(expected, actual, name)
Utils->>Utils: Compare expected vs actual attributes
alt Mismatch found
Utils->>Exception: Raise ResourceValueMismatch
else All match
Utils-->>TestCase: Validation succeeds
end
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/hold', '/verified', '/lgtm', '/wip'} |
![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
🧹 Nitpick comments (5)
tests/model_registry/rest_api/utils.py (1)
94-104: Add docstring for better documentation.The validation function is well-implemented with efficient use of walrus operator and list comprehension. However, it would benefit from a docstring to document its purpose and parameters.
def validate_resource_attributes( expected_params: dict[str, Any], actual_resource_data: dict[str, Any], resource_name: str ) -> None: + """ + Validate that actual resource data matches expected parameters. + + Args: + expected_params: Dictionary of expected parameter values + actual_resource_data: Dictionary of actual resource data to validate + resource_name: Name of the resource being validated (for logging) + + Raises: + ResourceValueMismatch: If any expected parameter doesn't match actual data + """🧰 Tools
🪛 Pylint (3.3.7)
[convention] 94-94: Missing function or method docstring
(C0116)
tests/model_registry/rest_api/conftest.py (2)
52-65: Consider adding docstring for consistency.The
updated_model_versionfixture follows the established pattern well with proper ID extraction and assertion. Consider adding a docstring to document its purpose.@pytest.fixture() def updated_model_version( request: pytest.FixtureRequest, model_registry_rest_url: str, model_registry_rest_headers: dict[str, str], registered_model_rest_api: dict[str, Any], ) -> dict[str, Any]: + """Update a model version via PATCH request using test parameters."""🧰 Tools
🪛 Pylint (3.3.7)
[convention] 60-60: Line too long (104/100)
(C0301)
[convention] 53-53: Missing function or method docstring
(C0116)
[warning] 55-55: Redefining name 'model_registry_rest_url' from outer scope (line 11)
(W0621)
[warning] 56-56: Redefining name 'model_registry_rest_headers' from outer scope (line 17)
(W0621)
[warning] 57-57: Redefining name 'registered_model_rest_api' from outer scope (line 26)
(W0621)
68-81: Consider adding docstring for consistency.The
updated_registered_modelfixture is well-implemented and consistent with the pattern. Consider adding a docstring to document its purpose.@pytest.fixture() def updated_registered_model( request: pytest.FixtureRequest, model_registry_rest_url: str, model_registry_rest_headers: dict[str, str], registered_model_rest_api: dict[str, Any], ) -> dict[str, Any]: + """Update a registered model via PATCH request using test parameters."""🧰 Tools
🪛 Pylint (3.3.7)
[convention] 69-69: Missing function or method docstring
(C0116)
[warning] 71-71: Redefining name 'model_registry_rest_url' from outer scope (line 11)
(W0621)
[warning] 72-72: Redefining name 'model_registry_rest_headers' from outer scope (line 17)
(W0621)
[warning] 73-73: Redefining name 'registered_model_rest_api' from outer scope (line 26)
(W0621)
tests/model_registry/rest_api/test_model_registry_rest_api.py (2)
6-6: Import addition looks good, but consider fixing import order.The import of
validate_resource_attributesis necessary for the refactoring and correctly used throughout the file.However, consider addressing the import order issue flagged by pylint - third-party imports should come before first-party imports.
102-108: Good refactoring with improved documentation.The updated test method uses the centralized validation utility and has a more descriptive docstring.
Consider wrapping the long line (line 107) to comply with the 100-character limit:
- validate_resource_attributes( - expected_params=expected_param, actual_resource_data=updated_model_artifact, resource_name="model artifact" - ) + validate_resource_attributes( + expected_params=expected_param, + actual_resource_data=updated_model_artifact, + resource_name="model artifact" + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 107-107: Line too long (119/100)
(C0301)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/model_registry/rest_api/conftest.py(2 hunks)tests/model_registry/rest_api/constants.py(2 hunks)tests/model_registry/rest_api/test_model_registry_rest_api.py(3 hunks)tests/model_registry/rest_api/utils.py(2 hunks)utilities/exceptions.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/model_registry/rest_api/test_model_registry_rest_api.py (2)
tests/model_registry/rest_api/utils.py (1)
validate_resource_attributes(94-104)tests/model_registry/rest_api/conftest.py (4)
registered_model_rest_api(26-33)updated_model_artifact(37-49)updated_model_version(53-65)updated_registered_model(69-81)
tests/model_registry/rest_api/utils.py (1)
utilities/exceptions.py (1)
ResourceValueMismatch(125-128)
tests/model_registry/rest_api/conftest.py (1)
tests/model_registry/rest_api/utils.py (1)
execute_model_registry_patch_command(17-31)
🪛 Pylint (3.3.7)
utilities/exceptions.py
[warning] 128-128: Unnecessary pass statement
(W0107)
tests/model_registry/rest_api/test_model_registry_rest_api.py
[error] 8-8: Unable to import 'simple_logger.logger'
(E0401)
[convention] 8-8: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTER", "tests.model_registry.rest_api.utils.validate_resource_attributes", "utilities.constants.DscComponents"
(C0411)
[convention] 107-107: Line too long (119/100)
(C0301)
[convention] 142-142: Line too long (117/100)
(C0301)
[convention] 146-146: Line too long (117/100)
(C0301)
[convention] 181-181: Line too long (116/100)
(C0301)
tests/model_registry/rest_api/utils.py
[error] 5-5: Unable to import 'simple_logger.logger'
(E0401)
[convention] 94-94: Missing function or method docstring
(C0116)
tests/model_registry/rest_api/conftest.py
[convention] 60-60: Line too long (104/100)
(C0301)
[convention] 53-53: Missing function or method docstring
(C0116)
[warning] 55-55: Redefining name 'model_registry_rest_url' from outer scope (line 11)
(W0621)
[warning] 56-56: Redefining name 'model_registry_rest_headers' from outer scope (line 17)
(W0621)
[warning] 57-57: Redefining name 'registered_model_rest_api' from outer scope (line 26)
(W0621)
[convention] 69-69: Missing function or method docstring
(C0116)
[warning] 71-71: Redefining name 'model_registry_rest_url' from outer scope (line 11)
(W0621)
[warning] 72-72: Redefining name 'model_registry_rest_headers' from outer scope (line 17)
(W0621)
[warning] 73-73: Redefining name 'registered_model_rest_api' from outer scope (line 26)
(W0621)
🔇 Additional comments (7)
utilities/exceptions.py (1)
125-128: LGTM! Exception class follows established patterns.The new
ResourceValueMismatchexception class is well-defined and follows the consistent pattern used by other exception classes in this file. Thepassstatement is necessary for the empty class body.🧰 Tools
🪛 Pylint (3.3.7)
[warning] 128-128: Unnecessary pass statement
(W0107)
tests/model_registry/rest_api/constants.py (2)
5-5: Good type annotation update.Changing the type annotation from
dict[str, str]todict[str, Any]is appropriate since the dictionary now contains nested structures in thecustomPropertiesfields.
9-12: Well-structured custom properties addition.The custom properties are consistently structured across all three constants with appropriate metadata types (
MetadataBoolValueandMetadataStringValue). This enhances the test coverage for custom property validation scenarios.Also applies to: 19-22, 33-36
tests/model_registry/rest_api/utils.py (1)
12-12: Import addition looks good.The import of
ResourceValueMismatchis correctly placed and supports the new validation functionality.tests/model_registry/rest_api/conftest.py (1)
36-36: Good scope change for more granular testing.Changing the scope from
"class"to function-level (default) allows for more isolated test execution, which is appropriate for update operations that might have side effects.tests/model_registry/rest_api/test_model_registry_rest_api.py (2)
11-17: Well-structured test data for custom properties.The
CUSTOM_PROPERTYconstant provides comprehensive coverage for different metadata types (bool, string, double) which is excellent for testing custom properties functionality.
70-74: Excellent refactoring to centralize validation logic.The replacement of manual validation with the
validate_resource_attributesutility function improves code maintainability and consistency across tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/model_registry/rest_api/utils.py (1)
94-104: Add docstring for better documentation.The validation function logic is well-implemented and serves its purpose effectively. However, adding a docstring would improve maintainability and help other developers understand the function's purpose and parameters.
Add a docstring like this:
def validate_resource_attributes( expected_params: dict[str, Any], actual_resource_data: dict[str, Any], resource_name: str ) -> None: + """ + Validate that expected parameters match actual resource data. + + Args: + expected_params: Dictionary of expected key-value pairs + actual_resource_data: Dictionary of actual resource data to validate + resource_name: Name of the resource for logging and error messages + + Raises: + ResourceValueMismatch: If any expected values don't match actual values + """ errors: list[dict[str, list[Any]]]🧰 Tools
🪛 Pylint (3.3.7)
[convention] 94-94: Missing function or method docstring
(C0116)
tests/model_registry/rest_api/conftest.py (2)
52-65: Add docstring and fix line length.The fixture implementation is correct and follows the established pattern. However, it needs a docstring and has a line length issue.
Apply this diff to add documentation and fix formatting:
@pytest.fixture() def updated_model_version( request: pytest.FixtureRequest, model_registry_rest_url: str, model_registry_rest_headers: dict[str, str], registered_model_rest_api: dict[str, Any], ) -> dict[str, Any]: + """Update a model version using PATCH request with provided parameters.""" model_version_id = registered_model_rest_api["model_version"]["id"] - assert model_version_id, f"Model version id not found: {registered_model_rest_api['model_version']}" + assert model_version_id, ( + f"Model version id not found: {registered_model_rest_api['model_version']}" + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 60-60: Line too long (104/100)
(C0301)
[convention] 53-53: Missing function or method docstring
(C0116)
[warning] 55-55: Redefining name 'model_registry_rest_url' from outer scope (line 11)
(W0621)
[warning] 56-56: Redefining name 'model_registry_rest_headers' from outer scope (line 17)
(W0621)
[warning] 57-57: Redefining name 'registered_model_rest_api' from outer scope (line 26)
(W0621)
68-81: Add docstring for consistency.The fixture implementation is correct but should include a docstring for consistency with other fixtures.
Add a docstring:
@pytest.fixture() def updated_registered_model( request: pytest.FixtureRequest, model_registry_rest_url: str, model_registry_rest_headers: dict[str, str], registered_model_rest_api: dict[str, Any], ) -> dict[str, Any]: + """Update a registered model using PATCH request with provided parameters.""" model_id = registered_model_rest_api["register_model"]["id"]🧰 Tools
🪛 Pylint (3.3.7)
[convention] 69-69: Missing function or method docstring
(C0116)
[warning] 71-71: Redefining name 'model_registry_rest_url' from outer scope (line 11)
(W0621)
[warning] 72-72: Redefining name 'model_registry_rest_headers' from outer scope (line 17)
(W0621)
[warning] 73-73: Redefining name 'registered_model_rest_api' from outer scope (line 26)
(W0621)
tests/model_registry/rest_api/test_model_registry_rest_api.py (2)
102-108: Fix line length issue.The refactoring and added docstring are good improvements, but there's a line length violation that should be addressed.
Apply this diff to fix the line length:
- validate_resource_attributes( - expected_params=expected_param, actual_resource_data=updated_model_artifact, resource_name="model artifact" - ) + validate_resource_attributes( + expected_params=expected_param, + actual_resource_data=updated_model_artifact, + resource_name="model artifact" + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 107-107: Line too long (119/100)
(C0301)
110-147: Excellent test coverage for archive/unarchive functionality! Fix line length.This test method directly addresses the PR objective by testing model version updates including archive/unarchive functionality (ARCHIVED/LIVE states). The test coverage is comprehensive with description updates, state changes, and custom properties.
Fix the line length issue on line 146:
- validate_resource_attributes( - expected_params=expected_param, actual_resource_data=updated_model_version, resource_name="model version" - ) + validate_resource_attributes( + expected_params=expected_param, + actual_resource_data=updated_model_version, + resource_name="model version" + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 146-146: Line too long (117/100)
(C0301)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/model_registry/rest_api/conftest.py(2 hunks)tests/model_registry/rest_api/constants.py(2 hunks)tests/model_registry/rest_api/test_model_registry_rest_api.py(3 hunks)tests/model_registry/rest_api/utils.py(2 hunks)utilities/exceptions.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_registry/rest_api/constants.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/model_registry/rest_api/conftest.py (1)
tests/model_registry/rest_api/utils.py (1)
execute_model_registry_patch_command(17-31)
tests/model_registry/rest_api/test_model_registry_rest_api.py (2)
tests/model_registry/rest_api/utils.py (1)
validate_resource_attributes(94-104)tests/model_registry/rest_api/conftest.py (4)
registered_model_rest_api(26-33)updated_model_artifact(37-49)updated_model_version(53-65)updated_registered_model(69-81)
tests/model_registry/rest_api/utils.py (1)
utilities/exceptions.py (1)
ResourceValueMismatch(125-128)
🪛 Pylint (3.3.7)
tests/model_registry/rest_api/conftest.py
[convention] 60-60: Line too long (104/100)
(C0301)
[convention] 53-53: Missing function or method docstring
(C0116)
[warning] 55-55: Redefining name 'model_registry_rest_url' from outer scope (line 11)
(W0621)
[warning] 56-56: Redefining name 'model_registry_rest_headers' from outer scope (line 17)
(W0621)
[warning] 57-57: Redefining name 'registered_model_rest_api' from outer scope (line 26)
(W0621)
[convention] 69-69: Missing function or method docstring
(C0116)
[warning] 71-71: Redefining name 'model_registry_rest_url' from outer scope (line 11)
(W0621)
[warning] 72-72: Redefining name 'model_registry_rest_headers' from outer scope (line 17)
(W0621)
[warning] 73-73: Redefining name 'registered_model_rest_api' from outer scope (line 26)
(W0621)
tests/model_registry/rest_api/test_model_registry_rest_api.py
[error] 8-8: Unable to import 'simple_logger.logger'
(E0401)
[convention] 8-8: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTER", "tests.model_registry.rest_api.utils.validate_resource_attributes", "utilities.constants.DscComponents"
(C0411)
[convention] 107-107: Line too long (119/100)
(C0301)
[convention] 146-146: Line too long (117/100)
(C0301)
tests/model_registry/rest_api/utils.py
[error] 5-5: Unable to import 'simple_logger.logger'
(E0401)
[convention] 94-94: Missing function or method docstring
(C0116)
utilities/exceptions.py
[warning] 128-128: Unnecessary pass statement
(W0107)
🔇 Additional comments (5)
utilities/exceptions.py (1)
125-128: LGTM! Well-defined exception class.The new
ResourceValueMismatchexception follows the established pattern in this file and serves its purpose well for the validation utility. Thepassstatement is appropriate here as no additional behavior is needed beyond the baseExceptionclass.🧰 Tools
🪛 Pylint (3.3.7)
[warning] 128-128: Unnecessary pass statement
(W0107)
tests/model_registry/rest_api/conftest.py (1)
36-36: Good scope change for more granular testing.Changing from class scope to function scope allows for more isolated testing and is appropriate for update operations that may modify state.
tests/model_registry/rest_api/test_model_registry_rest_api.py (3)
11-17: Well-structured custom property constant.The
CUSTOM_PROPERTYconstant is well-defined and provides good test coverage for different metadata types including boolean, string, and double values.
70-74: Excellent refactoring to use centralized validation.The refactoring to use the
validate_resource_attributesutility function improves code consistency and maintainability across the test suite.
149-188: Great coverage for registered model archive/unarchive functionality.This test method complements the model version tests and provides comprehensive coverage for registered model updates including archive/unarchive functionality, which aligns perfectly with the PR objectives.
lugi0
left a comment
lugi0
left a comment
There was a problem hiding this comment.
Couple comments but won't hold the PR for it
rhods-ci-bot
added
commented-by-dbasunag
commented-by-lugi0
and removed
commented-by-dbasunag
labels
rhods-ci-bot
added
commented-by-dbasunag
and removed
commented-by-lugi0
commented-by-dbasunag
labels
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/model_registry/rest_api/test_model_registry_rest_api.py (2)
124-167: Excellent coverage for model version archive/unarchive functionality.This test method directly addresses the PR objective by providing comprehensive coverage for model version updates, including the critical archive/unarchive functionality (ARCHIVED/LIVE states) mentioned in RHOAIENG-24371. The test also covers custom properties validation.
Fix line length violations for better readability:
pytest.param( - {"resource_name": "model_version", "api_name": "model_versions", "data": {"state": "ARCHIVED"}}, + { + "resource_name": "model_version", + "api_name": "model_versions", + "data": {"state": "ARCHIVED"} + }, {"state": "ARCHIVED"}, id="test_validate_updated_version_state_archived", ), pytest.param( - {"resource_name": "model_version", "api_name": "model_versions", "data": {"state": "LIVE"}}, + { + "resource_name": "model_version", + "api_name": "model_versions", + "data": {"state": "LIVE"} + }, {"state": "LIVE"}, id="test_validate_updated_version_state_unarchived", ), pytest.param( - {"resource_name": "model_version", "api_name": "model_versions", "data": CUSTOM_PROPERTY}, + { + "resource_name": "model_version", + "api_name": "model_versions", + "data": CUSTOM_PROPERTY + }, CUSTOM_PROPERTY, id="test_validate_updated_version_custom_properties", ),🧰 Tools
🪛 Pylint (3.3.7)
[convention] 137-137: Line too long (112/100)
(C0301)
[convention] 142-142: Line too long (108/100)
(C0301)
[convention] 147-147: Line too long (106/100)
(C0301)
169-212: Comprehensive registered model testing with good coverage.This test method provides excellent coverage for registered model updates including archive/unarchive functionality, complementing the model version tests. The testing approach is consistent and thorough.
Fix line length violations for better readability:
pytest.param( - {"resource_name": "register_model", "api_name": "registered_models", "data": {"state": "ARCHIVED"}}, + { + "resource_name": "register_model", + "api_name": "registered_models", + "data": {"state": "ARCHIVED"} + }, {"state": "ARCHIVED"}, id="test_validate_updated_model_state_archived", ), pytest.param( - {"resource_name": "register_model", "api_name": "registered_models", "data": {"state": "LIVE"}}, + { + "resource_name": "register_model", + "api_name": "registered_models", + "data": {"state": "LIVE"} + }, {"state": "LIVE"}, id="test_validate_updated_model_state_unarchived", ), pytest.param( - {"resource_name": "register_model", "api_name": "registered_models", "data": CUSTOM_PROPERTY}, + { + "resource_name": "register_model", + "api_name": "registered_models", + "data": CUSTOM_PROPERTY + }, CUSTOM_PROPERTY, id="test_validate_updated_registered_model_custom_properties", ),🧰 Tools
🪛 Pylint (3.3.7)
[convention] 182-182: Line too long (116/100)
(C0301)
[convention] 187-187: Line too long (112/100)
(C0301)
[convention] 192-192: Line too long (110/100)
(C0301)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/model_registry/rest_api/conftest.py(2 hunks)tests/model_registry/rest_api/constants.py(2 hunks)tests/model_registry/rest_api/test_model_registry_rest_api.py(2 hunks)tests/model_registry/rest_api/utils.py(2 hunks)utilities/exceptions.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_registry/rest_api/constants.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/model_registry/rest_api/conftest.py (2)
utilities/exceptions.py (1)
MissingParameter(131-134)tests/model_registry/rest_api/utils.py (1)
execute_model_registry_patch_command(17-31)
tests/model_registry/rest_api/test_model_registry_rest_api.py (3)
tests/model_registry/rest_api/utils.py (1)
validate_resource_attributes(94-104)utilities/constants.py (1)
DscComponents(146-164)tests/model_registry/rest_api/conftest.py (2)
registered_model_rest_api(27-34)updated_model_registry_resource(38-54)
tests/model_registry/rest_api/utils.py (1)
utilities/exceptions.py (1)
ResourceValueMismatch(125-128)
🪛 Pylint (3.3.7)
tests/model_registry/rest_api/conftest.py
[convention] 47-47: Line too long (102/100)
(C0301)
[convention] 38-38: Missing function or method docstring
(C0116)
[warning] 40-40: Redefining name 'model_registry_rest_url' from outer scope (line 12)
(W0621)
[warning] 41-41: Redefining name 'model_registry_rest_headers' from outer scope (line 18)
(W0621)
[warning] 42-42: Redefining name 'registered_model_rest_api' from outer scope (line 27)
(W0621)
tests/model_registry/rest_api/test_model_registry_rest_api.py
[error] 8-8: Unable to import 'simple_logger.logger'
(E0401)
[convention] 8-8: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTER", "tests.model_registry.rest_api.utils.validate_resource_attributes", "utilities.constants.DscComponents"
(C0411)
[convention] 137-137: Line too long (112/100)
(C0301)
[convention] 142-142: Line too long (108/100)
(C0301)
[convention] 147-147: Line too long (106/100)
(C0301)
[convention] 182-182: Line too long (116/100)
(C0301)
[convention] 187-187: Line too long (112/100)
(C0301)
[convention] 192-192: Line too long (110/100)
(C0301)
tests/model_registry/rest_api/utils.py
[error] 5-5: Unable to import 'simple_logger.logger'
(E0401)
[convention] 99-99: Line too long (105/100)
(C0301)
[convention] 94-94: Missing function or method docstring
(C0116)
utilities/exceptions.py
[warning] 128-128: Unnecessary pass statement
(W0107)
[warning] 134-134: Unnecessary pass statement
(W0107)
🔇 Additional comments (6)
utilities/exceptions.py (1)
125-134: Well-designed exception classes for validation framework.The new exception classes
ResourceValueMismatchandMissingParameterare well-named and serve clear purposes in the validation framework. They provide specific error types for resource validation mismatches and missing required parameters.The
passstatements are not strictly necessary but are commonly used for clarity in empty class bodies.🧰 Tools
🪛 Pylint (3.3.7)
[warning] 128-128: Unnecessary pass statement
(W0107)
[warning] 134-134: Unnecessary pass statement
(W0107)
tests/model_registry/rest_api/utils.py (1)
12-12: Good addition of centralized validation utility.The import of
ResourceValueMismatchsupports the new centralized validation approach, which is a good improvement for consistency across tests.tests/model_registry/rest_api/conftest.py (1)
8-8: Good addition of MissingParameter exception import.The import supports proper error handling in the generalized fixture for missing required parameters.
tests/model_registry/rest_api/test_model_registry_rest_api.py (3)
11-17: Good addition of structured custom property test data.The
CUSTOM_PROPERTYconstant provides well-structured test data for validating custom metadata properties across different data types (bool, string, double). This supports comprehensive testing of the custom properties functionality.
70-74: Excellent refactoring to use centralized validation.The replacement of manual validation logic with the
validate_resource_attributesutility function improves consistency and maintainability across tests.
76-122: Good restructuring of model artifact tests.The parameterization has been updated to use the new structured format with
resource_name,api_name, anddatafields, which aligns well with the generalized fixture. The validation approach is consistent and clean.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/model_registry/rest_api/utils.py (1)
94-116:⚠️ Potential issueFix falsy value handling in validation logic.
The validation function provides excellent centralization, but there's a potential bug in the condition on line 112:
if (not actual_resource_data.get(key) or actual_resource_data[key] != expected_params[key])The
not actual_resource_data.get(key)check will incorrectly trigger for legitimate falsy values like0,False, or empty strings. This could cause false validation failures.Apply this diff to fix the falsy value handling:
- if (not actual_resource_data.get(key) or actual_resource_data[key] != expected_params[key]) + if (key not in actual_resource_data or actual_resource_data[key] != expected_params[key])Also fix the line length violation:
- {key: [f"Expected value: {expected_params[key]}, actual value: {actual_resource_data.get(key)}"]} + { + key: [ + f"Expected value: {expected_params[key]}, " + f"actual value: {actual_resource_data.get(key)}" + ] + }🧰 Tools
🪛 Pylint (3.3.7)
[convention] 110-110: Line too long (105/100)
(C0301)
🧹 Nitpick comments (3)
tests/model_registry/rest_api/conftest.py (1)
37-65: Excellent fixture generalization with minor formatting fix needed.The transformation from
updated_model_artifacttoupdated_model_registry_resourceis a great improvement that enables testing updates across different model registry entities. The parameter validation and error handling are appropriate.Fix the line length violation:
- raise MissingParameter("resource_name and api_name are required parameters for this fixture.") + raise MissingParameter( + "resource_name and api_name are required parameters for this fixture." + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 58-58: Line too long (102/100)
(C0301)
[warning] 40-40: Redefining name 'model_registry_rest_url' from outer scope (line 12)
(W0621)
[warning] 41-41: Redefining name 'model_registry_rest_headers' from outer scope (line 18)
(W0621)
[warning] 42-42: Redefining name 'registered_model_rest_api' from outer scope (line 27)
(W0621)
tests/model_registry/rest_api/test_model_registry_rest_api.py (2)
131-174: Excellent coverage for model version archive/unarchive functionality.This test method directly addresses the PR objective by comprehensively testing model version updates including archive/unarchive functionality (ARCHIVED/LIVE states). The test coverage is thorough and well-structured.
Fix the line length violations for better code style compliance:
- pytest.param( - {"resource_name": "model_version", "api_name": "model_versions", "data": STATE_ARCHIVED}, - STATE_ARCHIVED, - id="test_validate_updated_version_state_archived", - ), + pytest.param( + { + "resource_name": "model_version", + "api_name": "model_versions", + "data": STATE_ARCHIVED + }, + STATE_ARCHIVED, + id="test_validate_updated_version_state_archived", + ),Apply similar formatting to other long lines (149, 154).
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 144-144: Line too long (105/100)
(C0301)
[convention] 149-149: Line too long (101/100)
(C0301)
[convention] 154-154: Line too long (106/100)
(C0301)
176-219: Comprehensive registered model archive/unarchive testing.This test method provides excellent coverage for registered model updates including archive/unarchive functionality, perfectly complementing the model version tests and fulfilling the PR objectives.
Fix the line length violations for consistency:
- pytest.param( - {"resource_name": "register_model", "api_name": "registered_models", "data": STATE_ARCHIVED}, - STATE_ARCHIVED, - id="test_validate_updated_model_state_archived", - ), + pytest.param( + { + "resource_name": "register_model", + "api_name": "registered_models", + "data": STATE_ARCHIVED + }, + STATE_ARCHIVED, + id="test_validate_updated_model_state_archived", + ),Apply similar formatting to lines 194 and 199.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 189-189: Line too long (109/100)
(C0301)
[convention] 194-194: Line too long (105/100)
(C0301)
[convention] 199-199: Line too long (110/100)
(C0301)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/model_registry/rest_api/conftest.py(2 hunks)tests/model_registry/rest_api/constants.py(2 hunks)tests/model_registry/rest_api/test_model_registry_rest_api.py(2 hunks)tests/model_registry/rest_api/utils.py(2 hunks)utilities/exceptions.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_registry/rest_api/constants.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/model_registry/rest_api/conftest.py (2)
utilities/exceptions.py (1)
MissingParameter(131-134)tests/model_registry/rest_api/utils.py (1)
execute_model_registry_patch_command(17-31)
tests/model_registry/rest_api/utils.py (1)
utilities/exceptions.py (1)
ResourceValueMismatch(125-128)
🪛 Pylint (3.3.7)
tests/model_registry/rest_api/test_model_registry_rest_api.py
[error] 8-8: Unable to import 'simple_logger.logger'
(E0401)
[convention] 8-8: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTER", "tests.model_registry.rest_api.utils.validate_resource_attributes", "utilities.constants.DscComponents"
(C0411)
[convention] 144-144: Line too long (105/100)
(C0301)
[convention] 149-149: Line too long (101/100)
(C0301)
[convention] 154-154: Line too long (106/100)
(C0301)
[convention] 189-189: Line too long (109/100)
(C0301)
[convention] 194-194: Line too long (105/100)
(C0301)
[convention] 199-199: Line too long (110/100)
(C0301)
tests/model_registry/rest_api/conftest.py
[convention] 58-58: Line too long (102/100)
(C0301)
[warning] 40-40: Redefining name 'model_registry_rest_url' from outer scope (line 12)
(W0621)
[warning] 41-41: Redefining name 'model_registry_rest_headers' from outer scope (line 18)
(W0621)
[warning] 42-42: Redefining name 'registered_model_rest_api' from outer scope (line 27)
(W0621)
tests/model_registry/rest_api/utils.py
[error] 5-5: Unable to import 'simple_logger.logger'
(E0401)
[convention] 110-110: Line too long (105/100)
(C0301)
utilities/exceptions.py
[warning] 128-128: Unnecessary pass statement
(W0107)
[warning] 134-134: Unnecessary pass statement
(W0107)
🔇 Additional comments (3)
utilities/exceptions.py (1)
125-134: Well-designed exception classes for model registry validation.The new exception classes
ResourceValueMismatchandMissingParameterare appropriately designed with clear docstrings and proper inheritance. They provide specific error types for the model registry validation framework.🧰 Tools
🪛 Pylint (3.3.7)
[warning] 128-128: Unnecessary pass statement
(W0107)
[warning] 134-134: Unnecessary pass statement
(W0107)
tests/model_registry/rest_api/test_model_registry_rest_api.py (2)
11-24: Excellent test constants supporting comprehensive archive/unarchive testing.The new constants provide well-structured test data for validating model registry updates, including the archive/unarchive functionality mentioned in the PR objectives. The
CUSTOM_PROPERTYconstant with metadata types is particularly well-designed.
77-81: Good refactoring to use centralized validation.The replacement of manual validation logic with the
validate_resource_attributesfunction improves code consistency and maintainability.
|
Status of building tag latest: success. |
4 participants
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores