671856 - testSearchSuggestions/testMultipleEngines needs to look for two engines with suggestions, not just first two engines

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[[:Aleksej]]

http://mozmill-crowd.brasstacks.mozilla.com/#/functional/report/ca2afbf1bd5ce4e8e57d4acd642ddd6a

testSearch/testSearchSuggestions.js takes first two search engines and compares their suggestions. However, not all engines provide suggestions, so this fails with the Bulgarian build, in whose list only the first (Google) and the last (Wikipedia) engine provide suggestions.

The script needs to find two engines *that provide suggestions*.

Comment 1

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

Aleksej, would you have interests to try to fix a broken tests on your own? As usual I could mentor you through the whole process. This one shouldn't be that hard and could be a good challenge for you. At least when you have interests in coding.

Comment 2

[u279076]

(In reply to Henrik Skupin (:whimboo) in comment #1)
> Aleksej, would you have interests to try to fix a broken tests on your own?
> As usual I could mentor you through the whole process. This one shouldn't be
> that hard and could be a good challenge for you. At least when you have
> interests in coding.

Due to lack of response from Aleksej, I'm going to assume that is a "no". Vlad, can you assign this to one of the new members of the Softvision Desktop Automation team?

Comment 3

[u279076]

Forgot to CC you Vlad.  See comment 2.

Comment 4

[Maniac Vlad Florin (:vladmaniac)]

Done! 

Anthony, no need to cc me in the future, i'm a bug watcher for this component :) 
This looks like we are going to test for Google and Wikipedia engines then compare the suggestions results.

Comment 5

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

So here a proposal for a fix, which was on my own list for this week but lets Remus work on it.

1. Iterate through all existent search engines in setupModule and collect those which support suggestions.
2. If there are more than 2 engines run the test
3. If there is none or only one engine skip the test

Those steps should work with all other locales and simply skips the test if the condition hasn't been met.

Comment 6

[u279076]

How do we skip a test on a conditional? Is there an example?

Comment 7

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

See http://hg.mozilla.org/qa/mozmill-tests/file/3aa1c10a2d1f/tests/functional/restartTests/testSoftwareUpdateAutoProxy/test2.js#l42

Comment 8

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

(In reply to Henrik Skupin (:whimboo) from comment #5)
> 1. Iterate through all existent search engines in setupModule and collect
> those which support suggestions.

Gavin, is there a way to retrieve if a search engine supports suggestions? I can't find any property or method on the nsISearchEngine interface. Is there another function we could use to get this information?

Comment 9

[Remus Pop (:RemusPop)]

I think this could be helpful: http://mzl.la/mS9UEE

It says:
> To support search suggestions, a search plugin needs to define an extra <Url>
> element with its type attribute set to "application/x-suggestions+json".

So basically we could just check if the search engine plugin has that extra url.

Comment 10

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

Search suggestions are handled here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchSuggestions.js

But not sure how to get there from a search engine. I would suggest that you get in contact with Gavin via IRC.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> Gavin, is there a way to retrieve if a search engine supports suggestions?

engine.supportsResponseType("application/x-suggestions+json");

Comment 12

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/17217349

Comment 13

[Remus Pop (:RemusPop)]

Setting dependency because we need to know what the search API is going to provide.

Comment 14

[Remus Pop (:RemusPop)]

Attachment: proposed patch

This gets the suggestions for the installed search engines that supports suggestions. We get all the suggestions in setupModule in order to be able to skip the test if the there are less than 2 engines which support and provide suggestions.

The test itself checks if the provided suggestions from the search engines are different.

Comment 15

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

Comment on attachment 556777 [details] [diff] [review]
proposed patch

>+  searchEngines = searchBar.visibleEngines;
>+
>+  // Get suggestions for all search engines
>+  for (var i = 0; i < searchEngines.length; i++) {
>+    searchBar.selectedEngine = searchEngines[i].name;
>+
>+    // Get suggestions if they are available
>+    suggested = searchBar.getSuggestions("Moz");
>+    if (suggested != null) 
>+      if (suggested.length != 0) {
>+        suggestions.push(suggested);
>+        maxIndex = suggested.length;
>+      }
>+    searchBar.clear();

Please don't retrieve the suggestions in setupModule. Only call our new hasSuggestions method of the search API to determine which of the installed engines support suggestions. If that number is lower than 2 we will skip the test.

Comment 16

[Remus Pop (:RemusPop)]

Attachment: patch v2

getSuggestions now happens in the test function. Assertions are made using the assertions API.

Comment 17

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

Comment on attachment 556831 [details] [diff] [review]
patch v2

>+var enginesWithSuggestions = [ ];

Don't make it a global variable...

>+function setupModule() {

but add the aModule parameter to the function and assign it to aModule.enginesWithSuggestions. That way we have bound it to the module.

>+  // Skip test if we have less than 2 search engines with suggestions
>+  if (enginesWithSuggestions.length < 2)
>+    testMultipleEngines.__force_skip__ = "No suggestions are available for " + 
>+                                         "at least 2 search engines";

The wording of the message needs an update, e.g. "At least two search engines with suggestions are necessary for comparison".

>+function testMultipleEngines() {
>+  var maxIndex = 0, suggestions = [ ], suggested;

Each variable in its own line please.

>+  for (var i = 0; i < enginesWithSuggestions.length; i++) {

Why do you have changed it so we now iterate over all engines with suggestions? Please give a bit more context with a comment when you you attach a patch.

>+    // Get suggestions
>+    suggested = searchBar.getSuggestions("Moz");
>+      if (suggested.length != 0) {
>+        suggestions.push(suggested);
>+        if (maxIndex < suggested.length)
>+          maxIndex = suggested.length;
>+      }
>+      searchBar.clear();

Wrong indentation. Also you should find a better name for suggested, e.g. suggestions vs. allSuggestions. You can also get rid of the second if clause by using Math.min().

>+  for (var j = 0; j < suggestions.length-1; j++) {

Please correct the spacing for 'length-1'.

>+    for (i = 0; i < maxIndex; i++) {
>+
>+      if (suggestions[j][i] != suggestions[j+1][i]) {

Better start with j=1 in the first for loop. Also declare i with var. Otherwise it will be put into the global scope.

Comment 18

[Remus Pop (:RemusPop)]

Attachment: patch v3

In the setup module we check if we have engines that support suggestions and we skip the test otherwise.
in the test function we get suggestions for the found engines and we keep suggestions for the first two of them that return suggestions.

Trying to stick to the original code.

Comment 19

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

Comment on attachment 557825 [details] [diff] [review]
patch v3

>+    // Exit the for loop in case we have suggestions for 2 engines
>+    if (allSuggestions.length == 2)
>+      break;
>   }
> 
>   // Check that at least one suggestion is different
>   var different = false;
>-  var maxIndex = Math.max(suggestions[0].length, suggestions[1].length);
>+  var maxIndex = Math.max(allSuggestions[0].length, allSuggestions[1].length);

If we don't have two engines it will try to access an invalid index and the test will fail with a non-meaningful error message. Please check for the entries first.

Otherwise it looks good.

Comment 20

[Remus Pop (:RemusPop)]

This should be easy. How about using assert.equal checking if we have 2 engines and stop the testing with a fail in case we don't?

Comment 21

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

I think that should be fair enough. Hopefully it will never happen.

Comment 22

[Remus Pop (:RemusPop)]

Attachment: patch v4

Added an assertion to fail the test in case at least 2 search engines do not provide us with suggestions.

Comment 23

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

Comment on attachment 558422 [details] [diff] [review]
patch v4

>+  assert.equal(allSuggestions.length, 2, "At least 2 search engines need to " +
>+               "provide suggestions");
>+

Can you just re-phrase the message to "Suggestions from two search engines are available." With that change you will get my r+.

Comment 24

[Remus Pop (:RemusPop)]

Attachment: patch v5

Updated assert.equal message.

Comment 25

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

Comment on attachment 558461 [details] [diff] [review]
patch v5

Looks good so far. I have had to make some minor updates to the coding style. Please check the interdiff between the upcoming patch and your last one for details. 

Further I did the following:

* We don't need expect, given that the final check could also be an assert.
* I have moved the 'enginesWithSuggestions' variable into setupModule to make it dependent to the module.

I will check the patch in on all branches right away.

Comment 26

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

Attachment: For check-in

Or well, lets get feedback/review from you first for those changes.

Comment 27

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

Aleksej, does the final patch fixes the problem for your locale under test?

Comment 28

[Remus Pop (:RemusPop)]

Comment on attachment 558556 [details] [diff] [review]
For check-in

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

Looks good to be checked in.
I verified this over the released Bulgarian build of Firefox and it passed. As said earlier, it got suggestions for Google and Wikipedia.

Thanks for all the help Henrik.

Comment 29

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/669af7b84328 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/32a467dab51a (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/4f7aefd8bdba (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/9c8def98b5b5 (release)

Comment 30

[u279076]

Aleksej, please mark this as VERIFIED if you can no longer reproduce this failure by checking out the most recent code.

Comment 31

[[:Aleksej]]

VERIFIED FIXED with Bulgarian Nightly build Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110907 Firefox/9.0a1

Thanks!