Last Comment Bug 794687 - Add --timeout argument to Marionette
: Add --timeout argument to Marionette
Status: RESOLVED FIXED
[good first bug][lang=python][mentor=...
:
Product: Testing
Classification: Components
Component: Marionette (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla24
Assigned To: mathieu bultel:mbu
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-26 17:20 PDT by Jonathan Griffin (:jgriffin)
Modified: 2013-07-01 10:13 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
wontfix
wontfix
fixed


Attachments
This is my submission to fix this bug (9.01 KB, patch)
2013-05-17 15:52 PDT, mathieu bultel:mbu
malini: review-
Details | Diff | Review
Change according to mdas review (3.13 KB, patch)
2013-05-21 12:37 PDT, mathieu bultel:mbu
no flags Details | Diff | Review
merge two patchs (9.21 KB, patch)
2013-05-22 15:29 PDT, mathieu bultel:mbu
malini: review+
Details | Diff | Review

Description Jonathan Griffin (:jgriffin) 2012-09-26 17:20:38 PDT
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.
Comment 1 Malini Das [:mdas] - Away, not checking bugmail 2013-05-15 13:33:07 PDT
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.
Comment 2 mathieu bultel:mbu 2013-05-16 02:21:53 PDT
i start to work on it this morning.
Comment 3 mathieu bultel:mbu 2013-05-16 02:53:52 PDT
ok, i fix it.. i test it and wait for Malini to discuss about it.

mbu
Comment 4 Malini Das [:mdas] - Away, not checking bugmail 2013-05-16 14:55:28 PDT
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?
Comment 5 David Burns :automatedtester 2013-05-17 04:03:44 PDT
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
Comment 6 Malini Das [:mdas] - Away, not checking bugmail 2013-05-17 14:21:49 PDT
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.
Comment 7 mathieu bultel:mbu 2013-05-17 15:52:51 PDT
Created attachment 751250 [details] [diff] [review]
This is my submission to fix this bug
Comment 8 Jonathan Griffin (:jgriffin) 2013-05-17 17:03:51 PDT
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.
Comment 9 mathieu bultel:mbu 2013-05-18 01:19:51 PDT
Jonathan, 

thanks, it's done ;)
Comment 10 Malini Das [:mdas] - Away, not checking bugmail 2013-05-21 11:02:37 PDT
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"
Comment 11 mathieu bultel:mbu 2013-05-21 11:57:12 PDT
(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 ?
Comment 12 mathieu bultel:mbu 2013-05-21 12:37:48 PDT
Created attachment 752320 [details] [diff] [review]
Change according to mdas review
Comment 13 Malini Das [:mdas] - Away, not checking bugmail 2013-05-22 06:47:55 PDT
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.
Comment 14 mathieu bultel:mbu 2013-05-22 15:29:01 PDT
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 :)
Comment 15 Malini Das [:mdas] - Away, not checking bugmail 2013-05-23 07:22:38 PDT
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.
Comment 16 Malini Das [:mdas] - Away, not checking bugmail 2013-05-23 07:26:32 PDT
landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d054db406b
Comment 17 Phil Ringnalda (:philor) 2013-05-23 21:06:34 PDT
https://hg.mozilla.org/mozilla-central/rev/f2d054db406b
Comment 18 Jonathan Griffin (:jgriffin) 2013-06-27 17:55:29 PDT
http://hg.mozilla.org/releases/mozilla-b2g18/rev/6c5d44151329

Note You need to log in before you can comment on or make changes to this bug.