Closed Bug 1296597 Opened 8 years ago Closed 8 years ago

Allow Marionette to quit a running Firefox instance

Categories

(Remote Protocol :: Marionette, defect, P3)

defect

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(1 file)

The method quit_in_app() which has been brought to us via bug 1279243 doesn't look that it is working as expected. When looking at the code I would expect a constant hang due to the call to wait_for_port(). At this time Firefox does not run, and Marionette is blocked launching a new instance. Also quit_in_app() should not pre-define the eRestart flag, which is only for restarts but not when we want to quit Firefox only.
Just to add, I had a Vidyo call with Andre today and we figured out those and other issues.
Bug 1279243 was a refactor, but I now think quit_in_app should have been called restart_in_app all along. The restart flag is there intentionally.
Andre don't want a restart method. The in_app one was already working before. What he needs is a way to really quit firefox from inside the application, which means without the 'eRestart' flag. If you remove it you will see that it hangs in wait_for_port(). I should have a working patch on Monday maybe already tests included.
Summary: Fix issues with quit_in_app() → Allow Marionette to quit a running application instance.
Summary: Allow Marionette to quit a running application instance. → Allow Marionette to quit a running application instance
Attachment #8784409 - Flags: review?(dburns)
Andre, the attached patch should completely unblock you for the telemetry tests. Even we cannot instruct Marionette from not launching Firefox at the beginning yet, quit() can be called as first command in the test. This will allow the modification of the profile afterward. Another call to start_session() will launch a new instance of Firefox. If there is anything else please let me know and maybe better add it to bug 1294403.
Comment on attachment 8784409 [details] Bug 1296597 - Allow Marionette to quit a running application instance. https://reviewboard.mozilla.org/r/73882/#review72492 ::: testing/marionette/client/marionette_driver/marionette.py:1065 (Diff revision 2) > - "eRestart", > - ] > - self._send_message("quitApplication", {"flags": restart_flags}) > - self.client.close() > > - try: > + self.reset_timeouts() Why are you resetting the timeouts here? ::: testing/marionette/harness/marionette/tests/unit/unit-tests.ini:111 (Diff revision 2) > [test_execute_isolate.py] > [test_click_scrolling.py] > [test_profile_management.py] > skip-if = buildapp == 'b2g' > +[test_quit_restart.py] > +skip-if = buildapp == 'b2g' can you also skip if mobile
Blocks: 1298800
Comment on attachment 8784409 [details] Bug 1296597 - Allow Marionette to quit a running application instance. https://reviewboard.mozilla.org/r/73882/#review72492 > Why are you resetting the timeouts here? We also reset the timeouts in restart() and that was the case from the very beginning as it looks like. If this is not wanted let me know and I can file a new bug to change that. I feel a bit nervous doing it here in this patch. > can you also skip if mobile I don't see why this should be necessary. All of our restart tests as formerly part of test_profile_management.py were not marked as skip by Maja for Fennec: https://hg.mozilla.org/integration/autoland/file/tip/testing/marionette/harness/marionette/tests/unit/unit-tests.ini#l113 Anyway I triggered another try build specifically for Fennec. So lets see how it works for quit: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b136a1db5810
Comment on attachment 8784409 [details] Bug 1296597 - Allow Marionette to quit a running application instance. https://reviewboard.mozilla.org/r/73882/#review72492 > I don't see why this should be necessary. All of our restart tests as formerly part of test_profile_management.py were not marked as skip by Maja for Fennec: https://hg.mozilla.org/integration/autoland/file/tip/testing/marionette/harness/marionette/tests/unit/unit-tests.ini#l113 > > Anyway I triggered another try build specifically for Fennec. So lets see how it works for quit: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=b136a1db5810 FWIW, I wasn't sure about the status of test_profile_management.py for Fennec, but it does currently fail. I list this test in Bug 1297394 as something to look into. Also, re try, keep in mind that Mn runs on Fennec debug builds only -- the opt build in the try push above is unnecessary.
Actually we have a failure for restarts and quit on Fennec. And this is for the chunk Mn5 now (Mn4 on autoland): https://treeherder.mozilla.org/#/jobs?repo=try&revision=b136a1db5810&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=26514697 > TEST-UNEXPECTED-ERROR | test_quit_restart.py TestQuitRestart.test_force_quit | OSError: [Errno 2] No such file or directory: '/tmp/tmpZ4Hmpt' It looks different to the failure from autoland which says "In app initiated quit only supported in Firefox". Not sure why actually, but I will check. https://treeherder.mozilla.org/logviewer.html#?job_id=2683300&repo=autoland#L1899
So quitApplication in driver.js is blocking a request to quit/restart for Fennec: https://dxr.mozilla.org/mozilla-central/rev/1a5b53a831e5a6c20de1b081c774feb3ff76756c/testing/marionette/driver.js#2568 I don't know why. But as it looks like way more efforts are necessary to get all this working for Fennec. So I would suggest that we move this out to a new bug and I stop running this for Fennec.
Blocks: 1298921
Summary: Allow Marionette to quit a running application instance → Allow Marionette to quit a running Firefox instance
Comment on attachment 8784409 [details] Bug 1296597 - Allow Marionette to quit a running application instance. https://reviewboard.mozilla.org/r/73882/#review72492 > FWIW, I wasn't sure about the status of test_profile_management.py for Fennec, but it does currently fail. I list this test in Bug 1297394 as something to look into. > > Also, re try, keep in mind that Mn runs on Fennec debug builds only -- the opt build in the try push above is unnecessary. I disabled the new test_quit_restart.py test for Fennec for the time being.
David, please let me know what you think about the one outstanding issue. My proposal is to file a new bug to get this behavior changed, which also included restart().
Flags: needinfo?(dburns)
Comment on attachment 8784409 [details] Bug 1296597 - Allow Marionette to quit a running application instance. https://reviewboard.mozilla.org/r/73882/#review73356
Attachment #8784409 - Flags: review?(dburns) → review+
Flags: needinfo?(dburns)
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3b37159b1121 Allow Marionette to quit a running application instance. r=automatedtester
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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: