Last Comment Bug 757916 - Mozmill test for quitting the application
: Mozmill test for quitting the application
Status: RESOLVED FIXED
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Alex Lakatos[:AlexLakatos]
:
:
Mentors:
https://moztrap.mozilla.org/manage/ca...
Depends on: 779533
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-23 11:06 PDT by Alex Lakatos[:AlexLakatos]
Modified: 2012-08-15 16:11 PDT (History)
5 users (show)
anthony.s.hughes: in‑moztrap+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
patch v1.0 (1.28 KB, patch)
2012-05-23 11:09 PDT, Alex Lakatos[:AlexLakatos]
dave.hunt: review-
Details | Diff | Splinter Review
patch v2.0 (1.63 KB, patch)
2012-05-24 05:11 PDT, Alex Lakatos[:AlexLakatos]
dave.hunt: review-
Details | Diff | Splinter Review
patch v3.0 (2.37 KB, patch)
2012-07-31 11:38 PDT, Alex Lakatos[:AlexLakatos]
hskupin: review-
Details | Diff | Splinter Review
logfile (100.33 KB, text/x-log)
2012-08-01 06:11 PDT, Alex Lakatos[:AlexLakatos]
no flags Details
traceback (2.02 KB, text/plain)
2012-08-01 06:12 PDT, Alex Lakatos[:AlexLakatos]
no flags Details
minimized testcase (766 bytes, application/zip)
2012-08-01 09:06 PDT, Alex Lakatos[:AlexLakatos]
no flags Details
patch v4.0 (2.26 KB, patch)
2012-08-06 06:08 PDT, Alex Lakatos[:AlexLakatos]
hskupin: review-
Details | Diff | Splinter Review
patch v5.0 (2.26 KB, patch)
2012-08-08 08:43 PDT, Alex Lakatos[:AlexLakatos]
hskupin: review-
Details | Diff | Splinter Review
patch v5.1[default] (2.28 KB, patch)
2012-08-09 07:17 PDT, Alex Lakatos[:AlexLakatos]
hskupin: review+
Details | Diff | Splinter Review
patch v5.1[aurora, beta, release, esr] (2.33 KB, text/plain)
2012-08-09 07:18 PDT, Alex Lakatos[:AlexLakatos]
no flags Details

Description Alex Lakatos[:AlexLakatos] 2012-05-23 11:06:24 PDT
Create a mozmill test for quitting the application
It is needed to provide smoketest coverage.
https://litmus.mozilla.org/show_test.cgi?id=15441
Comment 1 Alex Lakatos[:AlexLakatos] 2012-05-23 11:09:05 PDT
Created attachment 626511 [details] [diff] [review]
patch v1.0

Because the window count is not 0 immediately, did a waitFor instead of an assert. Is that enough?
Comment 2 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-05-24 02:05:27 PDT
Comment on attachment 626511 [details] [diff] [review]
patch v1.0

>+  controller.startUserShutdown(4000, true);
>+  controller.mainMenu.click("#menu_FileQuitItem");
>+  
>+  var windowCount = mozmill.utils.getWindows("navigator:browser").length;
>+  
>+  controller.waitFor(function () {
>+    return !windowCount;
>+  }, "Browser window is closed: got '" + windowCount + "', expected '0'");
>+}

All the window related code you do not need. Mozmill will fail if the browser doesn't go down. Check it by commenting out the menu click.
Comment 3 Dave Hunt (:davehunt) 2012-05-24 04:30:28 PDT
Comment on attachment 626511 [details] [diff] [review]
patch v1.0

Please can you rename the folder to testMenu_quitApplication.

I can also confirm Henrik's comment regarding the window code, so please go ahead and remove it.

Could you also add this test to the tests/functional/restartTests/manifest.ini file.
Comment 4 Alex Lakatos[:AlexLakatos] 2012-05-24 05:11:12 PDT
Created attachment 626761 [details] [diff] [review]
patch v2.0

Renamed the folder. Removed the window code. Added the folder to the manifest.
Comment 5 Dave Hunt (:davehunt) 2012-05-24 06:19:37 PDT
Comment on attachment 626761 [details] [diff] [review]
patch v2.0

This test is affected by bug 747299, where startUserShutdown causes jsbridge port issues. We could skip this test until we have Mozmill 2.0, or just set this bug as blocked by bug 744007.
Comment 6 Alex Lakatos[:AlexLakatos] 2012-05-24 06:41:07 PDT
If the test code is ok, I would opt for skipping it. If you think marking it as blocked and waiting is a better way to go, please do so.
Comment 7 Alex Lakatos[:AlexLakatos] 2012-07-31 11:38:02 PDT
Created attachment 647614 [details] [diff] [review]
patch v3.0

