1298800 - Add support for callbacks to restart() and quit() methods of Marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Right now both methods only allow a restart or quit of Firefox by a request via the API. We should also make it possible to trigger such an action via a callback. It would allow tests to specify eg. a click on a menuitem or using shortcut.

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#1061

quit() is about to get added via bug 1296597.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Sanyam, if you are still interested in this bug feel free to pick it up.

Just some updated notes... Both restart() and quit() methods need a new optional argument which defaults to None and accepts a callback. If a callback is given _request_in_app_shutdown() doesn't have to construct the flags and also doesn't have to send the quitApplication message. So add an if condition before which checks for a valid callback reference, which gets executed instead.

We also want to add more unit tests. This can be done in testing/marionette/harness/marionette/tests/unit/test_quit_restart.py. Maybe think about first what you think which tests might be helpful. I will then give feedback and they can get implemented.

Comment 3

[Sanyam Khurana [:CuriousLearner]]

Hi Whimboo!

Sorry for the late reply, I was having health issues. Sure, I'll like to take this up.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

That's great to hear that feel better and are still interested in taking this bug. Let me know if you need some other information. Otherwise I'm eagerly waiting for your patch. Thanks!

Comment 5

[Sanyam Khurana [:CuriousLearner]]

Attachment: Bug 1298800 - Add support for callbacks to restart() and quit() methods of Marionette;

Review commit: https://reviewboard.mozilla.org/r/79198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79198/

Comment 6

[Sanyam Khurana [:CuriousLearner]]

Comment on attachment 8791929 [details]
Bug 1298800 - Add support for callbacks to restart() and quit() methods of Marionette;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79198/diff/1-2/

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 8791929 [details]
Bug 1298800 - Add support for callbacks to restart() and quit() methods of Marionette;

https://reviewboard.mozilla.org/r/79198/#review78124

It's great to see the callback variant implemented! Code-wise it seems to be working fine, but we should do a try run once the issues, which I raised now, have been fixed. Those are mostly styling related.

::: testing/marionette/client/marionette_driver/marionette.py:1059
(Diff revision 2)
>  
>          :param in_app: If True, marionette will cause a quit from within the
>                         browser. Otherwise the browser will be quit immediately
>                         by killing the process.
> +        :param callback: If provided, then the callback would be executed
> +                         instead of _request_in_app_shutdown.

Please re-phrase the sentence to: "If provided, the callback will be used to trigger an ´in_app´ restart."

::: testing/marionette/client/marionette_driver/marionette.py:1068
(Diff revision 2)
>                                               "on Gecko instances launched by Marionette")
>  
>          self.reset_timeouts()
>  
>          if in_app:
> +            if callback:

Lets check that ´callback´ is of type function, so that the call will succeed.

::: testing/marionette/client/marionette_driver/marionette.py:1092
(Diff revision 2)
>                        profile.
>          :param in_app: If True, marionette will cause a restart from within the
>                         browser. Otherwise the browser will be restarted immediately
>                         by killing the process.
> +        :param callback: If provided, then the callback would be executed
> +                         instead of _request_in_app_shutdown.

Same as above.

::: testing/marionette/harness/marionette/tests/unit/test_quit_restart.py:78
(Diff revision 2)
>  
>          self.marionette.start_session()
>          self.assertNotEqual(self.marionette.session_id, self.session_id)
>          self.assertNotEqual(self.marionette.get_pref("browser.startup.page"), 3)
> +
> +    def test_in_app_callback_restart(self):

Please rename the test to ´test_in_app_restart_with_callback()´ and sort it alphabetically with other tests in this class.

::: testing/marionette/harness/marionette/tests/unit/test_quit_restart.py:81
(Diff revision 2)
>          self.assertNotEqual(self.marionette.get_pref("browser.startup.page"), 3)
> +
> +    def test_in_app_callback_restart(self):
> +        self.marionette.restart(in_app=True,
> +                                callback=lambda:
> +                                self.marionette._request_in_app_shutdown(shutdown_flags='eRestart'))

This seems to exceed the maximum allowed line of length so that the linter will fail. So better create a new method.

::: testing/marionette/harness/marionette/tests/unit/test_quit_restart.py:93
(Diff revision 2)
> +            self.assertNotEqual(self.marionette.session["processId"], self.pid)
> +
> +        # If a preference value is not forced, a restart will cause a reset
> +        self.assertNotEqual(self.marionette.get_pref("browser.startup.page"), 3)
> +
> +    def test_in_app_callback_quit(self):

Same comments apply to this test methods.

Comment 8

[Sanyam Khurana [:CuriousLearner]]

Comment on attachment 8791929 [details]
Bug 1298800 - Add support for callbacks to restart() and quit() methods of Marionette;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79198/diff/2-3/

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 8791929 [details]
Bug 1298800 - Add support for callbacks to restart() and quit() methods of Marionette;

https://reviewboard.mozilla.org/r/79198/#review78774

Just some small nits. Otherwise it looks to be complete. While I'm waiting for the remaining update I will already push a try build, so we can see the overall results of this change on all platforms.

::: testing/marionette/client/marionette_driver/marionette.py:1059
(Diff revision 3)
>  
>          :param in_app: If True, marionette will cause a quit from within the
>                         browser. Otherwise the browser will be quit immediately
>                         by killing the process.
> +        :param callback: If provided, the callback will be used to trigger
> +                         an `in_app` shutdown.

I think that I provided the wrong description. When I read it now it sounds like that we would force an in_app shutdown. That should be better:

If provided and `in_app` is True, the callback will be used to trigger the shutdown.

The same we should use for `restart()`.

::: testing/marionette/client/marionette_driver/marionette.py:1068
(Diff revision 3)
>                                               "on Gecko instances launched by Marionette")
>  
>          self.reset_timeouts()
>  
>          if in_app:
> +            if callback and callable(callback):

There is no need to check for `callback` first. `callable()` will return False in case of `None`.

::: testing/marionette/harness/marionette/tests/unit/test_quit_restart.py:69
(Diff revision 3)
>          # If a preference value is not forced, a restart will cause a reset
>          self.assertNotEqual(self.marionette.get_pref("browser.startup.page"), 3)
>  
> -    def test_in_app_clean_restart(self):
> -        with self.assertRaises(ValueError):
> -            self.marionette.restart(in_app=True, clean=True)
> +    def test_in_app_restart_with_callback(self):
> +        def callback():
> +            return self.marionette._request_in_app_shutdown(shutdown_flags='eRestart')

There is no `return` needed here given that `_request_in_app_shutdown()` doesn't return anything too.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Sanyam, when you have updated the patch, please check the open issues in mozreview and close them appropriately when you fixed them. Thanks.

Comment 11

[Sanyam Khurana [:CuriousLearner]]

Comment on attachment 8791929 [details]
Bug 1298800 - Add support for callbacks to restart() and quit() methods of Marionette;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79198/diff/3-4/

Comment 12

[Sanyam Khurana [:CuriousLearner]]

@whimboo sure, I didn't know I had to close them manually. I'll take care from the next time. I've updated the patch and marked the issues as resolved.

Thanks a lot!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6957fdb6a712
Add support for callbacks to restart() and quit() methods of Marionette; r=whimboo

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

The try build which I have triggered earlier was all a success, so with the latest update the patch is fine and I landed it on autoland. 

Thanks a lot for working on it Sanyam! Maybe you want to follow-up with bug 1298803?

Comment 15

[Sanyam Khurana [:CuriousLearner]]

Sure @whimboo, I'd love to :)

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6957fdb6a712