storage: STD for storage class migration cleanup #4340

coderabbitai · 2026-04-01T05:46:13Z

🛠️ 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.

@duyanyan, the file is now correctly placed at tests/storage/storage_migration/test_storage_class_migration_cleanup.py — that aligns with the feature-subdirectory layout rule. Thanks for addressing this! 🎉

🧠 Learnings used

Learnt from: Ahmad-Hafe Repo: RedHatQE/openshift-virtualization-tests PR: 3399 File: tests/global_config_gcp.py:24-25 Timestamp: 2026-01-20T13:55:28.139Z Learning: In the openshift-virtualization-tests repository, storage migration tests support same-to-same storage class migration (e.g., storage_class_a = StorageClassNames.GCP and storage_class_b = StorageClassNames.GCP) because migrating from a storage class to the same storage class is a valid migration scenario where a new PVC is still created.

Learnt from: jpeimer Repo: RedHatQE/openshift-virtualization-tests PR: 4108 File: tests/storage/cross_cluster_live_migration/test_cclm.py:88-90 Timestamp: 2026-03-09T10:25:32.742Z Learning: In tests/storage/cross_cluster_live_migration/test_cclm.py (TestCCLMSeveralVMs), test_source_vms_can_be_deleted intentionally has NO pytest-dependency on test_migrate_vm_from_remote_to_local_cluster. This is by design: the test acts as a cleanup step to delete source VMs and must run regardless of whether the migration test passes or fails, to avoid leaving orphaned resources.

Learnt from: jpeimer Repo: RedHatQE/openshift-virtualization-tests PR: 954 File: tests/storage/storage_migration/test_mtc_storage_class_migration.py:68-74 Timestamp: 2025-06-03T12:36:36.982Z Learning: In tests/storage/storage_migration/test_mtc_storage_class_migration.py, the broad Exception catching in the VM migration test is intentional - the user wants to catch every exception and save it for future triage rather than letting specific exceptions bubble up.

Learnt from: RoniKishner Repo: RedHatQE/openshift-virtualization-tests PR: 1641 File: tests/infrastructure/golden_images/test_common_templates_data_volumes.py:47-48 Timestamp: 2025-08-13T06:27:29.727Z Learning: In tests/infrastructure/golden_images/test_common_templates_data_volumes.py, when set_storage_class is True but get_diff_storage_class_from_matrix returns None (indicating only one storage class is available), the test should continue using the default storage class rather than being skipped. This covers special cluster architectures that only have a single storage class available.

Learnt from: hmeir Repo: RedHatQE/openshift-virtualization-tests PR: 3081 File: tests/install_upgrade_operators/product_install/test_install_openshift_virtualization.py:189-191 Timestamp: 2025-12-29T10:07:12.804Z Learning: In test_ocs_virt_default_storage_class in tests/install_upgrade_operators/product_install/test_install_openshift_virtualization.py, consecutive_checks_count=3 is sufficient for verify_boot_sources_reimported because datasources update quickly in the installation lane scenario after storage class changes.

Learnt from: CR Repo: RedHatQE/openshift-virtualization-tests PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-23T19:47:54.552Z Learning: Applies to **/test_*.py : Feature subdirectories REQUIRED - each feature MUST have its own subdirectory under component (e.g., tests/network/ipv6/); test file naming REQUIRED - ALWAYS use `test_<functionality>.py` format

Learnt from: CR Repo: RedHatQE/openshift-virtualization-tests PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-23T19:47:54.552Z Learning: Applies to **/utils.py : Local helper utilities location - place helper utils in `<feature_dir>/utils.py`; move to shared location in utilities/ or tests/conftest.py ONLY when used by different team directories

Learnt from: yossisegev Repo: RedHatQE/openshift-virtualization-tests PR: 3196 File: tests/network/upgrade/test_upgrade_network.py:4-4 Timestamp: 2025-12-22T16:27:40.244Z Learning: For PRs that remove tests, rely on pytest --collect-only to verify the test discovery results (which tests are selected/deselected) and ensure the removal is clean and the test module remains functional. Full test execution is not required for test deletion PRs. This guideline applies to test files anywhere under the tests/ directory (e.g., tests/network/upgrade/test_upgrade_network.py) and should be used for similar test-deletion scenarios across the repository.

Learnt from: rnetser Repo: RedHatQE/openshift-virtualization-tests PR: 4344 File: scripts/tests_analyzer/pytest_marker_analyzer.py:1750-1761 Timestamp: 2026-04-03T18:43:23.119Z Learning: In scripts/tests_analyzer/pytest_marker_analyzer.py (openshift-virtualization-tests), module-level import lines in diffs are intentionally treated as "safe unattributed" changes (setting has_unattributed_changes=True rather than returning None). This is a deliberate design decision: import path changes (e.g., from X.Y import Z → from A.B import Z due to file renames) are structural, not behavioral — the resolved symbol Z is identical. Treating imports as conservative fallbacks would reintroduce false positives for file rename PRs (e.g., `#4308`, `#4314`). Broken import paths are caught by pytest --collect-only, not smoke test execution. Do not flag this pattern as a false-positive risk.

Learnt from: rnetser Repo: RedHatQE/openshift-virtualization-tests PR: 4000 File: conftest.py:856-858 Timestamp: 2026-02-27T11:25:14.486Z Learning: In the openshift-virtualization-tests repository, the AI-analysis related code sections in conftest.py (specifically the code that calls setup_ai_analysis() and enrich_junit_xml()) are vendored from https://github.com/myk-org/jenkins-job-insight/blob/main/examples/pytest-junitxml and should be kept as-is to maintain consistency with upstream for easier maintenance and updates. Do not flag style issues in these vendored sections.

Learnt from: yossisegev Repo: RedHatQE/openshift-virtualization-tests PR: 3358 File: tests/network/sriov/test_sriov.py:21-21 Timestamp: 2026-01-07T09:52:12.342Z Learning: When a PR only removes or modifies pytest markers in tests (e.g., removing pytest.mark.post_upgrade) and the test logic remains unchanged, prefer verifying with pytest --collect-only instead of running the full test suite. This validates that marker usage and test selection behavior are preserved. If the test logic changes, or markers affect behavior beyond collection, run the full test suite to confirm.

Learnt from: rnetser Repo: RedHatQE/openshift-virtualization-tests PR: 0 File: :0-0 Timestamp: 2025-12-16T10:28:54.212Z Learning: In the openshift-virtualization-tests repository, when determining smoke test impact for test execution plans, only set "Run smoke tests: True" if there is a verified, traceable dependency path from smoke tests to the changed code. Session-scoped fixtures or infrastructure-sounding changes do NOT automatically imply smoke test impact - the dependency chain must be explicitly verified using shell scripts before recommending smoke test execution.

Learnt from: jpeimer Repo: RedHatQE/openshift-virtualization-tests PR: 4108 File: tests/storage/cross_cluster_live_migration/test_cclm.py:44-50 Timestamp: 2026-03-09T10:25:57.569Z Learning: In tests/storage/cross_cluster_live_migration/test_cclm.py, the fixtures `written_file_to_vms_before_cclm` and `vms_boot_time_before_cclm` are intentionally declared as explicit parameters in `test_migrate_vm_from_remote_to_local_cluster` (even though their return values are not used by that test body) to make clear that they must execute before `mtv_migration`. jpeimer prefers this explicit parameter ordering to signal pre-migration setup sequencing, and does not want them moved to `pytest.mark.usefixtures`.

Learnt from: rnetser Repo: RedHatQE/openshift-virtualization-tests PR: 4344 File: scripts/tests_analyzer/pytest_marker_analyzer.py:2412-2413 Timestamp: 2026-04-03T18:43:05.607Z Learning: In scripts/tests_analyzer/pytest_marker_analyzer.py (_extract_modified_items_from_conftest), returning empty sets (set(), set()) for file_status in ("added", "renamed") is intentional and correct. For "added", this was pre-existing behavior because a new conftest cannot break existing tests. For "renamed", content is identical to the original — only the path changes — so autouse fixtures, hooks, and all behavior are unchanged. Do not flag this early-return as ignoring behavioral impact.

