Closed Bug 600291 Opened 10 years ago Closed 8 years ago

Update test module testSearchAddons to reflect the new Add-ons API

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect, P3)

defect

Tracking

(firefox18 fixed, firefox19 fixed, firefox20 fixed, firefox21 fixed, firefox-esr17 fixed)

RESOLVED FIXED
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed
firefox21 --- fixed
firefox-esr17 --- fixed

People

(Reporter: aaronmt, Assigned: mario.garbi)

References

Details

(Whiteboard: s=121217 u=enhancement c=addons p=1)

Attachments

(1 file, 18 obsolete files)

3.83 KB, patch
AndreeaMatei
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
No description provided.
Attached patch Patch v1 - (default) (obsolete) — Splinter Review
Small test, couple of changes needed to reflect the new API and the new AOM. This test used to reflect litmus #8825 https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=8825
Attachment #479149 - Flags: review?(hskupin)
Attachment #479149 - Attachment is obsolete: true
Attachment #479149 - Flags: review?(hskupin)
Attached patch Patch v1 - (default) (obsolete) — Splinter Review
Removal of a debug line
Attachment #479153 - Flags: review?(hskupin)
Comment on attachment 479153 [details] [diff] [review]
Patch v1 - (default)

>- * Portions created by the Initial Developer are Copyright (C) 2009
>+ * Portions created by the Initial Developer are Copyright (C) 2010

Please don't touch the date.

>-  addonsManager = new AddonsAPI.addonsManager();
>+  am = new AddonsAPI.addonsManager(controller);

Please leave the variable name. It's more descriptive in tests.

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

Don't specify a parameter here. We should use a shortcut only in the test, which will test the various ways of opening the manager. All other tests have to fallback to the default behavior.

>+  // Search for the Add-on Compatability Reporter
>+  am.search({value: "rss"});
>+  am.selectedSearchFilter = "remote";

We are searching for rss not ACR here?

>+  // Verify that the search pane should be selected
>+  var currentCategory = am.selectedCategory;
>+  controller.assertDOMProperty(currentCategory, "name", "Search");

Just use the direct call to the class member in the assert call.

>+  controller.assert(function() {
>+    return am.getSearchResults().length && 
>+           am.getSearchResults().length <= maxResults;
>+  }, "Search results count is more than the maximum allowed");

What about the case when no results have been found? It's not part of the assert.

Also we have to check the link at the bottom which links to all results on AMO. You have removed that check.

>-  var searchField = addonsManager.getElement({type: "search_field"});
>+  var searchField = am.getElement({type: "search_textbox"});
>   controller.keypress(searchField, "VK_ESCAPE", {});
>-
>-  buttonPanel = searchButton.getNode().selectedPanel;
>-  controller.assertJS("subject.isClearButtonShown == true",
>-                      {isClearButtonShown: buttonPanel.getAttribute('class') != 'textbox-search-clear'});
>   controller.assertValue(searchField, "");

