Move and type OADP related functions from tests directory to the upper level utilities directory and add unit tests#3301
Conversation
📝 WalkthroughWalkthroughRelocates OADP test utilities into production Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3301 +/- ##
==========================================
+ Coverage 98.38% 98.42% +0.04%
==========================================
Files 25 25
Lines 2166 2222 +56
==========================================
+ Hits 2131 2187 +56
Misses 35 35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/data_protection/oadp/conftest.pytests/data_protection/oadp/test_velero.pytests/data_protection/oadp/utils.pyutilities/oadp.py
💤 Files with no reviewable changes (1)
- tests/data_protection/oadp/utils.py
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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: 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: EdDev
Repo: RedHatQE/openshift-virtualization-tests PR: 2920
File: utilities/network.py:31-31
Timestamp: 2025-12-07T13:52:56.070Z
Learning: In the openshift-virtualization-tests repository, the utilities/ package is not intended to be used as a valid, structured module with clear focus and ownership. The team prefers using specialized library locations like tests/network/libs/ for network-related helper code rather than placing everything in utilities/.
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1716
File: tests/virt/conftest.py:289-297
Timestamp: 2025-08-09T01:52:26.683Z
Learning: When user dshchedr moves working code from one location to another in the openshift-virtualization-tests repository, they prefer not to modify it unless there's a real issue, maintaining the original implementation to avoid introducing unnecessary changes.
Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:5-5
Timestamp: 2025-08-26T14:51:22.273Z
Learning: qwang1 contributed the Backup resource to openshift-python-wrapper and the PR was merged. They use uv for dependency management in openshift-virtualization-tests.
Learnt from: EdDev
Repo: RedHatQE/openshift-virtualization-tests PR: 2920
File: utilities/network.py:31-31
Timestamp: 2025-12-07T13:52:56.070Z
Learning: In the openshift-virtualization-tests repository, it is acceptable for utilities/ modules to import from tests/network/libs/ (e.g., utilities/network.py importing from tests/network/libs/sriovnetworknode). The libs modules under tests/network/libs/ are library/tool modules, not test code, and this import direction does not violate architectural principles in this codebase.
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: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:1-5
Timestamp: 2025-09-07T13:16:32.011Z
Learning: In the openshift-virtualization-tests project utilities/oadp.py, DynamicClient instances are passed as parameters rather than created internally, so kubernetes.config import is not needed for client creation fallback patterns.
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: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 2014
File: tests/storage/cdi_clone/test_clone.py:0-0
Timestamp: 2025-09-10T13:25:59.799Z
Learning: In the openshift-virtualization-tests repository, when creating DataVolume objects using ocp_resources, use `api_name="storage"` rather than the default `api_name="pvc"`.
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: dalia-frank
Repo: RedHatQE/openshift-virtualization-tests PR: 1845
File: tests/storage/cdi_import/test_import_http.py:280-282
Timestamp: 2025-08-27T08:34:49.132Z
Learning: The create_pod_for_pvc helper function is still used in other tests beyond the current PR scope and may need to be addressed in a separate PR to avoid breaking existing functionality.
📚 Learning: 2025-08-28T11:29:15.768Z
Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:5-5
Timestamp: 2025-08-28T11:29:15.768Z
Learning: In the openshift-virtualization-tests project, DynamicClient is imported from kubernetes.dynamic, not openshift.dynamic. The standard import pattern is `from kubernetes.dynamic import DynamicClient`.
Applied to files:
utilities/oadp.py
📚 Learning: 2025-09-07T13:16:32.011Z
Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:1-5
Timestamp: 2025-09-07T13:16:32.011Z
Learning: In the openshift-virtualization-tests project utilities/oadp.py, DynamicClient instances are passed as parameters rather than created internally, so kubernetes.config import is not needed for client creation fallback patterns.
Applied to files:
utilities/oadp.pytests/data_protection/oadp/conftest.pytests/data_protection/oadp/test_velero.py
📚 Learning: 2025-12-07T13:52:56.070Z
Learnt from: EdDev
Repo: RedHatQE/openshift-virtualization-tests PR: 2920
File: utilities/network.py:31-31
Timestamp: 2025-12-07T13:52:56.070Z
Learning: In the openshift-virtualization-tests repository, it is acceptable for utilities/ modules to import from tests/network/libs/ (e.g., utilities/network.py importing from tests/network/libs/sriovnetworknode). The libs modules under tests/network/libs/ are library/tool modules, not test code, and this import direction does not violate architectural principles in this codebase.
Applied to files:
tests/data_protection/oadp/conftest.pytests/data_protection/oadp/test_velero.py
📚 Learning: 2025-09-10T13:25:59.799Z
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 2014
File: tests/storage/cdi_clone/test_clone.py:0-0
Timestamp: 2025-09-10T13:25:59.799Z
Learning: In the openshift-virtualization-tests repository, when creating DataVolume objects using ocp_resources, use `api_name="storage"` rather than the default `api_name="pvc"`.
Applied to files:
tests/data_protection/oadp/conftest.pytests/data_protection/oadp/test_velero.py
📚 Learning: 2025-10-16T12:47:04.521Z
Learnt from: rlobillo
Repo: RedHatQE/openshift-virtualization-tests PR: 2249
File: tests/install_upgrade_operators/must_gather/test_must_gather.py:428-441
Timestamp: 2025-10-16T12:47:04.521Z
Learning: In openshift-virtualization-tests repository, DataVolumes in the openshift-virtualization-os-images namespace are volatile resources managed by DataImportCron. They can be created/destroyed between must-gather collection listing and file retrieval, requiring FileNotFoundError exception handling in test_crd_resources to skip these volatile resources gracefully while still validating DataVolumes in other namespaces. There is no pytest_generate_tests hook that filters out datavolumes from test parametrization.
Applied to files:
tests/data_protection/oadp/conftest.pytests/data_protection/oadp/test_velero.py
📚 Learning: 2025-12-15T12:33:06.686Z
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.
Applied to files:
tests/data_protection/oadp/conftest.pytests/data_protection/oadp/test_velero.py
📚 Learning: 2025-12-22T16:27:40.244Z
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.
Applied to files:
tests/data_protection/oadp/conftest.pytests/data_protection/oadp/test_velero.py
📚 Learning: 2025-08-09T01:52:26.683Z
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1716
File: tests/virt/conftest.py:289-297
Timestamp: 2025-08-09T01:52:26.683Z
Learning: When user dshchedr moves working code from one location to another in the openshift-virtualization-tests repository, they prefer not to modify it unless there's a real issue, maintaining the original implementation to avoid introducing unnecessary changes.
Applied to files:
tests/data_protection/oadp/test_velero.py
📚 Learning: 2025-05-21T09:41:46.532Z
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 954
File: tests/storage/storage_migration/conftest.py:156-176
Timestamp: 2025-05-21T09:41:46.532Z
Learning: In the RedHatQE/openshift-virtualization-tests codebase, the pattern of calling to_dict() on a DataVolume object and then accessing dv.res to populate data_volume_template is the preferred approach rather than building the template dict explicitly.
Applied to files:
tests/data_protection/oadp/test_velero.py
🧬 Code graph analysis (1)
tests/data_protection/oadp/conftest.py (1)
utilities/oadp.py (3)
VeleroRestore(149-186)create_rhel_vm(97-146)is_storage_class_support_volume_mode(197-201)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
🔇 Additional comments (6)
tests/data_protection/oadp/conftest.py (1)
15-20: Import updates look correct.The import sources for
VeleroRestoreandis_storage_class_support_volume_modehave been properly updated to referenceutilities.oadpinstead of the tests directory. This aligns with the PR objective to move OADP utilities out of tests.tests/data_protection/oadp/test_velero.py (1)
4-6: Import updates are correct.The import source for
check_file_in_vmhas been properly updated toutilities.oadp. Thewait_for_restored_dvfunction remains in the tests directory, indicating a selective migration of utilities.utilities/oadp.py (4)
8-9: New imports support the moved functionality correctly.The imports for
Restore,StorageProfile, andconsoleare appropriate for the newly addedVeleroRestoreclass and helper functions.Also applies to: 12-12
21-26: Constants imported for the moved utilities.The additional constants (
FILE_NAME_FOR_BACKUP,LS_COMMAND,TEXT_TO_TEST,TIMEOUT_20SEC) are correctly imported to support thecheck_file_in_vmfunction.
189-194: Helper function implementation is straightforward.The
check_file_in_vmfunction appropriately validates file existence and content in a VM via console interaction.
197-201: Storage class volume mode check is correctly implemented.The
is_storage_class_support_volume_modefunction properly queries the StorageProfile to determine volume mode support.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@utilities/unittests/test_oadp.py`:
- Around line 642-643: Replace the identity comparison "assert
restore.wait_complete is True" with a truthy assertion on the property and add a
failure message, e.g. assert restore.wait_complete, "restore.wait_complete
expected truthy but was falsy"; likewise add an assertion message to the timeout
check using restore.timeout (e.g. assert restore.timeout == 600,
"restore.timeout expected 600 but got {restore.timeout}") so both
restore.wait_complete and restore.timeout have clear, non-identity assertions
and helpful failure messages.
- Around line 45-46: The helper function assert_extra is dead code (unused) in
the test file; remove the entire assert_extra function definition to clean up
the test module and reduce maintenance overhead, ensuring no tests reference
assert_extra (if any do, replace those usages with direct assertions or
reintroduce a meaningful helper with tests updated accordingly) and run the test
suite to confirm no breakage.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utilities/unittests/test_oadp.py (1)
240-245: MEDIUM: Add failure messages to new assertions.
Failure messages make CI triage faster and are required by repo test guidelines.✅ Example improvement
- assert result == backup + assert result == backup, "VeleroBackup.__enter__ should return the backup instance"Please apply similar messages to other new
assertstatements in this file.
As per coding guidelines, test assertions must include failure messages.
🤖 Fix all issues with AI agents
In `@utilities/oadp.py`:
- Around line 134-138: The super().__exit__ calls in VeleroBackup.__exit__ and
VeleroRestore.__exit__ pass positional args; change them to use named arguments
to avoid ordering bugs by calling
super().__exit__(exception_type=exception_type, exception_value=exception_value,
traceback=traceback) in both methods and ensure the _velero_teardown invocation
remains unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@utilities/unittests/test_oadp.py`:
- Around line 638-639: Add descriptive failure messages to the VeleroRestore
assertions so test failures are easier to triage: update assertions checking
restore.wait_complete and restore.timeout (and the other VeleroRestore-related
asserts around the 653-657 block) to include clear messages like "'wait_complete
should default to True'" and "'timeout should default to 600'" (and similar
descriptive messages for the other assertions) so each assert reads assert
<condition>, '<descriptive message>'.
- Around line 771-784: Add descriptive failure messages to the assertions in the
volume-mode unit tests so failures are actionable: update the assert in
test_volume_mode_not_supported (which calls
is_storage_class_support_volume_mode) to include a message like "'expected
storage class to NOT support Block volume mode but it did'"; likewise add a
message to the corresponding positive test (the one asserting True) e.g.,
"'expected storage class to support Block volume mode but it did not'". Ensure
messages reference the storage_class_name and requested_volume_mode variables to
aid debugging.
…r level utilities directory For upcoming oadp-chaos test cases. And add unit tests for oadp utilities. Signed-off-by: Qixuan Wang <qwang@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/data_protection/oadp/test_velero.py (1)
108-121: MEDIUM: Consider@pytest.mark.usefixturesfor consistency.Tests
test_restore_cloned_dv(line 108) andtest_restore_uploaded_dv(line 115) passvelero_restore_second_namespace_with_datamoveras a parameter but don't use its return value — they only need its side effects.WHY this matters: Consistency within the same file improves readability. The tests at lines 92 and 101 already use the
@pytest.mark.usefixturespattern.However, this is a refactoring PR focused on moving utilities, so this change is out of scope per repository learnings about keeping refactoring PRs focused.
♻️ Example for a follow-up PR
`@pytest.mark.s390x` `@pytest.mark.polarion`("CNV-10590") +@pytest.mark.usefixtures("velero_restore_second_namespace_with_datamover") def test_restore_cloned_dv( cloned_rhel_dv, - velero_restore_second_namespace_with_datamover, ): wait_for_restored_dv(dv=cloned_rhel_dv)Based on learnings: "In the openshift-virtualization-tests repository, user rnetser prefers to keep refactoring PRs strictly focused on moving/organizing code into more granular modules without adding new functionality, error handling, or behavioral changes."
🤖 Fix all issues with AI agents
In `@utilities/oadp.py`:
- Around line 74-91: The helper function _velero_teardown is missing a return
type annotation and has two unused parameters; update its signature to add an
explicit return type (e.g., -> None) and rename the unused parameters to
_exception_value and _traceback to indicate intentional non-use, keeping the
rest of the logic intact; ensure references to ResourceTeardownError, LOGGER,
delete_velero_resource, and resource.client/name/kind remain unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@utilities/oadp.py`:
- Around line 195-211: The VeleroRestore class __init__ docstring is missing
Args entries; update the class-level docstring for VeleroRestore to include a
Google-style "Args:" section documenting each constructor parameter (name,
namespace, teardown, wait_complete, timeout, and **kwargs), describing types,
defaults, and the side effects (e.g., resource creation/teardown and optional
wait for completion) and any important behavior (timeouts and namespace default
ADP_NAMESPACE); ensure the docstring follows the existing Google format and
explicitly notes that this is a context manager with side effects.
In `@utilities/unittests/test_oadp.py`:
- Around line 48-54: Replace positional arguments to pytest.mark.parametrize
with explicit keyword arguments: change calls like
pytest.mark.parametrize(("kind", "name"), [("Backup","test-backup"), ...]) to
use argnames=("kind","name") and argvalues=[("Backup","test-backup"), ...].
Apply this change for every pytest.mark.parametrize occurrence in this file
(including the groups around the tuples at lines shown) so the calls use
argnames=... and argvalues=... to improve readability and comply with the
guideline.
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-3301 published |
|
/verified |
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:
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: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2141 🧠 Learnings used |
|
/lgtm |
|
/approve |
|
/lgtm |
|
Successfully removed PR tag: quay.io/openshift-cnv/openshift-virtualization-tests:pr-3301. |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
Short description:
For the upcoming oadp-chaos test cases, because
Import from tests is not allowed.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
Chores
✏️ Tip: You can customize this high-level summary in your review settings.