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

Adds Finds support to is_element_present() #17

Merged
merged 1 commit into from
Jan 4, 2017
Merged

Adds Finds support to is_element_present() #17

merged 1 commit into from
Jan 4, 2017

Conversation

martinpelikan
Copy link
Contributor

@martinpelikan martinpelikan commented Dec 29, 2016

Context
I'm still quite new to the page object design pattern, so this may not normally come up. I have a table of elements that need to be deleted one at a time via a menu/option. My solution is to iterate while there are still elements visible and delete them one at a time (avoiding the stale element reference in doing so). The problem is that Webium's nice is_element_present() method fails for Finds. This is my fix, which seems to work for me, and should not break existing behaviour. The question is, if it should check that all elements are visible, or if only the first is necessary...

Error Encountered

ntp.py:42: in delete_all
    while self.is_element_present('ntp_servers'):
/usr/lib/python3.5/site-packages/webium/base_page.py:29: in is_element_present
    is_displayed() if just_in_dom else wait(lambda: is_displayed(), timeout_seconds=timeout)
/usr/lib/python3.5/site-packages/webium/wait.py:16: in wait
    return wait_lib(*args, **kwargs)
/usr/lib/python3.5/site-packages/waiting/__init__.py:16: in wait
    for x in iterwait(result=result, *args, **kwargs):
/usr/lib/python3.5/site-packages/waiting/__init__.py:40: in iterwait
    result.result = predicate()
/usr/lib/python3.5/site-packages/webium/base_page.py:29: in <lambda>
    is_displayed() if just_in_dom else wait(lambda: is_displayed(), timeout_seconds=timeout)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

    def is_displayed():
        element = getattr(self, element_name, None)
        if not element:
            raise WebiumException('No element "{0}" within container {1}'.format(element_name, self))
>       return element.is_displayed()
E       AttributeError: 'list' object has no attribute 'is_displayed'

@ufranske
Copy link
Contributor

Hi martinpelikan!

Thanks for your PR. It looks good to me. This behavior is kind of tricky but the changes do not make the state of things worse. However before we merge this PR please could you:

  1. Add tests to https://github.com/wgnet/webium/blob/master/tests/simple_page/test_is_element_present.py:

One for SamplePage.anchor_list field (https://github.com/martinpelikan/webium/blob/f9a8a654f4dc147f00f79ebdeb8f9fc212f99f9b/tests/simple_page/__init__.py#L54)
Other - for SamplePage.empty_element_list field (https://github.com/martinpelikan/webium/blob/f9a8a654f4dc147f00f79ebdeb8f9fc212f99f9b/tests/simple_page/__init__.py#L56)

  1. Add a note about this tricky behavior in case of Finds field to docs: https://github.com/martinpelikan/webium/blob/f9a8a654f4dc147f00f79ebdeb8f9fc212f99f9b/docs/is_element_present.rst

@SUNx2YCH , @Khrol could please take a look as well?

raise WebiumException('No element "{0}" within container {1}'.format(element_name, self))
if isinstance(element, list):
if element:
Copy link
Contributor

Choose a reason for hiding this comment

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

nested if is not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, all([]) produces True. So, it depends on what you expect from Finds which does not contain any visible elements

Copy link
Contributor

Choose a reason for hiding this comment

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

Tests are needed to display what behaviour we are trying to achieve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Semantically, it might be more clear if I wrote this as if element != []: instead of if element:. I can change it to that if you prefer. This is mostly my habit when checking for empty sequences/iterables.

Finds returns a list of WebElements, not a single WebElement, and thus
is_displayed() fails when called upon said list. This commit adds
handling to check if there are any elements in the list, and if so,
determines if they are all visible.
@martinpelikan
Copy link
Contributor Author

martinpelikan commented Jan 3, 2017

I was able to spend some time today on addressing your reviews. Many thanks for being so quick to respond and review @Khrol and @ufranske!

Should my commit also include a version bump, or is this a responsibility of the maintainers?

Copy link
Contributor

@Khrol Khrol left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@SUNx2YCH SUNx2YCH merged commit f3f5956 into wgnet:master Jan 4, 2017
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.

4 participants