-
Notifications
You must be signed in to change notification settings - Fork 164
[RFR] New test: test_reconfigure_vm_vmware_multiple #10236
Conversation
3d744c2
to
2538194
Compare
2538194
to
87aae13
Compare
full_vm.wait_for_vm_state_change(full_vm.STATE_ON) | ||
else: | ||
raise Exception("Unknown power state - unable to continue!") | ||
raise Exception("Unknown power state - unable to continue!") |
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.
Optional: Umn, I would rather used either AssertionError
or ValueError
here to be more specific.
view = navigate_to(reconfig_request.parent, "All") | ||
# Get the latest request | ||
request_id = max( | ||
[ |
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.
The list is redundant here.
""" | ||
vms = create_vms_modscope |
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.
Optional:
I think you are expecting only one vm to get created here, but I am not sure. I like to do this:
vms, = create_vms_modscope
This makes clear the only single vm is expected. Also this statement has an advantage that it fails when there are more or less than expected vms returned by the fixture and I think it can be statically checked as well in many situations, so we may see a problem with static type checker.
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 am expecting 2 vms here, I need the whole vms list to get entities and navigate to the reconfiguration page. We already have tests to check reconfiguration for 1 VM, we're testing for multiple VMs here.
But to make sure I receive 2 vms, I added one more item({"num_vms": 2}) to the create_vms_modscope
parametrization to ensure it creates 2 vms, incase someone changes default value in the fixture. So as long as there's no other change in the fixture, it will work just fine for me.
try: | ||
reconfig_request.remove_request() | ||
except (RequestException, NoSuchElementException): | ||
# sometimes the delete button is not present on the page. |
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.
Thanks for the comment! It is helpful for me as a reader of the code.
new_memory = current_memory + 1 | ||
else: | ||
if current_memory < 1: | ||
pytest.skip("Cannot test when the value is less than the minimum required.") |
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.
Optional nitpick:
The skip message seem to say something different than what the code seem to be doing. I think it should read
"Cannot decrease memory bellow 1".
I mean: I cannot find the code which gets the info about what is the minimum requirement. There is just an assumption the minimum is 1, but it could be something else, like 3, right?
new_processor = current_processors + 1 | ||
else: | ||
if current_processors < 1: | ||
pytest.skip("Cannot test when value is less than the minimum required.") |
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.
There is double space on this line.
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.
sharp eye!
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 found just the double-space which I think should be fixed, otherwise I probably am just nitpicking.
c8563a3
to
54cc0b5
Compare
I detected some fixture changes in commit 54cc0b5 The global fixture
The global fixture
The local fixture
The local fixture
Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃 |
Thank you for the review @JaryN, your suggestion were good! |
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.
LGTM. thanks for automating :)
@pytest.mark.tier(2) | ||
@pytest.mark.provider([VMwareProvider]) | ||
def test_reconfigure_vm_vmware_mem_multiple(): | ||
@pytest.mark.provider([VMwareProvider], selector=ONE_PER_TYPE, scope="module") |
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.
This is effectively a ONE
selector, as VMwareProvider is a single type.
Purpose or Intent
InfraVmReconfigureView.is_displayed
to work when reconfiguring multiple VMs.-Deleting tests
PRT Run
{{ pytest: cfme/tests/infrastructure/test_vm_reconfigure.py::test_reconfigure_vm_vmware_multiple --use-provider=vsphere67-nested --long-running -svvv }}