Added moztrap id. This still fails with "can't connect to jsbridge". But I think we should still add the test to our repo and skip it, since we need it for the smoketests.
Comment 8 Alex Lakatos[:AlexLakatos] 2012-08-01 02:48:12 PDT
when running with:
> mozmill-restart --show-all -t tests/functional/restartTests/ -b ../../../default/firefox/firefox -l logfile.log
I get an error:
> Exception: Sorry, cannot connect to jsbridge extension, port 24242
Log file and traceback: https://gist.github.com/3225507
Comment 9 Dave Hunt (:davehunt) 2012-08-01 06:02:50 PDT
Please include details within the bug, either as a comment or attachment.
Comment 10 Alex Lakatos[:AlexLakatos] 2012-08-01 06:11:59 PDT
Created attachment 647925 [details]
logfile

Log file
Comment 11 Alex Lakatos[:AlexLakatos] 2012-08-01 06:12:46 PDT
Created attachment 647926 [details]
traceback

traceback from the terminal. Mozmill version is 1.5.17
Comment 12 Dave Hunt (:davehunt) 2012-08-01 08:56:06 PDT
I have been able to replicate this. Please can you produce a simplified test case and raise a bug against Mozmill?
Comment 13 Alex Lakatos[:AlexLakatos] 2012-08-01 09:06:04 PDT
Created attachment 647970 [details]
minimized testcase

A minimized testcase. Run the minimizedTestcase folder with mozmill-restart
Comment 14 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-06 03:56:40 PDT
Comment on attachment 647970 [details]
minimized testcase

This testcase is invalid because it claims that quit will restart the browser which in fact does not happen.
Comment 15 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-06 04:00:03 PDT
Comment on attachment 647614 [details] [diff] [review]
patch v3.0

>+[include:testMenu_quitApplication/manifest.ini]

Do we have a 'Menu' subgroup on MozTrap now? If not we should figure out with the QA team if subgroups are planned for better organizing of tests.

>+// Include required modules
>+var tabs = require("../../../../lib/tabs");
>+
>+function setupModule() {
>+  controller = mozmill.getBrowserController();
>+  tabs.closeAllTabs(controller);
>+}

No need to close all tabs. We do not interact with content at all, so just remove that.

