957248 - If no timeout is provided use the marionette.timeout value if it's set

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

Currently the Wait class takes an optional timeout value and defaults to DEFAULT_TIMEOUT. This means that tests using runtests.py have to do the following to allow the timeout to be overridden from the command line:

    timeout = timeout or (self.marionette.timeout and self.marionette.timeout / 1000) or 30

We should move this to the Wait class as it already has access to the marionette object:

    timeout = timeout or (self.marionette.timeout and self.marionette.timeout / 1000) or DEFAULT_TIMEOUT

This will still allow tests to set their own timeout, but it will fall back to whatever marionette.timeout is set to, or fall back to the default (5 seconds).

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: 0001-Bug-957248-Use-marionette.timeout-if-no-explicit-tim.patch

Attaching patch.  Note that it uses 1000.0 to force Python to treat it as a float.

Comment 2

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

Comment on attachment 8357436 [details] [diff] [review]
0001-Bug-957248-Use-marionette.timeout-if-no-explicit-tim.patch

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

A couple of small nits, but otherwise looks great to me.

::: testing/marionette/client/marionette/tests/unit/test_wait.py
@@ +139,2 @@
>          self.assertIsInstance(w.clock, wait.SystemClock)
> +    def test_timeout_inherited_from_marionette(self):

Nit: Add a space between these two tests.

::: testing/marionette/client/marionette/wait.py
@@ +46,3 @@
>              conditions, usually a Marionette instance.
>          :param timeout: How long to wait for the evaluated condition
> +            to become true.  The default timeout is the `timeout'

Nit: Should `timeout' be 'timeout'?

Comment 3

[Andreas Tolfsen ❲:ato❳]

Attachment: 0001-Bug-957248-Use-marionette.timeout-if-no-explicit-tim.patch

(In reply to Dave Hunt (:davehunt) from comment #2)
> Comment on attachment 8357436 [details] [diff] [review]
> 0001-Bug-957248-Use-marionette.timeout-if-no-explicit-tim.patch
> 
> Review of attachment 8357436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A couple of small nits, but otherwise looks great to me.
> 
> ::: testing/marionette/client/marionette/tests/unit/test_wait.py
> @@ +139,2 @@
> >          self.assertIsInstance(w.clock, wait.SystemClock)
> > +    def test_timeout_inherited_from_marionette(self):
> 
> Nit: Add a space between these two tests.

It seems the diff viewer in Bugzilla is doing something odd here, consider
the raw diff file I uploaded:

    https://bug957248.bugzilla.mozilla.org/attachment.cgi?id=8357436

> ::: testing/marionette/client/marionette/wait.py
> @@ +46,3 @@
> >              conditions, usually a Marionette instance.
> >          :param timeout: How long to wait for the evaluated condition
> > +            to become true.  The default timeout is the `timeout'
> 
> Nit: Should `timeout' be 'timeout'?

Fixed references to classes and properties per PEP 287.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5e49d0f2e86

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5e49d0f2e86