-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updates to New Host UI Traces tab #821
Conversation
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, make sure you correctly handle the situation when the modal is not displayed. Does it mean something?
self.browser.plugin.ensure_page_safe() | ||
view.traces.enable_traces.click() | ||
modal = EnableTracerView(self.browser) | ||
if modal.is_displayed: |
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.
else
?
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.
Mostly following convention in the file for dealing with modals - there isn't a meaningful situation where the modal doesn't pop up, the test should just fail at that point.
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.
Shouldn't an exception be thrown in that case? Or just handle it somehow, don't leave it in undefined state unless something, somewhere fails?
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 could throw an exception in that case explicitly. It will also always fail if this modal doesn't pop up since the next assertion won't be correct if the modal dialogue isn't interacted with. That's required for the title on the page to change.
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.
Generally speaking, I don't think this is necessary 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.
It will also always fail if this modal doesn't pop up since the next assertion won't be correct
In your specific test, yes. In any other use, who knows.
Please review the PR labels before merging to ensure that the PR gets cherry-picked to the right branches. The same applies to the robottelo PR. |
Would like to get this merged so we can run this before go/no go, thanks! |
To allow merge I am dismissing Ladislav's review as both review comments were addressed.
(cherry picked from commit 9977b6b)
Updates to the traces tab, as well as some support for the new Enable Traces modal. I didn't bother supporting all the possible uses of this modal, as it's not relevant for the current test. This can be added later if more traces tests are desired.