-
Notifications
You must be signed in to change notification settings - Fork 165
[1LP][RFR] Automating basic VMRC Console Test #9703
Conversation
b25e835
to
f5a966a
Compare
f5a966a
to
9a47309
Compare
3efd7e4
to
a842229
Compare
Restarting the PRT, 510 run resulted in Invalid. |
a842229
to
e54efb8
Compare
@@ -237,7 +237,8 @@ def switch_to_appliance(self): | |||
def switch_to_console(self): | |||
"""Switch focus to console tab/window.""" | |||
logger.info("Switching to console: window handle = {}".format(self.console_handle)) | |||
self.browser.selenium.switch_to.window(self.console_handle) | |||
if (self.console_handle and self.console_handle in self.browser.selenium.window_handles): | |||
self.browser.selenium.switch_to_window(self.console_handle) |
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.
In case of VMRC Console, the browser window for console is short-lived, hence it becomes important to check if the console_handle exists in available selenium.window_handles before attempting to switch to console or close the windw, hence this check.
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! Will wait on PRT results to move to 1LP.
else: | ||
vm.open_console(console='VMRC Console', invokes_alert=True) | ||
request.addfinalizer(vm.vm_console.close_console_window) | ||
request.addfinalizer(appliance.server.logout) |
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 curious, why do you need to explicitly logout here?
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.
@valaparthvi without a logout, it was not getting enough time between previous provider removal and next provider addition, neither it would refresh the VMs page to reflect new provider, so logging out seemed best move to avoid unpredictable failures.
cfme/common/provider.py
Outdated
@@ -435,7 +435,6 @@ def _fill_smartstate_endpoint_dicts(self, provider_attributes): | |||
|
|||
def _fill_vmrc_console_endpoint_dicts(self, provider_attributes): | |||
"""Fills dicts with VMRC console endpoint data | |||
|
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.
Can you remove this change? Kinda seems irrelevant to the PR.
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 guess it got introduced during rebase.
0c94df2
to
fcfff67
Compare
vm.open_console(console='VM Console', invokes_alert=True) | ||
assert vm.vm_console, 'VMConsole object should be created' | ||
request.addfinalizer(vm.vm_console.close_console_window) | ||
except ValueError: |
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.
Required If the VM console fails to launch here, we are giving a fast positive. Going to discuss a workaround offline on Monday
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.
Is the window handle different in 5.10 vs 5.11? Just a thought.
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 think Window Handles are Selenium thing and should have nothing to do with CFME version. Good question though, I will check.
@kedark3 @valaparthvi I can see in report, tests passed for 5.10 but on PRT. But status of 5.10 is Invalid. |
@@ -252,7 +253,7 @@ def wait_for_connect(self, timeout=30): | |||
|
|||
def close_console_window(self): | |||
"""Attempt to close Console window at the end of test.""" | |||
if self.console_handle is not None: | |||
if (self.console_handle and self.console_handle in self.browser.selenium.window_handles): |
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.
and
has high priority. you don't need braces here
fcfff67
to
4e43e81
Compare
6510614
to
f3f0503
Compare
f3f0503
to
b528905
Compare
@mshriver @john-dupuy Tests are now passing in PRT thanks to @izapolsk . Kindly review and let me know if we can merge it. 510z passed but PRT seems to be stuck, cfme.log indicate test was passed. |
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.
Some small comments, thanks for this PR @kedark3 👍
cfme/common/vm.py
Outdated
try: | ||
view.browser.IGNORE_SUBSEQUENT_ALERTS = True | ||
# Click console button given by type | ||
view.toolbar.access.item_select(console, handle_alert=invokes_alert) |
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.
Why the name change from handle_alert
to invokes_alert
? I would keep it consistent as handle_alert
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 not being changed in the PR, its been like that always. And to me it makes sense because, when I am calling it like:
vm.open_console(console='VM Console', invokes_alert=True)
it reads like, open VM console and that console invokes_alert. I think we can keep it as is unless you insist.
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.
That's fine with me, if you like it then we can keep it as is.
ad4ed14
to
bb734c9
Compare
I detected some fixture changes in commit bb734c9 The global fixture
Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃 |
Purpose or Intent
Note: This PR is built on top of work from #9701 and #9595, please review those first so that I can rebase this.
PRT Run
{{pytest: cfme/tests/infrastructure/test_vmrc_console.py}}
izapolsk: REQUIRES #9764 9764