If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement expected conditions class

RESOLVED FIXED in Firefox OS v1.3

Status

Testing
Marionette
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: davehunt, Assigned: ato)

Tracking

unspecified
mozilla29
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(b2g-v1.3 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
We should implement an ExpectedConditions class similar to that in WebDriver: http://selenium.googlecode.com/svn/trunk/docs/api/py/webdriver_support/selenium.webdriver.support.expected_conditions.html
(Assignee)

Updated

4 years ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
(Assignee)

Comment 1

4 years ago
Do we have an idea of which common expected conditions we want to have
in Marionette?  Do we want to support all of the conditions from
Selenium (see davehunt's link in comment #0)?  Fewer, more?

Since I'm unfamiliar with the use cases here, which conditions should
I plan to include initially?
(Reporter)

Comment 2

4 years ago
A good start would be to cover what we use in the functional Gaia UI tests:

* Waiting for an element to be present
* Waiting for an element to be not present
* Waiting for an element to be displayed
* Waiting for an element to be not displayed

I would be happy to add more as we need them.
(Assignee)

Comment 3

4 years ago
So far I've added the following methods:

  * element_present
  * element_stale
  * elements_present
  * element_displayed
  * element_not_displayed
  * element_selected
  * element_not_selected
  * element_enabled
  * element_not_enabled

The methods that require an element to be looked up (element_present and elements_present) takes a function/lambda instead of locators like the Selenium implementation does.  This reduces tight coupling with the Marionette object, but possibly makes it harder to use.  Any opinions on this?
(Assignee)

Comment 4

4 years ago
Created attachment 8356580 [details] [diff] [review]
0001-Bug-948075-Implement-expected-conditions-for-Marione.patch
Attachment #8356580 - Flags: review?(dave.hunt)
(Reporter)

Comment 5

4 years ago
Comment on attachment 8356580 [details] [diff] [review]
0001-Bug-948075-Implement-expected-conditions-for-Marione.patch

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

::: testing/marionette/client/marionette/expected.py
@@ +28,5 @@
> +
> +    """
> +
> +    def __init__(self, func):
> +        self.locator = func

As discussed on IRC, it would be great if this could also accept a locator.

@@ +33,5 @@
> +
> +    def __call__(self, marionette):
> +        return _find(marionette, self.locator)
> +
> +class element_stale(object):

Also discussed on IRC I think this would be better named element_not_present and additionally support a locator or lambda for an argument. This would then allow checking an element is not present when it has not previously been found.

@@ +101,5 @@
> +    def __init__(self, element):
> +        self.el = element
> +
> +    def __call__(self, marionette):
> +        return self.el.is_displayed()

If the element is stale then shouldn't we catch that and return False?

@@ +118,5 @@
> +    def __init__(self, element):
> +        super(element_not_displayed, self).__init__(element)
> +
> +    def __call__(self, marionette):
> +        return not super(element_not_displayed, self).__call__(marionette)

If the element is stale then shouldn't we catch that and return True? Maybe this applies to other expected classes too?

::: testing/marionette/client/marionette/tests/unit/test_expected.py
@@ +1,1 @@
> +# -*- fill-column: 100 -*-

I'm curious, what is this line for?

@@ +83,5 @@
> +        el = self.marionette.find_element(By.TAG_NAME, "p")
> +        visible = expected.element_displayed(el)(self.marionette)
> +        self.assertTrue(visible)
> +
> +    def test_element_displayed_when_hidden(self):

Could we also have a test when the element is no longer present?
Attachment #8356580 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 6

4 years ago
Created attachment 8357174 [details] [diff] [review]
0001-Bug-948075-Implement-expected-conditions-class-for-M.patch

Thanks!  I addressed most of the feedback in your review in attached
revised patch.

(In reply to Dave Hunt (:davehunt) from comment #5)
> @@ +33,5 @@
> > +
> > +    def __call__(self, marionette):
> > +        return _find(marionette, self.locator)
> > +
> > +class element_stale(object):
>
> Also discussed on IRC I think this would be better named
> element_not_present and additionally support a locator or lambda for
> an argument. This would then allow checking an element is not
> present when it has not previously been found.

Agree about the introducing an element(s)_not_found function, but I
think element_stale should be kept as they are checking for different
things.

element_not_found would try to look up an element, whereas
element_stale checks an existing element reference whether it's still
attached to the DOM.  You can't achieve the same effect with
element_not_found as it won't guarantee to use the same element ID due
to the new find_element call.

> @@ +101,5 @@
> > +    def __init__(self, element):
> > +        self.el = element
> > +
> > +    def __call__(self, marionette):
> > +        return self.el.is_displayed()
>
> If the element is stale then shouldn't we catch that and return
> False?

I wondered about this too.  An element that isn't displayed and an
element that's disappeared from the DOM are visually
indistinguishable, but essentially disparate situations.

We can either treat to be the same, which would make the behaviour of
this function different to that of WebDriver's WebElement#isDisplayed;
or we can say this function is meant to do what the user actually
expects.

> ::: testing/marionette/client/marionette/tests/unit/test_expected.py
> @@ +1,1 @@
> > +# -*- fill-column: 100 -*-
>
> I'm curious, what is this line for?

Emacs (-:
Attachment #8356580 - Attachment is obsolete: true
Attachment #8357174 - Flags: feedback?(dave.hunt)
(Reporter)

Comment 7

4 years ago
Comment on attachment 8357174 [details] [diff] [review]
0001-Bug-948075-Implement-expected-conditions-class-for-M.patch

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

(In reply to Andreas Tolfsen (:ato) from comment #6)
> > If the element is stale then shouldn't we catch that and return
> > False?
> 
> I wondered about this too.  An element that isn't displayed and an
> element that's disappeared from the DOM are visually
> indistinguishable, but essentially disparate situations.
> 
> We can either treat to be the same, which would make the behaviour of
> this function different to that of WebDriver's WebElement#isDisplayed;
> or we can say this function is meant to do what the user actually
> expects.

I think it would help test development if we were able to cover any circumstances that would indicate that an element is not visible. It's unlikely that the test author will know if an element is hidden or has been entirely removed from the DOM without debugging the AUT.

It might also make some sense to allow a locator when checking this condition as the target element may not have been looked up previously or the reference to the element is out of scope.
Attachment #8357174 - Flags: feedback?(dave.hunt) → feedback+
(Assignee)

Comment 8

4 years ago
Created attachment 8357814 [details] [diff] [review]
0001-Bug-948075-Add-expected-conditions-to-Marionette.patch
Attachment #8357174 - Attachment is obsolete: true
Attachment #8357814 - Flags: review?(dave.hunt)
(Reporter)

Comment 9

4 years ago
Comment on attachment 8357814 [details] [diff] [review]
0001-Bug-948075-Add-expected-conditions-to-Marionette.patch

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

A couple of unused imports, and a question about the element_not_displayed, which could be addressed in a follow-up patch if necessary.

::: testing/marionette/client/marionette/expected.py
@@ +172,5 @@
> +            return self.el.is_displayed()
> +        except errors.StaleElementException:
> +            return False
> +
> +class element_not_displayed(element_displayed):

The only thing I'm considering at the moment is whether we need to support expecting an element to be not present/visible at the same time. If an element has previously been looked up it could be either not visible or stale, however if it's not been looked up yet should we support passing the locator and returning if we get a NoSuchElementException? I'm going to continue checking the current uses of this, but wanted to submit my overdue review. :)

::: testing/marionette/client/marionette/tests/unit/test_expected.py
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import urllib
> +
> +import errors

Ununsed import.

@@ +7,5 @@
> +import errors
> +import expected
> +import marionette_test
> +
> +from wait import Wait

Unused import.
Attachment #8357814 - Flags: review?(dave.hunt) → review+
(Assignee)

Comment 10

4 years ago
(In reply to Dave Hunt (:davehunt) from comment #9)
> ::: testing/marionette/client/marionette/expected.py
> @@ +172,5 @@
> > +            return self.el.is_displayed()
> > +        except errors.StaleElementException:
> > +            return False
> > +
> > +class element_not_displayed(element_displayed):
>
> The only thing I'm considering at the moment is whether we need to
> support expecting an element to be not present/visible at the same
> time. If an element has previously been looked up it could be either
> not visible or stale, however if it's not been looked up yet should
> we support passing the locator and returning if we get a
> NoSuchElementException? I'm going to continue checking the current
> uses of this, but wanted to submit my overdue review. :)

In my opinion that seems a little too magical and to some extent blurs
the distinction between “present in DOM” and “visible to user”.

That concern aside, I see how the following code would be shorter and
more pleasant to write:

    a = Wait.until(expected.element_displayed(By.TAG_NAME, "a")

As opposed to:

    a = Wait.until(expected.element_present(By.TAG_NAME, "a"))
    Wait.until(expected.element_displayed(a))

But note that the two code examples above are not identical because we
can't do a Wait.until internally in element_displayed for the element
to become present in the DOM.

So it raises the following issue: Are we fine with
element_[not_]displayed doing a plain element search internally?  If
it takes time for the element to appear in the DOM, this would raise a
NoSuchElementException.

It could well be that's a reasonable compromise though, for brevity.
(Reporter)

Comment 11

4 years ago
I've changed my mind on this, I think the best approach is for element[_not]_displayed to take an element, as you have it in your patch. Otherwise we could have misleading test code that implies we're waiting for an element to disappear when in fact it may never appear. If we have special cases where we need something like this, we just won't use the expected conditions, or will combine them.
I just looked at the code now, and this isn't clear to me. Why are we using callable classes instead of functions for the expected conditions class?
(Assignee)

Comment 13

4 years ago
Not for any other reason than the Wait class holding the reference to the Marionette instance.

So when a condition is constructed with some arguments, that is passed to Wait.until as a function.  When the condition is called, Wait can supply it with a Marionette instance to operate on.

An example, in pseudo code:

    el = marionette.find_element(By.ID, "will-become-visible")
    Wait(marionette).until(expected.element_displayed(el))

This means you can pass in any lambda or function to the until method and grant it access to the Marionette instance stored in Wait.

The code above is equivalent to doing this:

    el = marionette.find_element(By.ID, "will-become-visible")
    def my_custom_func(marionette):
        return el.is_displayed()
    Wait(marionette.until(my_custom_func)
(Assignee)

Comment 14

4 years ago
Created attachment 8364396 [details] [diff] [review]
0001-Bug-948075-Add-expected-conditions-to-Marionette.patch

Removed to unused imports.
Attachment #8357814 - Attachment is obsolete: true
Attachment #8364396 - Flags: review?(mdas)
Attachment #8364396 - Flags: review?(dave.hunt)
(In reply to Andreas Tolfsen (:ato) from comment #13)
> Not for any other reason than the Wait class holding the reference to the
> Marionette instance.
> 
> So when a condition is constructed with some arguments, that is passed to
> Wait.until as a function.  When the condition is called, Wait can supply it
> with a Marionette instance to operate on.
> 
> An example, in pseudo code:
> 
>     el = marionette.find_element(By.ID, "will-become-visible")
>     Wait(marionette).until(expected.element_displayed(el))
> 
> This means you can pass in any lambda or function to the until method and
> grant it access to the Marionette instance stored in Wait.
> 
> The code above is equivalent to doing this:
> 
>     el = marionette.find_element(By.ID, "will-become-visible")
>     def my_custom_func(marionette):
>         return el.is_displayed()
>     Wait(marionette.until(my_custom_func)

Ah, I understand, thanks
Comment on attachment 8364396 [details] [diff] [review]
0001-Bug-948075-Add-expected-conditions-to-Marionette.patch

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

lgtm
Attachment #8364396 - Flags: review?(mdas) → review+
(Reporter)

Updated

4 years ago
Attachment #8364396 - Flags: review?(dave.hunt) → review+
(Reporter)

Updated

4 years ago
Blocks: 963299
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Reporter)

Comment 17

4 years ago
Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55cea8e4946e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/55cea8e4946e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
status-b2g-v1.3: --- → fixed
You need to log in before you can comment on or make changes to this bug.