997770 - Allow users to declare how long to wait before connection timeout

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

When the marionette client sends data to the server, it uses the default timeout period of 6 minutes as defined in the transport:

https://mxr.mozilla.org/mozilla-central/source/testing/marionette/transport/marionette_transport/transport.py#57

This should be a configurable value passed in when we create the Marionette object, so instead of doing this:

https://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette.py#514


We should do self.client = MarionetteTransport(self.host, self.port, my_passed_in_timeout). To get this working we have to change the transport's connect() function to use this default if it is set.

This is currently hindering our work on automating marketplace apps for tarako.

Comment 1

[Luv Agarwal(:lagarwal)]

Hey, i am interested in working on this.

Comment 2

[Luv Agarwal(:lagarwal)]

Malini, i am sorry but i didn't see that you are assigned this bug as the status was new. But i have made a patch for this. If you are not working on this then i can submit my patch and you can review that.

Thanks

Comment 3

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

(In reply to Luv Agarwal(:lagarwal) from comment #2)
> Malini, i am sorry but i didn't see that you are assigned this bug as the
> status was new. But i have made a patch for this. If you are not working on
> this then i can submit my patch and you can review that.
> 
> Thanks

Don't worry about it, I'm interested in seeing your patch. Is it fully working and can you upload it and r? me?

Comment 4

[Luv Agarwal(:lagarwal)]

Attachment: fix_997770.patch

Allow users to explicitly tell the connection timeout

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8447938 [details] [diff] [review]
fix_997770.patch

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

You might need to change MarionetteTransport.connect() callers such as http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/b2g_update_test.py#39

Comment 6

[Luv Agarwal(:lagarwal)]

Attachment: fix_997770.patch

made changes to the users of class - MarionetteTransport

Comment 7

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

Comment on attachment 8448789 [details] [diff] [review]
fix_997770.patch

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

Thanks for the patch! This works well. Since we're updating the api, will you update the version number for marionette-transport from '0.2' to '0.3' in the setup.py file in testing/marionette/transport/, and update the requirements.txt file in testing/marionette/client to the new version number? This will let the new change propagate. Thanks again!

::: testing/marionette/transport/marionette_transport/transport.py
@@ +17,5 @@
>  
>      max_packet_length = 4096
>      connection_lost_msg = "Connection to Marionette server is lost. Check gecko.log (desktop firefox) or logcat (b2g) for errors."
>  
> +    def __init__(self, addr, port, socket_timeout):

Can you update this to be an optional parameter, with 360.0 as default?

Comment 8

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

Comment on attachment 8447938 [details] [diff] [review]
fix_997770.patch

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

clearing flag

Comment 9

[Luv Agarwal(:lagarwal)]

Attachment: fix_997770.patch

Updated the version of marionette_transport.

Comment 10

[Luv Agarwal(:lagarwal)]

Malini when i made the changes in marionette_transport (now new marionette_transport has to considered when running setup.py develop in marionette client but still these changes are not at pypi) so i first ran 'setup.py develop' in client and then 'setup.py develop' in transport in order to consider the new marionette_transport.

So is this way only things are done or there is some other way also ?.

Comment 11

[Jonathan Griffin (:jgriffin)]

(In reply to Luv Agarwal(:lagarwal) from comment #10)
> Malini when i made the changes in marionette_transport (now new
> marionette_transport has to considered when running setup.py develop in
> marionette client but still these changes are not at pypi) so i first ran
> 'setup.py develop' in client and then 'setup.py develop' in transport in
> order to consider the new marionette_transport.
> 
> So is this way only things are done or there is some other way also ?.

Normally, you would run 'setup.py develop' for Marionette transport first, because that is a package the client will depend on, then run 'setup.py develop' for Marionette client.

Otherwise, 'setup.py develop' for the client will pull a version of marionette-transport from pypi, which is not what you want.

Comment 12

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8451218 [details] [diff] [review]
fix_997770.patch

Looks good; I'll push it to try and make sure it runs green there.

Comment 13

[Jonathan Griffin (:jgriffin)]

try run: https://tbpl.mozilla.org/?tree=Try&rev=1624f5a17670

Comment 14

[Jonathan Griffin (:jgriffin)]

Try looks good!

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d6c8b4d34c5

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8d6c8b4d34c5