Closed Bug 1015242 Opened 6 years ago Closed 5 years ago

Provide a combined expected condition for element present and displayed

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: davehunt, Assigned: davehunt)

References

Details

(Whiteboard: [good first bug] [mentor=ato])

Attachments

(1 file, 1 obsolete file)

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.
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))
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.
Andreas: Any thoughts on this? Could it be a mentored bug?
Flags: needinfo?(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.
Flags: needinfo?(ato)
Whiteboard: [good first bug] [mentor=ato]
(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.
Flags: needinfo?(ato)
Attached patch bug1015242.patch (obsolete) — Splinter Review
Here's a patch to see if I'm on the right path...
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
Attachment #8512619 - Flags: feedback?
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?
Flags: needinfo?(dave.hunt)
You're on the right path.  The API looks fine to me.
Flags: needinfo?(dave.hunt)
Flags: needinfo?(ato)
(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 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
Attachment #8512619 - Flags: feedback? → review?(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
Attachment #8512619 - Flags: review?(ato) → review+
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
Attachment #8512619 - Attachment is obsolete: true
Attachment #8529027 - Flags: review?(ato)
Attachment #8529027 - Flags: review?(ato) → review+
https://hg.mozilla.org/mozilla-central/rev/4a64ed135ea8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.