898568 - Test Failure "Suggestions Mozilla from Google and SAPO search providers are different - got 'false' " in /testSearch/testSearchSuggestions.js::testMultipleEngines

Status: RESOLVED FIXED

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

Description

[Tracy Walker [:tracy]]

reported from http://mozmill-release.blargon7.com/#/functional/report/b3aefd29a5c41bda1cc43616d31464cb

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

Happened on Ubuntu 12.10 (x86) with Release csb:
http://mozmill-release.blargon7.com/#/functional/report/b7ef1fb3d9703aeaf2c46e07d2e5519c 
Happened on Ubuntu 12.10 (x86) with Release tr:
http://mozmill-release.blargon7.com/#/functional/report/b7ef1fb3d9703aeaf2c46e07d2e609ed
Happened on Ubuntu 12.10 (x86) with Release cs:
http://mozmill-release.blargon7.com/#/functional/report/b7ef1fb3d9703aeaf2c46e07d2e4b464

Comment 2

[Andrei Eftimie]

Failed on it with "Bing":
http://mozmill-daily.blargon7.com/#/functional/report/6d53b7d749a60e65551b4b1d19afe113

Comment 3

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

Is this reproducible for the affected locales?

Comment 4

[Cosmin Malutan, [:cosmin-malutan]]

I couldn't reproduce. Neither with it or cs locales.

Comment 5

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

K, so given that it happens with beta and release candidate builds, we might have to get this fixed. Setting as P2 for now.

Comment 6

[Andrei Eftimie]

Failed on cs:
http://mozmill-release.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a153adb4

Different engine:
Suggestions Mozilla from Google and Seznam search providers are different - got 'false'

Comment 7

[Andreea Matei [:AndreeaMatei]]

Happened with Beta - en-GB locale, Linux Ubuntu 13.04:
http://mozmill-release.blargon7.com/#/functional/report/dc3d4d2f66af31b9e75e291fa0f8010f

Mario is trying to reproduce it on the same machine, no luck so far.

Comment 8

[mario garbi]

I tried to reproduce it on the failing machine most of the day with the same build but as Andreea said we had no luck in reproducing it. I have also tested it on my Ubuntu 12.04 without luck.

http://mozmill-crowd.blargon7.com/#/functional/reports?branch=25.0&platform=Linux&from=2013-09-15&to=2013-09-18

Comment 9

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.0

Hi,
I investigated this bug and I got to conclusion that it failed because we weren't waiting for all suggestions but only for pop-up to open. On very fast machines it might update only one suggestion (like in this case) for both engines compared.  In our case we searched for "Moz" and the first and only suggestion it was "Mozilla" for both search engines.

// Message with only one suggestion:
"Suggestions Mozilla from Google and SAPO search providers are different"
// Message with more than one suggestion:
"Suggestions mozilla firefox, mozilla, mozart, mozilla thunderbird, mozambique, mozy, mozzarella, moz, mozza, mozart requiem from Google and Yahoo search providers are different"

I propose to wait for nsIAutoCompleteController search status to be completed.
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIAutoCompleteController

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d02880
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d0378a
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d01c3b

Comment 10

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

Comment on attachment 813459 [details] [diff] [review]
patch_v1.0

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

With the two changes made you get my r+.

::: lib/search.js
@@ +414,5 @@
>   */
>  function searchBar(controller) {
>    this._controller = controller;
> +  this.autoCompleteCtrl = Cc["@mozilla.org/autocomplete/controller;1"].
> +                          getService(Ci.nsIAutoCompleteController);

I like that! Great investigation, Cosmin. Only one thing I would like to see is that we cache the service globally in that module. Also please don't shorten Controller.

@@ +715,2 @@
>          assert.waitFor(function () {
> +          return popup.getNode().state === 'open' &&

Do we really have to wait for open? I think it would be enough to wait for the finished match.

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.1

(In reply to Henrik Skupin (:whimboo) from comment #10)
> Do we really have to wait for open? I think it would be enough to wait for
> the finished match.

From what I tested so far it works just fine if we wait only for the finished match, I don't expect to fail without it, if it will at some point I will add it back.

Comment 12

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 813497 [details] [diff] [review]
patch_v1.1

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

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

Lets wait until Monday for backporting.

Comment 13

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: fallowUp_default.patch

By not waiting for pop-up state, I introduced a regression since caused those failures :
http://mozmill-daily.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750f07e77
http://mozmill-daily.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750f0a852
Here is the fallow up patch to fix that.

Comment 14

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: fallowUp_default.patch

I changed the end(&&) by mistake because of the rush, sorry.

Comment 15

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 813737 [details] [diff] [review]
fallowUp_default.patch

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

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

Hope this will do the trick.

Comment 16

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

Cosmin, as you mentioned earlier waiting for the popup open state is not necessary. Why do we have to re-introduce that check again?

Comment 17

[Cosmin Malutan, [:cosmin-malutan]]

It failed on testruns, it looks like when we type the second letter the search result is already in state 4, and it passes the assertion, the same as for further searches. So on the search for the next suggestion we need to check that is state is not already "complete" from the previous step.
Waiting for pop-up to open it just delays the controller and it changes from State 4 to 3 on the second search.
Dumps in assertion without waiting for open:
>TEST-START | /Users/cosmin/Desktop/clean/mozmill-tests/tests/functional/testSearch/testSearchSuggestions.js | testMultipleEngines
>
> autoCompleteController.searchStatus 1
> autoCompleteController.searchStatus 1
> autoCompleteController.searchStatus 1
> autoCompleteController.searchStatus 2
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 4TEST-UNEXPECTED-FAIL | /Users/cosmin/Desktop/clean/mozmill-tests/tests/functional/testSearch/testSearchSuggestions.js | testSearchSuggestions.js::testMultipleEngines

Dumps in assertion when waiting for state to be different than 4, before assertion:
> TEST-START | /Users/cosmin/Desktop/clean/mozmill-tests/tests/functional/testSearch/testSearchSuggestions.js | testMultipleEngines
>
> autoCompleteController.searchStatus 1
> autoCompleteController.searchStatus 1
> autoCompleteController.searchStatus 1
> autoCompleteController.searchStatus 2
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 2
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 2
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 2
> autoCompleteController.searchStatus 2
> autoCompleteController.searchStatus 2
> autoCompleteController.searchStatus 2
> autoCompleteController.searchStatus 2
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 2
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 2
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 2
> autoCompleteController.searchStatus 4
> autoCompleteController.searchStatus 2
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 3
> autoCompleteController.searchStatus 2
> autoCompleteController.searchStatus 4TEST-PASS | /Users/cosmin/Desktop/clean/mozmill-tests/tests/functional/testSearch/testSearchSuggestions.js | testSearchSuggestions.js::testMultipleEngines

Comment 18

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: fallow-up_2_default.patch

After those failures from friday:
http://mozmill-daily.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750f07e77
http://mozmill-daily.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750f0a852
I found out as I said earlier, that after the first search search-suggestions controller is already on state 4, and we should check that is not, on a new search.

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea2b7823
http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea2b8707
http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea2b74b0
http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea2b9a6b
http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea2bad13
http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea2bb1d0

Comment 19

[Andrei Eftimie]

Comment on attachment 814046 [details] [diff] [review]
fallow-up_2_default.patch

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

As noted above, good find in using nsIAutoCompleteController instead of just waiting for the panel to be open.

It would be great not to push several follow-up patches on issues.
In most instances it would be better to test things through than to quickly push fixes.

I like the approach, but please fix the comment noted below.

Also another nit: it is "follow" not "fallow"

::: lib/search.js
@@ +711,5 @@
>      for (var i = 0; i < searchTerm.length; i++) {
>        try {
>          this.type(searchTerm[i]);
> +
> +        // Check that is searching, so state is not already complete due to a previous search

Please fix this comment. This phrase is broken and confusing.
You can simplify it, just state what we are doing/waiting for:

// Wait for the search to start
Should probably suffice

Comment 20

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: follow-up_2_default.patch

(In reply to Andrei Eftimie from comment #19)
> It would be great not to push several follow-up patches on issues.
> In most instances it would be better to test things through than to quickly
> push fixes.
I will be more careful from now on, thanks.

> Please fix this comment. This phrase is broken and confusing.
> You can simplify it, just state what we are doing/waiting for:
> 
> // Wait for the search to start
> Should probably suffice
Done that.

Comment 21

[Andrei Eftimie]

Comment on attachment 814344 [details] [diff] [review]
follow-up_2_default.patch

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

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

ps. you had the patch header twice, and the User field had encoded brackets around the email address, I fixed it.

For backporting please merge all 3 landed patches into 1.
Thanks

Comment 22

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.0 aurora

Here is the patch for aurora
Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea5fd03f
http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea608773
http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea60e1e4
http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea602b8d
http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea5fe4d0
http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea60aa31

Comment 23

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

Comment on attachment 814371 [details] [diff] [review]
patch_v2.0 aurora

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

::: lib/search.js
@@ +716,4 @@
>          assert.waitFor(function () {
> +          return autoCompleteController.searchStatus ===
> +                 autoCompleteController.STATUS_SEARCHING;
> +        }, "", TIMEOUT_REQUEST_SUGGESTIONS);

I really hope we do not miss that status given that we have a 100ms timeout for the checks. If search has been finished in-between, we will timeout here.

Comment 24

[Cosmin Malutan, [:cosmin-malutan]]

Hi Gavin, I tried to catch an event from nsIAutoCompleteController to know when the search starts and ends, because if we are watching searchStatus attribute this might change to fast and our assertion will fail.
Do you know how we could catch the event that's causing the searchStatus attribute to change ? Or do you know another way we could get announced that the search has been completed ?

Comment 25

[Andrei Eftimie]

Comment on attachment 814371 [details] [diff] [review]
patch_v2.0 aurora

I'm taking the review flags out since we might change things on default, in that case we'll need another patch to backport.

Comment 26

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

I think there aren't any great ways to observe autocomplete behavior as it is. Polling as those patches do is likely to be flaky, particularly if the polling interval isn't low enough.

The autocomplete input binding offers searchbegin/searchcomplete pseudo-event handlers, but there can only be one specified using the "onsearchbegin" or "onsearchcomplete" attributes, which isn't ideal (and risks breaking other stuff if you use it for these tests and we then try to use it it again in-browser).

We could improve that situation if you wanted to file a bug and suggest a more robust way to be notified of these types of state changes.

Comment 27

[Andreea Matei [:AndreeaMatei]]

We should backout the followup that created 2 waitFors for now until we find a better solution, as we are still failing in bug 862687, cause we never get to the second waitFor.

http://hg.mozilla.org/qa/mozmill-tests/rev/df1a8418ed67 (default)

Switching back to affected as this is just a workaround and we want a better solution.

Comment 28

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

I haven't used MutationObservers yet that much, but would one of those help us here? That way we wouldn't miss a change of a property.

Comment 29

[Cosmin Malutan, [:cosmin-malutan]]

Hi whimboo, I looked in to this today, and the only thing that we can watch with MutationObsever is if pop-up is open, but for that we have state attribute.
I would say that this is fixed on nightly as it did't failed again since we wait for state "open" again too. 
The initial issue it was that after the pop-up opened we did't waited for a full update, something that we do now.
As for waiting for a new search to start, this is already implemented (in getSuggestions method, we wait for pop-up to open get suggestions, clear, and wait for pop-up to close).
If it won't fail again on nightly over the weekend I will make a backport patch and reports for the remaining branches.

Comment 30

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.1_aurora.patch

Here is the patch for Aurora as I said.
It did't fail on Nightly, so I say it's safe to rely on suggestions pop-up state (open/close) for determining a new search, and on nsIAutoCompleteController, that all suggestions have been appended. So we will not fail with only one suggestion.

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/bb6e314abd8f1e30e8af61ecd4ae5d6c
http://mozmill-crowd.blargon7.com/#/functional/report/bb6e314abd8f1e30e8af61ecd4a8aa56
http://mozmill-crowd.blargon7.com/#/functional/report/bb6e314abd8f1e30e8af61ecd4af9017

Comment 31

[Andrei Eftimie]

Comment on attachment 820875 [details] [diff] [review]
patch_v2.1_aurora.patch

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

So we seem to be green on Nightly with the current version.

Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/f9780d14879d (mozilla-aurora)

Comment 32

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.1_esr17

Here is the patch for Esr17

Reports
http://mozmill-crowd.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb896ea1b
http://mozmill-crowd.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb896dda2
http://mozmill-crowd.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb896dc82

Comment 33

[Cosmin Malutan, [:cosmin-malutan]]

Patch for aurora applies cleanly on release branch too.

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb8a9c186
http://mozmill-crowd.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb8a9d22f
http://mozmill-crowd.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb8a9c93c

Comment 34

[Andrei Eftimie]

Cosmin, before checking ESR17 we still need this on 25 and ESR24

And I don't think we're going to push any more changes to ESR17.

Comment 35

[Andrei Eftimie]

Transplanted to release:
http://hg.mozilla.org/qa/mozmill-tests/rev/4020a74904cd (mozilla-release)

Comment 36

[Andrei Eftimie]

Comment on attachment 824083 [details] [diff] [review]
patch_v2.1_esr17

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

Marking ESR17 as WONTFIX, we are not longer pushing anything to that branch.

Comment 37

[Andrei Eftimie]

Only 1 thing remaining, please check ESR24, then we can close this bug.

Comment 38

[Cosmin Malutan, [:cosmin-malutan]]

Patch applies cleanly on esr24 too.
Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb8a9f6f8
http://mozmill-crowd.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb8a9fb63
http://mozmill-crowd.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb8aa035a

Comment 39

[Andrei Eftimie]

Great, transplanted to esr24:
http://hg.mozilla.org/qa/mozmill-tests/rev/6dcbce2999fe (mozilla-esr24)

We're all done here,
Thanks Cosmin.