815361 - add a get_script_timeout call to marionette

Status: RESOLVED WONTFIX

(Remote Protocol :: Marionette, defect)

Description

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

Sometimes in the backend parts of the framework, we need to set script timeouts independent of the user's async scripts, but we shouldn't overwrite the timeout set by the user. We should add a 'get_script_timeout' call so we can get the already assigned script timeout and restore it if we overwrite it.

Comment 1

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

Attachment: patch v1.0

The attached test will pass once bug 811574 lands. I'll run it through try/ash when the time comes!

Comment 2

[Jonathan Griffin (:jgriffin)]

Comment on attachment 685351 [details] [diff] [review]
patch v1.0

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

Cool, thanks, I've been thinking we needed this for a while.

::: testing/marionette/client/marionette/tests/unit/test_execute_async_script.py
@@ +13,5 @@
> +    def test_get_script_timeout(self):
> +        #default timeout should be set
> +        self.marionette.delete_session()
> +        self.marionette.start_session()
> +        self.assertEqual(10000, self.marionette.get_script_timeout())

I think we should probably just assert that this is a non-zero number, so we don't have to change this test every time the default changes.

Comment 3

[David Burns :automatedtester]

The user needs to be aware of what they are setting so I don't really understand why this needs to be in the python code.

Setting things implicitly is going to lead to a number of support bugs because people dont understand what magic is going on in the background. Personally I wouldn't want the python parts of this bug to be available to the user, probably not even available over the wire at all.

Comment 4

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

(In reply to David Burns :automatedtester from comment #3)
> The user needs to be aware of what they are setting so I don't really
> understand why this needs to be in the python code.
> 
> Setting things implicitly is going to lead to a number of support bugs
> because people dont understand what magic is going on in the background.
> Personally I wouldn't want the python parts of this bug to be available to
> the user, probably not even available over the wire at all.

You may be right. I was going to use this so I can have long press and other calls like that timeout after the given timeout (which is separate form setScriptTimeout's amount) in https://bugzilla.mozilla.org/show_bug.cgi?id=815115, but I can do that by adding a new parameter to execute_async_script, so it will use a passed in timeout rather than the one set by setScriptTimeout. 

Jgriffin, dburns, thoughts?

Comment 5

[Jonathan Griffin (:jgriffin)]

Since we're adding a default timeout in bug 811574, I think get_script_timeout is useful to retrieve it.  Are you really disagreeing with having a default timeout, David, rather than this method?  I'm not 100% certain of that myself (since a useful default is highly context-sensitive), but a number of people have requested it because they've been confused when async scripts immediately timeout due to our current default timeout being effectively 0.

Comment 6

[David Burns :automatedtester]

(In reply to Jonathan Griffin (:jgriffin) from comment #5)
> Since we're adding a default timeout in bug 811574, I think
> get_script_timeout is useful to retrieve it.
> Are you really disagreeing
> with having a default timeout, David, rather than this method? 

No, I understand the need for it but since its a default let's rather have it documented.

My disagreement is that if we shouldnt have this accessible via the python bindings. Since its a default we can tell people what it is. If they they change it, via set_script_timeout, they need to track this themselves.

The scenario that I am trying to prevent is

def setUp(self):
    self.marionette.set_script_timeout(10)
    # Any other setup stuff

def testFoo(self):
    what_is_my_timeout_again = self.marionette.get_script_timeout()
    if what_is_my_timeout_again == 10:
        # We really shouldn't have it as 10 so lets make it 5
        self.marionette.set_script_timeout(5)
    else:
        # or maybe it should 8
        self.marionette.set_script_timeout(8)
    # Let's really start testing now



I have no disagreement with the default and have no disagreement with tracking it in the marionette code in Gecko.

I hope I have articulated this enough

Comment 7

[Jonathan Griffin (:jgriffin)]

Ok, I defer to your Selenium-fu.  Mdas, can you close this after you verify you don't need it?

Comment 8

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

As discussed in IRC, we will change the approach so that you may set the script timeout as a one-shot for an execute command: 816246. Closing this bug

Comment 9

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

oops, that's Bug 816246