Mozmill test for quitting the application

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: AlexLakatos, Assigned: AlexLakatos)

Tracking

unspecified
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(firefox17 fixed)

Details

(URL)

Attachments

(4 attachments, 6 obsolete attachments)

(Assignee)

Description

5 years ago
Create a mozmill test for quitting the application
It is needed to provide smoketest coverage.
https://litmus.mozilla.org/show_test.cgi?id=15441
(Assignee)

Comment 1

5 years ago
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?
Assignee: nobody → alex.lakatos
Status: NEW → ASSIGNED
Attachment #626511 - Flags: review?(dave.hunt)
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 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.
Attachment #626511 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 4

5 years ago
Created attachment 626761 [details] [diff] [review]
patch v2.0

Renamed the folder. Removed the window code. Added the folder to the manifest.
Attachment #626511 - Attachment is obsolete: true
Attachment #626761 - Flags: review?(dave.hunt)
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.
Attachment #626761 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
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.
Attachment #626761 - Attachment is obsolete: true
Attachment #647614 - Flags: review?(dave.hunt)
(Assignee)

Comment 8

5 years ago
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
Please include details within the bug, either as a comment or attachment.
(Assignee)

Comment 10

5 years ago
Created attachment 647925 [details]
logfile

Log file
(Assignee)

Comment 11

5 years ago
Created attachment 647926 [details]
traceback

traceback from the terminal. Mozmill version is 1.5.17
I have been able to replicate this. Please can you produce a simplified test case and raise a bug against Mozmill?
(Assignee)

Comment 13

5 years ago
Created attachment 647970 [details]
minimized testcase

A minimized testcase. Run the minimizedTestcase folder with mozmill-restart
(Assignee)

Updated

5 years ago
Depends on: 779533
Attachment #647970 - Attachment mime type: application/zip → text/plain
Attachment #647970 - Attachment mime type: text/plain → application/zip
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.
Attachment #647970 - Attachment is obsolete: true
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'.
Attachment #647614 - Flags: review?(dave.hunt) → review-
(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!
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.
(Assignee)

Comment 18

5 years ago
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?
Attachment #647614 - Attachment is obsolete: true
Attachment #649247 - Flags: review?(hskupin)
(Assignee)

Comment 19

5 years ago
(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

5 years ago
(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 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-
Attachment #649247 - Flags: review?(hskupin) → review-
(Assignee)

Comment 22

5 years ago
(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?
(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.
(Assignee)

Comment 24

5 years ago
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
Attachment #649247 - Flags: review- → review?(hskupin)
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
(Assignee)

Comment 26

5 years ago
(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.
(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 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.
Attachment #649247 - Flags: review?(hskupin) → review-
(Assignee)

Comment 29

5 years ago
Created attachment 650147 [details] [diff] [review]
patch v5.0

uncommented meta line
Attachment #649247 - Attachment is obsolete: true
Attachment #650147 - Flags: review?(hskupin)
Comment on attachment 650147 [details] [diff] [review]
patch v5.0

Patch fails to apply for the manifest file.
Attachment #650147 - Flags: review?(hskupin) → review-
(Assignee)

Comment 31

5 years ago
Created attachment 650542 [details] [diff] [review]
patch v5.1[default]

patch for the default branch. updated commit message for r=hskupin
Attachment #650147 - Attachment is obsolete: true
Attachment #650542 - Flags: review?(hskupin)
(Assignee)

Comment 32

5 years ago
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
Attachment #650544 - Flags: review?(hskupin)
Attachment #650542 - Flags: review?(hskupin) → review+
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!
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox17: --- → fixed
Flags: in-moztrap?(anthony.s.hughes)
Resolution: --- → FIXED
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.
Attachment #650544 - Attachment is patch: false
Attachment #650544 - Flags: review?(hskupin)
(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.
Forgot to set the flag.
Flags: in-moztrap?(anthony.s.hughes) → in-moztrap+
(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?
Flags: in-moztrap+ → in-moztrap?(anthony.s.hughes)
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.
Flags: in-moztrap?(anthony.s.hughes) → in-moztrap+
(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.
You need to log in before you can comment on or make changes to this bug.