Add --timeout argument to Marionette

RESOLVED FIXED in Firefox 24, Firefox OS v1.1hd

Status

Testing
Marionette
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jgriffin, Assigned: mbu)

Tracking

unspecified
mozilla24
Points:
---

Firefox Tracking Flags

(firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: [good first bug][lang=python][mentor=mdas])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
I think we should add a --timeout argument to the Marionette test runner, which would default to something sensible (30s?) and provide default values for script timeouts, search timeouts, and JS tests.  Tests could still override this using any of the existing mechanisms, but we'd have a sane way of managing test runs on slower machines and would no longer have to hardcode timeouts in every test, except where specifically needed.
So to start work on this bug, get the source code:
https://developer.mozilla.org/en-US/docs/Developer_Guide/Source_Code/Mercurial

Then, get a version of Firefox with marionette built into it, from within:
ftp://ftp.mozilla.org/pub/firefox/nightly/2013/

For example, for May 15th's mac build:
ftp://ftp.mozilla.org/pub/firefox/nightly/2013/05/2013-05-15-mozilla-central-debug/

I strongly recommend using virtualenv (https://pypi.python.org/pypi/virtualenv) when working in python. To install it run 'pip install virtualenvl'

Once you have this, go into the mozilla-central source code you downloaded and do:
cd testing/marionette/client/
virtualenv venv
source venv/bin/activate
python setup.py develop

Now you will have a marionette-client installed in your virtual environment! To test out if everything is in working order, let's run a sample test. To do this, you'll need to know where the firefox-bin lives in the Firefox version you downloaded. For mac, it's in FirefoxNightlyDebug.app/Contents/MacOS/firefox-bin
cd marionette/, for linux, once you untar the file, it should be under firefox/firefox-bin:

python runtests.py --binary=<path to firefox-bin> --type=desktop tests/unit/test_execute_script.py

This will run the test_execute_script.py tests against the firefox build you downloaded.

Now, to work on this bug, the change should occur in two files:
(mozilla-central)/testing/marionette/client/marionette/runtests.py
and
(mozilla-central)/testing/marionette/client/marionette/marionette_test.py

In the  class MarionetteTestOptions in runtests.py, we'll need to add a --timeout option to let us pass in the timeout, in milliseconds. We should have a default of 30000ms if no such option is passed in.

We will apply this timeout value in marionette_test.py. Right after we call start_session(), we want to set our timeout values. We will call set_search_timeout and set_script_timeout with these values.

To test your work, you should write a test in the same vein as (mozilla-central)/testing/marionette/client/marionette/tests/unit/test_execute_async_script.py and (mozilla-central)/testing/marionette/client/marionette/tests/unit/test_findelement.py, where you'd have a class whose setUp does not call set_script_timeout/set_search_timeout and will fallback on the defaults.
Whiteboard: [good first bug]
Whiteboard: [good first bug] → [good first bug][lang=python][mentor=mdas]
(Assignee)

Comment 2

4 years ago
i start to work on it this morning.
(Assignee)

Comment 3

4 years ago
ok, i fix it.. i test it and wait for Malini to discuss about it.

mbu
mbu noticed that we now have the 'timeouts' function, so we can also set page load timeout with this.

Another thing that was brought to our attention was that default values of 30secs for each timeout is against our current assumptions. We assume 0secs for search (implicit) timeouts. I think we should keep this the default behaviour, since we don't expect find_element() to wait until it finds an element. From webdriver spec: https://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html#timeouts-1 "If this command is never sent, the driver must default to an implicit wait of 0ms.", so if we don't pass --timeouts, it should be implied that we have a default of 0ms. 

jgriffin, dburns, any comment?
I am happy with this. 

Keeping 0ms as the default is the way forward for finding elements. For page loads this is tricky since we have been trying to solve this in Webdriver for a while and is still a hard problem
Ah, after Bug 815757 landed we now have default values for script timeout on the server side, and search timeout is 0s by default as well, so we'll just need to the page load timeout default. If we want to remove this later, we can, I think it's a good idea to have it in case any page we're navigating to gets stuck in a test in CI.
(Assignee)

Comment 7

4 years ago
Created attachment 751250 [details] [diff] [review]
This is my submission to fix this bug
(Reporter)

Comment 8

4 years ago
Hey Mathieu,

Go ahead and click on the details link for your patch, and set r? for :mdas, so she knows it's ready for review.
(Assignee)

Updated

4 years ago
Attachment #751250 - Flags: review?(mdas)
(Assignee)

Comment 9

4 years ago
Jonathan, 

thanks, it's done ;)
Comment on attachment 751250 [details] [diff] [review]
This is my submission to fix this bug

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

Great work! I tested this out locally and it works, and I'll be happy to r+ it once the comments are addressed.

::: testing/marionette/client/marionette/marionette_test.py
@@ +86,5 @@
>          self.start_time = time.time()
>          self.marionette = self._marionette_weakref()
>          if self.marionette.session is None:
>              self.marionette.start_session()
> +        if not self.marionette.timeout is None:

I find 

'if self.marionette.timeout is not None:'

to be a bit clearer

@@ +89,5 @@
>              self.marionette.start_session()
> +        if not self.marionette.timeout is None:
> +            self.marionette.timeouts(self.marionette.TIMEOUT_SEARCH, self.marionette.timeout)
> +            self.marionette.timeouts(self.marionette.TIMEOUT_SCRIPT, self.marionette.timeout)
> +            self.marionette.timeouts(self.marionette.TIMEOUT_PAGE, self.marionette.timeout)

we should add an 'else' block here, and set the page load time to 30seconds, just to make sure that if anything goes wrong during a test that requires page loading, we don't hang around waiting indefinitely.

::: testing/marionette/client/marionette/runtests.py
@@ +643,5 @@
>                          help='absolute path to directory containing breakpad symbols, or the url of a zip file containing symbols')
> +        self.add_option('--timeout',
> +                        dest='timeout',
> +                        type=int,
> +                        help='the timeout is set by default to 30000ms for load page, 0ms to search page and 10ms to script')

