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

[1LP][RFR] New Test: Checks moving VMWare vm to custom vm folder #9230

Merged
merged 2 commits into from
Dec 27, 2019

Conversation

ganeshhubale
Copy link
Member

@ganeshhubale ganeshhubale commented Aug 21, 2019

Purpose or Intent

PRT Run

{{ pytest: cfme/tests/automate/test_common_methods.py::test_move_vm_into_folder --use-provider=complete --use-template-cache -qsvv }}

@ganeshhubale ganeshhubale changed the title [WIPTEST] New Test: Checks moving VMWare vm to custom vm folder [RFR] New Test: Checks moving VMWare vm to custom vm folder Aug 22, 2019
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.

Nice test case! LGTM 👍

@john-dupuy john-dupuy changed the title [RFR] New Test: Checks moving VMWare vm to custom vm folder [1LP][RFR] New Test: Checks moving VMWare vm to custom vm folder Aug 22, 2019
Copy link
Contributor

@izapolsk izapolsk left a comment

Choose a reason for hiding this comment

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

good PR. I like it. there're two reservations though.

  1. please check my comment
  2. your test hasn't been run in PRT

"""Create Vm folder on VMWare provider"""
folder = provider.mgmt.create_folder(f"{fauxfactory.gen_alpha()}")
yield folder
vm_folder.Destroy()
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure there should be vm_folder instead of folder ?

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad!
It should be folder.
Thanks :)

@izapolsk izapolsk changed the title [1LP][RFR] New Test: Checks moving VMWare vm to custom vm folder [1LP][WIP] New Test: Checks moving VMWare vm to custom vm folder Aug 23, 2019
@dajoRH dajoRH added the WIP label Aug 23, 2019
@pytest.fixture(scope="module")
def vm_folder(provider):
"""Create Vm folder on VMWare provider"""
folder = provider.mgmt.create_folder(f"{fauxfactory.gen_alpha()}")
Copy link
Contributor

Choose a reason for hiding this comment

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

@ganeshhubale I think, we wont need string formatting here.

@ganeshhubale ganeshhubale changed the title [1LP][WIP] New Test: Checks moving VMWare vm to custom vm folder [1LP][WIPTEST] New Test: Checks moving VMWare vm to custom vm folder Aug 23, 2019
@dajoRH dajoRH added WIP-testing and removed WIP labels Aug 23, 2019
@dajoRH dajoRH changed the title [1LP][WIPTEST] New Test: Checks moving VMWare vm to custom vm folder [1LP][WIP] New Test: Checks moving VMWare vm to custom vm folder Aug 23, 2019
@ganeshhubale ganeshhubale changed the title [1LP][WIP] New Test: Checks moving VMWare vm to custom vm folder [1LP][WIPTEST] New Test: Checks moving VMWare vm to custom vm folder Aug 30, 2019
@dajoRH dajoRH added WIP-testing and removed WIP labels Aug 30, 2019
@dajoRH dajoRH changed the title [1LP][WIPTEST] New Test: Checks moving VMWare vm to custom vm folder [1LP][WIP] New Test: Checks moving VMWare vm to custom vm folder Sep 5, 2019
Copy link
Contributor

@kedark3 kedark3 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, just couple of changes required.

@pytest.fixture(scope="function")
def vm_folder(provider):
"""Create Vm folder on VMWare provider"""
folder = provider.mgmt.create_folder(fauxfactory.gen_alpha())
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be prefixed with something meaningful. Please use some context+fauxfactory.gen_alpha()
As a VMware admin I would like to know who is creating the folders.

"""Create Vm folder on VMWare provider"""
folder = provider.mgmt.create_folder(fauxfactory.gen_alpha())
yield folder

Copy link
Contributor

Choose a reason for hiding this comment

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

REQUIRED: Remove the folder after test is finished.

Copy link
Contributor

Choose a reason for hiding this comment

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

folder_ does not seem to be a good start for name, maybe add something like test_move_vm_folder_{fauxfactory name} might be longer name but gives more context.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ganeshhubale please review above suggestion, add little more context to name folder_ .. maybe test_folder_ .


@request.addfinalizer
def _finished():
fd = vm_folder.Destroy()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be part of fixture that created the folder.

Copy link
Contributor

@sbulage sbulage left a comment

Choose a reason for hiding this comment

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

Please check consider suggestion, else LGTM 💯

@@ -320,3 +321,60 @@ def _finalize():
assert provision_request.is_succeeded(
method="ui"
), f"Provisioning failed: {provision_request.row.last_message.text}"


@pytest.fixture(scope="function")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:
@ganeshhubale Can you please move this fixture to above, not in between tests.

@ganeshhubale ganeshhubale force-pushed the move_into branch 3 times, most recently from 3f0fe5c to b3ae9b9 Compare October 9, 2019 10:22
@ganeshhubale ganeshhubale force-pushed the move_into branch 2 times, most recently from 5e27c5b to d969923 Compare December 3, 2019 06:40
@dajoRH
Copy link
Contributor

dajoRH commented Dec 27, 2019

I detected some fixture changes in commit 8b0d4cd

The local fixture vm_folder is used in the following files:

  • cfme/tests/automate/test_common_methods.py
    • test_move_vm_into_folder

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

@ganeshhubale ganeshhubale changed the title [1LP][WIPTEST] New Test: Checks moving VMWare vm to custom vm folder [1LP][RFR] New Test: Checks moving VMWare vm to custom vm folder Dec 27, 2019
@izapolsk izapolsk self-requested a review December 27, 2019 11:52
except CandidateNotFound:
return False

wait_for(lambda: _check, fail_func=view.browser.refresh, timeout=600, delay=5,
Copy link
Contributor

@izapolsk izapolsk Dec 27, 2019

Choose a reason for hiding this comment

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

OPTIONAL: I'd advice wrapping this into try/except block with pytest.fail next time. currently it will appear broken if this wait_for fails.

@izapolsk izapolsk merged commit 60b048f into ManageIQ:master Dec 27, 2019
spusateri pushed a commit to spusateri/integration_tests that referenced this pull request Jan 27, 2020
…ageIQ#9230)

* New Test: This test case checks moving VMWare vm to custom folder via automate method

* Updated with requested changes
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