1015242 - Provide a combined expected condition for element present and displayed

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

As discussed on IRC two common conditions that are commonly combined are element_present and element_displayed. Currently this is achieved without expected conditions as:

Wait(m, ignored_exceptions=NoSuchElementException).until(
    lambda m: m.find_element(locator).is_displayed())

This has the disadvantage of repeatedly looking up the element even after it's been found and before it's displayed. It also doesn't take advantage of the expected conditions at all.

To achieve this using the expected conditions we must do something like:

Wait(m).until(expected.element_displayed(
    Wait(m).until(expected.element_present(locator))))

This is a little hard to read, and has the disadvantage of potentially doubling the timeout. We must first wait for the inner Wait to timeout before the outer Wait will do the same. Ideally we would be able to specify a single timeout.

It would be great if we could have a single element_displayed expected condition that could take either a locator (and perform the lookup) or an element. The timeout for the Wait should apply to the entire condition, including the lookup if needed. Alternatively, a new condition such as element_present_and_displayed would also work.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Also, if this condition performs a lookup and returns the element, it means subsequent visibility conditions on this can avoid performing the lookup again. For example:

element = Wait(m).until(expected.element_displayed(locator))
# do something that causes the element to be hidden
Wait(m).until(expected.element_not_displayed(element))

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Lastly, it would be great to do the same with element_not_displayed to allow for the situation where the element has not previously been looked up and may already be not present. An example might be a loading spinner or similar. For UI tests it's rare that we'll know if an element is hidden or removed entirely, so allowing element_not_displayed to take a locator and to return successfully if that locator does not find an element would be useful in some cases.

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Andreas: Any thoughts on this? Could it be a mentored bug?

Comment 4

[Andreas Tolfsen ❲:ato❳]

For element_present/element_not_present we allow the argument to be either a locator or an element reference.  It makes sense to allow the same for element_displayed/element_not_displayed.

It's also a good first bug.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Andreas Tolfsen (:ato) from comment #4)
> For element_present/element_not_present we allow the argument to be either a
> locator or an element reference.  It makes sense to allow the same for
> element_displayed/element_not_displayed.
> 
> It's also a good first bug.

I just had a quick look into this, and have a couple of questions. I can successfully accept either a locator or an element for the displayed expected classes, however these currently only return the visibility of the element. In comment 1 I mentioned returning the element found, but I'm not sure how that would fit into the fix.

Also, when expecting an element to be displayed we would want to first wait for it to be present, but when we're waiting for it to be not displayed we want to return early if the element is not present, but I'm not sure how we'd achieve this.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: bug1015242.patch

Here's a patch to see if I'm on the right path...

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

davehunt, this is a good first bug and you have a patch up here.  do you need somebody to review or provide feedback on the patch?

Comment 8

[Andreas Tolfsen ❲:ato❳]

You're on the right path.  The API looks fine to me.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Dave Hunt (:davehunt) from comment #5)
> In comment 1 I mentioned returning the element found, but I'm not
> sure how that would fit into the fix.

I discussed this with Andreas, and we agreed that element_displayed and element_not_displayed should only return the visibility of the element. If a reference to the element is needed then the user should first locate the element either using find_element or element_present.

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8512619 [details] [diff] [review]
bug1015242.patch

I took another look over this patch and feel it's actually ready for review. Here's a try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=82f0f4c466ed

Comment 11

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8512619 [details] [diff] [review]
bug1015242.patch

Review of attachment 8512619 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

::: testing/marionette/client/marionette/expected.py
@@ +167,5 @@
> +        displayed = Wait(marionette).until(expected.element_displayed(By.ID, "foo"))
> +
> +    Or by supplying an element::
> +
> +        el = Wait(marionette).until(expected.element_displayed(el))

Passing in el and returning to variable el is confusing.  Can you provide an example that first looks up el and returns to the variable `displayed`, as the previous example does?

@@ +184,3 @@
>  
>      def __call__(self, marionette):
> +        if not self.el:

if self.el is not None

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Provide a combined expected condition for element present and displayed. v2.0

Hmm.. there was actually a few issues with the last patch. First was the docstrings as pointed out by :ato but also some logic was wrong, though I'm not sure why it was passing before. Here's a new try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c6397df2ad07

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a64ed135ea8

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a64ed135ea8