The help message should tell you how to use --timeout, so it should say something like:

"if a --timeout value is given, it will set the default page load timeout, search timeout and script timeout to the given value. If not passed in, it will use the default values of 30000ms for page load, 0ms for search timeout and 10ms for script timeout"
Attachment #751250 - Flags: review?(mdas) → review-
(Assignee)

Comment 11

4 years ago
(In reply to Malini Das [:mdas] from comment #10)
> Comment on attachment 751250 [details] [diff] [review]
> This is my submission to fix this bug
> 
> Review of attachment 751250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great work! I tested this out locally and it works, and I'll be happy to r+
> it once the comments are addressed.
> 
> ::: testing/marionette/client/marionette/marionette_test.py
> @@ +86,5 @@
> >          self.start_time = time.time()
> >          self.marionette = self._marionette_weakref()
> >          if self.marionette.session is None:
> >              self.marionette.start_session()
> > +        if not self.marionette.timeout is None:
> 
> I find 
> 
> 'if self.marionette.timeout is not None:'
> 
> to be a bit clearer

hehe yes, i realize this just right after send the patch .. :)

> 
> @@ +89,5 @@
> >              self.marionette.start_session()
> > +        if not self.marionette.timeout is None:
> > +            self.marionette.timeouts(self.marionette.TIMEOUT_SEARCH, self.marionette.timeout)
> > +            self.marionette.timeouts(self.marionette.TIMEOUT_SCRIPT, self.marionette.timeout)
> > +            self.marionette.timeouts(self.marionette.TIMEOUT_PAGE, self.marionette.timeout)
> 
> we should add an 'else' block here, and set the page load time to 30seconds,
> just to make sure that if anything goes wrong during a test that requires
> page loading, we don't hang around waiting indefinitely.
> 

Ok, you want to set only the default value for page load or for script and search too ?

> ::: testing/marionette/client/marionette/runtests.py
> @@ +643,5 @@
> >                          help='absolute path to directory containing breakpad symbols, or the url of a zip file containing symbols')
> > +        self.add_option('--timeout',
> > +                        dest='timeout',
> > +                        type=int,
> > +                        help='the timeout is set by default to 30000ms for load page, 0ms to search page and 10ms to script')
> 
> The help message should tell you how to use --timeout, so it should say
> something like:
> 
> "if a --timeout value is given, it will set the default page load timeout,
> search timeout and script timeout to the given value. If not passed in, it
> will use the default values of 30000ms for page load, 0ms for search timeout
> and 10ms for script timeout"

Does i make a new patch according to your comments ?
(Assignee)

Comment 12

4 years ago
Created attachment 752320 [details] [diff] [review]
Change according to mdas review
Attachment #752320 - Flags: review?(mdas)
Comment on attachment 752320 [details] [diff] [review]
Change according to mdas review

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

Ah, it looks like this patch applies on top of your previous patch, since the diff is relative to your last patch, and this patch is missing the changes to marionette.py.

The changes look good, but can you please upload a new patch, one that incorporates the changes from both your patches? You can follow https://developer.mozilla.org/en-US/docs/Mercurial_Queues#Folding_multiple_patches_into_one to do so.
Attachment #752320 - Flags: review?(mdas)
(Assignee)

Comment 14

4 years ago
Created attachment 752998 [details] [diff] [review]
merge two patchs

Yes !
I don't use very well hg patch, so i hope this time is the good time :)
Attachment #751250 - Attachment is obsolete: true
Attachment #752320 - Attachment is obsolete: true
Attachment #752998 - Flags: review?(mdas)
Comment on attachment 752998 [details] [diff] [review]
merge two patchs

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

Looks good, thanks! I'll land this patch soon.
Attachment #752998 - Flags: review?(mdas) → review+
landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d054db406b
https://hg.mozilla.org/mozilla-central/rev/f2d054db406b
Assignee: nobody → mat.bultel
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(Reporter)

Comment 18

4 years ago
http://hg.mozilla.org/releases/mozilla-b2g18/rev/6c5d44151329
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox24: --- → fixed
status-firefox25: --- → fixed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/6c5d44151329
status-b2g-v1.1hd: --- → fixed
status-firefox25: fixed → ---
You need to log in before you can comment on or make changes to this bug.