Closed Bug 1298803 Opened 8 years ago Closed 5 years ago

Add test to quit Firefox via a shortcut

Categories

(Testing :: Firefox UI Tests, defect, P5)

Version 3
defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: whimboo, Unassigned, Mentored)

References

Details

(Whiteboard: [lang=py])

Attachments

(2 files)

Once bug 1298800 got fixed we can add a test which can cover the shortcut for quitting Firefox.
Sanyam, what we would need here is a new test in: https://dxr.mozilla.org/mozilla-central/source/testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py It should retrieve the menu shortcut for Quit from Firefox via the l10n library. For the latter we have already an instance on Puppeteer and you can access it via `self.get_entity()`. Check some of our other functional tests in how to work with that. The DTD entities can be found here: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-menubar.inc?from=browser-menubar.inc I would let you examine further requirements yourself, but if you struggle I'm around for help. So let me know.
Mentor: hskupin
Whiteboard: [lang=py]
Comment on attachment 8797131 [details] Bug 1298803 - Add test to quit Firefox via a shortcut; https://reviewboard.mozilla.org/r/82750/#review82800 ::: testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py:63 (Diff revision 1) > + if self.platform == 'win': > + key = 'quitApplicationCmdWin2.accesskey' > + else: > + key = 'quitApplicationCmdUnix.key' > + > + self.browser.send_shortcut(self.browser.get_entity(key)) Instead of sending the shortcut you should use the `self.marionette.quit()` method with a callback. Only with this method we can ensure to safely do the quit, and especially let Marionette client know about an expected quit. As such the above test should cause a `socket.error`. Doesn't it for you?
Attachment #8797131 - Flags: review?(hskupin) → review-
Comment on attachment 8797131 [details] Bug 1298803 - Add test to quit Firefox via a shortcut; https://reviewboard.mozilla.org/r/82750/#review83758 As discussed yesterday, the patch is not ready for a review yet. Some problems need to be solved first, which we can continue to do so via IRC.
Attachment #8797131 - Flags: review?(hskupin)
Sanyam, you can continue on this bug now given that the dependency has been fixed. Try to make it work with the menu entry first. Once it works we can check what has to be fixed for the shortcut.
Sanyam, do you have an update for us? Thanks
Flags: needinfo?(sanyam.khurana01)
Hey, is this still being worked on :) ?
It looks like that Sanyam is no longer with us. So feel free to pick-up the work on this bug. Adrian, given that you are new I would assume that there are a couple of questions. Don't hesitate to ask them here or on irc.mozilla.org/#automation. Details about our tests you can find here: https://developer.mozilla.org/en-US/docs/Mozilla/QA/firefox-ui-tests
Flags: needinfo?(sanyam.khurana01)
Hi, I'm in half way here. I'll be working on this. Will it be okay?
I have had a quick look at the patch and tested it myself. I can indeed reproduce this issue on my Mac with Firefox not shutting down after sending the Cmd+Q shortcut to the documentElement of the browser window. Interestingly this seem to be a general issue for entries under the menu with the program name. All other menus in the menubar work fine. I wonder if this is somewhat under OS control and we cannot reach it with sending a shortcut? Maybe the test will work for Unix and Linux, which I cannot test myself now. Markus, do you have an idea how we can get such shortcuts working under OS X? Where would we have to send those to? Thanks.
Flags: needinfo?(mstange)
Yes, I think it's the system that handles those shortcut keys. I don't know if there is a way to simulate keys at the level where the system would pick them up. I could try to figure it out but it would take me a long time and probably not be worth the effort.
Flags: needinfo?(mstange)
Ok, thank you Markus. Indeed its nothing important for us to see this working on OS X then. We can simply skip the test on that platform. Otherwise I suggest we get it finished and tested on Linux and Windows. There it should perfectly work.
Assignee: nobody → sanyam.khurana01
Status: NEW → ASSIGNED
Comment on attachment 8797131 [details] Bug 1298803 - Add test to quit Firefox via a shortcut; https://reviewboard.mozilla.org/r/82750/#review100200 As we noticed this doesn't work on OS X but lets get it done for Linux and Windows. Please see my inline comments for necessary fixes. Let me know if something is unclear to you. Thanks! ::: testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py:7 (Diff revision 3) > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > from firefox_puppeteer import PuppeteerMixin > from marionette import MarionetteTestCase > -from marionette_driver import Wait > +from marionette_driver import Wait, By Please keep the alphabetical order of imported items. ::: testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py:9 (Diff revision 3) > > from firefox_puppeteer import PuppeteerMixin > from marionette import MarionetteTestCase > -from marionette_driver import Wait > +from marionette_driver import Wait, By > > +from marionette_driver.keys import Keys There is no need for an empty line above here. ::: testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py:59 (Diff revision 3) > return selection_name == "input" > > Wait(self.marionette).until(has_input_selected) > + > + > + def test_quit_firefox_shortcut(self): Please only use a single empty line as delimiter between functions. See PEP8 for details. ::: testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py:61 (Diff revision 3) > Wait(self.marionette).until(has_input_selected) > + > + > + def test_quit_firefox_shortcut(self): > + > + def quit_via_shortcut_callback(): No need for the empty line above. ::: testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py:69 (Diff revision 3) > + else: > + key = 'quitApplicationCmdUnix.key' > + > + root_element = self.marionette.find_element(By.CSS_SELECTOR, ":root") > + > + root_element.send_keys(Keys.META, self.browser.get_entity(key)) Please make sure that you are using self.browser.send_shortcut() here. ::: testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py:72 (Diff revision 3) > + root_element = self.marionette.find_element(By.CSS_SELECTOR, ":root") > + > + root_element.send_keys(Keys.META, self.browser.get_entity(key)) > + > + self.marionette.quit(in_app=True, callback=quit_via_shortcut_callback) > + self.marionette.start_session() Before you start the session please check that it is None. ::: testing/marionette/puppeteer/firefox/firefox_puppeteer/ui/windows.py:417 (Diff revision 3) > > if kwargs[modifier] is True: > keys.append(keymap[modifier]) > > # Bug 1125209 - Only lower-case command keys should be sent > - keys.append(command_key.lower()) > + keys.append(command_key) There is no need for the comment above. Can you please remove it?
Attachment #8797131 - Flags: review?(hskupin) → review-
Sanyam, do you have an update for us? Or can't you work anytime longer on this bug? Thanks
Flags: needinfo?(sanyam.khurana01)
Hi Whimboo! Sorry, I've been busy these days. I will update this by the end of this week.
Flags: needinfo?(sanyam.khurana01)
As it looks like this bug is blocked by bug 1356154. Due to this regression we cannot go forward right now with getting this patch tested and landed. I hope that the regression will be fixed soon.
The reason was not in Marionette but in Puppeteer's restart() method. I will push a fix along with bug 1304656.
Depends on: 1304656
No longer depends on: 1356154
Sanyam, could you please rebase your patch against the latest code on mozilla-central and push an update? I would like to trigger a try build to see how it works. Thanks.
Flags: needinfo?(sanyam.khurana01)
Whimboo I've updated the review request. Please let me know if this needs any more changes.
Flags: needinfo?(sanyam.khurana01)
Comment on attachment 8797131 [details] Bug 1298803 - Add test to quit Firefox via a shortcut; https://reviewboard.mozilla.org/r/82750/#review135706 I have run the patch on try and it was fine on Linux. Sadly we cannot test it on OS X and Windows there yet. As such I run it locally on OS X and found a couple of issues which I pointed out inline. ::: testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py:58 (Diff revision 5) > """) > return selection_name == "input" > > Wait(self.marionette).until(has_input_selected) > + > + def test_quit_firefox_shortcut(self): Please see comment 14 on this bug. We will have to disable/skip this particular test on OS X. ::: testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py:68 (Diff revision 5) > + key = 'quitApplicationCmdUnix.key' > + > + self.browser.send_shortcut(self.browser.localize_entity(key), accel=True) > + > + self.marionette.quit(in_app=True, callback=quit_via_shortcut_callback) > + self.assertEqual(self.marionette.session, None) There is the method `assertIsNone` available which should be used here. ::: testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py:69 (Diff revision 5) > + > + self.browser.send_shortcut(self.browser.localize_entity(key), accel=True) > + > + self.marionette.quit(in_app=True, callback=quit_via_shortcut_callback) > + self.assertEqual(self.marionette.session, None) > + self.marionette.start_session() In case of the above assert failing, this line will not be run. As such Marionette fails in tearDown. I would suggest that you move this test into a separate class in this test file, and specify a custom `tearDown()` method, which restarts the session.
Attachment #8797131 - Flags: review?(hskupin) → review-
Comment on attachment 8797131 [details] Bug 1298803 - Add test to quit Firefox via a shortcut; https://reviewboard.mozilla.org/r/82750/#review141008 ::: testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py:72 (Diff revisions 5 - 6) > > self.browser.send_shortcut(self.browser.localize_entity(key), accel=True) > > + # Skip this test on Mac OS > + if self.puppeteer.platform == 'darwin': > + return Lets not add it via an if condition to the test, but better use a skipIf for the full test. You can import this decorator from `unittest` (https://docs.python.org/2/library/unittest.html#unittest.skipIf). Then just do a `sys.platform == 'darwin'`, and explain in the comment that it's not supported due to the native menu. ::: testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py:77 (Diff revision 6) > + return > + self.marionette.quit(in_app=True, callback=quit_via_shortcut_callback) > + self.assertIsNone(self.marionette.session) > + > + def tearDown(self): > + self.marionette.start_session() You have to make sure to call `tearDown` of the super class. Otherwise the clean-up is not fully done.
Attachment #8797131 - Flags: review?(hskupin) → review-
Comment on attachment 8797131 [details] Bug 1298803 - Add test to quit Firefox via a shortcut; https://reviewboard.mozilla.org/r/82750/#review141532 There are two small things left. Can you please fix those? In parallel I will trigger a try build now for the changes. You can find the results listed inside the mozreview request. ::: testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py:6 (Diff revisions 6 - 7) > # This Source Code Form is subject to the terms of the Mozilla Public > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +import unittest > +import sys Can you please exchange those two lines so we have an alphabetical order? thanks. ::: testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py:81 (Diff revisions 6 - 7) > self.marionette.quit(in_app=True, callback=quit_via_shortcut_callback) > self.assertIsNone(self.marionette.session) > > def tearDown(self): > self.marionette.start_session() > + super(self.__class__, self).tearDown() Better make it explicit by using `super(TestBrowserQuitShortcut, self).tearDown()` here.
Attachment #8797131 - Flags: review?(hskupin) → review-
Comment on attachment 8797131 [details] Bug 1298803 - Add test to quit Firefox via a shortcut; https://reviewboard.mozilla.org/r/82750/#review143350 It looks fine now. Thank you for bearing with all the updates. I will get this new test pushed now.
Attachment #8797131 - Flags: review?(hskupin) → review+
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/99ed53cfebd6 Add test to quit Firefox via a shortcut; r=whimboo
Thanks @Whimboo :)
Sanyam, in case you are interested in something else please let me know and we can find something for you. As best ask on IRC. Thanks.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Sadly I have to request a backout of this patch given that it causes test bustage on Windows: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&filter-searchStr=Firefox%20&filter-tier=1&filter-tier=2&filter-tier=3&bugfiler&selectedJob=100424307 Looks like we request a shutdown but the shortcut is not shutting down Firefox. As such our job hangs and finally gets killed.
Backed out at Henrik's request for causing Windows Marionette hangs. https://hg.mozilla.org/integration/mozilla-inbound/rev/66fd29976718caafa5cad88ebec62b9c06b13500
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla55 → ---
The reason for this seems to be that issuing the shortcut on Windows doesn't close the browser. Sanyam, do you have a Windows machine where you can test it?
Flags: needinfo?(sanyam.khurana01)
Whimboo, I've an old machine that I don't use anymore. I will try to do initial set up on it first and then will test this patch. Setting up the environment would take much time. I will update there once I'm done. Thanks!
Flags: needinfo?(sanyam.khurana01)
Sanyam, if it is taking that much time I could try to check it on one of our automation CI nodes... Just let me know. Thanks.
Henrik, I've been able to set-up the initial dev environment on Windows. I still need to get the latest pull of mozilla-central and then build it. I think you can check it on the one of the automation CI nodes. If however, it needs further inspection, I would start the build and then test the patch.
Sadly I won't have the time the next days. Given that you were able to set everything up, maybe you can have a look first? You would also have to test your changes locally, so it would be beneficial anyway. Thanks.
Sure, I would do it. Thanks!
Ryan, did something went wrong with the backout or merge to m-c? I can still see that the test is executed in our tests even when it shouldn't: https://treeherder.mozilla.org/logviewer.html#?job_id=100912228&repo=mozilla-central&lineNumber=46212 It clearly not existent anymore in the code: https://dxr.mozilla.org/mozilla-central/rev/default/testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py It's puzzling because we run all through the test packages we upload beside the builds.
Flags: needinfo?(ryanvm)
Hm, there was a nightly build still open because of the previous failure. It might be that it was blocking the test folder from being removed and updated. I killed this version of Firefox, and we can see how it looks tomorrow.
Flags: needinfo?(ryanvm)
Henrik, just to update; I'm facing issue with building Firefox on Windows. The build is terminated abruptly each time after running for around 1.5 hours (Did that 4 times). Then when I do ./mach run, it gives me Couldn't load XPCOM error. I'm investigating the issue, and would update soon. Apologies for the late reply.
Oh, you don't have to build Firefox. Sorry, that I didn't mention that before. What you need is just to setup a virtual environment with the necessary Python packages installed: 1. testing/marionette/client 2. testing/marionette/harness 3. testing/firefox-ui/harness In all three cases run `python setup.py develop` within the virtual env, and you will be able to run the `firefox-ui-functional` command right after.
Sanyam, do you have an update for us? Was the last comment helpful for you?
Flags: needinfo?(sanyam.khurana01)
Hi, Henrik! After running the `firefox-ui-functional` command, the both tests fail. Here are the logs: What I get in terminal: https://public.etherpad-mozilla.org/p/firefox-ui-functional-tests gecko.log in verbose mode with -vvv option enabled: https://public.etherpad-mozilla.org/p/gecko.log I get this exception while running the test case: UnknownException: [Exception... "The URI is malformed" nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)" location: "JS frame :: chrome://marionette/content/listener.js
Flags: needinfo?(sanyam.khurana01)
As the log shows you are trying to navigate to a ´null´ URL: > 1502102023242 Marionette TRACE 6 -> [0,48,"get",{"url":null}] Make sure that you pass arguments correctly.
Henrik, But the added test does not have any call to navigate to a URL. I'm not sure what is causing this. Can you please help?
Flags: needinfo?(hskupin)
You are using the puppeteer mixin class which itself makes sure that a test starts with a clean state by opening `about:` in the current tab: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/puppeteer/firefox/firefox_puppeteer/mixins.py#88 When you check this code you will notice that you haven't pulled the latest code for a very long time. So please do so before continuing to work on this bug.
Flags: needinfo?(hskupin)
Sanyam, it's been a while since the last update on this bug. I would like to know if you are still interested to get this patch finished and landed. It was really close... Please let me know. Thanks.
Flags: needinfo?(sanyam.khurana01)
Priority: -- → P5
Oops! somehow, I forgot about it. Extremely sorry. Setting this up right away.
Flags: needinfo?(sanyam.khurana01)
Whimboo, I've setup the repo with the virtualenv (including all dependencies as in https://bugzilla.mozilla.org/show_bug.cgi?id=1298803#c47). While running the `firefox-ui-functional` command for the browser windows shortcut file, I need to supply a binary option which refers to an existing path of the firefox. What exactly should be the version of this Firefox instance & should it be release version, nightly or what?
Flags: needinfo?(hskupin)
If you have a development environment use `mach bootstrap` to setup an Firefox artifact build. Once done run `mach build` to build Firefox. Then you can run the tests via `mach firefox-ui-functional %test_path%`. Mach will figure all out for you. There is no need for a self-created virtualenv anymore. I hope that answers your question.
Flags: needinfo?(hskupin)
Henrik, I was finally able to setup an artifact build and run the test that I wrote. The issue is that `self.puppeteer` instance didn't get any attribute `platform`. I changed that check to check for sys.platform instead and realized that the windows os would be detected as either `win32` or `win64`. Although I fixed that thing, the `quitApplicationCmdWin2.accesskey` doesn't actually quit Firefox and in-turn it is killed by the default 120 second window, after which the test fails.
Flags: needinfo?(hskupin)
(In reply to Sanyam Khurana [:CuriousLearner] from comment #58) > I was finally able to setup an artifact build and run the test that I wrote. > The issue is that `self.puppeteer` instance didn't get any attribute > `platform`. I changed that check to check for sys.platform instead and > realized that the windows os would be detected as either `win32` or `win64`. The platform you can get via marionette now. The puppeteer property has been removed via bug 1364349. See that bug for more details. > Although I fixed that thing, the `quitApplicationCmdWin2.accesskey` doesn't > actually quit Firefox and in-turn it is killed by the default 120 second > window, after which the test fails. On which platform? Also a full trace log would give me a way better idea what's going on. You can use `-vv --gecko-log -` to get that when running the Marionette command.
Flags: needinfo?(hskupin) → needinfo?(sanyam.khurana01)
Flags: needinfo?(sanyam.khurana01)
(In reply to Sanyam Khurana [:CuriousLearner] from comment #61) > Also, here is the test that was run: > https://paste.fedoraproject.org/paste/bYsAQCBEMVPUt6Bu7k3glQ You are setting the `accel` parameter to False, and as such it is not recognized as a command shortcut, and just `x` is getting dispatched. Which indeed will not cause Firefox to shutdown. Here the line from the trace log, which you should always try to analyze if something doesn't work: > 1518451519410 Marionette TRACE 5 -> [0,76,"sendKeysToElement",{"text":"x","id":"0d822281-a89a-4c46-a19f-5541e5a1a831"}] Set `accel` to True, and test it again. Check the trace log for such a line, and compare how different it is. Btw. it would be good if you could upload logs and such as attachments to the bug. Updates for your patch should always be uploaded as a patch to mozreview. Reason mainly is that pastebin implementations not necessarily keep data around forever. It looks like we are close... Thanks.
Hey Henrik, Yeah, I've seen the difference between different access modifiers sent along with the actual key. Thanks. I've just submitted a patch for the same. Please have a look.
Flags: needinfo?(hskupin)
Does the test work now? If yes, you should set me as reviewer inside of mozreview. Otherwise please give more details about your question when setting a ni?
Hey, Yes the patch is working now. I've tested this on Windows machine. Also you're already marked as reviewer in Moz-Review.
Oh, I didn't get a notification about the review because it is already set to r+. That's a little flaw of mozreview. In such a case you want to request an additional review by clicking the details button behind the attachment in the future. I will have a look.
Flags: needinfo?(hskupin)
The code looks still fine, and I triggered another try build. If that passes we can get the patch landed. Thank you for the update!
Sanyam, the try build shows problems on Linux because the used entity cannot be found. Maybe something changed meanwhile. Can you please have a look? Here the link to the failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2d8084ca6f19086d6c9db21986c8c90d2fe43e6&selectedJob=162533621
Flags: needinfo?(sanyam.khurana01)
Attachment #8797131 - Flags: review+ → review-
Hey Whimboo. I fixed the issue and it is up for review on MozReview. Now this patch should also works on Linux. Let me know if you need anything else :)
Flags: needinfo?(sanyam.khurana01) → needinfo?(hskupin)
Btw. there is no need to usually set ni? for me, at least not when you already asked for a review. I will trigger a try build to see how it works.
Flags: needinfo?(hskupin)
Attachment #8797131 - Attachment is obsolete: true
Attachment #8797131 - Attachment is obsolete: false
Comment on attachment 8951842 [details] Bug 1298803 - Add test to quit Firefox via a shortcut; https://reviewboard.mozilla.org/r/221136/#review228208 Please see my inline comment. Also make sure that you combine the commits into a single one. Use `hg commit --amend` when doing updates, or run a `hg histedit` for squashing afterward. ::: testing/firefox-ui/tests/functional/keyboard_shortcuts/test_browser_window.py:72 (Diff revision 1) > def quit_via_shortcut_callback(): > > if self.marionette.session_capabilities['platformName'] == 'windows_nt': > key = 'quitApplicationCmd.key' > else: > - key = 'quitApplicationCmdUnix.key' > + key = 'closeCmd.key' This is not the command to quit Firefox, but to close the currently selected browser window. Please check the following code to find out what's needed: https://dxr.mozilla.org/mozilla-central/rev/3904c3f9314fd040828c5f1cf1fcc86fd8adfe3e/browser/base/content/browser-menubar.inc#97-106
Attachment #8951842 - Flags: review?(hskupin) → review-
Henrik, I've tried `quitApplicationCmd.accesskey` but it doesn't seem to work. Can you please help?
Flags: needinfo?(hskupin)
Henrik, are there any updates here?
Sorry for the delay but I was out most of last week. So accesskey won't work because this is only the underlined letter in the menu for quick access. What you want is more that one: https://dxr.mozilla.org/mozilla-central/rev/3904c3f9314fd040828c5f1cf1fcc86fd8adfe3e/browser/base/content/browser-menubar.inc#107-109
Flags: needinfo?(hskupin)
I tried `key_quitApplication` on Linux but the test fail with Key not found error.
Flags: needinfo?(hskupin)
Hm, I see. So it looks like there is no working shortcut available. It means that you will have to go through the menu instead by retrieving the element with the id `menu_FileQuitItem`, and clicking on it. Maybe this could also work for MacOS.
Flags: needinfo?(hskupin)

I don't think it's worth tracking this test addition anytime longer. Lots of users are most likely using this shortcut on a day by day basis. If there is a need at some time we could still reopen this bug.

Assignee: sanyam.khurana01 → nobody
Status: REOPENED → RESOLVED
Closed: 8 years ago5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: