678456 - getSuggestions in search.js returns suggestions for the first letter only

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Remus Pop (:RemusPop)]

Running testSearchSuggestions.js and making use of getSuggestions() will return suggestions only for the first letter of the search term.

So for calling searchBar.getSuggestions("arelgfjreklgjegkl") we will get the following results:
suggestions: [
["amazon", "aol", "att", "american airlines", "apple", "addicting games", "american idol", "autotrader", "amtrak", "abc"], 

["amazon", "aol", "at&t", "adam sandler death 2011", "autotrader", "amy winehouse", "anne hathaway", "american airlines", "american idol", "addicting games"] ]
for the first two search engines.

We should get no results for that search term.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

On which Firefox build you have tested this? It's mainly the reason because the letters are getting entered too fast. You could add a small sleep between each keypress and check again.

Comment 2

[Remus Pop (:RemusPop)]

I have tested on the released Firefox.
Build identifier: Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20100101 Firefox/5.0

I also noticed that for this release the test needs to wait for the info in suggestion to change. Otherwise entered letters do not change the suggestions pop-up

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

I wonder if this is still a valid bug. Andreea, would you like to mentor this bug?

Comment 4

[Andreea Matei [:AndreeaMatei]]

Sure, I will check if it's still valid when I'll find some time and close it otherwise.

Comment 5

[Balazs Juhasz (:balazs)]

I checked this bug, and this is still valid.

Running testSearchSuggestions.js which uses searchBar.getSuggestions("moz") gets the following suggestions:

[mapquest,maps,msn,miley cyrus,minecraft,macy's,michaels,mcdonalds,menards,metro pcs]
[mapquest,myspace,msn,maps,macy's,mail,movies,miley cyrus,music,my yahoo]

During running the test, it is visible that after we enter the first we wait for suggestions popup to appear. But after it is appeared, all remaining characters entered in very short time (since the popup remains open during typing). Code part from lib/search.js:

    for (var i = 0; i < searchTerm.length; i++) {
      try {
        this.type(searchTerm[i]);
        assert.waitFor(function () {
          return popup.getNode().state === 'open' &&
                 autoCompleteController.searchStatus ===
                 autoCompleteController.STATUS_COMPLETE_MATCH;
        }, "", TIMEOUT_REQUEST_SUGGESTIONS);
      }
      catch (e) {
        // We are not interested in handling the timeout for now
      }
    }

The problem is that the function continues before the suggestions could be refreshed, and return the results which was available after the first charterer entered. I would suggest to wait for popup-open only after the whole search term has been typed - with a maximum timeout in case we do not have any search result. After that, maybe typing characters here one-by-one is not necessary. The search suggestion works if the user paste longer text inside it.

    this.type(searchTerm);
    try {
      assert.waitFor(function () {
        return popup.getNode().state === 'open' &&
               autoCompleteController.searchStatus ===
               autoCompleteController.STATUS_COMPLETE_MATCH;
      }, "", TIMEOUT_REQUEST_SUGGESTIONS);
    }
    catch (e) {
      // We are not interested in handling the timeout for now
    }


With the suggested modifications searching for "moz" gives the following results:
[mozilla firefox,mozilla,mozart,mozilla thunderbird,mozambique,mozilla lightbeam,mozy,moz,mozart requiem,mozzarella]
[mozilla firefox,mozilla,mozart,mozilla download,mozzarella,mozambique,mozilafirefoxdownload,mozilla thunderbird,mozy,mozila]

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Thank you Balazs! That looks fine. Let me CC Cosmin, who recently added this code.

Would you mind to upload the current code for discussion? Thanks.

Comment 7

[Balazs Juhasz (:balazs)]

Attachment: Search for whole search term instead of first letter only - v1

Contains the modification mentioned in comment 5

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Balazs, is your patch ready for feedback or review? If yes, please mark a such.

Comment 9

[Balazs Juhasz (:balazs)]

Comment on attachment 823470 [details] [diff] [review]
Search for whole search term instead of first letter only - v1

Sorry, I was not sure to what to set here. You just asked the code for discussion, so I uploaded the modifications what I made.

The patch is ready for review, I will set the flag now.

Comment 10

[Andrei Eftimie]

Comment on attachment 823470 [details] [diff] [review]
Search for whole search term instead of first letter only - v1

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

Hi Balazs,

As you said this issue is still valid, and your patches fixes it.
The bugs referenced in the comment above bug 542990 and bug 392633 might still be valid, but they are really old bugs.

I do like getting rid of the sequential typing, this has an r+ from me.

Please do run some testruns with this patch.
I'll land it after we have some confirmation that nothing breaks.

Comment 11

[Balazs Juhasz (:balazs)]

Hello Andrei,

Yes, I saw that these bugs are not solved yet, so I left the reference to these bugs in the comment part of the code. Should I remove these references?

Here are some testruns:
Linux x64:
Functional:
http://mozmill-crowd.blargon7.com/#/functional/report/d17690a112360a2b3155acaa276a8622

Remote:
http://mozmill-crowd.blargon7.com/#/remote/report/d17690a112360a2b3155acaa276a88a2

Endurance:
http://mozmill-crowd.blargon7.com/#/endurance/report/d17690a112360a2b3155acaa276a9190

Addons:
http://mozmill-crowd.blargon7.com/#/addons/report/d17690a112360a2b3155acaa276a9bb1

Windows 7 x64:
Functional:
http://mozmill-crowd.blargon7.com/#/functional/report/d17690a112360a2b3155acaa276a9ed4

Remote:
http://mozmill-crowd.blargon7.com/#/remote/report/d17690a112360a2b3155acaa276aac76

Endurance:
http://mozmill-crowd.blargon7.com/#/endurance/report/d17690a112360a2b3155acaa276aae5c

Addons:
http://mozmill-crowd.blargon7.com/#/addons/report/d17690a112360a2b3155acaa276ab9c2

Comment 12

[Andrei Eftimie]

Thanks Balazs.

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/489c9002d66d (default)

I think we're done here.
I wasn't sure if we should backport this; we've decided with the team we should probably not.

Marking as Resolved

Comment 13

[Cosmin Malutan, [:cosmin-malutan]]

Looks to me that this is still blocked by bug 542990 and bug 392633, and we either wait for those to get fixed or find a different way to get suggestions for the whole world typed in and not just for the first letter. Please see bug 942737.

Comment 14

[Balazs Juhasz (:balazs)]

The last patch's most important thing was to add wait for search suggestion results after entering the whole search term. Removing the typing of searchterm one-by-one was only an improvement. Maybe we could check if the typing remains in a for-cycle and the popup check done afterwards

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

So what's up with this bug? It somehow got reopened? What's left to do here?

Comment 16

[Cosmin Malutan, [:cosmin-malutan]]

Typing all letters at once instead of one by one as it was before made the test fail with "Suggestions from two search engines are available" due to bug 392633. But since we moved to our own engines this shouldn't be the case, and the failure should be fixed in bug 942737.

Comment 17

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Balazs Juhasz (:balazs) from comment #14)
> The last patch's most important thing was to add wait for search suggestion
> results after entering the whole search term. Removing the typing of
> searchterm one-by-one was only an improvement. Maybe we could check if the
> typing remains in a for-cycle and the popup check done afterwards
The patch it's exactly removing the for loop and the comment which states why the loop was added  and nothing else. While the bug 392633 is still open the fix should stay in library. Since this land failures like "Suggestions from two search engines are available" increased a lot because we overrun the server response(bug 392633). We can have this fix once we have search engines that work completely locally or bug 392633 get's fixed until then I think we should back-out this patch.

Having suggestions only for first letter it's not that bad but it can be fixed if we make sure we are not in finish state of the first letter:
>    for (var i = 0; i < aSearchTerm.length; i++) {
>      try {
>        this.type(aSearchTerm[i]);
>        expect.waitFor(() => autoCompleteController.searchStatus !==
>                             autoCompleteController.STATUS_COMPLETE_MATCH,
>                       "", TIMEOUT_REQUEST_SUGGESTIONS);
>        expect.waitFor(() => {
>          return popup.getNode().state === 'open' &&
>                 autoCompleteController.searchStatus ===
>                 autoCompleteController.STATUS_COMPLETE_MATCH;
>        }, "", TIMEOUT_REQUEST_SUGGESTIONS);
>      }
>      catch (e) {
>        // We are not interested in handling the timeout for now
>      }
>    }
http://hg.mozilla.org/qa/mozmill-tests/file/489c9002d66d/lib/search.js#l712

Comment 18

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: follow-up patch

This patch as I said it will make the getSuggestion to type letters one by one and also to don't return after the first found.

Comment 19

[Andrei Eftimie]

Comment on attachment 8516014 [details] [diff] [review]
follow-up patch

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

I think I understand now what you want this to do, in regards to queueing async calls and waiting for each of them in turn to resolve, so they don't step over each other's toes.

::: firefox/lib/search.js
@@ +723,5 @@
> +        this.type(aSearchTerm[i]);
> +        expect.waitFor(() => autoCompleteController.searchStatus !==
> +                             autoCompleteController.STATUS_COMPLETE_MATCH,
> +                       "", TIMEOUT_REQUEST_SUGGESTIONS);
> +        expect.waitFor(() => {

Make this the same style as the above waitFor call (without curly brackets and return statement)

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Comment on attachment 8516014 [details] [diff] [review]
follow-up patch

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

Usually we should not re-open a bug which is months old. In the future please really file a new one. Otherwise it looks fine to me, but really check my inline comment. If there is really no indication for a race-condition you have my r+.

::: firefox/lib/search.js
@@ +723,5 @@
> +        this.type(aSearchTerm[i]);
> +        expect.waitFor(() => autoCompleteController.searchStatus !==
> +                             autoCompleteController.STATUS_COMPLETE_MATCH,
> +                       "", TIMEOUT_REQUEST_SUGGESTIONS);
> +        expect.waitFor(() => {

Personally I find the way with brackets way better readable as that one. It may also be because of the indentation.

@@ +726,5 @@
> +                       "", TIMEOUT_REQUEST_SUGGESTIONS);
> +        expect.waitFor(() => {
> +          return popup.getNode().state === 'open' &&
> +                 autoCompleteController.searchStatus ===
> +                 autoCompleteController.STATUS_COMPLETE_MATCH;

Keep in mind that this code will still fail when the response comes in too quick! This is most likely going to be a race condition. An event we could listen for would be much better here. Got this a stress test with locally hosted search engines? If not please really do so. I don't want that we move from one frequent failure to the next one.

Comment 21

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: follow-up patch v2.0

(In reply to Henrik Skupin (:whimboo) from comment #20)
> Comment on attachment 8516014 [details] [diff] [review]
> Usually we should not re-open a bug which is months old. In the future
> please really file a new one. Otherwise it looks fine to me, but really
> check my inline comment. If there is really no indication for a
> race-condition you have my r+.
The race condition if it occurs it will be when we wait for a new search to begin and it already stepped over that, in that case it will delay the test with 1s, that's all, we will still have the suggestions.

> Got this a stress test with locally
> hosted search engines? If not please really do so. I don't want that we move
> from one frequent failure to the next one.
I hosted the engine responses with Apache locally and I ran  the test for 300 times and it passes.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

(In reply to Cosmin Malutan from comment #21)
> > Usually we should not re-open a bug which is months old. In the future
> > please really file a new one. Otherwise it looks fine to me, but really
> > check my inline comment. If there is really no indication for a
> > race-condition you have my r+.
> The race condition if it occurs it will be when we wait for a new search to
> begin and it already stepped over that, in that case it will delay the test
> with 1s, that's all, we will still have the suggestions.

No, if the response of the search is coming in too fast, your first waitFor call will run until it times out. Because the state you are waiting for is missing, e.g. it was set and reset while our waitFor was in the 100ms delay.

> > Got this a stress test with locally
> > hosted search engines? If not please really do so. I don't want that we move
> > from one frequent failure to the next one.
> I hosted the engine responses with Apache locally and I ran  the test for
> 300 times and it passes.

Well, so lets see.

Comment 23

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #22)
> No, if the response of the search is coming in too fast, your first waitFor
> call will run until it times out. Because the state you are waiting for is
> missing, e.g. it was set and reset while our waitFor was in the 100ms delay.
Indeed, but we are in a try-catch and we only care about the suggestions, which we will have.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

This is indeed a good safe guard, and should keep us running.

Comment 25

[Andrei Eftimie]

Comment on attachment 8516547 [details] [diff] [review]
follow-up patch v2.0

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

Let's get this in then:
(I updated the commit message slightly, it had the wrong bug id)

https://hg.mozilla.org/qa/mozmill-tests/rev/d03a0d08cd1a (default)

Comment 26

[Andrei Eftimie]

Pushed to Aurora as well:
https://hg.mozilla.org/qa/mozmill-tests/rev/7d1b0c6ba8a6 (mozilla-aurora)

Comment 27

[Andrei Eftimie]

Cosmin, please check how this performs on Nightly & Aurora by tomorrow, and check backports for Beta, Release and ESR31

Comment 28

[Cosmin Malutan, [:cosmin-malutan]]

No failure on the last nightly ran, let's backport it to aurora and beta.
http://mozmill-daily.blargon7.com/#/functional/top?app=All&branch=All&platform=All&from=2014-11-04&to=

Comment 29

[Andrei Eftimie]

Transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/907d141f2822 (mozilla-beta)

Comment 30

[Cosmin Malutan, [:cosmin-malutan]]

No failure with this applied on beta, please backport it to release and close it.
http://mozmill-release.blargon7.com/#/functional/failure?app=Firefox&branch=34.0&platform=All&from=2014-11-06&to=&test=%2FtestSearch%2FtestSearchSuggestions.js&func=testMultipleEngines

Comment 31

[Andrei Eftimie]

Transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/edc0ac2c084f (mozilla-release)

Have come conflicts on ESR31:
> Hunk #1 FAILED at 12
> Hunk #2 FAILED at 710
> 2 out of 2 hunks FAILED -- saving rejects to file firefox/lib/search.js.rej

Comment 32

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.1[ESR31]

This is the patch for esr31. Thanks

Comment 33

[Andrei Eftimie]

Comment on attachment 8519742 [details] [diff] [review]
patch v2.1[ESR31]

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

https://hg.mozilla.org/qa/mozmill-tests/rev/34a9fa286f3d (mozilla-esr31)