Closed Bug 1144075 Opened 9 years ago Closed 2 years ago

marionette.restart() needs parameter for additional restart flags

Categories

(Testing :: Marionette Client and Harness, defect, P3)

defect

Tracking

(firefox97 fixed)

RESOLVED FIXED
97 Branch
Tracking Status
firefox97 --- fixed

People

(Reporter: whimboo, Assigned: whimboo, Mentored)

References

Details

(Keywords: pi-marionette-client, Whiteboard: [lang=py])

User Story

Before starting to work on the bug please get comfortable with Marionette by going through the following document: https://firefox-source-docs.mozilla.org/testing/marionette/marionette/NewContributors.html

Attachments

(2 files, 3 obsolete files)

Right now the marionette.restart() method doesn't accept additional restart flags to be used.

Those will be necessary for the Firefox UI tests when we have to run the change architecture tests for OS X, which flip between 64bit and 32bit mode.
Blocks: 1258982
marionette.restart() does now take additional args.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
No, it does not given the following function signature:

> def restart(self, clean=False, in_app=False, callback=None):

It only allows an in_app restart but that's all. Same applies also to quit.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
OS: All → Unspecified
Priority: -- → P3
Hardware: All → Unspecified
Version: Trunk → unspecified
Here a specific example:

https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/testing/marionette/client/marionette_driver/marionette.py#1126

Specific constants to be used:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIAppStartup#Constants

There is no way yet to specify `eRestartNotSameProfile`. Maybe we should just make it a boolean argument for `restart()`.

When doing that I would also consider to integrate a `safe_mode` argument, which then would allow us to call:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIAppStartup#restartInSafeMode()
I'd like to give it a shot.
Thanks Petru. Let me know if you have questions at any time.
Mentor: hskupin
Status: REOPENED → NEW
User Story: (updated)
Whiteboard: [lang=py]
Hello,
Is this bug still open or has it been fixed? Can I work on it for my first bug?
Hi Franck. So Petru is working on this bug. As I have seen you also asked on another bug, which will be perfect for you.

Hi! I am a new contributor and I would like to know if this bug is still to be fixed? Can I try working on it as my first bug?

Sushmita, you are welcome to work on this bug. See the user story in how to get started.

Hi, Is this bug being worked on? If not i'd like to take a crack on it.

It’s been a while since the last update from the person interested.
Please have a go (-:

Hi guys,

I'd like some pointers/comments on this. Just to be sure I am thinking in the right direction.

Looking at the code I have come up with the following idea.

First, adding the boolean to the method. And secondly, introducing a elif to catch the newly introduced parameter and start the application with a clean profile.

def restart(self, clean=False, eRestartNotSameProfile=False, in_app=False, callback=None):
...
                    self.cleanup()
                    reraise(exc, "Requested restart of the application was aborted", tb)
        elif eRestartNotSameProfile:
            self.delete_session()
            self.instance.restart(clean=True)
            self.raise_for_port(timeout=self.DEFAULT_STARTUP_TIMEOUT)
        else:
            self.delete_session()
            self.instance.restart(clean=clean)
...

This however does not take into effect what would happen if both eRestartNotSameProfile and in_app are True.

So I wouldn't add each of the restart/shutdown flags as separate kwargs for both the restart() and quit() methods. Instead do it like what _request_in_app_shutdown() has. That way we only have to implement it now, and won't have to change it later if other flags are wanted.

Only note that with the safe mode flag set, the JS code of Marionette (driver.js) has to call restartInSafeMode() instead of quit().

Hi,

I just posted this patch, its is no where near complete. I just want to get feedback on the thought process that I am on.

Assignee: nobody → dev
Status: NEW → ASSIGNED

Sebastiaan, you created a new review in Phabricator instead of re-using the former one. Not sure what you did locally but make sure that the link to the Phabricator revision in the commit message will be kept before pushing it out. Thanks.

Attachment #9061237 - Attachment is obsolete: true
Attachment #9061753 - Attachment is obsolete: true
Assignee: u637402 → nobody
Status: ASSIGNED → NEW

As part of our groups H2 goal to learn how the process works for contributing to mozilla-central, I would like to help out with this bug. Also will help with my goal of improving my Python coding skill. I will work through the docs of getting my mozilla-central environment up and running and will let you know if I have any questions.

Assignee: nobody → dkl

Hey David, I was out last week. Given that your comment is 6 days old, I wonder if you hit some blockers in getting the tests run.

Flags: needinfo?(dkl)

No reply from assignee. Moving the bug back into the free to choose bucket list.

Assignee: dkl → nobody
Flags: needinfo?(dkl)

Hello, can I work on this?

Yes, feel free to pick it up, and let us know when you have questions. Therefore you can also join us on Matrix in the #interop channel.

Assignee: nobody → renancleyson.f
Status: NEW → ASSIGNED
Attachment #9196134 - Attachment description: Bug 1144075 - Add support to restart flags on marionette client → Bug 1144075 - Add flags to marionette when shutdown in app

Renan, I'm very sorry for the late reply and that your review request for the patch totally got under the wire! I will make sure to have a look at it early next week when I'm back at work.

Actually I gave an advice within the Phabricator review. So are you still interested to continue on this bug?

Flags: needinfo?(renancleyson.f)
Assignee: renancleyson.f → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(renancleyson.f)
Assignee: nobody → renancleyson.f
Status: NEW → ASSIGNED
Attachment #9196134 - Attachment is obsolete: true
Assignee: renancleyson.f → nobody
Status: ASSIGNED → NEW
Blocks: 1726465
Assignee: nobody → hskupin
Status: NEW → ASSIGNED

Quick update here:

  • The eRestartNotSameProfile flag doesn't exist anymore, and as such isn't needed.
  • The eForceQuit flag is meanwhile handled by Marionette itself and is determined my evaluating the response when notifying the quit-application-requested observer. So I don't think that we should add a force flag to the Marionette client code.
  • Safe Mode isn't handled by a flag but as separate option given that it restarts the browser via Services.startup.restartInSafeMode()
  • The eSilently flag will be added over on bug 1726465 because it needs further updates to the Marionette code.
  • If no flag gets passed in the code in driver.js will now have to automatically fallback to eAttemptQuit.
Attachment #9256232 - Attachment description: Bug 1144075 - [marionette] Allow to restart Firefox in safe mode. → Bug 1144075 - [marionette-client] Allow to restart Firefox in safe mode.
Attachment #9256231 - Attachment description: Bug 1144075 - [marionette] Remove obsolete restart code from before Firefox 55. → Bug 1144075 - [marionette-client] Remove obsolete restart code from before Firefox 55.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8179ad2048b2
[marionette-client] Remove obsolete restart code from before Firefox 55. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/d40a900e78d5
[marionette-client] Allow to restart Firefox in safe mode. r=webdriver-reviewers,jdescottes
Status: ASSIGNED → RESOLVED
Closed: 6 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 97 Branch
Product: Testing → Remote Protocol

Moving bugs for Marionette client due to component changes.

Component: Marionette → Marionette Client and Harness
Product: Remote Protocol → Testing
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: