-
Notifications
You must be signed in to change notification settings - Fork 165
[1LP][RFR]Fixing dual_nic_migration Test #9880
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mnadeem92 : I am trying to understand the need of this change . Why are we removing parameters from test and giving values in fixture ?
The Test case failed as the created VM was not visible in migration plan, the reason was in mapping it select only 1 network map as we passes the parameterized value from Test case ["VM Network", "ovirtmgmt", Templates.DUAL_NETWORK_TEMPLATE], where as the instance gets created using "DUAL_NETWORK_TEMPLATE", so we need to add both network at the same time to the migration mapping. So i just take examples from other similar test cases and their fixture like "mapping_data_dual_vm_obj_dual_datastore" where we set input directly in fixture only, IMO it is good to keep same pattern and it is the easy way to fix the issue. |
source_provider.one_of(VMwareProvider) and source_provider.version != 6.5 and | ||
"DPortGroup" in source_type, | ||
reason='Dual network of DPortGroup only supported on source provider version 6.5' | ||
) | ||
def test_dual_nics_migration(request, appliance, provider, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will still not apply for Vmware67-ims and vmware6.0 I think . Please check , else you will have to add this uncollect code again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sshveta DPortGroup is now available in Vsphere67-ims , So we are good to go with it and AFAIK we are not executing our suits on vsphere6.0, So we are good to remove this uncollected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm suspect about default value to fixture parameter. please recheck it one time.
Else LGTM
cfme/fixtures/v2v_fixtures.py
Outdated
@@ -523,7 +523,10 @@ def mapping_data_dual_vm_obj_dual_datastore(request, appliance, source_provider, | |||
|
|||
@pytest.fixture(scope="function") | |||
def mapping_data_vm_obj_dual_nics(request, appliance, source_provider, provider, | |||
source_type, dest_type, template_type): | |||
template_type=Templates.DUAL_NETWORK_TEMPLATE): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is good practice i.e. providing default value to fixture.
pytest doesn't consider arguments with default values as targets for fixtures/parameterization.
Signed-off-by: mnadeem92 <[email protected]>
2051e48
to
2daf6e1
Compare
I detected some fixture changes in commit 2daf6e1 The global fixture
Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃 |
Signed-off-by: mnadeem92 [email protected]
{{ pytest: cfme/tests/v2v/test_v2v_migrations.py -k "test_dual_nics_migration" --use-provider rhv43 --use-provider vsphere67-ims --provider-limit 2 -v }}