521499 - [mozmill] - Test the Get Addons Tab

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Aakash Desai [:aakashd]]

Attachment: patch for review

The test case first launches and verifies the tab button elements of the addon manager and then verifies the visibility of the update button in the extensions pane. From there, it verifies the functionality and visibility of elements in the getAddons tab (list of recommended addons, browse all addons link, see all recommended addons link and search field).

Litmus Test Cases Represented:
 * Testcase ID #6112 - Launch Add-ons manager
 * Testcase ID #6113 - Get Add-ons Tab
 * Testcase ID #8154 - Launch Add-ons manager
 * Testcase ID #8155 - Get Add-ons Tab 

Note: Henrik, check out the last assertJS as there might be an issue with MozMill where the waitForElement and waitForPageLoad commands are passing on the browser window, but haven't really finished yet. The last assertJS is commented out so as to expedite the review process.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Aakash, as you have asked on bug 521458 you can now use UtilsAPI.appInfo for getting application related information, e.g. UtilsAPI.appInfo.name or UtilsAPI.appInfo.locale.

Could you update the patch before I start the review process? I don't have time today but within the next days. So it would be great to have a complete test.

Comment 2

[Aakash Desai [:aakashd]]

Attachment: patch updated due to bug 521458

Done and done, Henrik.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 406094 [details] [diff] [review]
patch updated due to bug 521458

>+++ b/firefox/testAddons/testGetAddons.js	Tue Oct 13 14:30:29 2009 

Please call all instances AddonsManager.

>+/**
>+ * Testcase ID #6112 - Launch Add-ons manager
>+ * Testcase ID #6113 - Get Add-ons Tab
>+ * Testcase ID #8154 - Launch Add-ons manager
>+ * Testcase ID #8155 - Get Add-ons Tab 
>+ */

Please follow the style as we had until today. Otherwise we will have problems later with mass-changing the tests to add the uids of tests in the new testcase manager

>+var setupModule = function(module) {
>+  module.controller = mozmill.getBrowserController();
>+  addonsController = mozmill.getAddonsController();

Please don't use getAddonsController for this test. We should explicitely call Tools | Addons.

>+//  module.ai = Cc["@mozilla.org/xre/app-info;1"]
>+//                 .getService(Ci.nsIXULAppInfo);

Please blow that away. :)

>+  // Open the extensions pane in the addons manager
>+  var getAddonsPane = new elementslib.ID(addonsController.window.document, "search-view");
>+  addonsController.waitThenClick(getAddonsPane, gTimeout);

We switch through all of the panes later so why you select the first pane?

>+  // Verify elements of the addon manager are visible
>+  var panes = [
>+               {button: "search-view", label:"Get Add-ons"}, 
>+               {button: "extensions-view", label:"Extensions"},
>+               {button: "themes-view", label:"Themes"},
>+               {button: "plugins-view", label:"Plugins"}
>+              ];

Please don't use hardcoded labels. We have to get them from the property/dtd file.

>+  addonsController.waitForElement(new elementslib.ID(addonsController.window.document, "searchfield"));

Why do we need this?

>+  for each (pane in panes) 
>+  {

For if statements or loops the brackets are in the same line. Only for functions we move it to the next one.

>+    var buttonCheck = new elementslib.ID(addonsController.window.document, pane.button);
>+  	
>+    UtilsAPI.assertElementVisible(addonsController, buttonCheck, true);
>+    addonsController.assertProperty(buttonCheck, "label", pane.label);
>+  }

We should click the tab and check if the resulting pane has been opened.

>+  // Verify recommended addons are shown within a nominal amount of time
>+  var footerField = new elementslib.ID(addonsController.window.document, "urn:mozilla:addons:search:status:footer");
>+  addonsController.waitForElement(footerField, 30000);
>+  addonsController.waitForEval(addonsController.assertProperty(footerField, "hidden", false) == 1);

