Closed Bug 1298800 Opened 8 years ago Closed 8 years ago

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

Categories

(Remote Protocol :: Marionette, defect)

Version 3
defect
Not set
normal

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: whimboo, Assigned: CuriousLearner, Mentored)

References

Details

(Whiteboard: [lang=py])

Attachments

(1 file)

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.
Blocks: 1298803
Blocks: 1258982
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.
Mentor: hskupin
Whiteboard: [lang=py]
Flags: needinfo?(sanyam.khurana01)
Hi Whimboo!

Sorry for the late reply, I was having health issues. Sure, I'll like to take this up.
Flags: needinfo?(sanyam.khurana01)
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 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.
Attachment #8791929 - Flags: review?(hskupin)
Assignee: nobody → sanyam.khurana01
Status: NEW → ASSIGNED
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.
Attachment #8791929 - Flags: review?(hskupin) → review+
Sanyam, when you have updated the patch, please check the open issues in mozreview and close them appropriately when you fixed them. Thanks.
@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!
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6957fdb6a712
Add support for callbacks to restart() and quit() methods of Marionette; r=whimboo
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?
Sure @whimboo, I'd love to :)
Blocks: 1304656
https://hg.mozilla.org/mozilla-central/rev/6957fdb6a712
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1309556
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: