Closed Bug 850881 Opened 11 years ago Closed 11 years ago

Implement explicit wait support class

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox27 fixed, firefox28 fixed, firefox29 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla29
Tracking Flags:
Tracking Status
firefox27 --- fixed
firefox28 --- fixed
firefox29 --- fixed

People

(Reporter: davehunt, Assigned: ato)

References

Details

(Whiteboard: [mentor=davehunt][lang=py])

Attachments

(1 file, 2 obsolete files)

0001-Bug-850881-Implement-explicit-wait-condition-class.patch
13.76 KB, patch
davehunt
: review+
Details | Diff | Splinter Review 
We tend to roll our own waits in Marionette based tests, whereas we should lift the WebDriverWait from the Selenium project and lean on those. See http://selenium.googlecode.com/svn/trunk/docs/api/py/webdriver_support/selenium.webdriver.support.wait.html

At the same time, we should also lift the ExpectedConditions class: http://selenium.googlecode.com/svn/trunk/docs/api/py/webdriver_support/selenium.webdriver.support.expected_conditions.html
davehunt has a working patch here: https://github.com/mozilla/b2gperf/commit/b9e004fdd2117c5f3558871047d52e3c898d0196, but I was thinking about having these methods as integrated in the Marionette object, since we can't use this function without a Marionette instance, and we already have some helper methods in there like wait_for_port and such.

This way, instead of doing
from marionette import MarionetteWait
MarionetteWait(self.marionette, 30).until(lambda m: name.is_displayed() or not name.get_attribute('hidden')) 


We can do
self.marionette.wait(30, lambda m: name.is_displayed() or not name.get_attribute('hidden'))

dburns, any comment on this approach?
davehunt seemed to be fine with the suggested change
ah, I overlooked until_not()

we can use: 

self.marionette.wait(<time>).until(<function>) and self.marionette.wait(<time>).until_not(<function>)

where self.marionette.wait(<time>) will produce a MarionetteWait object, that needs until or until_not to be called in order to execute

or self.marionette.wait_for(<time>, <function>) and self.marionette.wait_for_not(<time>, <function>)

or self.marionette.wait(<time>, until=<function>) and self.marionette.wait(<time>, until_not=<function>)

