Skip to content
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

Return exception arg for Browser.wait_for_element #134

Merged
merged 3 commits into from
Jan 21, 2019

Conversation

abalakh
Copy link

@abalakh abalakh commented Jan 16, 2019

PR returns exception arg for Browser.wait_for_element back (which was removed in #131) as airgun relies heavily on it and extending wait_for_element on our end is problematic due to #132

@@ -78,6 +78,13 @@ def test_wait_for_element_visible_fail_except(browser):
browser.wait_for_element('#invisible_appear_p', visible=True, timeout=1.5)


def test_wait_for_element_visible_fail_none(browser):
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉

# Click on the button
browser.click('#invisible_appear_button')
assert browser.wait_for_element(
'#invisible_appear_p', visible=True, timeout=1.5, exception=False) is None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please expand/parametrize the test to also pass exception=True in a pytest.raises block?

Copy link
Collaborator

Choose a reason for hiding this comment

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

nvm, I see its separate test case already.

Copy link
Author

Choose a reason for hiding this comment

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

I can, but isn't this scenario already covered in test_wait_for_element_visible_fail_except? Or you want to check explicitly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is, I'd like to combine them, do you mind if I push a commit to your PR with an updated test? I'm targeting to merge and release this now(ish)

Copy link
Author

Choose a reason for hiding this comment

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

sure thing

Copy link
Collaborator

Choose a reason for hiding this comment

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

Making sure updated tests pass in travis, they did locally in Tox.

Collapse two cases into one parametrized case
wait_for_element exception control, true/false behavior validation
@mshriver mshriver merged commit 9a98784 into RedHatQE:master Jan 21, 2019
@mshriver
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants