Closed Bug 1279243 Opened 4 years ago Closed 4 years ago

Add quit_in_app() to marionette driver.

Categories

(Testing :: Marionette, defect, P3)

defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: areinald.bug, Assigned: areinald.bug)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file)

Those methods are needed by session-test in order to examine and/or change the profile between Firefox restarts.
Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

https://reviewboard.mozilla.org/r/58728/#review55666

::: testing/marionette/client/marionette_driver/marionette.py:1008
(Diff revision 1)
>              self.instance.restart(prefs)
>              self.raise_for_port(self.wait_for_port())
>              self.start_session()
>              self._reset_timeouts()
>  
> +    def quit_instance(self):

I think it would be more accurate to call this method `disconnect_instance`, since the gecko instance is not being stopped.

::: testing/marionette/client/marionette_driver/marionette.py:1013
(Diff revision 1)
> +    def quit_instance(self):
> +        """
> +        This will terminate the currently running instance.
> +        """
> +        if not self.instance:
> +            raise errors.MarionetteException("restart can only be called "

Update doc string to replace `restart`

::: testing/marionette/client/marionette_driver/marionette.py:1028
(Diff revision 1)
> +        # The instance is restarting itself; we will no longer be able to
> +        # track it by pid, so mark it as 'detached'.
> +        self.instance.detached = True
> +        self.raise_for_port(self.wait_for_port())
> +
> +    def restart_instance(self):

I'm not sure why you need this in a separate method. Why not just call `start_session` and `_reset_timeouts` directly whereever you need to?

In any case, based on my understanding, this should be called something like `reset_session`, with the docstring updated accordingly. It's not doing anything to an instance.

::: testing/marionette/client/marionette_driver/marionette.py:1058
(Diff revision 1)
> -            # track it by pid, so mark it as 'detached'.
> -            self.instance.detached = True
>          else:
>              self.delete_session()
>              self.instance.restart(clean=clean)
> -        self.raise_for_port(self.wait_for_port())
> +            self.raise_for_port(self.wait_for_port())

Why is this indented into the else block now? Did you mean to just move it into quit_instance()?

Please also edit your commit message to indicate that this is a refactor and explain why the changes are needed.
Attachment #8761612 - Flags: review?(mjzffr) → review-
Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58728/diff/1-2/
Attachment #8761612 - Flags: review- → review?(mjzffr)
https://reviewboard.mozilla.org/r/58728/#review55666

> I think it would be more accurate to call this method `disconnect_instance`, since the gecko instance is not being stopped.

The current restart() method:
1. comments "This will terminate the currently running instance"
2. calls self._send_message("quitApplication", {"flags": restart_flags})

This why I assume the instance is stopped. Am I wrong?

> Update doc string to replace `restart`

Done.

> I'm not sure why you need this in a separate method. Why not just call `start_session` and `_reset_timeouts` directly whereever you need to?
> 
> In any case, based on my understanding, this should be called something like `reset_session`, with the docstring updated accordingly. It's not doing anything to an instance.

Because:
1. I assume _reset_timeouts() is meant to be private.
2. even those 2 lines form a logical sequence which can be factored as they will be called from both restart() and my own code to come.

Again, the comment in restart() states "... and spawn a new instance with the same profile" which is what I want. Is the comment misleading?

> Why is this indented into the else block now? Did you mean to just move it into quit_instance()?

Because self.raise_for_port(self.wait_for_port()) is called from my new quit_instance() method called in the "then block", so now it should only be called in the "else block".

Done.
https://reviewboard.mozilla.org/r/58728/#review55666

> The current restart() method:
> 1. comments "This will terminate the currently running instance"
> 2. calls self._send_message("quitApplication", {"flags": restart_flags})
> 
> This why I assume the instance is stopped. Am I wrong?

My mistake, it is being stopped, but the name and docstring still don't reflect the behaviour. The instance is restarted (see `restart_flags`) from within the browser, so a name like `restart_in_app` is more accurate.

> Because:
> 1. I assume _reset_timeouts() is meant to be private.
> 2. even those 2 lines form a logical sequence which can be factored as they will be called from both restart() and my own code to come.
> 
> Again, the comment in restart() states "... and spawn a new instance with the same profile" which is what I want. Is the comment misleading?

I think it's better to just use `_reset_timeouts()` directly -- go ahead and rename it to `reset_timeouts()`; it's ok if it's non-private.

(Either way, yes, the name `restart_instance` and its docstring are misleading because the instance is not restarted by this new method. The comment in `restart()` about spawing a new instance refers to `self.instance.restart(...)` and `self._send_message("quit...")`)
Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

https://reviewboard.mozilla.org/r/58728/#review57846

In the future, I suggest submitting changes like this as part of a patch series for the relevant session-test work so that we can understand/discuss the context of the refactoring.
Attachment #8761612 - Flags: review?(mjzffr) → review-
https://reviewboard.mozilla.org/r/58728/#review55666

> My mistake, it is being stopped, but the name and docstring still don't reflect the behaviour. The instance is restarted (see `restart_flags`) from within the browser, so a name like `restart_in_app` is more accurate.

What does "restarted from within the browser" mean? Isn't it like the user manually quitting the browser from the menu? Just wanted to make sure as this is what I need.

> I think it's better to just use `_reset_timeouts()` directly -- go ahead and rename it to `reset_timeouts()`; it's ok if it's non-private.
> 
> (Either way, yes, the name `restart_instance` and its docstring are misleading because the instance is not restarted by this new method. The comment in `restart()` about spawing a new instance refers to `self.instance.restart(...)` and `self._send_message("quit...")`)

There is another reason to add the "restart_instance" method: I need to restart the session with self.session_id. Should I also use this field outside of the object?
https://reviewboard.mozilla.org/r/58728/#review57846

I actually have a bunch of interdependent bugs for this session-test work, each having one or more patches. How does the "patch series" work in this configuration?
https://reviewboard.mozilla.org/r/58728/#review57846

Here's an example of a patch series where some small changes in mozrunner are grouped under a bug for Marionette: https://reviewboard.mozilla.org/r/61358/ Whether patches go in the same bug or not is fluid: I try to think of it in terms of what would make sense to land independently and what can be understood by a reviewer without extra context.
In that example, when adding something substantial to mozrunner was needed as a dependency, that work went under a separate bug: https://reviewboard.mozilla.org/r/58256/
https://reviewboard.mozilla.org/r/58728/#review55666

> What does "restarted from within the browser" mean? Isn't it like the user manually quitting the browser from the menu? Just wanted to make sure as this is what I need.

Yes, when user quits manually, `nsiAppStartup.quit()` is called (but without restart flags). In general, "from within the browser" means it's restarted via a chrome-level JavaScript call (See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIAppStartup#quit() and https://dxr.mozilla.org/mozilla-central/rev/63cc31d6cc1c8089590461016ce0b4a2fb77ecbc/testing/marionette/driver.js#2576-2591) instead of being restarted externally by mozrunner's ProcessHandler.

> There is another reason to add the "restart_instance" method: I need to restart the session with self.session_id. Should I also use this field outside of the object?

I'm ok with using self.session_id outside of the object.
Summary: Add quit_instance() and restart_instance() to marionette driver. Use those methods to implement restart(). → Add quit_in_app() to marionette driver.
Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58728/diff/2-3/
Attachment #8761612 - Attachment description: Bug 1279243 - Add quit_instance() and restart_instance() to marionette driver. Use those methods to implement restart(); → Bug 1279243 - Add quit_in_app() to marionette driver;
Attachment #8761612 - Flags: review- → review?(mjzffr)
Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

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

> Yes, when user quits manually, `nsiAppStartup.quit()` is called (but without restart flags). In general, "from within the browser" means it's restarted via a chrome-level JavaScript call (See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIAppStartup#quit() and https://dxr.mozilla.org/mozilla-central/rev/63cc31d6cc1c8089590461016ce0b4a2fb77ecbc/testing/marionette/driver.js#2576-2591) instead of being restarted externally by mozrunner's ProcessHandler.

Removed restart_instance() method, and will make the 2 calls from outside the class.
Renamed quit_instance() to quit_in_app().

> I'm ok with using self.session_id outside of the object.

Ok, then the method restart_instance() is supressed and the bug title was changed.
https://reviewboard.mozilla.org/r/58728/#review57846

Right, that will make sense for future patches which belong to meta-bug 1247589. I'll ask you how to achieve that when time comes.
Priority: -- → P3
Keywords: checkin-needed
has problems to apply:

Hunk #3 FAILED at 1039
1 out of 3 hunks FAILED -- saving rejects to file testing/marionette/client/marionette_driver/marionette.py.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue
Flags: needinfo?(areinald)
Keywords: checkin-needed
Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58728/diff/4-5/
Rebased latest patch to central.
Testing before asking for checkin again.
Flags: needinfo?(areinald)
Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58728/diff/5-6/
Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58728/diff/6-7/
Rebased on feaaf1af1065.
Works as expected.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb2126f4034
Add quit_in_app() to marionette driver. r=maja_zf
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2bb2126f4034
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Component: Telemetry → Marionette
Product: Toolkit → Testing
Hm, I wasn't aware of this bug and had a look at the code changes now. Some parts feel wrong to me. I also miss a unit test for the new feature. So I'm going ahead and file a new bug for getting this more streamlined.
Blocks: 1294403
You need to log in before you can comment on or make changes to this bug.