Closed Bug 669600 Opened 10 years ago Closed 9 years ago

Mozmill test for opening Addons Manager via keyboard shortcut

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: vladmaniac, Assigned: vladmaniac)

Details

(Whiteboard: [mozmill-functional][mozmill-aom])

Attachments

(1 file, 6 obsolete files)

Tracking of mozmill test creation for opening addons manager via keyboard shortcut 

See also bug 465090 (landing of this feature) 

Litmus test: https://litmus.mozilla.org/show_test.cgi?id=15619
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/15405049
Attached patch patch v1.0 (obsolete) — Splinter Review
Requesting for f? from Henrik, to see it catched the correct approach on this one.
Assignee: nobody → vlad.maniac
Status: NEW → ASSIGNED
Attachment #544216 - Flags: feedback?(hskupin)
Comment on attachment 544216 [details] [diff] [review]
patch v1.0

>+++ b/tests/functional/testAddonsManager/testOpenClose.js

Please use the name 'testAddons' as sub folder. We want to stay in sync with the subgroups on Litmus.

>+var modalDialog = require("../../../lib/modal-dialog");
>+var utils = require("../../../lib/utils");

Those are not necessary.

>+const TAB_COUNT = 2;
>+const TAB_INDEX = 1;

I don't think there is a need for those constants.

