Skip to content
This repository has been archived by the owner on Apr 7, 2022. It is now read-only.

[1LP][RFR] updating TC to fix failure #10005

Merged
merged 1 commit into from
Apr 1, 2020

Conversation

mnadeem92
Copy link
Contributor

@mnadeem92 mnadeem92 commented Mar 20, 2020

Signed-off-by: mnadeem92 [email protected]

{{ pytest: cfme/tests/v2v/test_v2v_cancel_migrations.py -k "test_cancel_migration_attachments" --use-provider={osp13-ims,rhv-ims} --use-provider vsphere67-ims --provider-limit 2 -v }}

PRT: Should not block this PR due to fail in 5.10 as the failure is not associated with this PR, we have high priority for 5.11 as build is going to deliver shortly.
5.11: Passed
5.10: Failed due to fixture issue (cfme/fixtures/skip_not_implemented.py:9: ) need to address in a separate PR

Travis is failing due to : #10029

@mnadeem92 mnadeem92 force-pushed the fix_cancel_migration branch from a3e338c to 93eb206 Compare March 20, 2020 16:13
@mnadeem92 mnadeem92 changed the title updating TC to fix failure [WIPTEST] updating TC to fix failure Mar 20, 2020
@mnadeem92 mnadeem92 force-pushed the fix_cancel_migration branch 2 times, most recently from e257727 to 249f916 Compare March 23, 2020 14:00
@mnadeem92 mnadeem92 changed the title [WIPTEST] updating TC to fix failure [RFR] updating TC to fix failure Mar 24, 2020
Copy link
Contributor

@sshveta sshveta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mnadeem92 : Instead of making this change see if you can use the existing method
https://github.com/ManageIQ/integration_tests/blob/93eb206095c137646397b84e6f2a614fa72a7bf1/cfme/tests/v2v/test_v2v_cancel_migrations.py#L51

def cancel_migration_plan () is a fixture in the same test file and it can wait for the plan .

cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jarovo jarovo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, just some suggestion for better readability

@mnadeem92 mnadeem92 force-pushed the fix_cancel_migration branch from 249f916 to cab4d42 Compare March 25, 2020 09:10
@mnadeem92
Copy link
Contributor Author

@sshveta I would prefer to press cancel button in between when some disk gets already migrated, the fixture cancel_migration_plan () press cancel button as soon as plan went to in-progress state by checking the presence of spinner and at this point we are not sure if actual disk migration gets started as it first allocate conversion host and then power-off the VM, run pre-migration playbook etc, However this PR actually check the migration timing from the GUI and press cancel button after a specific time period of disk migration. I think this is a good ad-on to provide the control on performing operation based on migration elapsed timing.

@sshveta
Copy link
Contributor

sshveta commented Mar 25, 2020

@sshveta I would prefer to press cancel button in between when some disk gets already migrated, the fixture cancel_migration_plan () press cancel button as soon as plan went to in-progress state by checking the presence of spinner and at this point we are not sure if actual disk migration gets started as it first allocate conversion host and then power-off the VM, run pre-migration playbook etc, However this PR actually check the migration timing from the GUI and press cancel button after a specific time period of disk migration. I think this is a good ad-on to provide the control on performing operation based on migration elapsed timing.

@mnadeem92 : In_progress is a method to track progress of plans , I don't think we should change it for each test . If test can be handled by fixtures then that should be first approach . You can modify cancel_migration_plan fixture as per your need . It is using

 request_details_list = migration_plan.get_plan_vm_list(wait_for_migration=False)

which you can modify can wait for whatever time you need.

@mnadeem92
Copy link
Contributor Author

@sshveta Indeed the In_progress fixture should track the process of the plan, However IMO the fixture should gave below option as the In progress is a time taking process which might take 25+ Minutes

  1. Check plan progress and exit
  2. Check plan progress and continue wait till the time its gets completed

Now, lets talk about the all cancel Test cases is not wait till actual migration started, it cancel the migration even before that which is not the recommended way to perform the cancel operation, As per engineering team the migration should gets cancel after at-least disk migration gets started.

So, here is my plan to do changes.

  1. Modify the actual fixture In_progress to gave option to track plan progress status and exit based on the given time duration, default flow will be as it is where it will wait till the plan progress gets completed.

  2. Modify the "cancel_migration_plan" local fixture to use the above fixture to make sure cancel button pressed only after disk migration gets started,

  3. All Cancel related TC will use the local fixture cancel_migration_plan for their operation.

Please let me know if required, we can have a quick call to discuss the same.

@john-dupuy
Copy link
Contributor

@sshveta okay to move this one to 1LP?

@nachandr
Copy link
Contributor

@mnadeem92 , please add a comment stating that the recommended way to test the migration cancel operation is to cancel the migration after disk migration starts. This would apply to all the tests in the test_v2v_cancel_migrations.py module.

@mnadeem92 mnadeem92 changed the title [RFR] updating TC to fix failure [WIP] updating TC to fix failure Mar 26, 2020
@mnadeem92 mnadeem92 force-pushed the fix_cancel_migration branch from cab4d42 to dd9a70c Compare March 31, 2020 15:16
@mnadeem92 mnadeem92 changed the title [WIP] updating TC to fix failure [RFR] updating TC to fix failure Mar 31, 2020
Copy link
Contributor

@sshveta sshveta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good Nadeem , I think we can remove parameters now .

@@ -137,10 +137,8 @@ def _get_plan_status_and_cancel():
@pytest.mark.parametrize(
"source_type, dest_type, template_type",
[["nfs", "nfs", Templates.RHEL7_MINIMAL]])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need parametrization now . Infra mapping is coming from the fixture .

@mnadeem92 mnadeem92 force-pushed the fix_cancel_migration branch from dd9a70c to f06d26f Compare April 1, 2020 05:31
Signed-off-by: mnadeem92 <[email protected]>
@mnadeem92 mnadeem92 force-pushed the fix_cancel_migration branch from f06d26f to 185d54f Compare April 1, 2020 11:18
@sshveta sshveta changed the title [RFR] updating TC to fix failure [1LP][RFR] updating TC to fix failure Apr 1, 2020
@jawatts jawatts self-assigned this Apr 1, 2020
Copy link
Contributor

@jawatts jawatts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting on travis, I restarted it

@jawatts jawatts merged commit 7c7dc78 into ManageIQ:master Apr 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants