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

Status: RESOLVED FIXED

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

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default)

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

Comment 2

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default)

Removal of a debug line

Comment 3

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

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.

Comment 4

[Aaron Train [:aaronmt]]

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?

Comment 5

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

(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.

Comment 6

[Aaron Train [:aaronmt]]

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();

Comment 7

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

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.

Comment 8

[Aaron Train [:aaronmt]]

(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.

Comment 9

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

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.

Comment 10

[Aaron Train [:aaronmt]]

Attachment: Minimized testcase

Minimized testcase

Comment 11

[Aaron Train [:aaronmt]]

Attachment: Testcase

Comment 12

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

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.

Comment 13

[Aaron Train [:aaronmt]]

Attachment: Patch v2 - (default)

Comment 14

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

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.

Comment 15

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (default)

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

Comment 16

[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.

Comment 17

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

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.

Comment 18

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

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.

Comment 19

[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?

Comment 20

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

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.

Comment 21

[mario garbi]

Attachment: patch v4.0 Default

Fixed the test so it would work with our current repository.

Comment 22

[Andreea Matei [:AndreeaMatei]]

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.

Comment 23

[mario garbi]

Attachment: patch v4.1

on Linux:
http://mozmill-crowd.blargon7.com/#/remote/report/23d8fbdd0190d4b0496d6b129ff05b08

Comment 24

[Andreea Matei [:AndreeaMatei]]

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.

Comment 25

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

(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.

Comment 26

[Andreea Matei [:AndreeaMatei]]

(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.

Comment 27

[mario garbi]

Attachment: patch v4.2

Linux report:
http://mozmill-crowd.blargon7.com/#/remote/report/23d8fbdd0190d4b0496d6b129ff72e05

Comment 28

[Andreea Matei [:AndreeaMatei]]

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.

Comment 29

[mario garbi]

Attachment: patch v4.3

Fixed that minor space problem in the comment.

Comment 30

[Andreea Matei [:AndreeaMatei]]

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.

Comment 31

[mario garbi]

Attachment: patch v4.4

Fixed the patch message.

Comment 32

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 701756 [details] [diff] [review]
patch v4.4

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

Ok, thanks. Lets see how this goes.

Comment 33

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

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.

Comment 34

[mario garbi]

Attachment: patch v4.5

Linux - Firefox Default testrun_remote reports:
http://mozmill-crowd.blargon7.com/#/remote/report/9e41582ed5e806fa373351d9d316a043
http://mozmill-crowd.blargon7.com/#/remote/report/9e41582ed5e806fa373351d9d316ac14

Comment 35

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

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'.

Comment 36

[mario garbi]

Attachment: patch v4.6

Modified as requested
Linux - mozilla-default report:
http://mozmill-crowd.blargon7.com/#/remote/report/9e41582ed5e806fa373351d9d3185936

Comment 37

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

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().

Comment 38

[mario garbi]

Attachment: patch v4.7

Modified as requested
Linux - mozilla-default report :
http://mozmill-crowd.blargon7.com/#/remote/report/9e41582ed5e806fa373351d9d31df2fa

Comment 39

[Andreea Matei [:AndreeaMatei]]

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.

Comment 40

[Andreea Matei [:AndreeaMatei]]

Oh, and be aware that you missed to remove that extra line Henrik requested, between var resultsLength and assert.ok().

Comment 41

[mario garbi]

Attachment: patch v4.8

Linux - default firefox testrun_remote report :
http://mozmill-crowd.blargon7.com/#/remote/report/9e41582ed5e806fa373351d9d33db56b

Comment 42

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 703811 [details] [diff] [review]
patch v4.8

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

All good now. Thanks!

Comment 43

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

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.

Comment 44

[mario garbi]

Attachment: patch v4.9

Fixed as requested, reports:
http://mozmill-crowd.blargon7.com/#/remote/report/9e41582ed5e806fa373351d9d348a051

Comment 45

[Andreea Matei [:AndreeaMatei]]

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.

Comment 46

[mario garbi]

Attachment: patch v5.0

Linux testrun report:
http://mozmill-crowd.blargon7.com/#/remote/report/72e15dc943833b8fcba70aeb51057209

Comment 47

[Andreea Matei [:AndreeaMatei]]

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.

Comment 48

[mario garbi]

Attachment: patch v5.1

Linux report :
http://mozmill-crowd.blargon7.com/#/remote/report/72e15dc943833b8fcba70aeb513a6198

Comment 49

[mario garbi]

Win XP:
http://mozmill-crowd.blargon7.com/#/remote/report/72e15dc943833b8fcba70aeb516934b4

Mac OS X 10.8.2 (x86_64):
http://mozmill-crowd.blargon7.com/#/remote/report/72e15dc943833b8fcba70aeb51b3aa01

Linux Ubuntu 12.04 (x86):
http://mozmill-crowd.blargon7.com/#/remote/report/72e15dc943833b8fcba70aeb51b3b7a5

Comment 50

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 705813 [details] [diff] [review]
patch v5.1

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

Looks good to me now. Thanks!

Comment 51

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

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.

Comment 52

[mario garbi]

Attachment: patch v5.2

Fixed as requested.

Comment 53

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 708465 [details] [diff] [review]
patch v5.2

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

Requested changes in place.

Comment 54

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

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)

Comment 55

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

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.

Comment 56

[mario garbi]

The patch does indeed applies cleanly across all branches. I can come with additional testrun reports for each branch if you want them.

Comment 57

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

(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.

Comment 58

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

No, just run those and we are fine. Make sure we don't see any failures on default too.

Comment 59

[mario garbi]

They run without problems on default:

Linux:
http://mozmill-crowd.blargon7.com/#/remote/report/4435f497b86a8b1845c2e0223010a4e5

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

Comment 60

[mario garbi]

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.

Comment 61

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

What about tests for aurora and release?

Comment 62

[mario garbi]

Linux - mozilla-release :
http://mozmill-crowd.blargon7.com/#/remote/report/ad726b5c70cf80fbf8135edfca023b2c

Linux - aurora :
http://mozmill-crowd.blargon7.com/#/remote/report/ad726b5c70cf80fbf8135edfca02ce44

Comment 63

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

http://hg.mozilla.org/qa/mozmill-tests/rev/3e8ee90e8d2b (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/d762d5589866 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/45b95e144c4c (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/a12cf1cd8804 (esr17)