Closed Bug 1316622 Opened 8 years ago Closed 8 years ago

Implement Get Timeouts command in Marionette

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(12 files, 1 obsolete file)

Bug 1316622 - Rename Marionette command timeouts to setTimeouts;
58 bytes, text/x-review-board-request
automatedtester
: review+
whimboo
: review+
Details
Bug 1316622 - Make implicit wait timeout default to 0;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1316622 - Move internal interface construction last;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1316622 - Add timeout reset API to Marionette Python client;
58 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1316622 - New timeouts inteface in Marionette Python client;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1316622 - Deprecate old Marionette timeouts API;
58 bytes, text/x-review-board-request
automatedtester
: review+
whimboo
: review+
Details
Bug 1316622 - Move Marionette unit tests to new timeouts API;
58 bytes, text/x-review-board-request
automatedtester
: review+
whimboo
: review+
Details
Bug 1316622 - Move Marionette harness tests to new timeouts API;
58 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1316622 - Move microformats Marionete tests to new timeouts API;
58 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1316622 - Move wptrunner to new Marionette timeouts API;
58 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 1316622 - Correct Marionette timeouts tests;
58 bytes, text/x-review-board-request
automatedtester
: review+
whimboo
: review+
Details
Bug 1316622 - Remove wait utility dependency on Marionette default timeout;
58 bytes, text/x-review-board-request
whimboo
: review+
Details
Blocks: webdriver
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment on attachment 8809561 [details]
Bug 1316622 - Rename Marionette command timeouts to setTimeouts;

https://reviewboard.mozilla.org/r/92098/#review92256

::: testing/marionette/client/marionette_driver/marionette.py:1320
(Diff revision 1)
>              be raised
>  
>          """
>          try:
> -            self._send_message("timeouts", {"script": timeout})
> +            self._send_message("setTimeouts", {"script": timeout})
>          except errors.MarionetteException as e:

Lets fix this bogus exception check and replace it with `UnknownCommandException` in all three cases here.

So what happens with the backward compat changes as done by bug 1308453? I feel we miss the time since `timeouts` is using the new format. Means we need two layers of fallback. Or do I miss something?

::: testing/marionette/driver.js:2741
(Diff revision 1)
>    "log": GeckoDriver.prototype.log,
>    "getLogs": GeckoDriver.prototype.getLogs,
>    "setContext": GeckoDriver.prototype.setContext,
>    "getContext": GeckoDriver.prototype.getContext,
>    "executeScript": GeckoDriver.prototype.executeScript,
> -  "timeouts": GeckoDriver.prototype.timeouts,
> +  "timeouts": GeckoDriver.prototype.setTimeouts,  // deprecated

Can we put deprecated entries at the beginning or end of this list? It would make it easier to spot and remove them. Also we might want to mention when we are going to remove them? In this case it would be for Firefox 55 (when ESR45 support has been ended).

::: testing/marionette/harness/marionette/tests/unit/test_timeouts.py:83
(Diff revision 1)
>      def test_compat_input_types(self):
>          # When using the spec-incompatible input format which we have
>          # for backwards compatibility, it should be possible to send ms
>          # as a string type and have the server parseInt it to an integer.
>          body = {"type": "script", "ms": "30000"}
> +        self.marionette._send_message("setTimeouts", body)

Given that this is a protected method, I wonder if we should make `send_message()` public if we want/have to use it like that.
Attachment #8809561 - Flags: review?(hskupin) → review-
Comment on attachment 8809564 [details]
Bug 1316622 - Add timeout reset API to Marionette Python client;

https://reviewboard.mozilla.org/r/92104/#review92258

::: testing/marionette/driver.js:1551
(Diff revision 1)
>        yield browserListening;
>      }
>    }
>  };
>  
> +GeckoDriver.prototype.getTimeouts = function(cmd, resp) {

Please remember to add documentation for newly added functions.
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review92260

::: testing/marionette/client/marionette_driver/marionette.py
(Diff revision 1)
>          self.session_id = None
>          self.window = None
>          self.chrome_window = None
>          self.baseurl = baseurl
>          self._test_name = None
> -        self.timeout = timeout

What happens with the timout argument? It looks like that we do no longer need it.

::: testing/marionette/client/marionette_driver/timeout.py:23
(Diff revision 1)
> +        # => 10
> +
> +    """
> +
> +    def __init__(self, conn):
> +        self._conn = conn

