[CCLM] Add cross-storage cross-cluster migration tests#4650
Open
jpeimer wants to merge 3 commits intoRedHatQE:mainfrom
Open
[CCLM] Add cross-storage cross-cluster migration tests#4650jpeimer wants to merge 3 commits intoRedHatQE:mainfrom
jpeimer wants to merge 3 commits intoRedHatQE:mainfrom
Conversation
Signed-off-by: Jenia Peimer <jpeimer@redhat.com>
…zation-tests into cclm_cross_storage
- Add type hints to remote_cluster_storage_classes_names fixture - Add type hints to remote_cluster_source_storage_class fixture - Add type hints to local_cluster_target_storage_class fixture - Add docstring to get_storage_class_for_storage_migration() - Add docstrings to all three new fixtures - Fix line length in local_cluster_target_storage_class - Remove unused constant TESTS_CLASS_NAME_STORAGE_B_TO_A Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Jenia Peimer <jpeimer@redhat.com>
Contributor
📝 WalkthroughWalkthroughThe PR centralizes storage class constants and validation utilities by moving them from storage_migration-specific modules to shared storage package utilities, and updates cross-cluster live migration tests to dynamically select storage classes based on configuration and available cluster resources. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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-5
requested review from
dalia-frank and
ema-aka-young
openshift-virtualization-qe-bot-5
requested review from
josemacassan,
kgoldbla and
kshvaika
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/storage/cross_cluster_live_migration/test_cclm.py (1)
1-17:⚠️ Potential issue | 🟠 MajorHIGH: Add module-level traceability for the new CCLM suite.
This file adds a new feature test suite, but there is no module docstring with an STP link or Jira/RFE fallback. The repo requires that traceability for new feature tests so coverage can be tracked. As per coding guidelines, new feature tests MUST include an STP link in the module docstring; if no STP exists, include the Jira/RFE link instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/storage/cross_cluster_live_migration/test_cclm.py` around lines 1 - 17, The module lacks the required traceability docstring; add a module-level docstring at the top of the file (above imports) that includes the STP link for this CCLM test suite (or a Jira/RFE link if no STP exists) so the new feature tests (e.g., TESTS_CLASS_NAME_SEVERAL_VMS, TESTS_CLASS_NAME_WINDOWS_VM, TESTS_CLASS_NAME_STORAGE_A_TO_B) are traceable; ensure the docstring is succinct and clearly labeled as the STP/Jira reference for this test module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/storage/cross_cluster_live_migration/test_cclm.py`:
- Around line 1-17: The module lacks the required traceability docstring; add a
module-level docstring at the top of the file (above imports) that includes the
STP link for this CCLM test suite (or a Jira/RFE link if no STP exists) so the
new feature tests (e.g., TESTS_CLASS_NAME_SEVERAL_VMS,
TESTS_CLASS_NAME_WINDOWS_VM, TESTS_CLASS_NAME_STORAGE_A_TO_B) are traceable;
ensure the docstring is succinct and clearly labeled as the STP/Jira reference
for this test module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f86f8ea7-076e-4166-bebd-0c5d30ab449c
📒 Files selected for processing (8)
tests/storage/constants.pytests/storage/cross_cluster_live_migration/conftest.pytests/storage/cross_cluster_live_migration/test_cclm.pytests/storage/storage_migration/conftest.pytests/storage/storage_migration/constants.pytests/storage/storage_migration/test_storage_class_migration.pytests/storage/storage_migration/utils.pytests/storage/utils.py
💤 Files with no reviewable changes (2)
- tests/storage/storage_migration/constants.py
- tests/storage/storage_migration/utils.py
Contributor
Author
|
/build-and-push-container |
Contributor
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-4650 published |
Contributor
|
/lgtm |
Contributor
Author
|
/verified openshift-virtualization-tests-runner/5084/ |
Contributor
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Your deliverable: Test Execution Plan
Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL: Before setting "Run smoke tests: True", you MUST use shell scripts to: |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3275 🏁 Script executed: Length of output: 1092 🏁 Script executed: Length of output: 1735 🧠 Learnings used |
rnetser
reviewed
Apr 30, 2026
| ) | ||
| @pytest.mark.usefixtures("remote_cluster_source_storage_class", "local_cluster_target_storage_class") | ||
| class TestCCLMFromStorageAtoB: | ||
| @pytest.mark.polarion("CNV-15955") |
Collaborator
There was a problem hiding this comment.
@jpeimer the flow for adding new tests require adn STP first then implementation.
This allows proper review for the planned tests before the implementation.
btw, the polarion case appears created directly as approved and automated
Contributor
Author
There was a problem hiding this comment.
These are the tests for the existing feature that was introduced before the STP existed. If we need an STP now - the tests will be delayed even further. cc @aglitke
I don't know why polarion test cases were created as approved and automated. From the talk yesterday in the office, I understood that there is no value in polarion, so I'm not sure how much time we should spend on it.
Is there anything that needs to be improved in this PR?
Collaborator
There was a problem hiding this comment.
The tests were not reviewed. It is not related to an STP (which should have been done regardless of the new STP)
I do not understand how the ppl who reviewed this PR marked as lgtm without first reviewing what needs to be tested.
If we need an STP now
This is not new just moved to the code and the change to do it in the code was raised a while back.
Collaborator
There was a problem hiding this comment.
why didn't you flag this as not following the required flow in the repo?
Contributor
There was a problem hiding this comment.
@rnetser, you're right to call this out. This was an oversight on my part.
The new TestCCLMFromStorageAtoB class (with Polarion IDs CNV-15954 through CNV-15960) is genuinely new test coverage — not a re-enabled or bug-unblocked test — so the STD-first workflow applies and I should have flagged it as a HIGH severity issue:
[HIGH]
tests/storage/cross_cluster_live_migration/test_cclm.py—TestCCLMFromStorageAtoBintroduces new test scenarios (CNV-15954–CNV-15960) without a prior STD (Standard Test Definition) PR review. Per the repo's required flow, the sequence must be: STP review → STD review → implementation. Submitting test implementation without prior STD review is not permitted for new feature tests.
Additionally, since there is no STP referenced, the module docstring should include a link to the corresponding RFE or Jira epic for coverage tracking.
I failed to apply the STD-first workflow check here. The exception learning (for re-enabled/bug-unblocked tests) does not apply to this case, and I should not have treated it as such. I'll ensure this check is applied correctly going forward.
🧠 Learnings used
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: 4108
File: tests/storage/cross_cluster_live_migration/test_cclm.py:44-50
Timestamp: 2026-03-09T10:25:54.144Z
Learning: In tests/storage/cross_cluster_live_migration/test_cclm.py, keep fixtures written_file_to_vms_before_cclm and vms_boot_time_before_cclm as explicit test function parameters in test_migrate_vm_from_remote_to_local_cluster to enforce pre-migration sequencing. Do not convert them to pytest.mark.usefixtures; this signals the required execution order to run before mtv_migration, per jpeimer's preference.
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/utils.py:78-94
Timestamp: 2026-03-09T10:50:33.114Z
Learning: In tests/storage/cross_cluster_live_migration/utils.py, the broad `except Exception` in `verify_compute_live_migration_after_cclm` is intentional: the function must attempt migration on every VM in `local_vms` and aggregate all per-VM failures into `vms_failed_migration` for collective reporting, rather than failing fast on the first error. Do not flag this as a BLE001 violation in future reviews.
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: 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: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-09-29T19:05:24.987Z
Learning: For PR `#1904` test execution, the critical validation point is test_connectivity_over_migration_between_localnet_vms which should fail gracefully on cloud clusters but pass on bare-metal/PSI clusters, representing the core nmstate conditional logic functionality.
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: josemacassan
Repo: RedHatQE/openshift-virtualization-tests PR: 2300
File: tests/storage/online_resize/conftest.py:94-96
Timestamp: 2025-10-30T10:43:48.886Z
Learning: In tests/storage/online_resize/conftest.py, the `running_rhel_vm` fixture is intentionally designed to only invoke `running_vm(vm=rhel_vm_for_online_resize)` without returning or yielding a value. It's used as a dependency fixture to ensure the VM is running before other fixtures/tests execute, while the actual VM object is accessed via the `rhel_vm_for_online_resize` fixture.
Learnt from: acinko-rh
Repo: RedHatQE/openshift-virtualization-tests PR: 4571
File: tests/storage/cdi_clone/test_clone.py:0-0
Timestamp: 2026-04-28T13:59:51.123Z
Learning: In tests/storage/cdi_clone/test_clone.py (RedHatQE/openshift-virtualization-tests), the test `test_successful_vm_from_cloned_dv_windows_with_vtpm` intentionally does NOT call `verify_vtpm_in_windows_vm`. The vTPM configuration is guaranteed structurally by the instancetype/preference used to create the VM. The `verify_vtpm_in_windows_vm` function (which checks VM XML and runs a WMIC query via SSH) was debug-only scaffolding that the team confirmed is not needed in production tests. Do not flag the absence of `verify_vtpm_in_windows_vm` in this test as missing coverage.
Learnt from: CR
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-27T15:43:46.511Z
Learning: Applies to tests/**/*.py : New feature tests MUST follow STD-first workflow: STP review, then STD review, then implementation. NEVER submit test implementation without prior STD review
Learnt from: dalia-frank
Repo: RedHatQE/openshift-virtualization-tests PR: 4603
File: tests/data_protection/oadp/test_velero.py:101-130
Timestamp: 2026-04-27T15:40:31.167Z
Learning: When reviewing tests in RedHatQE/openshift-virtualization-tests, do not require STD-first workflow and STP/RFE/Jira-epic module docstring/traceability for tests that are being re-enabled after previously being blocked by a product bug (e.g., after a CNV Jira bug fix). Only enforce these STD-first and STP/RFE/Jira-epic module docstring requirements for genuinely new feature tests; previously blocked tests that are now unblocked/re-enabled should not be flagged as missing traceability.
Learnt from: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 4578
File: tests/network/localnet/nad_ref_change/test_nad_ref_change.py:18-18
Timestamp: 2026-04-26T11:44:20.150Z
Learning: In this repository’s STD (Standard Test Definition) PRs under `tests/**`, it’s expected that Polarion IDs may be placeholders (e.g., `pytest.mark.polarion("CNV-00000")`) while the test scenarios are still being agreed upon. If the test scenario is disabled for the moment (e.g., the scenario/module sets `__test__ = False`), reviewers should not treat placeholder Polarion IDs as a blocking issue. Real Polarion IDs should be filled in before or during the implementation PR.
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: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-16T14:00:59.076Z
Learning: In the openshift-virtualization-tests repository, when responding to test execution plan requests from openshift-virtualization-qe-bot-3, CodeRabbit must post ONLY an inline review comment on the Files Changed tab and then stop immediately without generating any follow-up comments in the PR discussion thread. No acknowledgment messages, no confirmation of posting, no explanation - silence after posting the inline review equals success. Additional comments create empty/meaningless reviews that clutter the PR.
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: CR
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-27T15:43:46.511Z
Learning: Applies to tests/**/*.py : RFE/Jira link REQUIRED when no STP exists - if there is no STP, the module docstring MUST include a link to the RFE or Jira epic (not support cases) for coverage tracking
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: 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: EdDev
Repo: RedHatQE/openshift-virtualization-tests PR: 3612
File: tests/network/user_defined_network/ip_specification/test_ip_specification.py:10-10
Timestamp: 2026-01-26T20:29:54.623Z
Learning: In the RedHatQE/openshift-virtualization-tests repository, when introducing STD (Standard Test Definition) PRs that define test case structure without implementation, it's acceptable and intentional to use `__test__ = False` to block pytest collection until the tests are fully implemented in a follow-up PR. The STD workflow involves: (1) defining test structure, markers, and documentation first, (2) implementing the actual test logic and removing the collection blocker in a subsequent PR.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1168
File: tests/storage/storage_migration/utils.py:92-99
Timestamp: 2025-06-10T09:41:58.035Z
Learning: When CodeRabbit flags technical issues in code reviews, do not resolve the comments without first addressing them and getting a response from the user. Proper procedure is to discuss the technical issues and proposed solutions before marking comments as resolved.
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 4108
File: tests/storage/cross_cluster_live_migration/utils.py:97-156
Timestamp: 2026-03-09T11:11:18.237Z
Learning: In tests/storage/cross_cluster_live_migration/utils.py, the `parse_boot_time_from_console_output` function intentionally uses minute-level precision (matching `who -b` output like "system boot YYYY-MM-DD HH:MM") for boot-time comparison in CCLM tests. This is acceptable because a file-write operation is performed on each VM between initial boot and migration start, making it practically impossible for the full migration cycle to complete within the same minute as the initial boot. Do not flag minute-level precision as insufficient for boot-time comparison in this context.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3062
File: conftest.py:333-333
Timestamp: 2025-12-16T15:09:49.597Z
Learning: In the openshift-virtualization-tests repository, when conftest.py or utilities/bitwarden.py changes affect py_config["os_login_param"], smoke test impact must be determined by: (1) finding all smoke tests using `rg "pytest.mark.smoke"`, (2) checking each for VM creation patterns (VirtualMachineForTests, running_vm, VirtualMachineForTestsFromTemplate), (3) tracing whether running_vm is called with default check_ssh_connectivity=True, which accesses vm.login_params property that reads py_config["os_login_param"][vm.os_flavor]. The dependency chain is: smoke test → VM creation → running_vm → wait_for_ssh_connectivity → vm.login_params → os_login_param. Any smoke test creating VMs with SSH connectivity (the default) depends on os_login_param.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3062
File: conftest.py:333-333
Timestamp: 2025-12-16T20:11:03.645Z
Learning: In the openshift-virtualization-tests repository, when determining smoke test impact for changes affecting py_config["os_login_param"], follow this verification methodology: (1) Find all smoke tests: `rg "pytest.mark.smoke" --type=py -B2 | grep "def test_"`, (2) For each smoke test file, search for VM creation patterns: `rg "VirtualMachineForTests|running_vm|VirtualMachineForTestsFromTemplate|wait_for_ssh|check_ssh_connectivity"`, (3) Trace the dependency chain: smoke test → VirtualMachineForTests/running_vm() → wait_for_ssh_connectivity() (default enabled) → vm.login_params property → py_config["os_login_param"][vm.os_flavor], (4) Check utilities/virt.py for login_params usage: `rg "os_login_param|login_params" utilities/virt.py -C3`. Any smoke test creating VMs with default SSH connectivity checks (running_vm with check_ssh_connectivity=True) depends on os_login_param, even if the test doesn't directly reference it.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1244
File: utilities/os_utils.py:248-250
Timestamp: 2025-07-08T05:51:06.314Z
Learning: User rnetser prefers fail-fast code design where functions should raise exceptions immediately when encountering unexpected or malformed input, rather than implementing defensive error handling with fallbacks. They value making problems visible immediately during development over graceful degradation.
Learnt from: vamsikrishna-siddu
Repo: RedHatQE/openshift-virtualization-tests PR: 2199
File: tests/storage/test_online_resize.py:108-113
Timestamp: 2025-09-28T14:43:07.181Z
Learning: In the openshift-virtualization-tests repo, PR `#2199` depends on PR `#2139` which adds architecture-specific OS_FLAVOR attributes to the Images.Cirros class (OS_FLAVOR_CIRROS for x86_64/ARM64, OS_FLAVOR_FEDORA for s390x), enabling conditional logic based on the underlying OS flavor in tests.
Learnt from: vamsikrishna-siddu
Repo: RedHatQE/openshift-virtualization-tests PR: 2199
File: tests/storage/test_online_resize.py:108-113
Timestamp: 2025-09-28T14:43:07.181Z
Learning: In the openshift-virtualization-tests repo, PR `#2199` depends on PR `#2139` which adds the OS_FLAVOR attribute to the Images.Cirros class, making Images.Cirros.OS_FLAVOR available for conditional logic in tests.
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: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 2045
File: tests/virt/cluster/vm_lifecycle/conftest.py:46-47
Timestamp: 2025-09-15T06:49:53.478Z
Learning: In the openshift-virtualization-tests repo, large fixture refactoring efforts like the golden image data source migration are handled incrementally by directory/team ownership. The virt/cluster directory is handled separately from virt/node, tests/infra, tests/storage, etc., with each area managed by relevant teams in follow-up PRs.
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: 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: 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: 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: 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: 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: 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: 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/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.
Learnt from: yossisegev
Repo: RedHatQE/openshift-virtualization-tests PR: 4328
File: tests/network/flat_overlay/test_multi_network_policy.py:32-32
Timestamp: 2026-04-14T13:00:57.514Z
Learning: In this repository’s pytest quarantine tests, do not flag or change `pytest.mark.jira(<id>, run=False)` as ineffective. `run=False` on `pytest.mark.jira()` is intentionally handled by the `pytest_jira` plugin, which conditionally skips the test when the referenced Jira issue is open. This is the correct behavior for **Category 1 (Product Bug)** quarantines; the `pytest.mark.xfail(run=False)` approach is reserved for **Category 2 (Automation Issue)** quarantines only (as documented in `docs/QUARANTINE_GUIDELINES.md`).
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 4328
File: tests/network/flat_overlay/test_multi_network_policy.py:0-0
Timestamp: 2026-04-14T16:15:31.065Z
Learning: When reviewing Python tests, avoid redundant parentheses around f-strings in `pytest.mark.xfail` decorators. Prefer `pytest.mark.xfail(reason=f"...", run=False)` over `pytest.mark.xfail(reason=(f"..."), run=False)`, and flag/suggest removing the extra wrapping parentheses where found.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 4328
File: tests/network/flat_overlay/test_multi_network_policy.py:32-32
Timestamp: 2026-04-14T16:15:33.012Z
Learning: For this repository (RedHatQE/openshift-virtualization-tests), when reviewing any PR with "Quarantine" in the title or a `quarantine` label, check compliance with `docs/QUARANTINE_GUIDELINES.md` in any affected pytest test code. Specifically: (1) For Category 1 (Product Bug), require `pytest.mark.jira("CNV-XXXXX", run=False)` and do not suggest replacing it with `xfail` (the `pytest_jira` plugin conditionally skips when the Jira issue is open). (2) For Category 2 (Automation Issue), require `pytest.mark.xfail(run=False, reason=...)` (pytest handles the skip). During review, flag quarantine PRs that use the wrong marker/category, omit a Jira ticket reference for Category 1, or use `run=False` in an incorrect context; raise these compliance questions even if later resolution confirms the marker was correct.
Learnt from: servolkov
Repo: RedHatQE/openshift-virtualization-tests PR: 4542
File: tests/network/libs/bgp.py:333-354
Timestamp: 2026-04-21T19:08:39.771Z
Learning: In this codebase, when using `retry` imported from `timeout_sampler`, the decorated function must indicate success by returning a truthy value. The decorator retries until it encounters a truthy `sample` (i.e., it does `if sample: return sample`). Therefore, ensure `retry`-decorated functions include `return True` (or another truthy value) on success; avoid removing/altering it on the assumption that the caller ignores the return value. If the function returns `None` (or has no return statement), it will keep retrying and typically end in `TimeoutExpiredError`.
Collaborator
There was a problem hiding this comment.
@jpeimer (this is not the proper flow but with the current constraints) to unblock you , can you please have a follow up pr to add the test design?
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Contributor
Author
There was a problem hiding this comment.
@rnetser Thank you! Yes, sure, I will open a follow-up PR
rnetser
requested changes
Apr 30, 2026
Contributor
|
/lgtm |
Contributor
|
@coderabbitai the https://github.com/RedHatQE/openshift-virtualization-tests/blob/main/AGENTS.md#pr-discipline was not followed on this PR, why haven't you noticed it? |
Contributor
|
Review rate limit: 1/1 review remaining, available now. |
openshift-virtualization-qe-bot-5
added
lgtm-rnetser
approved-rnetser
and removed
changes-requested-rnetser
labels
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
10 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:
Jira: https://redhat.atlassian.net/browse/CNV-60016
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
Summary by CodeRabbit
New Features
Tests
Assisted-by: Claude noreply@anthropic.com