Use addonsManager.clearSearchField. To check the cross we would need a new entry in getElements(). You can add it with this test.
Attachment #479153 - Flags: review?(hskupin) → review-
Depends on: 562946
Testing of the search-icon on the search box depends on bug 562946. With testing the search-allresults-link at the bottom of all the results requires a mouse scroll down into view, are we able to do such in Mozmill?
(In reply to comment #4)
> testing the search-allresults-link at the bottom of all the results requires a
> mouse scroll down into view, are we able to do such in Mozmill?

Well, clicking on it will scroll the element into view. So it should work.
Should, but it does not. I'm not getting any click (i.e., no tab opens) event on the link, perhaps it's a targeting issue or just out of view.

Code in question, after a search:

 var allResults = new elementslib.ID(controller.tabs.activeTab, "search-allresults-link");
 controller.click(allResults);
 controller.waitForPageLoad();
For debugging purposes you could add a sleep infront of the click event. Eventually the widget isn't ready yet? If that's not the case scroll down manually and check if it works when the link is visible.
(In reply to comment #7)
> For debugging purposes you could add a sleep infront of the click event.
> Eventually the widget isn't ready yet? If that's not the case scroll down
> manually and check if it works when the link is visible.

Yeah, that works beautifully if I manually scroll down to the bottom of the results listing.
Then I wonder if this is only a problem in xul namespace. Using that way for content works fine. What happens if you record those steps with Mozmill. Also could you upload a minimized testcase for it, I could use for testing? Please try to avoid including shared modules as usual. Thanks.
Attached file Minimized testcase (obsolete) —
Minimized testcase
Attachment #480740 - Attachment is obsolete: true
Attached file Testcase (obsolete) —
There is nothing we can do for now to scroll a label into the view. Therefore modify the pref "extensions.getAddons.maxResults" to only show one single result. So the "See all results" link will appear directly beneath it.
Attached patch Patch v2 - (default) (obsolete) — Splinter Review
Attachment #479153 - Attachment is obsolete: true
Attachment #481937 - Flags: review?(hskupin)
Comment on attachment 481937 [details] [diff] [review]
Patch v2 - (default)

>+const RELATIVE_ROOT = '../../shared-modules';
>+const MODULE_REQUIRES = ['AddonsAPI', 'PrefsAPI', 'TabbedBrowsingAPI', 'ToolbarAPI'];

Please update those lines to match the new CommonJS module inclusion.

>+const MAX_RESULTS = 'extensions.getAddons.maxResults';

Please prefix the constant with "PREF_".

>+  // XXX: Currently unable to scroll the 'Show All Results' link into view
>+  // Temporarily set the preference to show a single result
>+  PrefsAPI.preferences.setPref(MAX_RESULTS, 1);

You can remove the comment. With the pref set we exactly test the desired behavior. You should also move the '1' to a constant. IMO using '2' would even be better, so we could check that multiple results are getting returned.

>+  // XXX: Currently unable to scroll the 'Show All Results' link into view
>+  // Temporarily clear the user preference
>+  PrefsAPI.preferences.clearUserPref(MAX_RESULTS);

Same applies here.

>+  // Verify that the search pane should be selected
>+  controller.assertDOMProperty(addonsManager.selectedCategory, "name", "Search");   

This should be part of the waitForSearchFinished method of the addons module.

>+  // Verify that results have returned
>+  controller.assert(function() {
>+    return addonsManager.getSearchResults().length > 0;
>+  }, "Search query should return remote AMO results");

Having some more data in the message would be great for our reports. My proposal would be something like:

"Add-ons found when searching remote for 'rss' - Got 0, expected '>=0'."

For the format (how it is used in the mochitests) see http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1288999911.1289001075.1737.gz 

>+  // Verify that the number of addons is within the maxResults range
>+  var maxResults = PrefsAPI.preferences.getPref("extensions.getAddons.maxResults", -1);
>+  controller.assert(function() {
>+    return addonsManager.getSearchResults().length <= maxResults;
>+  }, "Search results count is more than the maximum allowed");

There is no need to retrieve the preference. Simply make use of the constant, as proposed above. Also update the message and add the current number of results: "... - Got 4, expected '<= 2'". 

>+  // Verify that clicking the 'Show All Results' link opens a new tab with AMO 

nit: 'on AMO'

>+  var allResults = new elementslib.ID(
>+                     controller.tabs.activeTab, 
>+                     "search-allresults-link"
>+                   );

Formatting's like that we will have to talk about within the next days. Getting this link should be part of the shared module too.

>+  controller.click(allResults);
>+  controller.waitForPageLoad();

We are opening a new tab here. So waitForPageLoad is not enough.

>+  // Verify that the URL contains AMO
>+  controller.assert(function() { 
>+    return (locationBar.contains('addons.mozilla.org'));
>+  }, "'Show All Results' link should open addons.mozilla.org");    

nit: No need for extra brackets around locationBar.contains. And the message shouldn't contain 'should' because we know what has to happen.
Attachment #481937 - Flags: review?(hskupin) → review-
Attached patch Patch v3 - (default) (obsolete) — Splinter Review
All but one thing addressed, and includes API changes to addons.js

>+  controller.click(allResults);
>+  controller.waitForPageLoad();
> We are opening a new tab here. So waitForPageLoad is not enough.

It becomes the active tab on click
Attachment #480742 - Attachment is obsolete: true
Attachment #481937 - Attachment is obsolete: true
Attachment #490715 - Flags: review?(hskupin)
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.
Product: Testing → Mozilla QA
Version: Trunk → unspecified
Comment on attachment 490715 [details] [diff] [review]
Patch v3 - (default)

Sorry that it has been taken so long for review. But it slipped through my request view.

>+var teardownModule = function() {
>   addonsManager.close();
>+  tabBrowser.closeAllTabs();

Now with the add-ons manager as tab, we don't have to call .close separately.

>+  // Verify that results have returned
>+  controller.assert(function() {
>+    return addonsManager.getSearchResults().length > 0;
>+  }, "Add-ons found when searching for remote 'rss' - Got:  " +
>+     addonsManager.getSearchResults().length + " Expected: '>=0'"
>+  );

As given by the review for restore homepage, we have to use a consistent way for got/expected. Also try to make messages as crisp as possible. 

>+  controller.assert(function() {

There has to be a blank between function and ().

>+   return addonsManager.getSearchResults().length <= NUMBER_OF_MAX_RESULTS;
>+  }, "Number of Add-ons retrieved should be within range - Got: " + 
>+     addonsManager.getSearchResults().length + " Expected: " + "'<= " + 
>+     NUMBER_OF_MAX_RESULTS + "'"
>+  );

You are using addonsManager.getSearchResults().length a couple of times. It should be cached before any of the assert calls.

>+  // Verify that clicking the 'Show All Results' link opens a new tab on AMO   
>+  var allResults = addonsManager.getElements({type: "search_allResultsLink"});
>+  controller.click(allResults[0]);
>+  controller.waitForPageLoad();

We have to make sure to use a search term which will always produce at least 2 results here. It's kinda hard to predict for add-ons on AMO. Can we make sure that 'rss' is the right term? If not we should search for add-ons, which are getting updated really quickly.

>+  // Verify that the URL contains an AMO address
>+  controller.assert(function() { 
>+    return locationBar.contains('addons.mozilla.org');
>+  }, "URL contains: " + locationBar.value + " Expected: addons.mozilla.org");

The URL shouldn't be hard-coded. Do we want to execute the test against the preview site?

>+++ b/shared-modules/addons.js
>[..]
>     mozmill.utils.waitForEval("subject.isSearching == false", 
>                               timeout, 100, this);
>+    this._controller.assertDOMProperty(this.selectedCategory, "name", "Search");

This will break in localized builds. Check for the id.


>+      case "search_allResultsLink":
>+        nodeCollector.queryNodes("#search-allresults-link");
>+        break;
>       // Search
>       // Bug 599775 - Controller needs to handle radio groups correctly
>       // Means for now we have to use the radio buttons

This should go under search some lines down.
Attachment #490715 - Flags: review?(hskupin) → review-
I would like to see old bugs finished. So lets have a look at this bug and patch and figure out what's still necessary to see updated.
Assignee: aaron.train → nobody
Status: ASSIGNED → NEW
Priority: -- → P3
Whiteboard: s=121217 u=enhancement c=addons p=1
Assignee: nobody → mario.garbi
It would seem the test is last present in 1.9.2 branch and has been removed ever since. What am I to do now?
Tests have been removed for Firefox 4.0 because about:addons has been completely rewritten. So the test also needs to be revised. So you should check if we already have a similar test in our suite. If not we should update this test and integrate it again on our currently supported branches.
Attached patch patch v4.0 Default (obsolete) — Splinter Review
Fixed the test so it would work with our current repository.
Attachment #490715 - Attachment is obsolete: true
Attachment #696045 - Flags: review?(hskupin)
Attachment #696045 - Flags: review?(dave.hunt)
Attachment #696045 - Flags: review?(andreea.matei)
Comment on attachment 696045 [details] [diff] [review]
patch v4.0 Default

Review of attachment 696045 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/testAddons/testSearchAddons.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> + // Include required modules
> +var addons = require("../../../lib/addons");
> +var { assert, expect } = require("../../../lib/assertions");

This will need to be above. We use special characters above alpha-numeric ones.

@@ +10,5 @@
> +var toolbars = require("../../../lib/toolbars");
> +
> +const PREF_MAX_RESULTS = 'extensions.getAddons.maxResults';
> +const NUMBER_OF_MAX_RESULTS = 2;
> +const SEARCH_TERM = 'rss';

Since we are using this term only once, there's no need to declare it separately.

@@ +11,5 @@
> +
> +const PREF_MAX_RESULTS = 'extensions.getAddons.maxResults';
> +const NUMBER_OF_MAX_RESULTS = 2;
> +const SEARCH_TERM = 'rss';
> +const adressAMO = "addons.mozilla.org";

I would use here AMO_DOMAIN. Please also sort them all alphabetically.

@@ +29,5 @@
> + }
> +
> + /**
> +  * Test the search for Add-ons
> +  */

Please move the whole block one space to the left, no need for the whitespace before.

@@ +31,5 @@
> + /**
> +  * Test the search for Add-ons
> +  */
> +function testSearchAddons() {
> +  // Open the Add-on's manager

We can remove this comment, it's understandable what we do here.

@@ +43,5 @@
> +  var resultsLength = am.getSearchResults().length;
> +
> +  assert.waitFor(function () {
> +    return resultsLength > 0;
> +  }, "Add-ons found when searching for remote 'rss' - Got: " +

Lets use a message which expresses that we expect to have results after the search, skipping the "got" part.

@@ +49,5 @@
> +  );
> +
> +  // Verify that the number of addons is within the maxResults range
> +  expect.waitFor(function () {
> +   return resultsLength <= NUMBER_OF_MAX_RESULTS;

You have only one space indent from the parent.

@@ +51,5 @@
> +  // Verify that the number of addons is within the maxResults range
> +  expect.waitFor(function () {
> +   return resultsLength <= NUMBER_OF_MAX_RESULTS;
> +  }, "Expected maximum " + NUMBER_OF_MAX_RESULTS+ " search results"
> +  );

Please use the closing bracket at the end of the message (also for the assert from above), single quotes for the const and a whitespace before second "+".

@@ +54,5 @@
> +  }, "Expected maximum " + NUMBER_OF_MAX_RESULTS+ " search results"
> +  );
> +
> +  // Verify that clicking the 'Show All Results' link opens a new tab on AMO
> +  // var allResults = addonsManager.getElements({type: "search_allResultsLink"});

Why would we need this comment here?

@@ +56,5 @@
> +
> +  // Verify that clicking the 'Show All Results' link opens a new tab on AMO
> +  // var allResults = addonsManager.getElements({type: "search_allResultsLink"});
> +  var allResults = new elementslib.ID(controller.tabs.activeTab,
> +                   "search-allresults-link");

The second parameter has to start after the open bracket.
Attachment #696045 - Flags: review?(hskupin)
Attachment #696045 - Flags: review?(dave.hunt)
Attachment #696045 - Flags: review?(andreea.matei)
Attachment #696045 - Flags: review-
Status: NEW → ASSIGNED
Attached patch patch v4.1 (obsolete) — Splinter Review
on Linux:
http://mozmill-crowd.blargon7.com/#/remote/report/23d8fbdd0190d4b0496d6b129ff05b08
Attachment #696045 - Attachment is obsolete: true
Attachment #700392 - Flags: review?(hskupin)
Attachment #700392 - Flags: review?(dave.hunt)
Attachment #700392 - Flags: review?(andreea.matei)
Comment on attachment 700392 [details] [diff] [review]
patch v4.1

Review of attachment 700392 [details] [diff] [review]:
-----------------------------------------------------------------

We're getting closer, just a few things to be changed.

::: tests/remote/restartTests/testSearchAddons/test1.js
@@ +2,5 @@
> + * 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/. */
> +
> + // Include required modules
> +var { assert, expect } = require("../../../../lib/assertions");

You are not using expect anymore.

@@ +28,5 @@
> +  addons.resetAmoPreviewUrls();
> +  tabBrowser.closeAllTabs();
> + }
> +
> +function testSearchAddons() {

Why did you remove the jsdoc for this function?

@@ +29,5 @@
> +  tabBrowser.closeAllTabs();
> + }
> +
> +function testSearchAddons() {
> +  // Open the Add-on's manager

This one needs to be removed instead.

@@ +42,5 @@
> +
> +  assert.waitFor(function () {
> +    if (resultsLength > 0)
> +      return resultsLength <= NUMBER_OF_MAX_RESULTS;
> +  }, NUMBER_OF_MAX_RESULTS + "Add-ons found when searching for remote 'rss'");

If this fails, there are not 2 (number_of_max_results) addons found.

@@ +54,5 @@
> +  // Verify that the URL contains an AMO address
> +  assert.waitFor(function () {
> +    return locationBar.contains(addons.AMO_PREVIEW_DOMAIN);
> +  }, "URL contains: " + locationBar.value);
> + }

We need a new blank line at the end of all tests, as required by the style guide.
Attachment #700392 - Flags: review?(hskupin)
Attachment #700392 - Flags: review?(dave.hunt)
Attachment #700392 - Flags: review?(andreea.matei)
Attachment #700392 - Flags: review-
(In reply to Andreea Matei [:AndreeaMatei] from comment #24)
> > +  assert.waitFor(function () {
> > +    if (resultsLength > 0)
> > +      return resultsLength <= NUMBER_OF_MAX_RESULTS;
> > +  }, NUMBER_OF_MAX_RESULTS + "Add-ons found when searching for remote 'rss'");
> 
> If this fails, there are not 2 (number_of_max_results) addons found.

More important is that you always have to return a value in the callback. Otherwise will will get an exception from Mozmill.
(In reply to Henrik Skupin (:whimboo) from comment #25)
> More important is that you always have to return a value in the callback.
> Otherwise will will get an exception from Mozmill.

I've used a weird text to search, got0 results and the timeout error. But if so, we should use a combined return condition.
Attached patch patch v4.2 (obsolete) — Splinter Review
Linux report:
http://mozmill-crowd.blargon7.com/#/remote/report/23d8fbdd0190d4b0496d6b129ff72e05
Attachment #700392 - Attachment is obsolete: true
Attachment #700441 - Flags: review?(hskupin)
Attachment #700441 - Flags: review?(dave.hunt)
Attachment #700441 - Flags: review?(andreea.matei)
Comment on attachment 700441 [details] [diff] [review]
patch v4.2

Review of attachment 700441 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/remote/restartTests/testSearchAddons/test1.js
@@ +43,5 @@
> +  var resultsLength = am.getSearchResults().length;
> +
> +  assert.waitFor(function () {
> +    return resultsLength > 0 && resultsLength <= NUMBER_OF_MAX_RESULTS;
> +  }, resultsLength + "Add-ons found when searching for remote 'rss'");

Still need a space here, otherwise the number will be attached to the string.
Attachment #700441 - Flags: review?(hskupin)
Attachment #700441 - Flags: review?(dave.hunt)
Attachment #700441 - Flags: review?(andreea.matei)
Attachment #700441 - Flags: review-
Attached patch patch v4.3 (obsolete) — Splinter Review
Fixed that minor space problem in the comment.
Attachment #700441 - Attachment is obsolete: true
Attachment #701043 - Flags: review?(hskupin)
Attachment #701043 - Flags: review?(dave.hunt)
Attachment #701043 - Flags: review?(andreea.matei)
Comment on attachment 701043 [details] [diff] [review]
patch v4.3

Review of attachment 701043 [details] [diff] [review]:
-----------------------------------------------------------------

The commit message is not well placed, please use hg qrefresh -m "Message" to do so.
Attachment #701043 - Flags: review?(hskupin)
Attachment #701043 - Flags: review?(dave.hunt)
Attachment #701043 - Flags: review?(andreea.matei)
Attachment #701043 - Flags: review-
Attached patch patch v4.4 (obsolete) — Splinter Review
Fixed the patch message.
Attachment #701043 - Attachment is obsolete: true
Attachment #701756 - Flags: review?(hskupin)
Attachment #701756 - Flags: review?(dave.hunt)
Attachment #701756 - Flags: review?(andreea.matei)
Comment on attachment 701756 [details] [diff] [review]
patch v4.4

Review of attachment 701756 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, thanks. Lets see how this goes.
Attachment #701756 - Flags: review?(hskupin)
Attachment #701756 - Flags: review?(dave.hunt)
Attachment #701756 - Flags: review?(andreea.matei)
Attachment #701756 - Flags: review+
Comment on attachment 701756 [details] [diff] [review]
patch v4.4

Review of attachment 701756 [details] [diff] [review]:
-----------------------------------------------------------------

This is not a restart test, so it cannot be put under /remote/restartTests. Instead use the same folder structure as given by the functional tests for non-restart tests.
Attachment #701756 - Flags: review-
Depends on: 831306
Attached patch patch v4.5 (obsolete) — Splinter Review
Linux - Firefox Default testrun_remote reports:
http://mozmill-crowd.blargon7.com/#/remote/report/9e41582ed5e806fa373351d9d316a043
http://mozmill-crowd.blargon7.com/#/remote/report/9e41582ed5e806fa373351d9d316ac14
Attachment #701756 - Attachment is obsolete: true
Attachment #703210 - Flags: review?(hskupin)
Attachment #703210 - Flags: review?(dave.hunt)
Attachment #703210 - Flags: review?(andreea.matei)
Blocks: 831306
No longer depends on: 831306
Comment on attachment 703210 [details] [diff] [review]
patch v4.5

Review of attachment 703210 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/remote/addonsTests/testSearchAddons/test1.js
@@ +43,5 @@
> +  var resultsLength = am.getSearchResults().length;
> +
> +  assert.waitFor(function () {
> +    return resultsLength > 0 && resultsLength <= NUMBER_OF_MAX_RESULTS;
> +  }, resultsLength + " Add-ons found when searching for remote 'rss'");

Not sure why this has to be a waitFor() call if you don't retrieve the resultsLength value again inside the callback. I think this might fail.

@@ +49,5 @@
> +  // Verify that clicking the 'Show All Results' link opens a new tab on AMO
> +  var allResults = new elementslib.ID(controller.tabs.activeTab,
> +                                      "search-allresults-link");
> +  controller.click(allResults);
> +  controller.waitForPageLoad();

Is the new tab focused immediately? We should check that otherwise the test will give a false positive because we are operating on the current tab.

@@ +54,5 @@
> +
> +  // Verify that the URL contains an AMO address
> +  assert.waitFor(function () {
> +    return locationBar.contains(addons.AMO_PREVIEW_DOMAIN);
> +  }, "URL contains: " + locationBar.value);

Why waitFor() again? I do not see anything we would have to wait for here.

::: tests/remote/manifest.ini
@@ +1,2 @@
>  [include:restartTests/manifest.ini]
> +[include:addonsTests/manifest.ini]

The subfolder has to get a prefix of 'test'. Similar to the functional testrun non-restart tests. So remove this extra folder called 'addonsTests'.
Attachment #703210 - Flags: review?(hskupin)
Attachment #703210 - Flags: review?(dave.hunt)
Attachment #703210 - Flags: review?(andreea.matei)
Attachment #703210 - Flags: review-
Attached patch patch v4.6 (obsolete) — Splinter Review
Modified as requested
Linux - mozilla-default report:
http://mozmill-crowd.blargon7.com/#/remote/report/9e41582ed5e806fa373351d9d3185936
Attachment #703210 - Attachment is obsolete: true
Attachment #703282 - Flags: review?(hskupin)
Attachment #703282 - Flags: review?(dave.hunt)
Attachment #703282 - Flags: review?(andreea.matei)
Comment on attachment 703282 [details] [diff] [review]
patch v4.6

Review of attachment 703282 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/remote/testAddons/manifest.ini
@@ +1,1 @@
> +[test1.js]

Given that we do not have a restart test you should give the file a better name. test1 is only necessary for restart tests.

::: tests/remote/testAddons/test1.js
@@ +42,5 @@
> +  // Verify that results have returned and the max number of results
> +  var resultsLength = am.getSearchResults().length;
> +
> +  assert.ok(resultsLength > 0 && resultsLength <= NUMBER_OF_MAX_RESULTS,
> +            resultsLength + " Add-ons found when searching for remote 'rss'");

Putting the amount of found addons in the message can cause a false assumption. It's not what we are testing. You may want to say: "The right amount of add-ons have been found"?

nit: kill the extra line above the assert so we have a single block.

@@ +48,5 @@
> +  // Verify that clicking the 'Show All Results' link opens a new tab on AMO
> +  var allResults = new elementslib.ID(controller.tabs.activeTab,
> +                                      "search-allresults-link");
> +  controller.click(allResults);
> +  assert.ok(tabBrowser.selectedIndex === 2, "Show all results tab has been selected");

I think that should stay as waitFor() otherwise we can face temporary failures if it takes longer to open the new tab.

@@ +53,5 @@
> +  controller.waitForPageLoad();
> +
> +  // Verify that the URL contains an AMO address
> +  assert.ok(locationBar.contains(addons.AMO_PREVIEW_DOMAIN),
> +            "URL contains: " + locationBar.value);

You don't want to use .ok() here but .contains().
Attachment #703282 - Flags: review?(hskupin)
Attachment #703282 - Flags: review?(dave.hunt)
Attachment #703282 - Flags: review?(andreea.matei)
Attachment #703282 - Flags: review-
Attached patch patch v4.7 (obsolete) — Splinter Review
Modified as requested
Linux - mozilla-default report :
http://mozmill-crowd.blargon7.com/#/remote/report/9e41582ed5e806fa373351d9d31df2fa
Attachment #703282 - Attachment is obsolete: true
Attachment #703313 - Flags: review?(hskupin)
Attachment #703313 - Flags: review?(dave.hunt)
Attachment #703313 - Flags: review?(andreea.matei)
Comment on attachment 703313 [details] [diff] [review]
patch v4.7

Review of attachment 703313 [details] [diff] [review]:
-----------------------------------------------------------------

Besides my comment below, looks good now, tested also on OS X:
http://mozmill-crowd.blargon7.com/#/remote/report/9e41582ed5e806fa373351d9d33d96f0

::: tests/remote/testAddons/testSearchAddons.js
@@ +55,5 @@
> +  controller.waitForPageLoad();
> +
> +  // Verify that the URL contains an AMO address
> +  assert.contain(locationBar.value, addons.AMO_PREVIEW_DOMAIN,
> +                 "URL contains: " + locationBar.value);

I wouldn't use the locationBar value in the message since assert.contain will show in it's message that value. There is no need to repeat it.
Attachment #703313 - Flags: review?(hskupin)
Attachment #703313 - Flags: review?(dave.hunt)
Attachment #703313 - Flags: review?(andreea.matei)
Attachment #703313 - Flags: review-
Oh, and be aware that you missed to remove that extra line Henrik requested, between var resultsLength and assert.ok().
Attached patch patch v4.8 (obsolete) — Splinter Review
Linux - default firefox testrun_remote report :
http://mozmill-crowd.blargon7.com/#/remote/report/9e41582ed5e806fa373351d9d33db56b
Attachment #703313 - Attachment is obsolete: true
Attachment #703811 - Flags: review?(hskupin)
Attachment #703811 - Flags: review?(dave.hunt)
Attachment #703811 - Flags: review?(andreea.matei)
Comment on attachment 703811 [details] [diff] [review]
patch v4.8

Review of attachment 703811 [details] [diff] [review]:
-----------------------------------------------------------------

All good now. Thanks!
Attachment #703811 - Flags: review?(hskupin)
Attachment #703811 - Flags: review?(dave.hunt)
Attachment #703811 - Flags: review?(andreea.matei)
Attachment #703811 - Flags: review+
Comment on attachment 703811 [details] [diff] [review]
patch v4.8

Review of attachment 703811 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry that I have to put back the review status to r- but we do not want element identifiers in the test. Also please take care of the other nits.

::: tests/remote/testAddons/testSearchAddons.js
@@ +41,5 @@
> +
> +  // Verify that results have returned and the max number of results
> +  var resultsLength = am.getSearchResults().length;
> +  assert.ok(resultsLength > 0 && resultsLength <= NUMBER_OF_MAX_RESULTS,
> +            "The right amount of add-ons have been found");

Thinking more I wonder if we should change it better to two distinct assert checks. First for existent search results and second that we do not show more as the max number. For the latter we could indeed place the constant in the message.

@@ +45,5 @@
> +            "The right amount of add-ons have been found");
> +
> +  // Verify that clicking the 'Show All Results' link opens a new tab on AMO
> +  var allResults = new elementslib.ID(controller.tabs.activeTab,
> +                                      "search-allresults-link");

We do not want to expose element identifiers to the test itself. All those have to end-up in the library, in this case addons.js. You can request an element via getElement then.

@@ +47,5 @@
> +  // Verify that clicking the 'Show All Results' link opens a new tab on AMO
> +  var allResults = new elementslib.ID(controller.tabs.activeTab,
> +                                      "search-allresults-link");
> +  controller.click(allResults);
> +  assert.waitFor(function() {

nit: space between function and the opening bracket. That affects multiple lines in this file.
Attachment #703811 - Flags: review-
Attached patch patch v4.9 (obsolete) — Splinter Review
Fixed as requested, reports:
http://mozmill-crowd.blargon7.com/#/remote/report/9e41582ed5e806fa373351d9d348a051
Attachment #703811 - Attachment is obsolete: true
Attachment #703897 - Flags: review?(hskupin)
Attachment #703897 - Flags: review?(dave.hunt)
Attachment #703897 - Flags: review?(andreea.matei)
Comment on attachment 703897 [details] [diff] [review]
patch v4.9

Review of attachment 703897 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/remote/testAddons/testSearchAddons.js
@@ +40,5 @@
> +  am.selectedSearchFilter = "remote";
> +
> +  // Verify that results have returned and the max number of results
> +  var resultsLength = am.getSearchResults().length;
> +  assert.ok(resultsLength > 0, am.selectedSearchFilter + " Addons have been found");

Please always do a negative check, make your test to fail to see the results of that.
This message is "[object Object] Addons have been found - got 'true'". Only "Addons have been found" should be enough.

@@ +43,5 @@
> +  var resultsLength = am.getSearchResults().length;
> +  assert.ok(resultsLength > 0, am.selectedSearchFilter + " Addons have been found");
> +
> +  //Verify that we have a maximum of two results with these parameters
> +  assert.ok(resultsLength <= NUMBER_OF_MAX_RESULTS, am.selectedSearchFilter +

This message is "[object Object] addons have been found and are less than 2 - got 'true'"". Leave only number of max result in your message, also remove the part that they were found, which was covered in the first assert.

@@ +47,5 @@
> +  assert.ok(resultsLength <= NUMBER_OF_MAX_RESULTS, am.selectedSearchFilter +
> +            " addons have been found and are less than " + NUMBER_OF_MAX_RESULTS);
> +
> +  // Verify that clicking the 'Show All Results' link opens a new tab on AMO
> +  controller.click(am.getElements({type:"allResults"})[0]);

We should get this element into a variable before clicking it. It would be more readable.
Attachment #703897 - Flags: review?(hskupin)
Attachment #703897 - Flags: review?(dave.hunt)
Attachment #703897 - Flags: review?(andreea.matei)
Attachment #703897 - Flags: review-
Attached patch patch v5.0 (obsolete) — Splinter Review
Linux testrun report:
http://mozmill-crowd.blargon7.com/#/remote/report/72e15dc943833b8fcba70aeb51057209
Attachment #703897 - Attachment is obsolete: true
Attachment #704787 - Flags: review?(hskupin)
Attachment #704787 - Flags: review?(dave.hunt)
Attachment #704787 - Flags: review?(andreea.matei)
Comment on attachment 704787 [details] [diff] [review]
patch v5.0

Review of attachment 704787 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/remote/testAddons/testSearchAddons.js
@@ +44,5 @@
> +  assert.ok(resultsLength > 0, " Addons have been found");
> +
> +  //Verify that we have a maximum of two results with these parameters
> +  assert.ok(resultsLength <= NUMBER_OF_MAX_RESULTS,
> +         " Number of addons are less than: " + NUMBER_OF_MAX_RESULTS);

You can remove the whitespace from the start of the message, also to the above assert message. We use those only when we attach something else to the message in order to not be linked together.
This is not well indented. Please check your patch before uploading.

@@ +48,5 @@
> +         " Number of addons are less than: " + NUMBER_OF_MAX_RESULTS);
> +
> +  // Verify that clicking the 'Show All Results' link opens a new tab on AMO
> +  var allResults = am.getElements({type:"allResults"})[0];
> +

No need for the extra line, the variable is connected to the click, should be a single block.
Attachment #704787 - Flags: review?(hskupin)
Attachment #704787 - Flags: review?(dave.hunt)
Attachment #704787 - Flags: review?(andreea.matei)
Attachment #704787 - Flags: review-
Attached patch patch v5.1 (obsolete) — Splinter Review
Linux report :
http://mozmill-crowd.blargon7.com/#/remote/report/72e15dc943833b8fcba70aeb513a6198
Attachment #704787 - Attachment is obsolete: true
Attachment #705813 - Flags: review?(hskupin)
Attachment #705813 - Flags: review?(dave.hunt)
Attachment #705813 - Flags: review?(andreea.matei)
Comment on attachment 705813 [details] [diff] [review]
patch v5.1

Review of attachment 705813 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me now. Thanks!
Attachment #705813 - Flags: review?(hskupin)
Attachment #705813 - Flags: review?(dave.hunt)
Attachment #705813 - Flags: review?(andreea.matei)
Attachment #705813 - Flags: review+
Comment on attachment 705813 [details] [diff] [review]
patch v5.1

Review of attachment 705813 [details] [diff] [review]:
-----------------------------------------------------------------

Please get the small issues fixed so we can get it landed.

::: tests/remote/testAddons/testSearchAddons.js
@@ +11,5 @@
> +
> +const NUMBER_OF_MAX_RESULTS = 2;
> +const PREF_MAX_RESULTS = "extensions.getAddons.maxResults";
> +
> +var setupModule = function() {

Please add the missing white-space after function. This applies to any instance in that file.

@@ +42,5 @@
> +  // Verify that results have returned and the max number of results
> +  var resultsLength = am.getSearchResults().length;
> +  assert.ok(resultsLength > 0, "Addons have been found");
> +
> +  //Verify that we have a maximum of two results with these parameters

nit: Missing space before verify.

@@ +44,5 @@
> +  assert.ok(resultsLength > 0, "Addons have been found");
> +
> +  //Verify that we have a maximum of two results with these parameters
> +  assert.ok(resultsLength <= NUMBER_OF_MAX_RESULTS,
> +            "Number of addons are less than: " + NUMBER_OF_MAX_RESULTS);

Please correct the wording. It's less or equal.
Attachment #705813 - Flags: review-
Attached patch patch v5.2Splinter Review
Fixed as requested.
Attachment #705813 - Attachment is obsolete: true
Attachment #708465 - Flags: review?(hskupin)
Attachment #708465 - Flags: review?(dave.hunt)
Attachment #708465 - Flags: review?(andreea.matei)
Comment on attachment 708465 [details] [diff] [review]
patch v5.2

Review of attachment 708465 [details] [diff] [review]:
-----------------------------------------------------------------

Requested changes in place.
Attachment #708465 - Flags: review?(hskupin)
Attachment #708465 - Flags: review?(dave.hunt)
Attachment #708465 - Flags: review?(andreea.matei)
Attachment #708465 - Flags: review+
Comment on attachment 708465 [details] [diff] [review]
patch v5.2

Review of attachment 708465 [details] [diff] [review]:
-----------------------------------------------------------------

http://hg.mozilla.org/qa/mozmill-tests/rev/0908e1264d6e (default)
Attachment #708465 - Flags: checkin+
Well, lets think about to backport it, given it's an older feature and the patch should apply cleanly across branches. Mario, please check that through.
The patch does indeed applies cleanly across all branches. I can come with additional testrun reports for each branch if you want them.
(In reply to mario garbi from comment #56)
> The patch does indeed applies cleanly across all branches. I can come with
> additional testrun reports for each branch if you want them.

Yes please, or at least confirm that this causes no regressions for you on the other branches.
No, just run those and we are fine. Make sure we don't see any failures on default too.
As for backporting I've tested the patch on

Linux ESR-17 :
http://mozmill-crowd.blargon7.com/#/remote/report/4435f497b86a8b1845c2e02230110869

WinXp Beta:
http://mozmill-crowd.blargon7.com/#/remote/report/4435f497b86a8b1845c2e0223011473e

As it doesn't modify any files we shouldn't have any backporting problems.
What about tests for aurora and release?
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.