>+function testOpenClose() {
>+  am.open({type: "shortcut"});
>+  controller.waitForPageLoad(TIMEOUT_PAGELOAD);

There is no need to wait for the page being loaded. If the add-ons manager has been opened, that's all what we want to know here.

>+  // Verify if addons manager is opened via keyboard shortcut
>+  controller.assert(function() {
>+    return am.isOpen == true;
>+  }, "Add-on Manager should open - got '" + 
>+        am.isOpen + "', expected 'true'");

That's already getting checked in the API. So no need for an extra assert call.

>+  // Open a new window
>+  controller.click(new elementslib.Elem(controller.menus['file-menu']
>+                                       .menu_newNavigator));
>+  controller.sleep(TIMEOUT_CLICK);

Please use controller.mainMenu here. Also please don't use sleep. Instead use utils.handleWindow to get the controller of the newly opened window. We have to ensure to close it in teardownModule.

>+  // Opening addons manager once again 
>+  am.open({type: "shortcut"});
>+  controller.waitForPageLoad(TIMEOUT_PAGELOAD);

We haven't closed the add-ons manager, so the right tab should automatically be selected. Also no waitForPageLoad call required. Further use am.getTabs() to check that you get only one item back.

>+  // Make sure that we work on the correct window  
>+  var windows = mozmill.utils.getWindows("navigator:browser");

As said above you should now have two controllers. As example see testNewTab.js. Those you can use for an extra test.

>+  controller.assert(function() {
>+    return getTabCount == TAB_COUNT;
>+  }, "There should be 2 opened tabs - got '" + 
>+        (getTabCount == TAB_COUNT) + "', expected 'true'");

Please use the new assert.equal method from the assertions module now.

>+  // Make sure that we focus the correct tab by verifying tab index  
>+  var tabIndex = tabsBrowser.selectedIndex; 
>+
>+  controller.assert(function() {
>+    return tabIndex == TAB_INDEX;
>+  }, "Selected tab should be AOM tab - got '" + 
>+        (tabIndex == TAB_INDEX) + "', expected 'true'"); 

You can use the return value from am.getTabs() here.

>+  // Make sure that we focus the correct tab by veryfing tab Url
>+  var activeTabUrl = controller.tabs.activeTab.location;
>+
>+  utils.assertLoadedUrlEqual(controller, activeTabUrl);

That's not necessary. It's all implicit in getTabs(), which checks for tabs with 'about:addons' in all windows.
Attachment #544216 - Flags: feedback?(hskupin) → feedback-
Whiteboard: [mozmill-addons]
Attached patch patch v1.1 (obsolete) — Splinter Review
Attachment #544216 - Attachment is obsolete: true
Attachment #545161 - Flags: feedback?(anthony.s.hughes)
The patch contains requested changes except: 

* using am.getTabs() does not help me here in any of the situations. 
this gets the list of all tabs with the about:addons url in them. What I need is the tab index of the currently selected tab, and second, i need to get the Url from the currently active tab, and compare it with expected. getTabs() does not help me in this manner. Otherwise, all requested changes are honored. 

Anthony, i will make sure to change the 'am' variable into 'addonsManager' when requesting r?. Right now i need confirmation that this is working as expected. 

Thanks guys!
(In reply to comment #5)
> * using am.getTabs() does not help me here in any of the situations. 
> this gets the list of all tabs with the about:addons url in them. What I
> need is the tab index of the currently selected tab, and second, i need to
> get the Url from the currently active tab, and compare it with expected.
> getTabs() does not help me in this manner. Otherwise, all requested changes
> are honored. 

And that's what you get from getTabs. It's an array of controllers and an index associated to the open tab with the expected URL. And there is no need to compare the URL afterward. That's already covered in unit tests. This function works reliable.
Comment on attachment 545161 [details] [diff] [review]
patch v1.1

>+/**
>+* Tests opening Add-ons Manager via keyboard shortcut 
>+*/
>+function testOpenAomViaShortcut() {

Please call the test function and file "testKeyboardShortcut" -- we can simplify the name since reports include the folder name (ie. testAddonsManager/testKeyboardShortcut.js)

>+  am.open({type: "shortcut"});

Yes, please rename all instances of am to addonsManager.

>+  // Open a new window
>+  controller.mainMenu.click("#menu_newNavigator");

I'm not sure why it's necessary in this test to open a new window. This test should simply attempt to open and close the Add-ons Manager tab in the current window via keyboard shortcut.
Attachment #545161 - Flags: feedback?(anthony.s.hughes) → feedback+
Thanks for f+ Anthony! 

Please check litmus: https://litmus.mozilla.org/show_test.cgi?id=15619 
and bug bug 465090.The test requires more than just opening Aom via shortcut 

"Note: If the Add-ons Manager was already open, no new tab will be created but the existing one will be re-used, even when another window is active."
> "Note: If the Add-ons Manager was already open, no new tab will be created but 
> the existing one will be re-used, even when another window is active."

As I understand this, it means that the Add-ons Manager tab will be opened in a new tab on first launch and the same tab in all following launches. Even if you open a new window, the tab will be opened in the first window.

Also, as I understand the test, it is not the purpose this test to test the Add-ons Manager always opens in the same window/tab. It's simply warning testers so they don't think it's broken behaviour to always open in the original tab instead of the active window.

Of course, we COULD test this behaviour. However, I think that might be out of scope for this test. Henrik, as QA owner for add-ons manager, what do you say about this test?
Whiteboard: [mozmill-addons] → [mozmill-aom]
When testing these corner cases in the mozmill test, i've made use of 
https://bugzilla.mozilla.org/show_bug.cgi?id=465090#c32 

If the requirements have changed, than we should definitely stay focused on 
the real purpose of this test.
(In reply to comment #10)
> When testing these corner cases in the mozmill test, i've made use of 
> https://bugzilla.mozilla.org/show_bug.cgi?id=465090#c32 
> 
> If the requirements have changed, than we should definitely stay focused on 
> the real purpose of this test.

Henrik, as QA Owner for Add-ons Manager, what's your opinion on this matter?
Yes, the tab handling should be well covered by Mochitests. So we don't have to obey the additional notes. Thanks for spotting this Anthony.
Attached patch patch v1.2 (obsolete) — Splinter Review
Simplified patch -- we decided to give up extra - testing 

Right now the test does: 
* open addons manager via keyboard shortcut 
* check that an addons manager instance was successfully opened in a new tab
Attachment #545643 - Flags: review?(anthony.s.hughes)
Comment on attachment 545643 [details] [diff] [review]
patch v1.2

>+++ b/tests/functional/testAddons/testKeyboardShortcut.js

Vlad, can you please ensure to have the group name (what listed as [.*:xyz] in the summary of the litmus test) in the file name, like testManagerKeyboardShortcut.js. That let us group the tests in that folder.

>+const TAB_COUNT = 2;
>+const AOM_TAB_COUNT = 1;

As I have probably said earlier, we do not have to put everything as constants. These are clear checks against a fixed number.

>+  // Check that there are only two opened tabs
>+  var getTabCount = controller.tabs.length; 
>+  
>+  expect.equal(getTabCount, TAB_COUNT, "There are only two opened tabs");

You should do pre-check just for sanity reasons. Finally the lines above can be combined and the message should be made more clear. It does not reflect what you are really looking for.

>+  // Check that an Add-ons Manager instance was opened in a new tab
>+  var addonsManagerTab = addonsManager.getTabs().length;
>+
>+  expect.equal(addonsManagerTab, AOM_TAB_COUNT, 
>+               "Add-ons manager was sucessfully opened in a new tab");

This test is already performed in addonsManager.open.
Attachment #545643 - Flags: feedback-
Attached patch patch v1.3 (obsolete) — Splinter Review
Asking for f? from Henrik before r request
Attachment #545161 - Attachment is obsolete: true
Attachment #545643 - Attachment is obsolete: true
Attachment #545647 - Flags: feedback?(hskupin)
Attachment #545643 - Flags: review?(anthony.s.hughes)
Comment on attachment 545647 [details] [diff] [review]
patch v1.3

>+function testOpenAomViaShortcut() {

Most of the name is covered by the file name, as what Anthony has already been mentioned. Just try to reduce duplication.

>+  // Check that there are two opened tabs
>+  var getTabCount = controller.tabs.length; 
>+  
>+  expect.equal(getTabCount, 2, 
>+               "The Add-ons manager has been opened in a new tab, " +  
>+               "given the number of opened tabs is " + getTabCount);  

The message here can simply be "The Add-ons Manager has been opened in a second tab". There is also no need for the extra variable which you are using here.
Attachment #545647 - Flags: feedback?(hskupin) → feedback-
Hmm...good old hg is playing games with me again. I'll re-submit shortly. Thanks
Attached patch patch v1.4 (obsolete) — Splinter Review
Asking for f? from Henrik
Attachment #545647 - Attachment is obsolete: true
Attachment #545651 - Flags: feedback?(hskupin)
Comment on attachment 545651 [details] [diff] [review]
patch v1.4

Please address all of my review comments.
Attachment #545651 - Flags: feedback?(hskupin) → feedback-
Are you referring to the function name here?
Attached patch patch v1.5 (obsolete) — Splinter Review
Patch with requested changes
Attachment #545651 - Attachment is obsolete: true
Attachment #545687 - Flags: review?(anthony.s.hughes)
Attachment #545687 - Flags: feedback?(hskupin)
Comment on attachment 545687 [details] [diff] [review]
patch v1.5

Just remove the tabsBrowser instantiation and everything looks fine. Thanks.
Attachment #545687 - Flags: feedback?(hskupin) → feedback+
Comment on attachment 545687 [details] [diff] [review]
patch v1.5

Agreed with Henrik's last comment -- tabBrowser is not used in this test so it can be removed from setupModule(). r+ given that change -- please resubmit your patch so I can land it.

Thanks
Attachment #545687 - Flags: review?(anthony.s.hughes) → review+
Added patch with requested changes. Moved out so much from that test, sorry i missed that declaration
Attachment #545687 - Attachment is obsolete: true
Attachment #545868 - Flags: review?(anthony.s.hughes)
Comment on attachment 545868 [details] [diff] [review]
patch v1.6 [checked-in]

Sorry I missed this before, but your assertion only tests that a new tab has been opened. Can you add an assertion to test that the new tab is indeed the Addons Manager tab? Perhaps you can use the tab title.
Attachment #545868 - Flags: review?(anthony.s.hughes) → review-
Please check out Comment 14 . Henrik advises that addonsManager.open() already tests this. 

I did check out for this using getTabs().
Anthony, please see my comment 14. That the right page is opened is tested in the API and existent Mochitests.
(In reply to comment #26)
> Please check out Comment 14 . Henrik advises that addonsManager.open()
> already tests this. 

If that's the case then the assert is not necessary in your test at all. Just checking for a new tab doesn't prove that the addons manager has been opened. On the other hand, all tests should have an assert of some kind. I suppose this is fine as is.

I'm going to have Henrik do a followup review to see what he thinks.
Attachment #545868 - Flags: review?(hskupin)
Comment on attachment 545868 [details] [diff] [review]
patch v1.6 [checked-in]

>+function testKeyboardShortcut() {
>+  addonsManager.open({type: "shortcut"});
>+
>+  // Check that there are two opened tabs
>+  expect.equal(controller.tabs.length, 2, 
>+               "The Add-ons Manager has been opened in a second tab");  
>+}

Ok, I have checked the code again. We do an internal check for the right URL when we wait for the addons manager has been opened. That's only done if you specify the waitFor boolean flag beside the type of event for the open() method. So please add this property and we should be fine.
Attachment #545868 - Flags: review?(hskupin) → review-
Comment on attachment 545868 [details] [diff] [review]
patch v1.6 [checked-in]

Well, my fault. waitFor has a fallback to true, if not specified. So this patch is fine re: tab check. Since we also close all tabs affront, I don't see a reason to check for isOpened() at the beginning.
Attachment #545868 - Flags: review- → review+
(In reply to Henrik Skupin (:whimboo) from comment #30)
> Comment on attachment 545868 [details] [diff] [review]
> patch v1.6
> 
> Well, my fault. waitFor has a fallback to true, if not specified. So this
> patch is fine re: tab check. Since we also close all tabs affront, I don't
> see a reason to check for isOpened() at the beginning.

I assume based on that this patch can be checked-in as is without changes? Also, what branches does this need to land on?
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #31)
> 
> I assume based on that this patch can be checked-in as is without changes?
> Also, what branches does this need to land on?

I'm assuming since this is a new test, this would land on default, aurora, and beta. What about 2.0 and 1.9.2?
We are only landing new tests on default, aurora, beta, and release. We do not support older branches.
Thanks Henrik! can we land this one or do we need other changes on this patch?
This has been forgotten to land. Anthony, can you please do that now? Thanks.
Comment on attachment 545868 [details] [diff] [review]
patch v1.6 [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/dc12c04348b6 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/9da15dff3d4d (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/d858e546275f (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/322faea0805d (mozilla-release)
Attachment #545868 - Attachment description: patch v1.6 → patch v1.6 [checked-in]
Attachment #545868 - Flags: review- → review+
RESOLVED FIXED -- please check tomorrow's results and mark this as VERIFIED if passing; REOPEN if failing so we can back it out.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Also update the Litmus test so it notes that the test is covered by Mozmill.
(In reply to Henrik Skupin (:whimboo) from comment #38)
> Also update the Litmus test so it notes that the test is covered by Mozmill.

Litmus test has been updated. Thanks.
Whiteboard: [mozmill-aom] → [mozmill-functional][mozmill-aom]
This test did not fail at all. Marking verified
Status: RESOLVED → VERIFIED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.