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

[1LP][RFR] Update snapshot tests to use create_vm fixture #9942

Merged
merged 9 commits into from
Mar 3, 2020

Conversation

prichard77
Copy link
Contributor

@prichard77 prichard77 commented Feb 20, 2020

Purpose or Intent

  • Updating tests in test_snapshot.py to use new create_vm global fixture.

PRT Run

The function test_verify_revert_snapshot fails for rhv providers. This was failing before the update to create_vm and is not caused by it. I am going to begin work on fixing test_verify_revert_snapshot for rhv providers in another PR shortly.

@dajoRH dajoRH added lint-ok and removed needs-lint labels Feb 23, 2020
@prichard77 prichard77 changed the title [WIP] Update snapshot tests to use create_vm fixture [WIPTEST] Update snapshot tests to use create_vm fixture Feb 23, 2020
@dajoRH dajoRH added WIP-testing and removed WIP labels Feb 23, 2020
@prichard77
Copy link
Contributor Author

prichard77 commented Feb 23, 2020

PRT is invalid for some reason. I am not sure why. I have initiated a rerun. I tested each test function in test_snpshot.py locally and all passed except for test_verify_revert_snapshot, as I mentioned in earlier comment. I'll be fixing this soon.

@prichard77 prichard77 changed the title [WIPTEST] Update snapshot tests to use create_vm fixture [RFR] Update snapshot tests to use create_vm fixture Feb 23, 2020
@prichard77
Copy link
Contributor Author

PRT keeps going to Invalid. Might be an issue with it. I'll bring it up Monday. No need to bother anyone on a Sunday.

@dajoRH
Copy link
Contributor

dajoRH commented Feb 24, 2020

I detected some fixture changes in commit f3f27dd

The local fixture create_vm is used in the following files:

  • cfme/tests/infrastructure/test_vm_clone.py
    • test_vm_clone
    • test_vm_clone_neg

Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃

@prichard77
Copy link
Contributor Author

I updated create_vm by wrapping up vm.cleanup_on_provider in a wait_for with HandleException=True. I can add exception handling directly to cleanup_on_provider in another PR or do it here?

Copy link
Contributor

@john-dupuy john-dupuy left a comment

Choose a reason for hiding this comment

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

Just one question about your change to the local create_vm in test_vm_clone.py. Changes LGTM though and PRT results show that the vm-name has your nic in it. PRT failures are not a result of changes here.

@prichard77
Copy link
Contributor Author

I added a wait_for to added a wait_for to cleanup_on_provide that has handle_exception=True by default. It was suggested we use kwarg to control if we want to handle exceptions in the cleanup, but I don't know how this get used as we do not call create_vm directly (it's a fixture).

@dgaikwad
Copy link
Contributor

@prichard77 Please check PRT failure

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.

Nothing much going on in this PR, mostly just s/name/othername/g

Good job anyway. It seems like the create_vm gets some momentum.

@prichard77
Copy link
Contributor Author

I added a wait_for to added a wait_for to cleanup_on_provide that has handle_exception=True by default. It was suggested we use kwarg to control if we want to handle exceptions in the cleanup, but I don't know how this get used as we do not call create_vm directly (it's a fixture).

"""Clean up entity on the provider if it has been created on the provider

Helper method to avoid NotFoundError's during test case tear down.
"""
if self.exists_on_provider:
self.mgmt.cleanup()
wait_for(lambda: self.mgmt.cleanup, handle_exception=handle_cleanup_exception,
timeout=900)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a lengthy timeout, if cleanup fails for some reason and we are stuck because handle_exception=True, we could spend 15 min here. I think 300 or maybe even 180 would be fine here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reduced to 300.

Copy link
Contributor

@john-dupuy john-dupuy left a comment

Choose a reason for hiding this comment

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

1 question about timeout, but LGTM, moving to [1LP][WIPTEST]

@john-dupuy john-dupuy changed the title [RFR] Update snapshot tests to use create_vm fixture [1LP][WIPTEST] Update snapshot tests to use create_vm fixture Feb 26, 2020
@prichard77 prichard77 changed the title [1LP][WIPTEST] Update snapshot tests to use create_vm fixture [1LP][RFR] Update snapshot tests to use create_vm fixture Feb 26, 2020
@jawatts jawatts self-assigned this Feb 28, 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.

Looks good! waiting on evaluation of PRT failures

@jawatts jawatts changed the title [1LP][RFR] Update snapshot tests to use create_vm fixture [1LP][WIPTEST] Update snapshot tests to use create_vm fixture Feb 28, 2020
@prichard77
Copy link
Contributor Author

I mentioned in earlier comments that AFAIK the failures are for tests with BZs against them and/or were failing before the change was made to use create_vm.

@jawatts
Copy link
Contributor

jawatts commented Feb 28, 2020

Thanks PJ!

@prichard77 prichard77 changed the title [1LP][WIPTEST] Update snapshot tests to use create_vm fixture [1LP][RFR] Update snapshot tests to use create_vm fixture Feb 28, 2020
@prichard77
Copy link
Contributor Author

What are we waiting on here? i have added several comments on the PRT results and we have 3 approvals. Did I miss something?

@mshriver
Copy link
Member

mshriver commented Mar 3, 2020

@prichard77 it was 1LPWIPTEST during freeze on Friday, and was marked 1LPRFR at 5:24PM EST.

Nothing gets merged until the release is tagged, which it was last night.

2nd level reviewers haven't come back to the RFR lists to re-review yet.

@mshriver mshriver merged commit 012785f into ManageIQ:master Mar 3, 2020
@prichard77 prichard77 deleted the useCreateVM4snap branch March 10, 2020 18:33
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