Closed Bug 957248 Opened 7 years ago Closed 7 years ago

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

Categories

(Testing :: Marionette, defect)

defect
Not set
normal

Tracking

(b2g-v1.3 fixed)

RESOLVED FIXED
mozilla29
Tracking Status
b2g-v1.3 --- fixed

People

(Reporter: davehunt, Assigned: ato)

Details

Attachments

(1 file, 1 obsolete file)

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).
Assignee: nobody → ato
Status: NEW → ASSIGNED
Attaching patch.  Note that it uses 1000.0 to force Python to treat it as a float.
Attachment #8357436 - Flags: review?(dave.hunt)
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'?
Attachment #8357436 - Flags: review?(dave.hunt) → review+
(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.
Attachment #8357436 - Attachment is obsolete: true
Attachment #8357739 - Flags: review?(dave.hunt)
Attachment #8357739 - Flags: review?(dave.hunt) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f5e49d0f2e86
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.