Learnt from: rnetser Repo: RedHatQE/openshift-virtualization-tests PR: 1723 File: conftest.py:496-502 Timestamp: 2025-08-27T12:38:26.496Z Learning: In PR `#1723`, the user rnetser clarified that updating downstream string comparisons to handle DataVolume enums from StorageClassConfig is not part of the current PR scope, which focuses only on adding conformance storage class support.

Learnt from: rnetser Repo: RedHatQE/openshift-virtualization-tests PR: 2469 File: utilities/sanity.py:139-142 Timestamp: 2025-11-08T07:36:57.616Z Learning: In the openshift-virtualization-tests repository, user rnetser prefers to keep refactoring PRs (like PR `#2469`) strictly focused on moving/organizing code into more granular modules without adding new functionality, error handling, or behavioral changes. Such improvements should be handled in separate PRs.

Learnt from: jpeimer Repo: RedHatQE/openshift-virtualization-tests PR: 0 File: :0-0 Timestamp: 2025-12-22T15:56:00.157Z Learning: In the openshift-virtualization-tests repository, when responding to test execution plan requests from openshift-virtualization-qe-bot-3, do NOT use "REQUEST_CHANGES" review type if the PR author has already marked the PR as verified (e.g., with `/verified` command). Test execution plans are informational guides, not blocking requirements. Use COMMENT event for informational test plans, or only REQUEST_CHANGES if there are actual code issues that need to be addressed before merging.

Learnt from: dshchedr Repo: RedHatQE/openshift-virtualization-tests PR: 4118 File: utilities/database.py:0-0 Timestamp: 2026-03-17T01:32:02.617Z Learning: In the RedHatQE/openshift-virtualization-tests repository, CodeRabbit should post targeted inline comments at each applicable location in the Files Changed tab, rather than aggregating multiple issues into a single PR discussion thread reply. This guideline applies to all Python files (any file ending in .py) changed in a PR; for non-Python files, follow the same inline-comment-at-location principle if relevant.

Learnt from: dshchedr Repo: RedHatQE/openshift-virtualization-tests PR: 4118 File: utilities/database.py:0-0 Timestamp: 2026-03-17T01:32:02.617Z Learning: In RedHatQE/openshift-virtualization-tests, when reviewing Python files, post targeted inline comments on the Files changed tab at the exact location (file and line) of the issue rather than opening a single discussion thread for multiple issues. This should be done for each applicable location to improve traceability and clarity. If multiple issues exist in the same file, address them with separate inline comments pointing to the specific lines.

Learnt from: azhivovk Repo: RedHatQE/openshift-virtualization-tests PR: 3024 File: tests/network/connectivity/utils.py:17-17 Timestamp: 2025-12-15T12:33:06.686Z Learning: In the test suite, ensure the ipv6_network_data fixture returns a factory function (Callable) and that all call sites invoke it to obtain the actual data dict, i.e., use ipv6_network_data() at call sites. This enables future extensibility for configuring secondary interfaces' IP addresses without changing call sites. Apply this pattern to all Python test files under tests.

Learnt from: servolkov Repo: RedHatQE/openshift-virtualization-tests PR: 3387 File: tests/network/provider_migration/libprovider.py:50-52 Timestamp: 2026-01-12T11:24:13.825Z Learning: In the RedHatQE/openshift-virtualization-tests repository, when catching exceptions in Python, use LOGGER.error before re-raising and do not replace it with LOGGER.exception in except blocks. This follows the established pattern across the codebase.

Learnt from: rnetser Repo: RedHatQE/openshift-virtualization-tests PR: 3366 File: tests/storage/cdi_clone/test_clone.py:5-9 Timestamp: 2026-01-12T14:25:05.723Z Learning: In Python tests and utility code across the repository, bitmath.parse_string_unsafe correctly parses Kubernetes quantities (e.g., '4Gi', '512Mi', PVC storage requests) without supplying system=bitmath.NIST. There are 30+ usages indicating this is the standard behavior. Reviewers should verify that code that builds or compares quantity strings does not pass the NIST parameter, and if a new test relies on quantity parsing, assume no NIST parameter is required unless explicitly documented.

Learnt from: dshchedr Repo: RedHatQE/openshift-virtualization-tests PR: 3404 File: tests/virt/upgrade/conftest.py:291-301 Timestamp: 2026-01-14T04:08:23.032Z Learning: In the openshift-virtualization-tests repository, when using VirtualMachine objects from ocp-resources in tests, if vm.ready is True, vm.vmi is guaranteed to exist. Therefore, you can access vm.vmi.instance.status or vm.vmi attributes without additional defensive checks (e.g., if vm.vmi: ...). Do not rely on vm.vmi being present when vm.ready may be False; guard those code paths accordingly. This guideline applies to tests under tests/ (notably in virt/upgrade/conftest.py and related test modules) and should be followed for any code paths that assume vm.vmi exists only when vm.ready is True.

Learnt from: Anatw Repo: RedHatQE/openshift-virtualization-tests PR: 3376 File: tests/network/general/test_ip_family_services.py:96-96 Timestamp: 2026-01-18T09:44:17.044Z Learning: In the openshift-virtualization-tests repository, function-scoped fixtures must use pytest.fixture() with empty parentheses (not pytest.fixture without parentheses). This repo follows this convention despite Ruff PT001. Apply this consistently to all Python test files under tests/ (not just this one) to maintain repository-wide consistency.

Learnt from: EdDev Repo: RedHatQE/openshift-virtualization-tests PR: 3273 File: tests/network/connectivity/test_ovs_linux_bridge.py:5-9 Timestamp: 2026-01-18T13:18:48.808Z Learning: In tests/network/connectivity/test_ovs_linux_bridge.py and similar test files, prefer importing ipaddress as a module and using qualified calls like ipaddress.ip_interface(...) rather than from ipaddress import ip_interface. This preserves module context for readability, especially when chaining properties (e.g., ipaddress.ip_interface(...).ip). This is an intentional exception to the general rule favoring specific imports, and should apply to test files under the tests directory where module context aids understanding.

