Dave Hunt [:davehunt] [he/him] ⌚BST Reporter
	Description • 11 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

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 1 • 11 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?

	Dave Hunt [:davehunt] [he/him] ⌚BST Reporter
	Comment 2 • 11 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.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 3 • 10 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?

	Dave Hunt [:davehunt] [he/him] ⌚BST Reporter
	Comment 5 • 10 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?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 6 • 10 years ago

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 (-:

	Dave Hunt [:davehunt] [he/him] ⌚BST Reporter
	Comment 7 • 10 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.

	Dave Hunt [:davehunt] [he/him] ⌚BST Reporter
	Comment 9 • 10 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.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 10 • 10 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.

	Dave Hunt [:davehunt] [he/him] ⌚BST Reporter
	Comment 11 • 10 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.

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 12 • 10 years ago

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?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 13 • 10 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)

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 14 • 10 years ago

Removed to unused imports.

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 15 • 10 years ago

(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

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 16 • 10 years ago

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

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

lgtm

	Dave Hunt [:davehunt] [he/him] ⌚BST Reporter
	Comment 17 • 10 years ago

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55cea8e4946e

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/55cea8e4946e