Please combine those two calls into waitForEval and use getNode() to supply the element as parameter as subject to the function. Further we need a timeout.

>+  var recommendedAddonsPane = new elementslib.ID(addonsController.window.document, "extensionsView");
>+  addonsController.assertJS(0 < recommendedAddonsPane.getNode().itemCount <= maxResults);

Wow, this is a strange comparision. :) It will not return the value you expect. You would have to split it up into two separate ones.

>+  var locale = PrefsAPI.preferences.getPref("general.useragent.locale", 

please use the appInfo object from UtilsAPI now on.

>+  addonsController.assertJS(footerField.getNode().getAttribute('link') == recommendedUrl);

We should not only assert but also click this link to check that it gets opened in a new tab. I know that's not part of this litmus test but I cannot find it anywhere else too. You can simply c&p the code from my extension installation test.

>+  addonsController.click(browseAllAddons);
>+  controller.waitForPageLoad(controller.tabs.activeTab);
>+  controller.waitForElement(new elementslib.Link(controller.tabs.activeTab, "Mozilla"));

Please check the above mentioned installation how to do that correctly. I spend a bit of time on that too to get it working without problems.

>+  var browseAddonUrl = PrefsAPI.preferences.getPref("extensions.getAddons.browseAddons", "");
>+  
>+  browseAddonUrl = browseAddonUrl.replace(/%LOCALE%/g, locale);
>+  browseAddonUrl = browseAddonUrl.replace(/%APP%/g, UtilsAPI.appInfo.name.toLowerCase());
>+
>+  var locationBar = new elementslib.ID(controller.window.document, "urlbar");
>+  var pageUrl = locationBar.getNode().value;
>+  
>+  controller.assertJS(browseAddonUrl == browseAllAddons.getNode().getAttribute('homepageURL'));
>+  //controller.assertJS(pageUrl.indexOf("addons.mozilla") != -1);

You should not compare with browseAllAddons but the locationBar value as you started to do in the commented out line.

Comment 4

[Aakash Desai [:aakashd]]

Attachment: patch after last review

It's not possible to remove the waitForElement for the footerfield as the
testcase will fail as a result. I've changed them to assertJS' instead after
the waitForElement. I've added the other changes

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 407383 [details] [diff] [review]
patch after last review

>+  // Open the addons manager and verify the extensions pane is selected
>+  controller.click(new elementslib.Elem(controller.menus["tools-menu"].menu_openAddons));
>+  controller.sleep(500);

Please update the comment so it reflects what we are doing here. We don't check for the extensions pane.

>+  var window = mozmill.wm.getMostRecentWindow('Extension:Manager');
>+  var addonsController = new mozmill.controller.MozMillController(window);  

Can you add a little sleep after that? We don't use any wait function for the next checks.

>+var testGetAddonsTab = function()
>+{
>+  // Open the addons manager and verify the extensions pane is selected
>+  controller.click(new elementslib.Elem(controller.menus["tools-menu"].menu_openAddons));

No need to open it again. Just move the addonsController declaration from the first test to setupModule and assign null. That way you can reuse it in this test.

>+  addonsController.assertJS(addonsController.assertProperty(footerField, "hidden", false) == 1);

What's that? Why you have added assertJS around it?

>+  addonsController.assertJS(recommendedAddonsPane.getNode().itemCount > 0);

Why not 0?

>+  addonsController.assertJS(footerField.getNode().getAttribute('link') == recommendedUrl);

That's bad that we cannot use assertProperty for that. Can you please file a bug so that function checks if a property is accessible and falls-back to getAttribute if it's not the case? Thanks. 

>+  controller.waitForEval("subject.length == 2", gTimeout, 100, controller.tabs);

I know I did that in my own test too but can you move the .tabs property into the expression itself? That will give us more information.

>+  var locationBar = new elementslib.ID(controller.window.document, "urlbar");
>+  var pageUrl = locationBar.getNode().value;
>+  
>+  controller.assertJS(pageUrl.indexOf("addons.mozilla.org") != -1);
>+  controller.assertJS(pageUrl.indexOf(UtilsAPI.appInfo.locale) != -1);
>+  controller.assertJS(pageUrl.indexOf(UtilsAPI.appInfo.name.toLowerCase()) != -1);

That are a lot of checks which could cause us trouble in the future if something will be changed. Can we do it the same way as we do in this test;

http://hg.mozilla.org/qa/mozmill-tests/file/tip/firefox/testSecurity/testSafeBrowsingWarningPages.js#l92

Comment 6

[Aakash Desai [:aakashd]]

Attachment: patch with changes from comment #5

I could only really change half of your proposed changes on account of varying reasons that either broke the testscript or is not possible to do for this testscript. Case in points:

1. I can't use 0 because the litmus testcase says that at least one recommended addon should be shown when the pane is brought up.
2. cannot move controller.tabs to the expression as I get a "subject is undefined" error when I do that.
3. cannot change the assert's the end of testGetAddonsTab because the link re-directs to a url that can't be tested like the ones in testWarningPages (which I wrote, so I know what I did there ...and used that as a reference point for this testscript).


Bug 526248 was filed for a request for the helper function.

Comment 7

[Aakash Desai [:aakashd]]

Attachment: patch with addon manager close

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 410072 [details] [diff] [review]
patch with addon manager close

(In reply to comment #6)
> 1. I can't use 0 because the litmus testcase says that at least one recommended

So while we are in online mode we will always get at least 1 recommended add-on. The only case when we really get 0 recommended add-ons will probably be caused by a network drop so nothing can be fetched from AMO. Given that nearly all of our tests require the network active we can go that way we have now.

> 2. cannot move controller.tabs to the expression as I get a "subject is
> undefined" error when I do that.

You mean "controller.waitForEval("subject.tabs.length == 2", gTimeout, 100, controller);" will fail? That should not happen. The 'tabs' property is always existent for the controller instance. And it works for me.

> 3. cannot change the assert's the end of testGetAddonsTab because the link
> re-directs to a url that can't be tested like the ones in testWarningPages
> (which I wrote, so I know what I did there ...and used that as a reference
> point for this testscript).

We can test that. We retrieve both URL's from the preferences and replace the placeholders. So opening this URL directly should be identical to the URL which gets opened by clicking on both links.

(In reply to comment #7)
> patch with addon manager close

I still don't see it.

Comment 9

[Aakash Desai [:aakashd]]

Attachment: new patch

This patches switches to subject.tabs.length and adds a comment with some line separation for when the addons manager is closed. I'm not sure how to get the redirected link, so if you can update that. I'd like to see how you implement it.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 412928 [details] [diff] [review]
new patch

This version has been not tested. It always fails because we close the addons manager in the middle of the test. I'll come up with a correction so we can finish this test.

Also please check bug 514696 which you have reviewed and which shows how to close additional windows.

For the URL check I will come up with a follow-up bug which will affect a couple of tests.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch (corrected)

That's the corrected version. Works on all platforms and for 1.9.2 and 1.9.1

Comment 12

[Aakash Desai [:aakashd]]

Comment on attachment 412947 [details] [diff] [review]
Patch (corrected)

Ah, I see the issue. It doesn't matter when the addons manager is closed, just as long as it is. This test passes on XP and OSX. 

Previously, I didn't test on XP, but it was passing on OSX via IDE and CLI.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

> (From update of attachment 412947 [details] [diff] [review])
> Ah, I see the issue. It doesn't matter when the addons manager is closed, just
> as long as it is. This test passes on XP and OSX. 

That's not true. The line where it was closed before didn't trigger the opening of the "Browse All Add-ons" page.

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/907927cc853e
http://hg.mozilla.org/qa/mozmill-tests/rev/a20884419de9(In reply to comment #12)

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.