Learnt from: yossisegev Repo: RedHatQE/openshift-virtualization-tests PR: 3495 File: tests/network/third_part_ip_request/test_third_party_ip_request.py:4-12 Timestamp: 2026-01-18T14:51:50.846Z Learning: In the openshift-virtualization-tests repository, tests consistently import pytest as a module (import pytest) and avoid from pytest import ...; this is the established pattern across 398+ test files. Do not flag or refactor imports to use specific pytest names in tests under tests/**. If a file already follows this pattern, leave it as is; this guideline applies broadly to Python test files under the tests directory.

Learnt from: servolkov Repo: RedHatQE/openshift-virtualization-tests PR: 3387 File: tests/network/provider_migration/libprovider.py:1-8 Timestamp: 2026-01-20T01:03:13.139Z Learning: In the openshift-virtualization-tests repository, Python imports should consistently use module-level imports for the logging module (i.e., import logging) rather than from logging import ... The established pattern spans 270+ files and should not be flagged for refactoring. Apply this guideline to Python files across the repo (e.g., tests/network/provider_migration/libprovider.py).

Learnt from: geetikakay Repo: RedHatQE/openshift-virtualization-tests PR: 3559 File: utilities/infra.py:251-254 Timestamp: 2026-01-21T21:26:41.805Z Learning: In the RedHatQE/openshift-virtualization-tests repository, when reviewing Python code, recognize that with Python 3.14 the syntax 'except ValueError, TypeError:' is valid if there is no 'as' clause, and should not be flagged as Python 2 syntax. If you use an 'as' binding (e.g., 'except (ValueError, TypeError) as e:'), parentheses are required. Ensure this pattern is version-consistent and not flagged as Python 2 syntax when 'as' is absent.

Learnt from: jpeimer Repo: RedHatQE/openshift-virtualization-tests PR: 3571 File: tests/storage/storage_migration/utils.py:158-167 Timestamp: 2026-01-25T13:18:21.675Z Learning: In reviews of the openshift-virtualization-tests repo (and similar Python code), avoid suggesting minor stylistic changes that require extra verification (e.g., removing dict.keys() checks for membership) unless the change has clear correctness or maintainability impact. Focus on fixes with observable behavior, security, performance, or maintainability benefits; defer low-impact style tweaks that are costly to verify.

Learnt from: azhivovk Repo: RedHatQE/openshift-virtualization-tests PR: 3619 File: tests/network/user_defined_network/test_user_defined_network.py:97-97 Timestamp: 2026-01-27T17:18:49.973Z Learning: In tests that exercise lookup_iface_status_ip from libs.net.vmspec, rely on the function's built-in descriptive error messages for failures. Do not add extra assertion messages for IP presence checks using this function; instead, assert on the function behavior or catch its exceptions as appropriate. This reduces duplication and clarifies failures.

Learnt from: EdDev Repo: RedHatQE/openshift-virtualization-tests PR: 3649 File: tests/network/user_defined_network/ip_specification/libipspec.py:1-4 Timestamp: 2026-01-29T05:30:13.982Z Learning: In the openshift-virtualization-tests repository, Python imports should use module import style for the standard library 'json' (import json) rather than 'from json import ...'. This improves readability by making calls like json.loads and json.dumps explicit, and aligns with patterns used for modules like logging, ipaddress, cloudinit, and pytest. Apply this to all Python test files under tests/ (and similar test directories).

Learnt from: rnetser Repo: RedHatQE/openshift-virtualization-tests PR: 3690 File: tests/after_cluster_deploy_sanity/test_after_cluster_deploy_sanity.py:65-65 Timestamp: 2026-02-02T17:41:12.759Z Learning: In test files, keep test_* functions with simple one-line docstrings. For helper functions, utilities, and library code with non-obvious return values or side effects, use Google-style docstrings with Args, Returns, and Side effects sections. Do not require Google-style docstrings for pytest test functions themselves.

Learnt from: RoniKishner Repo: RedHatQE/openshift-virtualization-tests PR: 3697 File: tests/infrastructure/instance_types/test_common_vm_instancetype.py:53-98 Timestamp: 2026-02-03T07:34:34.184Z Learning: In test files (Python, pytest), prefer using tier3 markers for categorization since tier2, tier1, and tier4 are not used in this repository. Do not rely on non-official markers; formalize and document the allowed markers in pytest.ini (or equivalent) to ensure consistent usage. When reviewing new tests, ensure markers align with this convention (only tier3, unless a project-wide decision defines additional tiers) and remove any unnecessary or undocumented markers.

Learnt from: vsibirsk Repo: RedHatQE/openshift-virtualization-tests PR: 3577 File: tests/virt/conftest.py:251-267 Timestamp: 2026-02-10T15:04:14.799Z Learning: In Python tests, remove all bare time.sleep() calls. Replace with a waiting mechanism such as TimeoutSampler from the timeout_sampler package or a function decorated with retry (with appropriate timeout/conditions) to ensure determinism and avoid flakiness. This applies to all Python tests under the tests directory (not just this file) to maintain consistent waiting behavior across the suite.

Learnt from: rnetser Repo: RedHatQE/openshift-virtualization-tests PR: 3847 File: utilities/virt.py:2449-2453 Timestamp: 2026-02-18T06:35:39.536Z Learning: In Python code, a function named clearly and self-descriptively can be deemed not to require a docstring. However, treat this as a context-specific guideline and not a universal rule. For public APIs or functions with side effects, prefer concise docstrings explaining behavior, inputs, outputs, and side effects. This guidance is based on the example in utilities/virt.py from RedHatQE/openshift-virtualization-tests where validate_libvirt_persistent_domain(vm, admin_client) was considered self-documenting.

Learnt from: jpeimer Repo: RedHatQE/openshift-virtualization-tests PR: 3926 File: tests/storage/cross_cluster_live_migration/utils.py:137-156 Timestamp: 2026-02-19T15:36:04.744Z Learning: In cross-cluster live migration (CCLM) tests vs storage-migration tests, boot-time verification uses different access methods: console-based (get_vm_boot_time_via_console) when SSH to VMs in the remote cluster is unavailable, and SSH-based (get_vm_boot_time) when VMs remain in the same cluster. Code reviews should enforce separate, clearly named verification functions for each scenario (e.g., verify_vms_boot_time_after_migration vs verify_vms_boot_time_after_storage_migration) and place them in the appropriate modules. This pattern applies to all Python test utilities under tests/storage.

Learnt from: vsibirsk Repo: RedHatQE/openshift-virtualization-tests PR: 3883 File: utilities/unittests/test_os_utils.py:333-425 Timestamp: 2026-02-23T16:31:34.505Z Learning: In integration/functional tests located under the tests/ directory, require assertion failure messages using the pattern: assert condition, "descriptive message". For unit tests under utilities/unittests/, rely on pytest's assertion introspection and descriptive test names; explicit failure messages are not required. This guidance helps maintain clear diagnostics for integration tests while keeping unit tests concise and leveraging pytest's built-in introspection.

Learnt from: vsibirsk Repo: RedHatQE/openshift-virtualization-tests PR: 3883 File: utilities/pytest_utils.py:441-463 Timestamp: 2026-02-23T16:33:22.070Z Learning: In Python code reviews, the guideline to always use named arguments for multi-argument calls does not apply to built-ins or methods that have positional-only parameters (those defined with a / in their signature). Do not flag or require named arguments for calls like dict.get(key, default=None, /), list.pop(), str.split(sep, maxsplit) and similar built-ins that cannot accept keyword arguments. Apply the named-argument rule only to functions/methods that explicitly accept keyword arguments.

Learnt from: yossisegev Repo: RedHatQE/openshift-virtualization-tests PR: 3873 File: tests/network/localnet/test_non_udn_localnet.py:7-9 Timestamp: 2026-02-25T10:52:09.679Z Learning: In the RedHatQE/openshift-virtualization-tests repository, networking infrastructure requirements (nmstate, localnet bridge mappings, NIC availability) are not automatically tier3; they are considered standard test environment capabilities. Only tests with truly platform-specific, time-consuming, or bare-metal requirements should be marked as tier3. Apply this guidance to all Python tests under tests/, including tests/network/localnet/test_non_udn_localnet.py, ensuring tier3 designation is reserved for genuine platform-specific or complex scenarios rather than general networking infra necessities.

Learnt from: yossisegev Repo: RedHatQE/openshift-virtualization-tests PR: 3873 File: tests/network/localnet/test_non_udn_localnet.py:19-27 Timestamp: 2026-02-25T11:00:02.013Z Learning: In the test codebase, do not import from conftest.py files. This avoids import ambiguity in pytest. Do not import constants or helpers defined in conftest.py into tests. If a value is needed in both conftest.py and test files, duplicate it in both places or place it in a separate utility module that is importable by tests.

Learnt from: azhivovk Repo: RedHatQE/openshift-virtualization-tests PR: 4147 File: tests/network/upgrade/test_upgrade_network.py:166-177 Timestamp: 2026-03-19T10:36:59.023Z Learning: In this repository’s pytest-based test files (under `tests/`), do not flag unused test method parameters/fixture arguments for removal when the parameters are intentionally kept only to enforce pytest fixture dependency ordering (e.g., an unused fixture like `bridge_on_one_node`). Treat this as an intentional convention consistent with other fixture definitions in the codebase, and do not open follow-up review issues for those unused parameters.

Learnt from: jpeimer Repo: RedHatQE/openshift-virtualization-tests PR: 4267 File: tests/storage/cross_cluster_live_migration/conftest.py:530-531 Timestamp: 2026-03-25T11:24:07.687Z Learning: In this repo’s OpenShift virtualization tests, it is a standard pattern to call `to_dict()` on `ocp-resources` objects (e.g., `DataVolume`) without using its return value. The call is used only to populate the object’s `res` attribute, which is then read or mutated (e.g., `dv.res[...] = ...`). Do not flag this as an unused return value and do not request adding an inline comment just to justify it, since maintainers treat this behavior as consistent and intentional across the codebase.

Learnt from: jpeimer Repo: RedHatQE/openshift-virtualization-tests PR: 4267 File: tests/storage/cross_cluster_live_migration/test_cclm.py:96-106 Timestamp: 2026-03-29T13:51:25.599Z Learning: In this repository, follow the existing pytest convention for `pytest.mark.parametrize` argument names: use a single comma-separated string for `argnames` (e.g., `"dv_wait_timeout, vms_for_cclm"`), not a tuple (e.g., `("dv_wait_timeout", "vms_for_cclm")`). Do not flag or suggest changing `argnames` to a tuple. Also note that PT006 is not enforced by Ruff in this repo, so reviewers should not treat PT006 as a reason to alter the `argnames` format.

Learnt from: azhivovk Repo: RedHatQE/openshift-virtualization-tests PR: 4318 File: tests/network/bandwidth/test_bandwidth.py:1-3 Timestamp: 2026-03-31T08:35:22.802Z Learning: In this repository, when reviewing Python test modules under tests/, only require an STP link in the module docstring for STD (new-feature) tests. If the test module is a support-exception test tied to SUPPORTEX-* tickets (e.g., SUPPORTEX-29574) and therefore has no STP, do not flag missing STP links in the module docstring.

Learnt from: azhivovk Repo: RedHatQE/openshift-virtualization-tests PR: 4318 File: tests/network/l2_bridge/bandwidth/test_bandwidth.py:5-27 Timestamp: 2026-03-31T10:39:33.409Z Learning: In this repository’s pytest suite, some STD (Standard Definition) placeholder tests under the `tests/` directory are intentionally implemented as docstring-only functions. They must be marked with `__test__ = False` immediately after the function definition (e.g., `test_x.__test__ = False`) to prevent pytest from collecting/executing them until the real test body exists. During code review, do not flag these as missing executable coverage and do not recommend removing or changing the `__test__ = False` setting when the placeholder pattern matches this accepted STD approach.

Learnt from: ema-aka-young Repo: RedHatQE/openshift-virtualization-tests PR: 4342 File: tests/storage/cdi_config/test_cdi_config.py:195-205 Timestamp: 2026-04-02T09:03:57.004Z Learning: When calling `utilities.storage.get_dv_size_from_datasource()` in this repo, do not require an extra `None` guard for its return value if the `datasource` argument originates from a fixture that uses `ensure_exists=True` (e.g., `fedora_data_source_scope_module`). Those fixtures guarantee the datasource exists and provides a valid size, and other repo usages follow the same pattern without additional fail-fast/`None` checks; therefore, reviewers should not flag missing `None` guarding in this specific fixture-driven scenario.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

coderabbitai · 2026-04-01T05:46:13Z

⚠️ Potential issue | 🟠 Major

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__ = False

Based 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).

coderabbitai · 2026-04-07T13:56:08Z

⚠️ Potential issue | 🟡 Minor

MEDIUM: Placeholder Polarion IDs and missing feature markers

All tests use CNV-XXXXX placeholder IDs that need real Polarion test case IDs before merge. Additionally, tests should have appropriate feature markers (e.g., @pytest.mark.storage_migration).

WHY: Polarion integration requires valid IDs for test tracking. Feature markers enable selective test execution and proper categorization.

As per coding guidelines: "All new tests MUST have markers - check pytest.ini for available markers, NEVER commit unmarked tests."

Also applies to: 48-48, 64-64, 95-95

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@tests/storage/storage_migration/test_storage_class_migration_cleanup.py` at line 29, Replace the placeholder Polarion ID "CNV-XXXXX" used in the pytest marker with the real Polarion test case ID(s) and add the appropriate feature marker (for example `@pytest.mark.storage_migration`) to each test; update the pytest marker line(s) where pytest.mark.polarion("CNV-XXXXX") appears (and the other occurrences noted) so they read something like `@pytest.mark.polarion`("REAL-ID") and include the feature marker above the test function (e.g., `@pytest.mark.storage_migration`) to satisfy test tracking and selective execution requirements.

rnetser · 2026-04-05T08:29:28Z

iiuc this should be part of the test setup - i.e the tested feature is to verify that after VM migration the source PVC is not deleted.
if migration fails - this should be setup error and not test failure

coderabbitai · 2026-04-07T13:56:08Z

🧹 Nitpick | 🔵 Trivial

LOW: Consider restructuring preconditions to align with fixture-based setup

As noted by reviewer rnetser, migration completion and VM state verification (items 2-3 in preconditions) should be handled by fixtures, not listed as preconditions that might be confused with in-test verification steps.

WHY: Test preconditions should describe the required state before the test runs. Migration execution and its success verification are setup responsibilities, typically handled by fixtures like storage_mig_migration (see conftest.py:85-94 which already waits for completion).

The current structure mixes setup responsibilities with verification intent. Consider restructuring:

Preconditions: Fixture-provided resources (VMs, migration plan configuration)

Steps: The actual verification this test performs

Expected: Outcomes of the verification steps

Also applies to: 49-62, 65-78

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@tests/storage/storage_migration/test_storage_class_migration_cleanup.py` around lines 30 - 46, Update the test docstrings to treat migration execution and its success verification as fixture responsibilities rather than preconditions: in test_retention_policy_default_behavior, list only fixture-provided resources (e.g., a VM and a MultiNamespaceVirtualMachineStorageMigrationPlan without retentionPolicy) as Preconditions and reference the fixture storage_mig_migration (which waits for completion) for setup; move "wait for migration to complete" and "verify VM is using new PVC/DataVolume" into Steps/Expected as the actual verification performed by the test (e.g., Steps: verify source PVC/DataVolume still exists; Expected: migration succeeded, source kept, VM running on new storage); apply the same restructuring to the other affected test docstrings around the indicated ranges so all preconditions describe setup only and verification/assertions are in Steps/Expected.

coderabbitai · 2026-04-07T13:56:09Z

⚠️ Potential issue | 🟡 Minor

MEDIUM: Missing required Preconditions: section in test docstring

The test_combined_namespace_and_spec_level_retention_policy docstring lacks the Preconditions: section. While preconditions exist in the class docstring, each STD test function must include all required sections.

WHY: Consistent docstring format enables tooling to parse and validate test definitions. The class-level preconditions should be referenced or restated in the test.

Proposed fix

def test_combined_namespace_and_spec_level_retention_policy(self): """ Test combination of namespace-level and spec-level retentionPolicy. Namespace-level policy should override spec-level policy for that namespace. + Preconditions: + 1. Two VMs with source PVCs/DataVolumes in different namespaces + 2. MultiNamespaceVirtualMachineStorageMigrationPlan created with: + - spec-level retentionPolicy=keepSource + - namespace-level retentionPolicy=deleteSource for specific namespace + 3. All migrations completed successfully + 4. Both VMs verified using new PVCs + Steps: 1. Verify source PVCs in namespaces WITHOUT namespace-level policy are kept (spec-level policy) 2. Verify source PVCs in namespaces WITH namespace-level policy=deleteSource are deleted Expected: - All migrations complete successfully - Source PVCs in namespaces with namespace-level policy are deleted - Source PVCs in other namespaces are kept (spec-level policy) """

As per coding guidelines: "STD test docstrings MUST follow format with Required sections: Preconditions:, Steps:, Expected:"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@tests/storage/storage_migration/test_storage_class_migration_cleanup.py` around lines 96 - 109, Add a required "Preconditions:" section to the docstring of the test_combined_namespace_and_spec_level_retention_policy test: either restate the class-level preconditions or reference them explicitly (e.g., "See class Preconditions: ...") so the test docstring contains "Preconditions:", "Steps:", and "Expected:" sections; update the docstring for test_combined_namespace_and_spec_level_retention_policy to include that section while keeping the existing Steps and Expected text intact.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: STD for storage class migration cleanup #4340

Diff view

Diff view

There are no files selected for viewing

coderabbitai bot Apr 1, 2026 •

edited

Loading

Uh oh!

duyanyan Apr 8, 2026

Uh oh!

coderabbitai bot Apr 8, 2026

Uh oh!

coderabbitai bot Apr 1, 2026

Uh oh!

coderabbitai bot Apr 7, 2026

Uh oh!

rnetser Apr 5, 2026

Uh oh!

coderabbitai bot Apr 7, 2026

Uh oh!

coderabbitai bot Apr 7, 2026

Uh oh!

storage: STD for storage class migration cleanup #4340

Are you sure you want to change the base?

storage: STD for storage class migration cleanup #4340

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

coderabbitai bot Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duyanyan Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

rnetser Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Apr 1, 2026 •

edited

Loading