storage: STD for storage class migration cleanup#4340
Open
duyanyan wants to merge 1 commit intoRedHatQE:mainfrom
Open
storage: STD for storage class migration cleanup#4340duyanyan wants to merge 1 commit intoRedHatQE:mainfrom
duyanyan wants to merge 1 commit intoRedHatQE:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughA new test module defining a test class for virtual machine storage migration plan retention policy behavior is added. The test class contains four methods that verify default, namespace-level, spec-level, and combined retention policy scenarios. Currently, the test methods contain only docstring documentation without executable test logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
requested review from
dalia-frank and
josemacassan
![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
🤖 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/storage/test_storage_class_migration_cleanup.py`:
- Line 1: Move the new test file
tests/storage/test_storage_class_migration_cleanup.py into a feature
subdirectory (e.g.,
tests/storage/storage_migration_cleanup/test_storage_class_migration_cleanup.py)
to comply with the repository test-layout rule; after moving, update any
imports, pytest collection references, and CI/test-suite config that reference
the original module name or path (search for occurrences of
test_storage_class_migration_cleanup or the original file path) so the test
still runs under its new package location and ownership boundary.
- Line 18: Remove the module-level "__test__ = False" and instead disable each
placeholder test individually by placing "__test__ = False" immediately after
each docstring-only test function definition (i.e., for every placeholder test
function in this file, add the per-test "__test__ = False" line directly
following its def block rather than using a file-wide flag).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 734f2cc4-bfae-4724-8213-ed4ec6a38f52
📒 Files selected for processing (1)
tests/storage/test_storage_class_migration_cleanup.py
| @@ -0,0 +1,118 @@ | |||
| """ | |||
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
MEDIUM: Place this new feature under a dedicated storage subdirectory
Current location (tests/storage/test_storage_class_migration_cleanup.py) violates the repository test-layout rule for feature isolation. Please move it to a feature folder (example: tests/storage/storage_migration_cleanup/test_storage_class_migration_cleanup.py) to align discovery and ownership boundaries.
As per coding guidelines: “Feature subdirectories REQUIRED - each feature MUST have its own subdirectory under component …”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/storage/test_storage_class_migration_cleanup.py` at line 1, Move the
new test file tests/storage/test_storage_class_migration_cleanup.py into a
feature subdirectory (e.g.,
tests/storage/storage_migration_cleanup/test_storage_class_migration_cleanup.py)
to comply with the repository test-layout rule; after moving, update any
imports, pytest collection references, and CI/test-suite config that reference
the original module name or path (search for occurrences of
test_storage_class_migration_cleanup or the original file path) so the test
still runs under its new package location and ownership boundary.
|
|
||
| import pytest | ||
|
|
||
| __test__ = False |
Contributor
There was a problem hiding this comment.
HIGH: Replace module-level __test__ = False with per-test flags
Using module-level __test__ = False disables the entire file and makes incremental activation of individual tests harder/safer in follow-up PRs. In this repository’s STD pattern, each placeholder test should be disabled explicitly right after its definition.
Proposed fix
-__test__ = False
@@
class TestMultiNamespaceVirtualMachineStorageMigrationPlanRetentionPolicy:
@@
def test_retention_policy_default_behavior(self):
"""
...
"""
+ test_retention_policy_default_behavior.__test__ = False
@@
def test_namespace_level_retention_policy_delete_source(self):
"""
...
"""
+ test_namespace_level_retention_policy_delete_source.__test__ = False
@@
def test_spec_level_retention_policy_delete_source(self):
"""
...
"""
+ test_spec_level_retention_policy_delete_source.__test__ = False
@@
def test_combined_namespace_and_spec_level_retention_policy(self):
"""
...
"""
+ test_combined_namespace_and_spec_level_retention_policy.__test__ = FalseBased on learnings: STD placeholder tests under tests/ are intentionally docstring-only and “must be marked with __test__ = False immediately after the function definition.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| __test__ = False | |
| class TestMultiNamespaceVirtualMachineStorageMigrationPlanRetentionPolicy: | |
| def test_retention_policy_default_behavior(self): | |
| """ | |
| ... | |
| """ | |
| test_retention_policy_default_behavior.__test__ = False | |
| def test_namespace_level_retention_policy_delete_source(self): | |
| """ | |
| ... | |
| """ | |
| test_namespace_level_retention_policy_delete_source.__test__ = False | |
| def test_spec_level_retention_policy_delete_source(self): | |
| """ | |
| ... | |
| """ | |
| test_spec_level_retention_policy_delete_source.__test__ = False | |
| def test_combined_namespace_and_spec_level_retention_policy(self): | |
| """ | |
| ... | |
| """ | |
| test_combined_namespace_and_spec_level_retention_policy.__test__ = False |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/storage/test_storage_class_migration_cleanup.py` at line 18, Remove the
module-level "__test__ = False" and instead disable each placeholder test
individually by placing "__test__ = False" immediately after each docstring-only
test function definition (i.e., for every placeholder test function in this
file, add the per-test "__test__ = False" line directly following its def block
rather than using a file-wide flag).
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
awels
approved these changes
Apr 3, 2026
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.
Short description:
Introduce STD for storage migration cleanup feature which allows the system to optionally decommission migration plans and legacy PVCs upon successful completion
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
https://redhat.atlassian.net/browse/CNV-81264
Summary by CodeRabbit