Update hyperv features test to use instance types #1126
Conversation
WalkthroughThe test suite for Hyper-V feature support in virtual machines was refactored to use new fixtures and abstractions for VM instantiation and configuration. The tests now utilize resource-based instance type and preference objects, replacing direct dictionary manipulation and template-based VM creation. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Report bugs in Issues The following are automatically added:
Available user actions:
PR will be approved when the following conditions are met:
Approvers and Reviewers
Supported /retest check runs
Supported labels
|
openshift-virtualization-qe-bot-2
requested review from
vsibirsk,
kbidarkar and
SiboWang1997
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
/verified |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/virt/node/hyperv_support/test_hyperv_features_in_vm.py (1)
46-54: Add docstring to document the fixture purpose and parameters.The fixture implementation is correct, but it's missing documentation. Consider adding a docstring to explain its purpose and expected parameters.
@pytest.fixture() def hyperv_instance_type(request): + """Create a VirtualMachineClusterInstancetype for Hyper-V testing. + + Args: + request: pytest request object containing param dict with optional 'model' key + for CPU model configuration. + + Yields: + VirtualMachineClusterInstancetype: Instance type configured for Hyper-V testing + """ with VirtualMachineClusterInstancetype(🧰 Tools
🪛 Pylint (3.3.7)
[convention] 47-47: Missing function or method docstring
(C0116)
[warning] 52-52: Redefining name 'hyperv_instance_type' from outer scope (line 47)
(W0621)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/virt/node/hyperv_support/test_hyperv_features_in_vm.py(6 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
tests/virt/node/hyperv_support/test_hyperv_features_in_vm.py
[error] 4-4: Unable to import 'ocp_resources.virtual_machine_cluster_instancetype'
(E0401)
[error] 5-5: Unable to import 'ocp_resources.virtual_machine_cluster_preference'
(E0401)
[error] 6-6: Unable to import 'pytest_testconfig'
(E0401)
[error] 8-13: Unable to import 'tests.os_params'
(E0401)
[error] 14-14: Unable to import 'utilities.constants'
(E0401)
[error] 15-15: Unable to import 'utilities.storage'
(E0401)
[error] 16-16: Unable to import 'utilities.virt'
(E0401)
[warning] 27-27: Redefining name 'hyperv_instance_type' from outer scope (line 47)
(W0621)
[warning] 28-28: Redefining name 'hyperv_preference' from outer scope (line 57)
(W0621)
[convention] 47-47: Missing function or method docstring
(C0116)
[warning] 52-52: Redefining name 'hyperv_instance_type' from outer scope (line 47)
(W0621)
[convention] 57-57: Missing function or method docstring
(C0116)
[warning] 63-63: Redefining name 'hyperv_preference' from outer scope (line 57)
(W0621)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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: build-container
- GitHub Check: tox
🔇 Additional comments (3)
tests/virt/node/hyperv_support/test_hyperv_features_in_vm.py (3)
4-5: LGTM!The new imports for instance type and preference resources align with the PR objective of migrating from templates to instance types.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 4-4: Unable to import 'ocp_resources.virtual_machine_cluster_instancetype'
(E0401)
[error] 5-5: Unable to import 'ocp_resources.virtual_machine_cluster_preference'
(E0401)
27-43: LGTM!The fixture has been successfully refactored to use the new instance type and preference approach, properly utilizing
VirtualMachineForTestswith the appropriate parameters.🧰 Tools
🪛 Pylint (3.3.7)
[warning] 27-27: Redefining name 'hyperv_instance_type' from outer scope (line 47)
(W0621)
[warning] 28-28: Redefining name 'hyperv_preference' from outer scope (line 57)
(W0621)
105-121: LGTM!The test parameterization has been correctly updated to use the new fixtures. The parameters properly specify OS flavor and preferences aligned with the instance type approach.
Signed-off-by: Roni Kishner<[email protected]>
RoniKishner
force-pushed
the
hyperv-instance-type
branch
from
6bcefc1 to
0c7b9e1
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/virt/node/hyperv_support/test_hyperv_features_in_vm.py (1)
56-65: 🛠️ Refactor suggestionSimplify preference creation and add documentation.
This fixture has the same complexity issue identified in previous reviews - it creates a preference, converts to dict, modifies it, and recreates it unnecessarily.
@pytest.fixture() def hyperv_preference(request): + """Create a VirtualMachineClusterPreference for Hyper-V testing. + + Args: + request: pytest request object containing param dict with: + - preference_name: Base preference name to use as template + - hyperv_dict: Optional dict of Hyper-V features to add + + Yields: + VirtualMachineClusterPreference: Preference configured for Hyper-V testing + """ - hyperv_preference_dict = VirtualMachineClusterPreference( - name=request.param.get("preference_name") - ).instance.to_dict() - hyperv_preference_dict["metadata"] = {"name": "hyperv-cluster-preference"} - if hyperv_dict := request.param.get("hyperv_dict"): - hyperv_preference_dict["spec"]["features"].update(hyperv_dict) - with VirtualMachineClusterPreference(kind_dict=hyperv_preference_dict) as hyperv_preference: + # Get the base preference as a template + base_preference = VirtualMachineClusterPreference( + name=request.param.get("preference_name") + ).instance.to_dict() + + # Customize the preference + base_preference["metadata"]["name"] = "hyperv-cluster-preference" + if hyperv_dict := request.param.get("hyperv_dict"): + base_preference["spec"]["features"].update(hyperv_dict) + + with VirtualMachineClusterPreference(kind_dict=base_preference) as hyperv_preference: yield hyperv_preference🧰 Tools
🪛 Pylint (3.3.7)
[convention] 57-57: Missing function or method docstring
(C0116)
[warning] 64-64: Redefining name 'hyperv_preference' from outer scope (line 57)
(W0621)
🧹 Nitpick comments (1)
tests/virt/node/hyperv_support/test_hyperv_features_in_vm.py (1)
46-53: Add docstring to explain the fixture purpose.The hyperv_instance_type fixture implementation is correct, but it's missing documentation to explain its purpose and parameters.
@pytest.fixture() def hyperv_instance_type(request): + """Create a VirtualMachineClusterInstancetype for Hyper-V testing. + + Args: + request: pytest request object containing param dict with: + - model: Optional CPU model to use (e.g., "host-passthrough") + + Yields: + VirtualMachineClusterInstancetype: Instance type configured for Hyper-V testing + """ with VirtualMachineClusterInstancetype(🧰 Tools
🪛 Pylint (3.3.7)
[convention] 47-47: Missing function or method docstring
(C0116)
[warning] 52-52: Redefining name 'hyperv_instance_type' from outer scope (line 47)
(W0621)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/virt/node/hyperv_support/test_hyperv_features_in_vm.py(6 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
tests/virt/node/hyperv_support/test_hyperv_features_in_vm.py
[error] 4-4: Unable to import 'ocp_resources.virtual_machine_cluster_instancetype'
(E0401)
[error] 5-5: Unable to import 'ocp_resources.virtual_machine_cluster_preference'
(E0401)
[error] 6-6: Unable to import 'pytest_testconfig'
(E0401)
[error] 8-13: Unable to import 'tests.os_params'
(E0401)
[error] 14-14: Unable to import 'utilities.constants'
(E0401)
[error] 15-15: Unable to import 'utilities.storage'
(E0401)
[error] 16-16: Unable to import 'utilities.virt'
(E0401)
[warning] 27-27: Redefining name 'hyperv_instance_type' from outer scope (line 47)
(W0621)
[warning] 28-28: Redefining name 'hyperv_preference' from outer scope (line 57)
(W0621)
[convention] 47-47: Missing function or method docstring
(C0116)
[warning] 52-52: Redefining name 'hyperv_instance_type' from outer scope (line 47)
(W0621)
[convention] 57-57: Missing function or method docstring
(C0116)
[warning] 64-64: Redefining name 'hyperv_preference' from outer scope (line 57)
(W0621)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- 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: build-container
- GitHub Check: tox
🔇 Additional comments (7)
tests/virt/node/hyperv_support/test_hyperv_features_in_vm.py (7)
4-5: LGTM! Appropriate imports for the refactoring.The new imports for VirtualMachineClusterInstancetype and VirtualMachineClusterPreference correctly support the transition from template-based to instance type/preference-based VM creation.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 4-4: Unable to import 'ocp_resources.virtual_machine_cluster_instancetype'
(E0401)
[error] 5-5: Unable to import 'ocp_resources.virtual_machine_cluster_preference'
(E0401)
14-16: LGTM! Updated imports align with the new VM creation approach.The imports for OS_FLAVOR constants, data_volume_template_with_source_ref_dict, and VirtualMachineForTests properly support the refactored VM instantiation logic.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 14-14: Unable to import 'utilities.constants'
(E0401)
[error] 15-15: Unable to import 'utilities.storage'
(E0401)
[error] 16-16: Unable to import 'utilities.virt'
(E0401)
22-43: LGTM! Excellent refactoring to use structured VM creation.The hyperv_vm fixture has been successfully updated to use VirtualMachineForTests with explicit instance type and preference parameters, replacing the previous template-based approach. This provides better separation of concerns and follows the recommended patterns.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 22-22: Missing function or method docstring
(C0116)
[refactor] 22-22: Too many arguments (6/5)
(R0913)
[refactor] 22-22: Too many positional arguments (6/5)
(R0917)
[warning] 27-27: Redefining name 'hyperv_instance_type' from outer scope (line 47)
(W0621)
[warning] 28-28: Redefining name 'hyperv_preference' from outer scope (line 57)
(W0621)
105-122: LGTM! Parameterization correctly updated for new fixture approach.The test parameterization properly uses the new hyperv_instance_type and hyperv_preference fixtures, and correctly specifies os_flavor instead of template-based configuration.
147-161: LGTM! Test parameters correctly demonstrate custom Hyper-V feature configuration.The parameterization effectively shows how to add custom Hyper-V features through the preference fixture, with the vendorid configuration being passed via the hyperv_dict parameter.
170-181: LGTM! Clean parameterization for EVMCS testing.The test parameter setup correctly demonstrates the simple case where only the base preference is needed without additional Hyper-V features.
201-212: LGTM! Fedora test correctly configured with EVMCS feature.The parameterization properly demonstrates adding the evmcs Hyper-V feature for Fedora VMs through the preference fixture's hyperv_dict parameter.
|
@RoniKishner Infra team should work only on infra tests, they should not update other component teams tests. I discussed this with both Dan and Dominik during the Ready-Ready meeting and it was mentioned and agreed that moving to instancetypes and preferences Epic is all about only introducing the fixtures and functions ( supporting stuff for the tests for the infra team itself. ) The Epic name was also updated due to this. |
|
@RoniKishner @kbidarkar I think that this PR should be marked as |
|
Moving to draft until we discuss this |
| data_volume_template=data_volume_template_with_source_ref_dict( | ||
| data_source=golden_image_data_source_scope_class | ||
| ), | ||
| disk_type=None, |
Until the Templates officially deprecated - we have to keep the coverage. So, we can't simply switch to using IT/Perf If Infra team is interested in covering same scenarios but with it/perf - they can clone existing tests into their component folder and update tests according to needs. |
What aspect of coverage is this test addressing? |
This test is about Hyper-V features, not about templates. But it was working with templates for a long time and we didn't have any issues with that. Templates are still supported, so from test point of view it still ok. |
Short description:
hyperv_vmwill now use preferences instead of templatesMore details:
The tests in
test_hyperv_features_in_vm.pycurrently use templates (windows-2019 and fedora) to create VMs. This PR updates them to use common-instance types instead, as that is the recommended approach.What this PR does / why we need it:
Change tests from using templates to common-instance types
jira-ticket:
https://issues.redhat.com/browse/CNV-62919
Summary by CodeRabbit