>+function testQuitApplication() {
>+  controller.startUserShutdown(4000, true);
>+  controller.mainMenu.click("#menu_FileQuitItem");

This does not restart the browser! You want to pass in false to startUserShutdown.

>+// testQuitApplication.meta = {moztrap_testid: 333};

In moztrap we have 'cases' so it should be 'moztrap_case'.
Comment 16 Dave Hunt (:davehunt) 2012-08-06 04:08:03 PDT
(In reply to Henrik Skupin (:whimboo) from comment #15)
> >+function testQuitApplication() {
> >+  controller.startUserShutdown(4000, true);
> >+  controller.mainMenu.click("#menu_FileQuitItem");

I misunderstood the restart boolean as a direction for Mozmill to restart the application. You're saying that it is an indication of whether the upcoming shutdown will itself cause a restart? If so, that makes a lot of sense and clears up my confusion - thanks!
Comment 17 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-06 04:10:07 PDT
If more tests have to be run, Mozmill will automatically start a new instance of Firefox. So this flag clearly states if Firefox get shutdown or restarted by the user action.
Comment 18 Alex Lakatos[:AlexLakatos] 2012-08-06 06:08:24 PDT
Created attachment 649247 [details] [diff] [review]
patch v4.0

(In reply to Henrik Skupin (:whimboo) from comment #15)
> Comment on attachment 647614 [details] [diff] [review]
> patch v3.0
> >+function testQuitApplication() {
> >+  controller.startUserShutdown(4000, true);
> >+  controller.mainMenu.click("#menu_FileQuitItem");
> 
> This does not restart the browser! You want to pass in false to
> startUserShutdown.
Even passing false I get the same error, but only dismissing the additional dialog stating that "Firefox is already running, but is not responding. To open a new window, you must first close the existing Firefox process, or restart your system."

> 
> >+// testQuitApplication.meta = {moztrap_testid: 333};
> 
> In moztrap we have 'cases' so it should be 'moztrap_case'.
I followed your suggestion from bug 758187 comment 36. Can we have a final decision, for consistency, please?
Comment 19 Alex Lakatos[:AlexLakatos] 2012-08-06 06:10:07 PDT
(In reply to Henrik Skupin (:whimboo) from comment #15)
> Comment on attachment 647614 [details] [diff] [review]
> patch v3.0
> 
> >+[include:testMenu_quitApplication/manifest.ini]
> 
> Do we have a 'Menu' subgroup on MozTrap now? If not we should figure out
> with the QA team if subgroups are planned for better organizing of tests.
As I understood from the QA team, there are no more subgroups in moztrap. Adding Ioana for feedback.
Comment 20 Ioana (away) 2012-08-06 06:33:22 PDT
(In reply to Alex Lakatos[:AlexLakatos] from comment #19)
> As I understood from the QA team, there are no more subgroups in moztrap.
> Adding Ioana for feedback.

MozTrap only contains test suites. They contain both attributes of Litmus subgroups and groups. A test suite contains a set of test cases (like the subgroup) and it is added directly to a testrun (like the groups). 

The Litmus tests/groups/subgroups were not ported to MozTrap yet. MozTrap only contains the test suites for feature release in Firefox 15 and later, and a smoke tests test suite used for beta testing.
Comment 21 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-07 06:02:12 PDT
Comment on attachment 649247 [details] [diff] [review]
patch v4.0

So I have re-checked the requirement here and I'm not sure why this test is based on the Litmus test. I thought we always were talking about MozTrap tests when it comes to Smoketests these days. If this test is really for Litmus then please scratch the moztrap meta information.

But what I think and what I get from https://wiki.mozilla.org/QA/Desktop_Firefox/Automation#MozTrap_Smoketests is that this should really be the Moztrap smoketest. So it has to cover steps as given in this case:

https://moztrap.mozilla.org/manage/case/665/

So this test doesn't cover nothing of the manual test. Clearly a r-
Comment 22 Alex Lakatos[:AlexLakatos] 2012-08-07 06:13:22 PDT
(In reply to Henrik Skupin (:whimboo) from comment #21)
> Comment on attachment 649247 [details] [diff] [review]
> patch v4.0
> 
> So I have re-checked the requirement here and I'm not sure why this test is
> based on the Litmus test. I thought we always were talking about MozTrap
> tests when it comes to Smoketests these days. If this test is really for
> Litmus then please scratch the moztrap meta information.
This testcase is for MozTrap, as the metadata indicates. It was logged before smoketests were covered by moztrap, and that's why the first comment has a litmus link.
> 
> But what I think and what I get from
> https://wiki.mozilla.org/QA/Desktop_Firefox/Automation#MozTrap_Smoketests is
> that this should really be the Moztrap smoketest. So it has to cover steps
> as given in this case:
> 
> https://moztrap.mozilla.org/manage/case/665/
This is clearly the wrong link. It's a mistake on the wiki, given that both the bug linked and the text are about quitting the application. Testcase 665 has nothing to with quitting the application.
The correct link there would be https://moztrap.mozilla.org/manage/case/333/, which this test follows.
> So this test doesn't cover nothing of the manual test. Clearly a r-
Do I need to re submit for review?
Comment 23 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-07 06:20:10 PDT
(In reply to Alex Lakatos[:AlexLakatos] from comment #22)
> > https://moztrap.mozilla.org/manage/case/665/
> This is clearly the wrong link. It's a mistake on the wiki, given that both
> the bug linked and the text are about quitting the application. Testcase 665
> has nothing to with quitting the application.

So please update the wiki accordingly. Would be nice if you can also check the other links for smoketests so we can ensure the right ones are referenced.

> > So this test doesn't cover nothing of the manual test. Clearly a r-
> Do I need to re submit for review?

Yes please.
Comment 24 Alex Lakatos[:AlexLakatos] 2012-08-07 09:02:29 PDT
Comment on attachment 649247 [details] [diff] [review]
patch v4.0

(In reply to Henrik Skupin (:whimboo) from comment #23)
> So please update the wiki accordingly. Would be nice if you can also check
> the other links for smoketests so we can ensure the right ones are
> referenced.
Edited the wiki. For some reason almost all links were wrong. Resubmitting for review
Comment 25 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-08 01:19:10 PDT
Short update for the moztrap case id. Please don't add this information to the test but the manifest entry of the test. Here an example:

[testQuitFirefox.js]
moztrap_case = 333
Comment 26 Alex Lakatos[:AlexLakatos] 2012-08-08 01:25:25 PDT
(In reply to Henrik Skupin (:whimboo) from comment #25)
> Short update for the moztrap case id. Please don't add this information to
> the test but the manifest entry of the test. Here an example:
> 
> [testQuitFirefox.js]
> moztrap_case = 333
What happens with restart tests? Those have multiple test files for the same test case.
Comment 27 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-08 01:31:09 PDT
(In reply to Alex Lakatos[:AlexLakatos] from comment #26)
> What happens with restart tests? Those have multiple test files for the same
> test case.

Only for Mozmill 1.5.x. Starting with Mozmill 2.0 those will be a single module. Given that 1.5 doesn't use manifests we will be fine and will update later. So for now if you have multiple files add it multiple times.
Comment 28 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-08 05:23:11 PDT
Comment on attachment 649247 [details] [diff] [review]
patch v4.0

>+[include:testMenu_quitApplication/manifest.ini]

I have to revert my last comments regarding the moztrap case in the manifest. We cannot do this because it can happen that we have to do some computation for it. Cases can be different for different platforms. So lets keep it in the test itself.

>+/**
>+ * Map test functions to moztrap tests
>+ */
>+// testQuitApplication.meta = {moztrap_case: 333};

You do not want to comment this out.
Comment 29 Alex Lakatos[:AlexLakatos] 2012-08-08 08:43:56 PDT
Created attachment 650147 [details] [diff] [review]
patch v5.0

uncommented meta line
Comment 30 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-08 14:02:55 PDT
Comment on attachment 650147 [details] [diff] [review]
patch v5.0

Patch fails to apply for the manifest file.
Comment 31 Alex Lakatos[:AlexLakatos] 2012-08-09 07:17:10 PDT
Created attachment 650542 [details] [diff] [review]
patch v5.1[default]

patch for the default branch. updated commit message for r=hskupin
Comment 32 Alex Lakatos[:AlexLakatos] 2012-08-09 07:18:08 PDT
Created attachment 650544 [details]
patch v5.1[aurora, beta, release, esr]

patch for the aurora, beta, release and esr branches. updated commit message for r=hskupin
Comment 33 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-10 03:34:10 PDT
Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/688ecd2662f8

Given that this test is only covered by MozTrap we will now update the appropriate MozTrap test. Anthony, can you please us the favor and cover that for the first Mozmill test for Moztrap? I know you wanted to move the test from the manual testrun to the automation one. But I miss any other details. So please take care of this update. Thanks!
Comment 34 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-10 03:35:41 PDT
Comment on attachment 650544 [details]
patch v5.1[aurora, beta, release, esr]

No, we never want to backport tests to older branches. That's only an exception for endurance tests.
Comment 35 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-13 09:52:16 PDT
(In reply to Henrik Skupin (:whimboo) from comment #33)
> Pushed to default:
> http://hg.mozilla.org/qa/mozmill-tests/rev/688ecd2662f8
> 
> Anthony, can you please us the favor and cover
> that for the first Mozmill test for Moztrap? 

We don't have a defined process for how we want to "cover" Moztrap tests which have been automated. I think we should try to nail down that process. For now I'll update the Moztrap test to indicate it has automation but I won't remove it from the testrun until we have a better defined process and a way of reporting/linking automation and manual results.

Let's schedule a meeting time to nail down this process.
Comment 36 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 10:51:08 PDT
Forgot to set the flag.
Comment 37 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-15 02:57:56 PDT
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #36)
> Forgot to set the flag.

Well as spoken in yesterdays meeting we want to add the 'in-qa-testsuite' and 'mozmill' tags to the case too. Not sure if I'm querying wrong but I can't find it. Also the link to the Mozmill test doesn't seem to work. Is the markup broken?
Comment 38 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-15 13:59:13 PDT
A couple points here...

1) I only updated the Firefox 17 version of the test since this has only status-firefox17:fixed.
https://moztrap.mozilla.org/manage/caseversion/2992/

2) For the mozmill-tests link in the Moztrap test I did not use any markup, it's just copy and paste. I'm not sure if we can do actual markup at yet.
https://hg.mozilla.org/qa/mozmill-tests/file/tip/tests/functional/restartTests/testMenu_quitApplication/test1.js

3) To query for the test by tag you should be able to find it doing the following:
* Manage > Cases
* Type "mozmill" in the "Manage Test Cases" field
* Select the "mozmill [tag]" line item

Let me know if this doesn't work for you. We should probably take any follow-up to this to email or verbal discussion.
Comment 39 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-15 16:11:04 PDT
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #38)
> 1) I only updated the Firefox 17 version of the test since this has only
> status-firefox17:fixed.
> https://moztrap.mozilla.org/manage/caseversion/2992/

Ah, right. My fault. But one note re: the caseversion. Please don't reference this one. Always reference the case id which would still be 333.

> 2) For the mozmill-tests link in the Moztrap test I did not use any markup,
> it's just copy and paste. I'm not sure if we can do actual markup at yet.
> https://hg.mozilla.org/qa/mozmill-tests/file/tip/tests/functional/
> restartTests/testMenu_quitApplication/test1.js

As Cameron told me it can be done. We can discuss this outside of the bug if there are questions.

Note You need to log in before you can comment on or make changes to this bug.