or another suggestion?
(In reply to Malini Das [:mdas] from comment #3)
> self.marionette.wait(<time>).until(<function>) and
> self.marionette.wait(<time>).until_not(<function>)
> 
> where self.marionette.wait(<time>) will produce a MarionetteWait object,
> that needs until or until_not to be called in order to execute

I prefer this option. We should also be able to take advantage of the expected conditions as mentioned in comment 0 so it could look like:

self.marionette.wait(30).until(expected_conditions.element_to_be_clickable('id' , 'elementId'))
Personally I prefer the WebDriverWait approach of not having it on the main driver object. Adding it to the Marionette leans towards making marionette a God Object and we should avoid that.

Also we can lift the WebDriver wait code, as is, from the selenium tree and drop it in the project since it doesnt have anything related to webdriver except for exceptions. It was designed to be reused and we should do that instead of reinventing the wheel.
One thing I forgot to mention is that if conformance tests (for the webdriver spec) use the WebDriverWait object for some reason, if we dont follow it then we will have to have a hack to be in the middle.

We already have a number of hacks in place for other conformance tests which, if possible, I would like to avoid!
(In reply to David Burns :automatedtester from comment #6)
> One thing I forgot to mention is that if conformance tests (for the
> webdriver spec) use the WebDriverWait object for some reason, if we dont
> follow it then we will have to have a hack to be in the middle.
> 
> We already have a number of hacks in place for other conformance tests
> which, if possible, I would like to avoid!

Good point!
We should definitely have a discussion about WebDriver compatibility at some point.  It was never the original intention that someone could write a WebDriver test in Python and have it executed by the Marionette test runner; it was more the intention that someone could write a WebDriver test in any WebDriver-supported language, and it would work using Marionette via the Marionette proxy.

I'll set up a meeting in the next couple of weeks so we can discuss this and figure out a long-term approach that makes sense.
I can give this a try over the weekend and next week.
Guy for the wait class Should it be WebDriverWait or MarionetteWait

Also as suggested we will take the code and the logic for the Selenium Project 
The file will have the original "Apache License, Version 2.0" license right?

Are there any tests we should Import/implement for this new file?
(In reply to Florin Strugariu [:Bebe] from comment #10)
> Guy for the wait class Should it be WebDriverWait or MarionetteWait

If we're just taking the code as-is from the Selenium project (as suggested by your next question, and probably the simplest solution) then we should keep WebDriverWait. If we customise it at all, I would personally suggest MarionetteWait.

> Also as suggested we will take the code and the logic for the Selenium
> Project 
> The file will have the original "Apache License, Version 2.0" license right?

We should keep any licenses intact if we're copying files with no changes.

> Are there any tests we should Import/implement for this new file?

Ideally we would have tests for this, yes. It should be possible to at least use the existing Selenium tests as a base.
Whiteboard: [mentor=davehunt][lang=py]
I somewhat accidentally did an implementation of an explicit wait
class as part of bug 804515.  This solves one part of the bug, as the
expected conditions (which should map down to this class) are still
remaining.  I've attached a patchset for review.
Attachment #8344190 - Flags: review?(mdas)
Attachment #8344190 - Flags: review?(jgriffin)
Attachment #8344190 - Flags: review?(dave.hunt)
Assignee: nobody → ato
Comment on attachment 8344190 [details] [diff] [review]
0001-Bug-850881-Implement-explicit-wait-condition-class.patch

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

This is great, I'm really looking forward to using this in several places! I've left the review flags for mdas and jgriffin because I'd like to see their feedback too. My main concern is making sure we have a consistent and clean way to use this in our tests.

::: testing/marionette/client/marionette/tests/unit/test_wait.py
@@ +92,5 @@
> +
> +    def test_construction_with_custom_exception(self):
> +        w = Wait(None, ignored_exceptions=Exception)
> +        assert Exception in w.exceptions
> +        assert len(w.exceptions) == 2

Similar to below, I would suggest we should make this len(wait.IGNORED_EXCEPTIONS) + 1

@@ +125,5 @@
> +        w = Wait(None)
> +        assert w.end is not None
> +
> +    def test_marionette_property(self):
> +        marionette = "cheddar"

:)

@@ +164,5 @@
> +        assert self.clock.ticks == 10
> +
> +    def test_default_ignored_exception(self):
> +        with self.assertRaises(errors.TimeoutException):
> +            self.w.until(lambda x: x.exception(e=errors.NoSuchElementException))

Rather than hard-code this exception again, could we do a for-each over wait.IGNORED_EXCEPTIONS?

::: testing/marionette/client/marionette/tests/unit/unit-tests.ini
@@ +97,4 @@
>  [test_getactiveframe_oop.py]
>  [test_submit.py]
>  [test_chrome_async_finish.js]
> +

Could we group this with the implicit wait tests?

::: testing/marionette/client/marionette/wait.py
@@ +26,5 @@
> +    page.
> +
> +    """
> +
> +    def __init__(self, marionette, timeout=DEFAULT_TIMEOUT,

A couple of comments on the timeout value. The Marionette client uses timeouts in milliseconds [1], and I think wherever possible we should avoid switching between milliseconds and seconds. Also, a Marionette instance has a timeout property [2], which can optionally be provided when instantiating it. When we use runtests.py there's also a --timeout argument [3] which is passed to the Marionette instance. If possible, we should avoid having to conditionally pass this timeout value to our Wait object. Maybe we could default to this property if it's set, and fall back to another value?

[1] http://hg.mozilla.org/mozilla-central/file/a79294ed7ebe/testing/marionette/client/marionette/marionette.py#l721
[2] http://hg.mozilla.org/mozilla-central/file/a79294ed7ebe/testing/marionette/client/marionette/marionette.py#l430
[3] http://hg.mozilla.org/mozilla-central/file/a79294ed7ebe/testing/marionette/client/marionette/runtests.py#l862

@@ +79,5 @@
> +                exceptions.append(ignored_exceptions)
> +        self.exceptions = tuple(set(exceptions))
> +
> +
> +    def until(self, condition, until=None):

The Selenium support class also has an until_not method. While this can be achieved by adding 'not' to the condition, I recall there was still a valid argument for including it. I'm happy to avoid added the until_not unless there's a good reason to add it, though.

@@ +120,5 @@
> +
> +            self.clock.sleep(self.interval)
> +
> +        if last_exc is not None:
> +            raise errors.TimeoutException(last_exc)

The TimeoutException does not currently give any indication of what the timeout was. This can make it difficult for debugging, especially when code could be using a timeout provided by the command line or another variable. This is probably a separate bug, though...
Attachment #8344190 - Flags: review?(dave.hunt) → review-
Let's make this just about the waits, and take care of expected conditions in a separate bug.
Summary: Implement wait and expected condition support classes from WebDriver → Implement explicit wait support class
Blocks: 948075
(In reply to Dave Hunt (:davehunt) from comment #13)
> 
> A couple of comments on the timeout value. The Marionette client uses
> timeouts in milliseconds [1], and I think wherever possible we should avoid
> switching between milliseconds and seconds. Also, a Marionette instance has
> a timeout property [2], which can optionally be provided when instantiating
> it. When we use runtests.py there's also a --timeout argument [3] which is
> passed to the Marionette instance. If possible, we should avoid having to
> conditionally pass this timeout value to our Wait object. Maybe we could
> default to this property if it's set, and fall back to another value?
> 
> [1]
> http://hg.mozilla.org/mozilla-central/file/a79294ed7ebe/testing/marionette/
> client/marionette/marionette.py#l721
> [2]
> http://hg.mozilla.org/mozilla-central/file/a79294ed7ebe/testing/marionette/
> client/marionette/marionette.py#l430
> [3]
> http://hg.mozilla.org/mozilla-central/file/a79294ed7ebe/testing/marionette/
> client/marionette/runtests.py#l862

In my opinion we should be converting this from seconds to milliseconds to send it over the wire since seconds are more idiomatic to python and fix the incorrect use of it. But that's just my opinion :)
(In reply to David Burns :automatedtester from comment #15)
> (In reply to Dave Hunt (:davehunt) from comment #13)
> 
> In my opinion we should be converting this from seconds to milliseconds to
> send it over the wire since seconds are more idiomatic to python and fix the
> incorrect use of it. But that's just my opinion :)

I have no preference except for consistency.
Comment on attachment 8344190 [details] [diff] [review]
0001-Bug-850881-Implement-explicit-wait-condition-class.patch

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

::: testing/marionette/client/marionette/tests/unit/test_wait.py
@@ +63,5 @@
> +        super(SystemClockTest, self).setUp()
> +        self.clock = wait.SystemClock()
> +
> +    def test_construction_initializes_time(self):
> +        assert self.clock._time == time

Please use unittest's asserts throughout this file, as well.

@@ +189,5 @@
> +    def test_system_exit(self):
> +        with self.assertRaises(SystemExit):
> +            self.w.until(lambda x: x.exception(e=SystemExit))
> +
> +    def test_false_bool_is_special(self):

what makes the false bool special? I'm not sure what this test name means.

@@ +214,5 @@
> +
> +    def test_value_timeout(self):
> +        r = self.w.until(lambda x: x.value("foo", wait=15))
> +        assert r is None
> +        assert self.clock.ticks == 10

There should be a test for the optional 'until' parameter of the until function

::: testing/marionette/client/marionette/wait.py
@@ +7,5 @@
> +import time
> +
> +DEFAULT_TIMEOUT = 5
> +DEFAULT_INTERVAL = 0.5
> +IGNORED_EXCEPTIONS = [errors.NoSuchElementException]

I don't think there should be any default ignored exceptions. If I just found out about Wait, I wouldn't have expected this field to be filled in at all. While we're going to use this field to ignore NoSuchElementException often, it's pretty presumptuous. Also, I would have to override this default if I'm waiting for an element to be removed from the dom (perhaps after some user action), and that seems strange.

@@ +26,5 @@
> +    page.
> +
> +    """
> +
> +    def __init__(self, marionette, timeout=DEFAULT_TIMEOUT,

++davehunt

@@ +56,5 @@
> +
> +        :param ignored_exceptions: Ignore specific types of exceptions
> +            while waiting for the condition.  Any exceptions not
> +            whitelisted will be allowed to propagate, terminating the
> +            wait.

the default ignored_exceptions is not mentioned here, but hopefully we won't use defaults.

@@ +94,5 @@
> +        continue polling for the condition until it returns
> +        successfully or a TimeoutException is raised.
> +
> +        :param condition: A callable function that will be checked for
> +        :param until: The predicate to wait on.

it isn't clear what the predicate has to be (ie: that it should be a function that will be passed a clock and an end time). Can this be clarified?

@@ +122,5 @@
> +
> +        if last_exc is not None:
> +            raise errors.TimeoutException(last_exc)
> +
> +        return rv

Say that my condition is never met during the timeout, and no exception is raised while you're checking the condition. In this case, rv is returned instead of a TimeoutException. Why is that? I would expect it to raise an exception so my test can fail.
Attachment #8344190 - Flags: review?(mdas) → review-
davehunt, mdas: Thanks for the feedback on the patch, I've made most
of the changes suggested but have a few concerns:

(In reply to Dave Hunt (:davehunt) from comment #13)
> A couple of comments on the timeout value. The Marionette client
> uses timeouts in milliseconds, and I think wherever possible we
> should avoid switching between milliseconds and seconds.

I agree about consistency, but as dburns points out, using
milliseconds isn't interoperable with the Python standard library
either.  Because this patch integrates with the time library in
particular, I'd be happy to file a bug on switching the client over to
use seconds instead.

> Also, a Marionette instance has a timeout property, which can
> optionally be provided when instantiating it. When we use
> runtests.py there's also a --timeout argument which is passed to the
> Marionette instance. If possible, we should avoid having to
> conditionally pass this timeout value to our Wait object. Maybe we
> could default to this property if it's set, and fall back to another
> value?

My feeling is that these values aren't synonymous with the intention
of explicit waits.  It seems that the --timeout flag lets you specify
how long it will take before a test should time out, defaulting to 30
seconds.

Having a 30 second timeout on a Wait instance would always cause the
whole test to time out if the condition isn't met.  The intention is
that it should retry the condition a couple of times before letting
the assertions in the test case determine if the test is a pass or a
fail.

For that reason it makes more sense for us to either have a sensible
timeout hard coded in Wait, or to require a timeout argument to be
given on instantiation.  However, I'm not sure if a 5 second timeout
with a 0.5 second polling interval is sensible.  I was imagining we
could change this as we see use cases appearing.

> The Selenium support class also has an until_not method. While this
> can be achieved by adding 'not' to the condition, I recall there was
> still a valid argument for including it.

Can you remember what the arguments were?  We can negate the until
function, but it seems rather redundant if people will be relying
mostly on the prefabricated expected conditions (bug 948075).

(In reply to Malini Das (:mdas) from comment #17)
> @@ +122,5 @@
> > +        if last_exc is not None:
> > +            raise errors.TimeoutException(last_exc)
> > +
> > +        return rv
>
> Say that my condition is never met during the timeout, and no
> exception is raised while you're checking the condition. In this
> case, rv is returned instead of a TimeoutException. Why is that? I
> would expect it to raise an exception so my test can fail.

I've found there to be arguments both for and against raising under
such circumstances:

  * There's nothing exceptional about a condition evaluating to false.

  * Raising an exception when a condition isn't met might violate the
    single responsibility principle, in that I'd expect the test to
    take care of the assertions.

    If an element can't be found, is it the Wait utility class'
    responsibility to fail the test by raising?

  * The user might expect None or False to be returned, although I
    suspect that's more the case in a strongly typed language where
    you'd define your expected return type.

One of the use cases of Wait is that it can also return things:

Although not in the requirements for this bug, I've designed Wait to
be able to return the value of the successful condition.  An example
of that is the ability to ignore certain exceptions, delegating the
try…catch encapsulation to Wait, instead of having to deal with this
in the conditions.

This means you can write quite elegant code that is syntactically
similar to the explicit wait classes found in Selenium's Java [1] and
Python [2] bindings:

    el = Wait(driver).until(lambda driver: return driver.find_element_by_id("foo"), ignore_exceptions=errors.NoSuchElementException)

[1] https://code.google.com/p/selenium/source/browse/java/client/src/org/openqa/selenium/support/ui/Wait.java
[2] https://code.google.com/p/selenium/source/browse/py/selenium/webdriver/support/wait.py
Status: NEW → ASSIGNED
(In reply to Andreas Tolfsen (:ato) from comment #18)
> davehunt, mdas: Thanks for the feedback on the patch, I've made most
> of the changes suggested but have a few concerns:
> 
> (In reply to Dave Hunt (:davehunt) from comment #13)
> > A couple of comments on the timeout value. The Marionette client
> > uses timeouts in milliseconds, and I think wherever possible we
> > should avoid switching between milliseconds and seconds.
> 
> I agree about consistency, but as dburns points out, using
> milliseconds isn't interoperable with the Python standard library
> either.  Because this patch integrates with the time library in
> particular, I'd be happy to file a bug on switching the client over to
> use seconds instead.

+1 however this will need to be carefully managed otherwise consumers will be waiting a thousand times longer than they expect! :)

> > Also, a Marionette instance has a timeout property, which can
> > optionally be provided when instantiating it. When we use
> > runtests.py there's also a --timeout argument which is passed to the
> > Marionette instance. If possible, we should avoid having to
> > conditionally pass this timeout value to our Wait object. Maybe we
> > could default to this property if it's set, and fall back to another
> > value?
> 
> My feeling is that these values aren't synonymous with the intention
> of explicit waits.  It seems that the --timeout flag lets you specify
> how long it will take before a test should time out, defaulting to 30
> seconds.
> 
> Having a 30 second timeout on a Wait instance would always cause the
> whole test to time out if the condition isn't met.  The intention is
> that it should retry the condition a couple of times before letting
> the assertions in the test case determine if the test is a pass or a
> fail.
> 
> For that reason it makes more sense for us to either have a sensible
> timeout hard coded in Wait, or to require a timeout argument to be
> given on instantiation.  However, I'm not sure if a 5 second timeout
> with a 0.5 second polling interval is sensible.  I was imagining we
> could change this as we see use cases appearing.

I wasn't proposing a 5 second timeout. The explicit waits we have currently use marionette.timeout if it's set. If we change this to seconds, I'd be happy to use: Wait(marionete, timeout=marionette.timeout) however we should handle when marionette.timeout is None and fall back to the default.

> > The Selenium support class also has an until_not method. While this
> > can be achieved by adding 'not' to the condition, I recall there was
> > still a valid argument for including it.
> 
> Can you remember what the arguments were?  We can negate the until
> function, but it seems rather redundant if people will be relying
> mostly on the prefabricated expected conditions (bug 948075).

I can't. I remember it being discussed on the changeset in google code, but I think this is lost since the move from svn to git.
Attachment #8344190 - Flags: review?(jgriffin)
Submitting a revised patch that addresses all of the concerns above.

Regarding the behaviour of unsuccessful condititons I'm leaning
towards raising a TimeoutException, since this seems more idiomatic to
Python where you cannot define an explicit expected return type.

I've also filed bug 949543 to address the seconds vs. milliseconds
issue.
Attachment #8344190 - Attachment is obsolete: true
Attachment #8346743 - Flags: review?(mdas)
Attachment #8346743 - Flags: review?(dave.hunt)
(In reply to Andreas Tolfsen (:ato) from comment #18)
> (In reply to Malini Das (:mdas) from comment #17)
> > @@ +122,5 @@
> > > +        if last_exc is not None:
> > > +            raise errors.TimeoutException(last_exc)
> > > +
> > > +        return rv
> >
> > Say that my condition is never met during the timeout, and no
> > exception is raised while you're checking the condition. In this
> > case, rv is returned instead of a TimeoutException. Why is that? I
> > would expect it to raise an exception so my test can fail.
> 
> I've found there to be arguments both for and against raising under
> such circumstances:
> 
>   * There's nothing exceptional about a condition evaluating to false.
> 
I feel there would be if I expect this wait to pass. The wait condition is expected to succeed, and on failure, it makes sense to throw, since it was unable to meet it goals. More on this in my last paragraph.

>   * Raising an exception when a condition isn't met might violate the
>     single responsibility principle, in that I'd expect the test to
>     take care of the assertions.
> 
>     If an element can't be found, is it the Wait utility class'
>     responsibility to fail the test by raising?
> 
Yes, I think so. Wait is being used within the unittest to wait on a particular value to be true within a period of time. If it doesn't evaluate to true, something has gone wrong in the test. If it will be used by some other library, the library is free to catch the exception and act accordingly, but as far is Wait's concerns go, it should only be responsible for doing its methods with success, and if it attempted to wait until a condition was true, but the condition never resolved to true, then it makes sense to throw.

>   * The user might expect None or False to be returned, although I
>     suspect that's more the case in a strongly typed language where
>     you'd define your expected return type.
> 
> One of the use cases of Wait is that it can also return things:
> 
> Although not in the requirements for this bug, I've designed Wait to
> be able to return the value of the successful condition.  An example
> of that is the ability to ignore certain exceptions, delegating the
> try…catch encapsulation to Wait, instead of having to deal with this
> in the conditions.
> 
> This means you can write quite elegant code that is syntactically
> similar to the explicit wait classes found in Selenium's Java [1] and
> Python [2] bindings:
> 
>     el = Wait(driver).until(lambda driver: return
> driver.find_element_by_id("foo"),
> ignore_exceptions=errors.NoSuchElementException)
> 
> [1]
> https://code.google.com/p/selenium/source/browse/java/client/src/org/openqa/
> selenium/support/ui/Wait.java
> [2]
> https://code.google.com/p/selenium/source/browse/py/selenium/webdriver/
> support/wait.py

As a general response, I think it's quite unpythonic to have something named 'wait' and 'until' to return a value. I feel like we're importing a design paradigm from Java into python. As a python programmer, I'd expect that if a function is unsuccessful at what I wanted it to do, it will raise an exception.  I'm trying to find some kind of definitive design guide for this, but there doesn't seem to be one for python, but I can't think of an API I've used that uses return values upon failure. It keeps things very simple rather than complex. The until() function will simply wait until something is true, or throw an exception if it can't do that.

If we want the function to both wait until and return a value, then the intention should be clear from the method call, something like 'return_after', although even with this name, it implies it will return after the condition is met, and I'd still think it should throw an error if the condition isn't met... I find it difficult to import this model into python in a way that is clear and simple.
If you take a look at the revised patch in comment #20, it will now
raise on not meeting the condition.

Because return values can freely be ignored (and commonly are in the
standard library), I don't think "until" having a return value is such
a big issue.

This also matches the behaviour of the explicit wait implemenation in
Selenium's Python binding that people are already used to.  The common
use case will also be for people to use the expected conditions (bug
948075), and not so commonly this class directly.
Comment on attachment 8346743 [details] [diff] [review]
0001-Bug-850881-Implement-explicit-wait-condition-class.patch

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

looks good, thanks!
Attachment #8346743 - Flags: review?(mdas) → review+
Comment on attachment 8346743 [details] [diff] [review]
0001-Bug-850881-Implement-explicit-wait-condition-class.patch

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

One question about the default polling interval, but otherwise looks good to me.

::: testing/marionette/client/marionette/wait.py
@@ +6,5 @@
> +import errors
> +import time
> +
> +DEFAULT_TIMEOUT = 5
> +DEFAULT_INTERVAL = 0.5

Will raised a point in a place where I'd used this same interval. Why are we waiting 0.5 seconds by default and not less? Is there a disadvantage to waiting 0.1 seconds as we'd potentially return 0.4 seconds earlier?
Attachment #8346743 - Flags: review?(dave.hunt) → review+
(In reply to Dave Hunt (:davehunt) from comment #24)
> ::: testing/marionette/client/marionette/wait.py
> @@ +6,5 @@
> > +DEFAULT_TIMEOUT = 5
> > +DEFAULT_INTERVAL = 0.5
>
> Will raised a point in a place where I'd used this same
> interval. Why are we waiting 0.5 seconds by default and not less? Is
> there a disadvantage to waiting 0.1 seconds as we'd potentially
> return 0.4 seconds earlier?

This is a good observation.  The only disadvantage of polling more
frequently is that it will possibly put more stress on Marionette.

It's a worthwhile performance improvement to try out.  mdas, do you
have any insight to whether this would cause any problems for
Marionette?
Flags: needinfo?(mdas)
(In reply to Andreas Tolfsen (:ato) from comment #25)
> (In reply to Dave Hunt (:davehunt) from comment #24)
> > ::: testing/marionette/client/marionette/wait.py
> > @@ +6,5 @@
> > > +DEFAULT_TIMEOUT = 5
> > > +DEFAULT_INTERVAL = 0.5
> >
> > Will raised a point in a place where I'd used this same
> > interval. Why are we waiting 0.5 seconds by default and not less? Is
> > there a disadvantage to waiting 0.1 seconds as we'd potentially
> > return 0.4 seconds earlier?
> 
> This is a good observation.  The only disadvantage of polling more
> frequently is that it will possibly put more stress on Marionette.
> 
> It's a worthwhile performance improvement to try out.  mdas, do you
> have any insight to whether this would cause any problems for
> Marionette?

Nope, 0.1sec frequency should be fine, marionette only has one session running at a time so I don't see this as an issue.
Flags: needinfo?(mdas)
Uploaded a new patch with 0.1 default waiting interval.
Attachment #8346743 - Attachment is obsolete: true
Attachment #8348767 - Flags: review?(dave.hunt)
Attachment #8348767 - Flags: review?(dave.hunt) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7211c8a062f2
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
I pressed my luck a bit too much on this. Backed out from b2g26 for test failures that I've already gone too far down the rabbit hole trying to fix.

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/b01ca2847626
status-b2g-v1.2: fixed → ---
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: