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

VERIFIED FIXED

Status

Mozilla QA
Mozmill Tests
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: Aleksej, Assigned: RemusPop)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [testday-20110715][mozmill-test-failure], URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

7 years ago
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*.
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.
(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?
Forgot to CC you Vlad.  See comment 2.
Assignee: nobody → remus.pop
Status: NEW → ASSIGNED
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.
Whiteboard: [testday-20110715] → [testday-20110715][mozmill-test-failure]
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.
How do we skip a test on a conditional? Is there an example?
(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?
(Assignee)

Comment 9

7 years ago
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.
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.
(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");
(Assignee)

Updated

7 years ago
Depends on: 680057

Comment 12

7 years ago
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/17217349
(Assignee)

Comment 13

7 years ago
Setting dependency because we need to know what the search API is going to provide.
Depends on: 680076
(Assignee)

Comment 14

7 years ago
Created attachment 556777 [details] [diff] [review]
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.
Attachment #556777 - Flags: review?(alex.lakatos)
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.
(Assignee)

Comment 16

7 years ago
Created attachment 556831 [details] [diff] [review]
patch v2

getSuggestions now happens in the test function. Assertions are made using the assertions API.
Attachment #556777 - Attachment is obsolete: true
Attachment #556831 - Flags: review?(hskupin)
Attachment #556777 - Flags: review?(alex.lakatos)
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.
Attachment #556831 - Flags: review?(hskupin) → review-
(Assignee)

Comment 18

7 years ago
Created attachment 557825 [details] [diff] [review]
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.
Attachment #556831 - Attachment is obsolete: true
Attachment #557825 - Flags: review?(hskupin)
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.
Attachment #557825 - Flags: review?(hskupin) → review-
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 20

7 years ago
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?
I think that should be fair enough. Hopefully it will never happen.
(Assignee)

Comment 22

7 years ago
Created attachment 558422 [details] [diff] [review]
patch v4

Added an assertion to fail the test in case at least 2 search engines do not provide us with suggestions.
Attachment #557825 - Attachment is obsolete: true
Attachment #558422 - Flags: review?(hskupin)
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+.
Attachment #558422 - Flags: review?(hskupin) → review+
(Assignee)

Comment 24

7 years ago
Created attachment 558461 [details] [diff] [review]
patch v5

Updated assert.equal message.
Attachment #558422 - Attachment is obsolete: true
Attachment #558461 - Flags: review?(hskupin)
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.
Attachment #558461 - Flags: review?(hskupin) → review+
Created attachment 558556 [details] [diff] [review]
For check-in

Or well, lets get feedback/review from you first for those changes.
Attachment #558461 - Attachment is obsolete: true
Attachment #558556 - Flags: review?(remus.pop)
Aleksej, does the final patch fixes the problem for your locale under test?
(Assignee)

Comment 28

7 years ago
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.
Attachment #558556 - Flags: review?(remus.pop) → review+
Aleksej, please mark this as VERIFIED if you can no longer reproduce this failure by checking out the most recent code.
(Reporter)

Comment 31

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

Thanks!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.