Expose quit shutdown/restart cause in Marionette Python client

RESOLVED FIXED in Firefox 58

Status

Testing
Marionette
P3
normal
RESOLVED FIXED
9 months ago
2 months ago

People

(Reporter: ato, Assigned: vedantc98, Mentored)

Tracking

Version 3
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [lang=py])

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

9 months ago
Following bug 1337743, the Marionette server returns a {"cause": "shutdown"}/{"cause": "restart"} JSON body when the ‘quit’ command is called.  This should be exposed in the client API or tested internally.

Because earlier Firefoxen do not return this JSON body and the client is being used for update tests, the patch for this needs to take into account that servers in earlier Geckos may return empty JSON objects.

This is a follow-up of the code review issue https://reviewboard.mozilla.org/r/123852/?reply_id=164072&reply_type=diff_comments#comment164072.
Priority: -- → P3
(Reporter)

Updated

9 months ago
Summary: Expose quit shutdown/restart cause in Marionette Python clientF → Expose quit shutdown/restart cause in Marionette Python client
Mentor: hskupin@gmail.com
Whiteboard: [lang=py]
Comment hidden (mozreview-request)
Comment on attachment 8912220 [details]
Bug 1350897 - Tested quit shutdown/restart cause in Marionette Python client.

https://reviewboard.mozilla.org/r/183590/#review189304

I put a couple of comments inline. Please have a look at those.

For testing your changes you can use the test file test_quit_restart.py (you might want to remove the skip lines for testing from all in_app test functions). Therefore flip the expected causes in marionette.py to run through and test the opposite. That will give you the feedback that all is working fine.

::: testing/marionette/client/marionette_driver/marionette.py:1084
(Diff revision 1)
>          body = None
>          if len(flags) > 0:
>              body = {"flags": list(flags)}
>  
> -        self._send_message("quitApplication", body)
> +        resp = self._send_message("quitApplication", body)
> +        return resp

You can return the response immediately here without creating this temporary variable.

Also use `key="cause"` here so `_send_message` extracts the status from the response automatically.

Also update the documentation for the return value in the section above the function.

::: testing/marionette/client/marionette_driver/marionette.py:1109
(Diff revision 1)
>              raise errors.MarionetteException("quit() can only be called "
>                                               "on Gecko instances launched by Marionette")
>  
>          if in_app:
>              if callable(callback):
> -                self._send_message("acceptConnections", {"value": False})
> +                causeJSON = self._send_message("acceptConnections", {"value": False})

As discussed on IRC in case of callbacks we don't have that information available. It's up to the consumer to do it correctly.

::: testing/marionette/client/marionette_driver/marionette.py:1115
(Diff revision 1)
>                  callback()
>              else:
> -                self._request_in_app_shutdown()
> +                causeJSON = self._request_in_app_shutdown()
>  
> +            if causeJSON:
> +                if causeJSON["cause"] is not "shutdown":

Please use `cause` as variable name and do the check with the "!=" operator. See https://stackoverflow.com/a/1504742/8592202.

::: testing/marionette/client/marionette_driver/marionette.py:1117
(Diff revision 1)
> -                self._request_in_app_shutdown()
> +                causeJSON = self._request_in_app_shutdown()
>  
> +            if causeJSON:
> +                if causeJSON["cause"] is not "shutdown":
> +                    raise errors.MarionetteException("The Marionette server returned an unexpected "
> +                                                     "cause of shutdown.")

Here we know that this is the `quit` method. As such we can clearly say that the application did not "shutdown". It's also good to put the exact cause into the error string. So something like that would be good: `"Unexpected shutdown reason '{}' for quitting the process".format(cause)`

Further we should move this whole block to the end of the if block, so we can make sure to clean-up the process.

::: testing/marionette/client/marionette_driver/marionette.py:1167
(Diff revision 1)
> -                self._request_in_app_shutdown("eRestart")
> +                causeJSON = self._request_in_app_shutdown("eRestart")
> +
> +            if causeJSON:
> +                if causeJSON["cause"] is not "restart":
> +                    raise errors.MarionetteException("The Marionette server returned an unexpected "
> +                                                     "cause of shutdown.")

Please apply the same changes as mentioned under `quit` here, except the small update for the error message which should refer to `restart`.
Attachment #8912220 - Flags: review?(hskupin) → review-
Comment hidden (mozreview-request)
Comment on attachment 8912220 [details]
Bug 1350897 - Tested quit shutdown/restart cause in Marionette Python client.

https://reviewboard.mozilla.org/r/183590/#review189902

Generally the changes look fine. Lets fix the remaining issues (including linting failures), and we will be fine to land it.

::: testing/marionette/client/marionette_driver/marionette.py:1044
(Diff revision 2)
>          Firefoxen.
>  
>          This method effectively calls `Services.startup.quit` in Gecko.
>          Possible flag values are listed at http://mzl.la/1X0JZsC.
>  
> +        The method returns the cause of shutdown as provided by the

Please check the same file for `:returns:`.

::: testing/marionette/client/marionette_driver/marionette.py:1134
(Diff revision 2)
>  
>          else:
>              self.delete_session(reset_session_id=True)
>              self.instance.close(clean=clean)
>  
> +        #Testing if the correct cause was returned

You might want to check with the linter that no formatting failures exist. Here for example is one, because it requires a space after `#`. Please check with `mach lint testing/marionette`.

Personally I think you can remove this comment because we have all in the exception message.

::: testing/marionette/client/marionette_driver/marionette.py:1136
(Diff revision 2)
>              self.delete_session(reset_session_id=True)
>              self.instance.close(clean=clean)
>  
> +        #Testing if the correct cause was returned
> +        if cause:
> +            if cause != "shutdown":

Lets make it a combined if condition: `if cause not in (None, "shutdown")`.

::: testing/marionette/client/marionette_driver/marionette.py:1190
(Diff revision 2)
>              self.instance.restart(clean=clean)
>              self.raise_for_port(timeout=self.DEFAULT_STARTUP_TIMEOUT)
> +
> +        #Testing if the correct cause was returned
> +        if cause:
> +            if cause != "restart":

Same as above.
Attachment #8912220 - Flags: review?(hskupin) → review-
(Assignee)

Comment 5

2 months ago
Thanks for the feedback, I'll fix the issues and push another patch.
Comment hidden (mozreview-request)
Comment on attachment 8912220 [details]
Bug 1350897 - Tested quit shutdown/restart cause in Marionette Python client.

https://reviewboard.mozilla.org/r/183590/#review190762

Lets just update the comment, and then we seem to be able to land it. I will trigger another try build to ensure that everything is fine.

::: testing/marionette/client/marionette_driver/marionette.py:1051
(Diff revision 3)
>              flags ending with `"Quit"` are present.
>  
>          :throws InvalidArgumentException: If there are multiple
>              `shutdown_flags` ending with `"Quit"`.
>  
> +        :returns: The cause of shutdown provided by the Marionette server.

It's not provided by Marionette server but just relayed. We should simply say: "The cause of shutdown".
Attachment #8912220 - Flags: review?(hskupin) → review+
Comment hidden (mozreview-request)
Thank you for the update Vedant! I'm going to push your patch now.
Assignee: nobody → vedantc98
Status: NEW → ASSIGNED

Comment 10

2 months ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f7995abac9a3
Tested quit shutdown/restart cause in Marionette Python client. r=whimboo
https://hg.mozilla.org/mozilla-central/rev/f7995abac9a3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.