add a get_script_timeout call to marionette

RESOLVED WONTFIX

Status

Testing
Marionette
RESOLVED WONTFIX
6 years ago
6 years ago

People

(Reporter: mdas, Assigned: mdas)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Created attachment 685351 [details] [diff] [review]
patch v1.0

The attached test will pass once bug 811574 lands. I'll run it through try/ash when the time comes!
Attachment #685351 - Flags: review?(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.
Attachment #685351 - Flags: review?(jgriffin) → review+
Depends on: 811574
No longer depends on: 811574
Depends on: 811574
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.
(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?
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.
(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
Ok, I defer to your Selenium-fu.  Mdas, can you close this after you verify you don't need it?
No longer blocks: 815115
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.