Closed Bug 1350897 Opened 7 years ago Closed 7 years ago

Expose quit shutdown/restart cause in Marionette Python client

Categories

(Remote Protocol :: Marionette, enhancement, P3)

Version 3
enhancement

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: ato, Assigned: vedantc98, Mentored)

Details

(Whiteboard: [lang=py])

Attachments

(1 file)

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
Summary: Expose quit shutdown/restart cause in Marionette Python clientF → Expose quit shutdown/restart cause in Marionette Python client
Mentor: hskupin
Whiteboard: [lang=py]
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 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-
Thanks for the feedback, I'll fix the issues and push another patch.
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+
Thank you for the update Vedant! I'm going to push your patch now.
Assignee: nobody → vedantc98
Status: NEW → ASSIGNED
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: