794687 - Add --timeout argument to Marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Jonathan Griffin (:jgriffin)]

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]

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]

i start to work on it this morning.

Comment 3

[mathieu bultel:mbu]

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]

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]

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]

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]

Attachment: This is my submission to fix this bug

Comment 8

[Jonathan Griffin (:jgriffin)]

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]

Jonathan, 

thanks, it's done ;)

Comment 10

[Malini Das [:mdas] - Away, not checking bugmail]

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]

(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]

Attachment: Change according to mdas review

Comment 13

[Malini Das [:mdas] - Away, not checking bugmail]

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]

Attachment: 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]

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]

landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d054db406b

Comment 17

[Phil Ringnalda (:philor)]

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

Comment 18

[Jonathan Griffin (:jgriffin)]

http://hg.mozilla.org/releases/mozilla-b2g18/rev/6c5d44151329

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/6c5d44151329