Can we name this `marionette` please?

::: testing/marionette/client/marionette_driver/timeout.py:37
(Diff revision 1)
> +        try:
> +            self._conn._send_message("setTimeouts", {name: ms})
> +        except errors.MarionetteException as e:
> +            # remove when 52.0a is stable
> +            if "Not a Number" in e.message:
> +                self._send_message("timeouts", {"type": name, "ms": ms})

`self._conn._send_message()`. Please also see my comments from above and the first review in this series.

::: testing/marionette/client/marionette_driver/timeout.py:74
(Diff revision 1)
> +        return self._get("page load")
> +
> +    @page_load.setter
> +    def page_load(self, sec):
> +        """Set the session's page load timeout.  This specifies the time
> +        to wait for the page loading to compelte.

`complete` and some extra spaces in all the comments in this file.

::: testing/marionette/client/marionette_driver/timeout.py:81
(Diff revision 1)
> +        """
> +        self._set("page load", sec)
> +
> +    @property
> +    def implicit(self):
> +        """Set the session's implicit wait timeout.  This specifies the

Mismatch in docstring.
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review92264
Attachment #8809565 - Flags: review?(hskupin) → review-
Comment on attachment 8809566 [details]
Bug 1316622 - Deprecate old Marionette timeouts API;

https://reviewboard.mozilla.org/r/92108/#review92266

::: testing/marionette/client/marionette_driver/marionette.py:1331
(Diff revision 1)
>          :param timeout: The maximum number of milliseconds an asynchronous
>              script can run without causing an ScriptTimeoutException to
>              be raised
>  
>          """
> -        try:
> +        warnings.warn(

Please also add the deprecation notice to the docstring so it appears on readthedocs.
Attachment #8809566 - Flags: review?(hskupin) → review+
Comment on attachment 8809567 [details]
Bug 1316622 - Move Marionette unit tests to new timeouts API;

https://reviewboard.mozilla.org/r/92110/#review92268

::: testing/marionette/harness/marionette/tests/unit/test_element_retrieval.py:140
(Diff revision 1)
>          self.assertRaises(NoSuchElementException, self.marionette.find_element, By.PARTIAL_LINK_TEXT, "cheese")
>          self.assertRaises(NoSuchElementException, self.marionette.find_element, By.TAG_NAME, "cheese")
>          self.assertRaises(NoSuchElementException, self.marionette.find_element, By.XPATH, "cheese")
>  
>      def test_not_found_implicit_wait(self):
> -        self.marionette.set_search_timeout(50)
> +        self.marionette.timeout.implicit = 1

We are changing this from 50ms to 1s. Is that change ok, for this test? I haven't read the specs but it looks like no float values are allowed for timeouts?

::: testing/marionette/harness/marionette/tests/unit/test_execute_async_script.py:153
(Diff revision 1)
> -            setTimeout("callback(foo())", 50);
> +            setTimeout("callback(foo())", 1500);
>              """)
>          self.assertRaises(JavascriptException,
>              self.marionette.execute_async_script, """
>              var callback = arguments[arguments.length - 1];
> -            setTimeout("callback(foo())", 50);
> +            setTimeout("callback(foo())", 1500);

Same for all of those values.
Attachment #8809567 - Flags: review?(hskupin) → review+
Comment on attachment 8809568 [details]
Bug 1316622 - Move Marionette harness tests to new timeouts API;

https://reviewboard.mozilla.org/r/92112/#review92270

::: testing/marionette/harness/session/session_test.py:350
(Diff revision 1)
>              marionette.navigate('data:text/html,<html>test page</html>')
>  
>          timeout = JSTest.timeout_re.search(js)
>          if timeout:
> -            timeout = timeout.group(3)
> -            marionette.set_script_timeout(timeout)
> +            ms = timeout.group(3)
> +            marionette.timeout.script = int(ms) / 1000

This file will be removed via bug 1316800 most likely today.
Attachment #8809568 - Flags: review?(hskupin) → review+
Comment on attachment 8809569 [details]
Bug 1316622 - Move microformats Marionete tests to new timeouts API;

https://reviewboard.mozilla.org/r/92114/#review92272
Attachment #8809569 - Flags: review?(hskupin) → review+
Comment on attachment 8809570 [details]
Bug 1316622 - Move mozapps Marionette tests to new timeouts API;

https://reviewboard.mozilla.org/r/92116/#review92274

We are going to remove those files via bug 1316707 most likely before your patch will land.
Attachment #8809570 - Flags: review?(hskupin) → review+
Attachment #8809571 - Flags: review?(hskupin) → review?(james)
Comment on attachment 8809572 [details]
Bug 1316622 - Correct Marionette timeouts tests;

https://reviewboard.mozilla.org/r/92120/#review92276
Attachment #8809572 - Flags: review?(hskupin) → review+
Comment on attachment 8809562 [details]
Bug 1316622 - Make implicit wait timeout default to 0;

https://reviewboard.mozilla.org/r/92100/#review92286
Attachment #8809562 - Flags: review?(dburns) → review+
Comment on attachment 8809563 [details]
Bug 1316622 - Move internal interface construction last;

https://reviewboard.mozilla.org/r/92102/#review92288
Attachment #8809563 - Flags: review?(dburns) → review+
Comment on attachment 8809564 [details]
Bug 1316622 - Add timeout reset API to Marionette Python client;

https://reviewboard.mozilla.org/r/92104/#review92290
Attachment #8809564 - Flags: review?(dburns) → review+
Comment on attachment 8809571 [details]
Bug 1316622 - Move wptrunner to new Marionette timeouts API;

https://reviewboard.mozilla.org/r/92118/#review92292

::: testing/web-platform/harness/wptrunner/executors/executorselenium.py:134
(Diff revision 1)
>  
>      def run(self):
>          timeout = self.timeout
>  
>          try:
> -            self.webdriver.set_script_timeout((timeout + extra_timeout) * 1000)
> +            self.webdriver.timeout.script = timeout + extra_timeout

This isn't marionette related.
Attachment #8809571 - Flags: review?(james) → review-
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review92260

> Mismatch in docstring.

in what way?
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review92260

> in what way?

nevermind... I see it now...
Comment on attachment 8809567 [details]
Bug 1316622 - Move Marionette unit tests to new timeouts API;

https://reviewboard.mozilla.org/r/92110/#review92504
Attachment #8809567 - Flags: review?(dburns) → review+
Comment on attachment 8809561 [details]
Bug 1316622 - Rename Marionette command timeouts to setTimeouts;

https://reviewboard.mozilla.org/r/92098/#review92256

> Lets fix this bogus exception check and replace it with `UnknownCommandException` in all three cases here.
> 
> So what happens with the backward compat changes as done by bug 1308453? I feel we miss the time since `timeouts` is using the new format. Means we need two layers of fallback. Or do I miss something?

We can’t because older Firefox versions used to throw `MarionetteException`s.  Because it is a generalisation of `UnknownCommandException`, we were able to change it throw `UnknownCommandException` later.

> Can we put deprecated entries at the beginning or end of this list? It would make it easier to spot and remove them. Also we might want to mention when we are going to remove them? In this case it would be for Firefox 55 (when ESR45 support has been ended).

Added a comment about which version it can be removed with.

I’ve run into a lot of problems lately with the ordering of this list, so instead of grouping them by whether they’re deprecated I suggest grouping it lexiographically.  I should also mention that I have a plan to remove this list altogether.

> Given that this is a protected method, I wonder if we should make `send_message()` public if we want/have to use it like that.

We generally don’t want consumers to rely on it.  It’s here used in a test as the implemented API sends a different body.  I think that’s accepted usage of a private method.
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review92260

> What happens with the timout argument? It looks like that we do no longer need it.

I’ve moved it to :578 and renamed it `default_timeouts`.
Comment on attachment 8809568 [details]
Bug 1316622 - Move Marionette harness tests to new timeouts API;

https://reviewboard.mozilla.org/r/92112/#review92270

> This file will be removed via bug 1316800 most likely today.

Will be resolved when rebasing.
Comment on attachment 8809571 [details]
Bug 1316622 - Move wptrunner to new Marionette timeouts API;

https://reviewboard.mozilla.org/r/92118/#review92292

> This isn't marionette related.

Oops.  Thanks for noticing this.
Comment on attachment 8809571 [details]
Bug 1316622 - Move wptrunner to new Marionette timeouts API;

https://reviewboard.mozilla.org/r/92118/#review93122
Attachment #8809571 - Flags: review?(james) → review+
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review92260

> I’ve moved it to :578 and renamed it `default_timeouts`.

Did you miss to push this change? I still cannot find its usage.
Comment on attachment 8809561 [details]
Bug 1316622 - Rename Marionette command timeouts to setTimeouts;

https://reviewboard.mozilla.org/r/92098/#review92256

> We can’t because older Firefox versions used to throw `MarionetteException`s.  Because it is a generalisation of `UnknownCommandException`, we were able to change it throw `UnknownCommandException` later.

`UnknownCommandException` exists since Firefox 40.0. Do we really care about that? I doubt so. The oldest release our update tests support is 45ESR right now. Or do I miss something else which would block us from changing this?

> Added a comment about which version it can be removed with.
> 
> I’ve run into a lot of problems lately with the ordering of this list, so instead of grouping them by whether they’re deprecated I suggest grouping it lexiographically.  I should also mention that I have a plan to remove this list altogether.

Keep in mind that with killing the old API with version 55.0 we have to uplift this patch to mozilla-aurora now which will be the next ESR release.

> We generally don’t want consumers to rely on it.  It’s here used in a test as the implemented API sends a different body.  I think that’s accepted usage of a private method.

It's a bit misleading then. Protected properties (not private in this case) should be used by the class and any subclass. But we seem to need it for tests and other modules like localization.py too. Maybe we have to think about a bit more about its visibility in the future. Fine for now.
Comment on attachment 8809561 [details]
Bug 1316622 - Rename Marionette command timeouts to setTimeouts;

https://reviewboard.mozilla.org/r/92098/#review92256

> `UnknownCommandException` exists since Firefox 40.0. Do we really care about that? I doubt so. The oldest release our update tests support is 45ESR right now. Or do I miss something else which would block us from changing this?

Right, excuse me.  I was reasoning the wrong way around.  When the command was called `timeouts` we needed to catch the `MarionetteException` because it would throw a non-descript generalised error.

Now that it has been renamed `setTimeouts`, which does not exist in earlier Geckos, we can catch `UnknownCommandException` like you say.

I will address this in the patch later in this series where I introduce the new API (in timeout.py).

> Keep in mind that with killing the old API with version 55.0 we have to uplift this patch to mozilla-aurora now which will be the next ESR release.

That is a good point.  A strong argument against sorting the list.
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review92260

> Did you miss to push this change? I still cannot find its usage.

It’s used in `Marionette.reset_timeouts`.
Comment on attachment 8809567 [details]
Bug 1316622 - Move Marionette unit tests to new timeouts API;

https://reviewboard.mozilla.org/r/92110/#review92268

> We are changing this from 50ms to 1s. Is that change ok, for this test? I haven't read the specs but it looks like no float values are allowed for timeouts?

I incorrectly made that assumption.  We can use floats in the Python API as they are sent as milliseconds to the server.
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review92260

> It’s used in `Marionette.reset_timeouts`.

Maybe I was not clear enough. The `__init__` method of the Marionette class has a timeout argument. This is no longer used in that method. Oh, I really checked this code 3 times now, and still cannot find a line with it contained. In line 573 where it was used before you removed it.
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review92260

> Maybe I was not clear enough. The `__init__` method of the Marionette class has a timeout argument. This is no longer used in that method. Oh, I really checked this code 3 times now, and still cannot find a line with it contained. In line 573 where it was used before you removed it.

OK.  That wasn’t clear to me.

Pushed a change which assigns the `timeout` ctor argument to the Marionette class to the `default_timeouts` property.
Comment on attachment 8809561 [details]
Bug 1316622 - Rename Marionette command timeouts to setTimeouts;

https://reviewboard.mozilla.org/r/92098/#review93782

::: testing/marionette/driver.js:2786
(Diff revision 3)
>    "log": GeckoDriver.prototype.log,
>    "getLogs": GeckoDriver.prototype.getLogs,
>    "setContext": GeckoDriver.prototype.setContext,
>    "getContext": GeckoDriver.prototype.getContext,
>    "executeScript": GeckoDriver.prototype.executeScript,
> -  "timeouts": GeckoDriver.prototype.timeouts,
> +  "getTimeouts": GeckoDriver.prototype.getTimeouts,

Generally we want that individual commits in a series will still allow us to bisect for a regression. As such each commit has to produce working code.

Here we end-up in a situation that `getTimeouts()` has not been implemented yet. Please combine commits 1 and 4 to fix that.
Attachment #8809561 - Flags: review?(hskupin) → review-
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review93784

::: testing/marionette/client/marionette_driver/marionette.py:578
(Diff revision 5)
> -        :param instance_args: args to pass to instance_class
> -        """
>          self.host = host
>          self.port = self.local_port = int(port)
>          self.bin = bin
> +        self.default_timeouts = timeout

Mind renaming the `timeout` argument to `timeouts`? Right now its confusing to me because I expect to pass-in a simple int but not a dictionary.

Also given that you move all timeout related code into the new module, is there a reason why we have to keep `default_timeouts` on the Marionette object? Why can't we make it a property of the Timeouts class?

::: testing/marionette/client/marionette_driver/marionette.py:793
(Diff revision 5)
> -
> -        if self.timeout is not None:
> -            for typ, ms in self.timeout:
> -                timeout_types[typ](ms)
>          else:
> -            self.set_page_load_timeout(30000)
> +            self.timeout.page_load = 30

What about a `Timeouts.reset()` method here? Or shouldn't it be available via `marionette.timeout.reset().

::: testing/marionette/client/marionette_driver/timeout.py:30
(Diff revision 5)
> +    def _set(self, name, sec):
> +        ms = sec * 1000
> +        try:
> +            self._marionette._send_message("setTimeouts", {name: ms})
> +        except errors.UnknownCommandException:
> +            # remove when 52.0a is stable

This is still not true. We have to keep it until ESR45 is no longer supported, which is Firefox 55.

Also I do not see the fallback to the second last changes for timeouts as mentioned in the other commit which was done on bug 1308453. Our update tests support 3 releases back. So given that we want this patch on Aurora (52ESR) it means Firefox 49. This version doesn't include the changes to bug 1308453.
Attachment #8809565 - Flags: review?(hskupin) → review-
Comment on attachment 8811293 [details]
Bug 1316622 - Remove wait utility dependency on Marionette default timeout;

https://reviewboard.mozilla.org/r/93446/#review93780

::: testing/marionette/client/marionette_driver/wait.py:15
(Diff revision 2)
>  DEFAULT_TIMEOUT = 5
>  DEFAULT_INTERVAL = 0.1
>  
>  
>  class Wait(object):
> +

It would help a lot if we can get white-space changes into a different commit or even bug.

::: testing/marionette/client/marionette_driver/wait.py:74
(Diff revision 2)
>              implementation details.
>  
>          """
>  
>          self.marionette = marionette
> -        self.timeout = timeout
> +        self.timeout = timeout or DEFAULT_TIMEOUT

You are loosing the timeout case of `0s` here. In such a case we would fallback to `5s`.
Attachment #8811293 - Flags: review?(hskupin) → review-
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review93784

> Mind renaming the `timeout` argument to `timeouts`? Right now its confusing to me because I expect to pass-in a simple int but not a dictionary.
> 
> Also given that you move all timeout related code into the new module, is there a reason why we have to keep `default_timeouts` on the Marionette object? Why can't we make it a property of the Timeouts class?

Adding a subsequent patch which removes the `timeout` ctor argument and the `default_timeouts` internal state.

I also like your idea of moving the timeout reset to a new API.  It adds `self.marionette.timeout.reset()` which resets the timeouts to their default values (as documented).

> What about a `Timeouts.reset()` method here? Or shouldn't it be available via `marionette.timeout.reset().

Agree.  See new commit.

> This is still not true. We have to keep it until ESR45 is no longer supported, which is Firefox 55.
> 
> Also I do not see the fallback to the second last changes for timeouts as mentioned in the other commit which was done on bug 1308453. Our update tests support 3 releases back. So given that we want this patch on Aurora (52ESR) it means Firefox 49. This version doesn't include the changes to bug 1308453.

If the `setTimeouts` command doesn’t exist, we can rely on sending the old data format to `timeouts` because it has backwards compatibility in the server: https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/driver.js#L1565

We _could_ add in the "Not a Number" fallback too, but I don’t think the recent data format change has been propagated very far.  So as long as `setTimeouts` returns "unknown command" we can use `timeouts` with the old data format, which would be compatible quite far back.
Comment on attachment 8811293 [details]
Bug 1316622 - Remove wait utility dependency on Marionette default timeout;

https://reviewboard.mozilla.org/r/93446/#review93780

> It would help a lot if we can get white-space changes into a different commit or even bug.

With the limited number of whitespaces changes made here, I don’t think it’s an issue.  I agree it would be if there were more of them.
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review93784

> If the `setTimeouts` command doesn’t exist, we can rely on sending the old data format to `timeouts` because it has backwards compatibility in the server: https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/driver.js#L1565
> 
> We _could_ add in the "Not a Number" fallback too, but I don’t think the recent data format change has been propagated very far.  So as long as `setTimeouts` returns "unknown command" we can use `timeouts` with the old data format, which would be compatible quite far back.

Just to recap... The former timeout changes via bug 1302707 had to be backed out from formerly beta (50.0) because it caused a regression as tracked on bug 1308453. On the latter bug we got the non-existent backward compatibility added to 51.0, and 52.0. Update tests are using the client version of the target build, which means eg. for a possible future update from 50.0 to 53.0 we use the current client (53), which would not contain the second fallback anymore in case your patch would be landed as it is. As result we would have the breakage because `marionette.restart()` calls `reset_timeouts()`. I hope all my thinking here is correct.
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review93980

::: testing/marionette/client/marionette_driver/timeout.py:31
(Diff revision 6)
> +        ms = sec * 1000
> +        try:
> +            self._marionette._send_message("setTimeouts", {name: ms})
> +        except errors.UnknownCommandException:
> +            # remove when 55 is stable
> +            self._send_message("timeouts", {"type": name, "ms": ms})

`self._marionette._send_message`
Attachment #8809565 - Flags: review?(dburns) → review+
Comment on attachment 8809561 [details]
Bug 1316622 - Rename Marionette command timeouts to setTimeouts;

https://reviewboard.mozilla.org/r/92098/#review93998
Attachment #8809561 - Flags: review?(dburns) → review+
Comment on attachment 8809566 [details]
Bug 1316622 - Deprecate old Marionette timeouts API;

https://reviewboard.mozilla.org/r/92108/#review94000
Attachment #8809566 - Flags: review?(dburns) → review+
Comment on attachment 8809572 [details]
Bug 1316622 - Correct Marionette timeouts tests;

https://reviewboard.mozilla.org/r/92120/#review94002
Attachment #8809572 - Flags: review?(dburns) → review+
Comment on attachment 8811293 [details]
Bug 1316622 - Remove wait utility dependency on Marionette default timeout;

https://reviewboard.mozilla.org/r/93446/#review94012
Attachment #8811293 - Flags: review?(hskupin) → review+
Comment on attachment 8809564 [details]
Bug 1316622 - Add timeout reset API to Marionette Python client;

https://reviewboard.mozilla.org/r/92104/#review94018

I would also like to see a review request for David for the actual API changes. Otherwise please see my inline comments.

::: testing/marionette/client/marionette_driver/marionette.py:548
(Diff revision 4)
>      DEFAULT_SOCKET_TIMEOUT = 60
>      DEFAULT_STARTUP_TIMEOUT = 120
>      DEFAULT_SHUTDOWN_TIMEOUT = 65  # Firefox will kill hanging threads after 60s
>  
>      def __init__(self, host="localhost", port=2828, app=None, bin=None,
>                   baseurl=None, timeout=None, socket_timeout=DEFAULT_SOCKET_TIMEOUT,

You also want to remove the parameter from the argument list.

::: testing/marionette/client/marionette_driver/marionette.py
(Diff revision 4)
> -
> -        if self.default_timeouts is not None:
> -            for typ, ms in self.default_timeouts:
> -                setattr(self.timeout, setters[typ], ms)
> -        else:
> -            self.timeout.page_load = 30

Was it a bug that we only reset `page_load` in that case? Now that you removed the `default_timeouts` we would always run into this condition and reset them all.

::: testing/marionette/client/marionette_driver/timeout.py:92
(Diff revision 4)
>          self._set("implicit", sec)
> +
> +    def reset(self):
> +        """Resets timeouts to their default values."""
> +        self.script = 30
> +        self.page_load = 300

Hm, from where did you get both values? Isn't the default page_load timeout 30s and script 10s as of now? So this change doesn't look right to me.

Also shouldn't we better define constants at the top of the file?
Attachment #8809564 - Flags: review?(hskupin) → review-
Comment on attachment 8809561 [details]
Bug 1316622 - Rename Marionette command timeouts to setTimeouts;

https://reviewboard.mozilla.org/r/92098/#review94026
Attachment #8809561 - Flags: review?(hskupin) → review+
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review94032
Attachment #8809565 - Flags: review?(hskupin) → review-
Comment on attachment 8809564 [details]
Bug 1316622 - Add timeout reset API to Marionette Python client;

https://reviewboard.mozilla.org/r/92104/#review94018

> Was it a bug that we only reset `page_load` in that case? Now that you removed the `default_timeouts` we would always run into this condition and reset them all.

Indeed, but in practice nothing disasterous happened in the try run.

Whether it was a bug depends on how you read the API documentation.  I would cetainly have expected it to reset everything to their defaults if no override was given.

> Hm, from where did you get both values? Isn't the default page_load timeout 30s and script 10s as of now? So this change doesn't look right to me.
> 
> Also shouldn't we better define constants at the top of the file?

These are the Marionette defaults that you get when you start a new session.  The Python client reset it to 30 when `reset_timeouts` was called.
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review93784

> Just to recap... The former timeout changes via bug 1302707 had to be backed out from formerly beta (50.0) because it caused a regression as tracked on bug 1308453. On the latter bug we got the non-existent backward compatibility added to 51.0, and 52.0. Update tests are using the client version of the target build, which means eg. for a possible future update from 50.0 to 53.0 we use the current client (53), which would not contain the second fallback anymore in case your patch would be landed as it is. As result we would have the breakage because `marionette.restart()` calls `reset_timeouts()`. I hope all my thinking here is correct.

But the `timeouts` command has taken the data structure `{"type": "script", "ms": timeout}` (_not_ `{"implicit": 123}`) since forever.  This input type fallback is still allowed in the recent release you refer to.
Comment on attachment 8809564 [details]
Bug 1316622 - Add timeout reset API to Marionette Python client;

https://reviewboard.mozilla.org/r/92104/#review94018

> Indeed, but in practice nothing disasterous happened in the try run.
> 
> Whether it was a bug depends on how you read the API documentation.  I would cetainly have expected it to reset everything to their defaults if no override was given.

Ok, given that it seems no-one was using the override it should be fine then.

> These are the Marionette defaults that you get when you start a new session.  The Python client reset it to 30 when `reset_timeouts` was called.

I see now that the above values are coming from the webdriver spec. It means that we will introduce different behavior because page load timeout is 30s and script timeout 10s afair. I don't say that's bad. It might even kill some more intermittent failures for page loads which access the network.
Comment on attachment 8809564 [details]
Bug 1316622 - Add timeout reset API to Marionette Python client;

https://reviewboard.mozilla.org/r/92104/#review94220
Attachment #8809564 - Flags: review?(hskupin) → review+
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review93784

> But the `timeouts` command has taken the data structure `{"type": "script", "ms": timeout}` (_not_ `{"implicit": 123}`) since forever.  This input type fallback is still allowed in the recent release you refer to.

Not sure I understand your last comment. The last official release which is 50.0 is using `("timeouts", {"type": "page load", "ms": timeout})` while current beta (51.0), aurora (52.0) and central (53.0) are using `("timeouts", {"page load": timeout})`.

Given that we already shipped the first beta of 51.0 we have to take that interim format into account, and carry it along until the 55.0 release too.
Attachment #8809570 - Attachment is obsolete: true
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review93784

> Not sure I understand your last comment. The last official release which is 50.0 is using `("timeouts", {"type": "page load", "ms": timeout})` while current beta (51.0), aurora (52.0) and central (53.0) are using `("timeouts", {"page load": timeout})`.
> 
> Given that we already shipped the first beta of 51.0 we have to take that interim format into account, and carry it along until the 55.0 release too.

The current beta (51), aurora (52), and central (53) all still support the input format `{type: "page load": ms: timeout}` as a fallback.  This means we can fall back to send this as input when the `setTimeouts` command returns the unknown command error.  Or to rephrase this slightly: there are no version of Firefox that _do not_ support `{type: "page load", ms: timeout}` as input, but only some versions support `setTimeouts`.
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review93784

> The current beta (51), aurora (52), and central (53) all still support the input format `{type: "page load": ms: timeout}` as a fallback.  This means we can fall back to send this as input when the `setTimeouts` command returns the unknown command error.  Or to rephrase this slightly: there are no version of Firefox that _do not_ support `{type: "page load", ms: timeout}` as input, but only some versions support `setTimeouts`.

If that is really the case I'm fine with it. I would suggest that you pick the 50.0 release and run at least one restart test against your modifications for mozilla-central. You can use the fx-ui-tests/functional/security/test_safebrowsing_initial_download.py. If that passes lets go on.
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review93784

> If that is really the case I'm fine with it. I would suggest that you pick the 50.0 release and run at least one restart test against your modifications for mozilla-central. You can use the fx-ui-tests/functional/security/test_safebrowsing_initial_download.py. If that passes lets go on.

Seems to work fine in Firefox 50:

```
% ./mach python
Python 2.7.9 (default, Jun 29 2016, 13:08:31) 
[GCC 4.9.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from marionette_driver.marionette import Marionette
>>> m = Marionette(bin="/home/ato/firefox-50/firefox")
>>> m.start_session()
{u'rotatable': False, u'raisesAccessibilityExceptions': False, u'takesScreenshot': True, u'acceptSslCerts': False, u'appBuildId': u'20161104212021', u'XULappId': u'{ec8030f7-c20a-464f-9b0e-13a3a9e97384}', u'processId': 9295, u'browserVersion': u'50.0', u'specificationLevel': 0, u'platform': u'LINUX', u'browserName': u'firefox', u'version': u'50.0', u'proxy': {}, u'platformVersion': u'3.16.0-4-amd64', u'takesElementScreenshot': True, u'platformName': u'linux', u'command_id': 1}
>>> m.timeout.reset()
```
Comment on attachment 8809565 [details]
Bug 1316622 - New timeouts inteface in Marionette Python client;

https://reviewboard.mozilla.org/r/92106/#review93784

> Seems to work fine in Firefox 50:
> 
> ```
> % ./mach python
> Python 2.7.9 (default, Jun 29 2016, 13:08:31) 
> [GCC 4.9.2] on linux2
> Type "help", "copyright", "credits" or "license" for more information.
> >>> from marionette_driver.marionette import Marionette
> >>> m = Marionette(bin="/home/ato/firefox-50/firefox")
> >>> m.start_session()
> {u'rotatable': False, u'raisesAccessibilityExceptions': False, u'takesScreenshot': True, u'acceptSslCerts': False, u'appBuildId': u'20161104212021', u'XULappId': u'{ec8030f7-c20a-464f-9b0e-13a3a9e97384}', u'processId': 9295, u'browserVersion': u'50.0', u'specificationLevel': 0, u'platform': u'LINUX', u'browserName': u'firefox', u'version': u'50.0', u'proxy': {}, u'platformVersion': u'3.16.0-4-amd64', u'takesElementScreenshot': True, u'platformName': u'linux', u'command_id': 1}
> >>> m.timeout.reset()
> ```

Greta that it works this way! Thanks for testing.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b77e4d6a3603
Rename Marionette command timeouts to setTimeouts; r=automatedtester,whimboo
https://hg.mozilla.org/integration/autoland/rev/b25048eb4ec1
Make implicit wait timeout default to 0; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/57c2be9dcffc
Move internal interface construction last; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/56b7e43f9d5c
New timeouts inteface in Marionette Python client; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/3aedb0ff7fe7
Deprecate old Marionette timeouts API; r=automatedtester,whimboo
https://hg.mozilla.org/integration/autoland/rev/97f78abeb92c
Move Marionette unit tests to new timeouts API; r=automatedtester,whimboo
https://hg.mozilla.org/integration/autoland/rev/e3acbc950e1c
Move Marionette harness tests to new timeouts API; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/509ad050bea4
Move microformats Marionete tests to new timeouts API; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/27bf0458f5cf
Move wptrunner to new Marionette timeouts API; r=jgraham
https://hg.mozilla.org/integration/autoland/rev/dcf546e59c97
Correct Marionette timeouts tests; r=automatedtester,whimboo
https://hg.mozilla.org/integration/autoland/rev/48ed544b55ea
Remove wait utility dependency on Marionette default timeout; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/4b7234a86ad9
Add timeout reset API to Marionette Python client; r=automatedtester,whimboo
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
Sheriffs: Test-only uplift as Marionette is hidden behind flag.
has problems to apply to beta:

warning: conflicts while merging testing/web-platform/harness/wptrunner/executors/executormarionette.py! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(ato)
Whiteboard: [checkin-needed-beta]
Thanks :Tomcat, we’re probably missing some dependency.  I don’t care enough to work out what it is.
Flags: needinfo?(ato)
Attachment #8809565 - Flags: review?(hskupin)
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: