Closed Bug 942737 Opened 11 years ago Closed 10 years ago

Test failure "Suggestions from two search engines are available - '1' should equal '2' " in testSearchSuggestions.js

Categories

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

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Type:
defect

Tracking

(firefox30 disabled, firefox31 wontfix, firefox32 wontfix, firefox33 affected, firefox34 affected, firefox35 affected, firefox36 affected, firefox-esr24 wontfix, firefox-esr31 affected)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox30 --- disabled
firefox31 --- wontfix
firefox32 --- wontfix
firefox33 --- affected
firefox34 --- affected
firefox35 --- affected
firefox36 --- affected
firefox-esr24 --- wontfix
firefox-esr31 --- affected

People

(Reporter: cosmin-malutan, Assigned: cosmin-malutan)

References

(
URL
)

Details

(Keywords: intermittent-failure, Whiteboard: [mozmill-test-failure][Blocked by bug 994102])

Attachments

(3 files, 10 obsolete files)

942737[skip].patch
1.76 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
patch_v4.1
15.12 KB, patch
whimboo
: review+
AndreeaMatei
: checkin+
Details | Diff | Splinter Review
942737[follow-up].patch
5.49 KB, patch
AndreeaMatei
: review-
Details | Diff | Splinter Review 
This failed on Windows 8 x86 & x64 with Nightly - it
http://mozmill-daily.blargon7.com/#/functional/report/456bebe92845279408c15c03e83dc8d9

I reproduced this locally.

I don't expect to be a timeout caused failure (bug 862687) since it failed only twice and only on Nightly in the past month. 
I will investigate more and return with more info.
I ran this test all day so:
In 500 runs it failed once: and with timeout for getting suggestion set to 30s
If we wait for all available engines suggestions it failed about 5 times in 100 runs
If I revert the changes from 678456, it doesn't fails in 100 runs 

So I would say that bug 678456 is still blocked by 542990 and 392633.
I will run some more tests tomorrow to be absolute sure, before I reopen it.
Today I changed the assertion from 2 to enginesWithSuggestions.length so it will catch all searches in 
http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/tests/functional/testSearch/testSearchSuggestions.js#l63
I run the test for 1000 times without the change from 678456, and it didn't failed once.
I run the test for 1000 with the change applied and it failed for 5 times.
status-firefox28: --- → affected
Depends on: 678456
Failed again:
http://mozmill-daily.blargon7.com/#/functional/report/40742e0746fd767e6d2fd5865929529a

We should look again into bug 678456 and see if we can find a permanent fix
Keywords: intermittent-failure
Whiteboard: [mozmill-test-failure]
Another report (thanks Anthony):
http://mozmill-release.blargon7.com/#/functional/report/0906e2c8b6f223d6588987bcd1ceec81
status-firefox28: --- → affected
OS: Windows 8 → All
Priority: P4 → P3
Summary: Test failure "Suggestions from two search engines are available - '1' should equal '2' " → Test failure "Suggestions from two search engines are available - '1' should equal '2' " in testSearchSuggestions.js
Firefox 28.0 (28.0, pt-PT, 20140213172947) Linux Ubuntu 13.10 (x86_64):
http://mozmill-release.blargon7.com/#/functional/report/0100465a763e4862d40040538421ac78
Firefox 28.0 (28.0, ms, 20140218122424) - Linux Ubuntu 13.10 (x86_64):
http://mozmill-release.blargon7.com/#/functional/report/6062fdddb60e88657bc78ec1f43b422d

Firefox 28.0 (28.0, pt-PT, 20140218122424) - Linux Ubuntu 13.10 (x86_64):
http://mozmill-release.blargon7.com/#/functional/report/6062fdddb60e88657bc78ec1f439e7d1
Firefox 28.0 (28.0, pt-PT, 20140224220227) - Linux Ubuntu 13.10 (x86_64):
http://mozmill-release.blargon7.com/#/functional/report/e248c845ffb052c61b3635551b04f388
Failed today multiple times on :

Firefox Beta ( 28.0 pl) on Mac OSX 10.8 - (mm-osx-108-4)
http://mozmill-release.blargon7.com/#/functional/report/e248c845ffb052c61b3635551b4db3a4

Firefox Beta (28.0 pt-BR) on Windows XP 32 - (mm-win-xp-32-1)
http://mozmill-release.blargon7.com/#/functional/report/e248c845ffb052c61b3635551b56c582

Firefox Beta (28.0 ja) on Windows 8.1 32 (mm-win81-32-4)
http://mozmill-release.blargon7.com/#/functional/report/e248c845ffb052c61b3635551b570476

Firefox Beta (28.0 ja) on Windows 7 32 (mm-win7-32-1)
http://mozmill-release.blargon7.com/#/functional/report/e248c845ffb052c61b3635551b56e921

Firefox Beta (28.0 ja) on Windows 7 64 (mm-win7-64-3)
http://mozmill-release.blargon7.com/#/functional/report/e248c845ffb052c61b3635551b573301

Firefox Beta (28.0 ja) on Windows 8 64 (mm-win-8-64-3)
http://mozmill-release.blargon7.com/#/functional/report/e248c845ffb052c61b3635551b5741b4
Following the failures we had today, I run the testSearch/ folder tests about 500 times on my Windows 8.1 64 and it didn't reproduce at all.

http://mozmill-crowd.blargon7.com/#/functional/reports?app=All&branch=28.0&platform=All&from=2014-02-28&to=2014-02-28
This test fails reliably with Firefox 28.0build1 pt-PT on Ubuntu 13.10
This is blocking QA from release testing. So its clearly a P1. Daniel, can you please have a look at this?
Assignee: cosmin.malutan → daniel.gherasim
Priority: P3 → P1
I investigated this a bit in the morning as it was reproducible with pt-PT build, thanks Anthony.

As I said in comment 2 this has been introduced again by bug 678456 which is still dependent on bug 392633. In bug 678456 we removed the for loop for typing letter by letter in order to have suggestions for the whole word and not only for the first latter as happened in some cases. 
Because bug 678456 is still dependent on bug 392633 we have to roll back to the for loop but also close the pop-up after each letter so we will have a fresh search.
I tested this and it works, Daniel can you make a patch for this please?  

>    // Bug 542990
>    // Bug 392633
>    // Type search term and wait for the popup or the timeout
>    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);
>        if (i < (searchTerm.length - 1)) {
>          popup.getNode().closePopup();
>        }
>      }
>      catch (e) {
>        dump("\n"+e.message+"\n");
>        // We are not interested in handling the timeout for now
>      }
>    }
With this it still fails if we close the pop-up, it might not open again when we type the second time if we don't have a suggestion, so that is not reliable.
At very least this will pass if we just introduce the for loop.
Thanks Anthony for your information, I've tested on the build you said and found this:

So in 100 runs, we always get suggestions from Google, but very often fail when getting them from SAPO, even though it is marked as an "ENGINE_WITH_SUGGESTIONS". Obtainig an emtpy _ARRAY_ will go into failing with the error present here.

That's something I can reproduce manually too, sometimes we fail on getting suggestions from this search provider.

So I followed some steps to see if it's real a problem with our code or with the SEARCH ENGINE.

1. I inversed the order to test the search engines ( SAPO -> GOOGLE)
  * Google always returns the right results;
  * SAPO fails a lot when getting the suggestions, again;
2. Modified the test to avoid SAPO
  * We always PASS;
3. Modified the test to just test the SAPO engine;
  * We fail a lot (about 50%);
4. Tested with GOOGLE repeated 5 times in the same test, to see if it's something with the steps we do.
  * No fail, runs ok all the time;

Having the for loop reintroduced as Cosmin suggested will cause lower frequency of failures but won't fix the problem.

So as Henrik said here https://bugzilla.mozilla.org/show_bug.cgi?id=392633#c5 ... for some search engines we fail getting the auto-complete results.

I'm thinking of 2 solutions here 

1. Skip the test not only if we don't have at least 2 search engines with SUGGESTIONS ENABLED, but to skip it if we don't have at least 2 search engines with SUGGESTIONS PRESENT, otherwise we would have problems with different search engines from different locales too, 
2. Retry several times to obtain the suggestions if we recieve an empty array from the first time.
First, we should really move out those search engine tests to the remote testrun. Nearly all of them using remote engines, which should not be allowed in our functional testrun. There is an open bug about that. Daniel, would be great if we could do this first, so we get cleaner functional tests.

Then we could figure out what we have to do to create our own search engine auto-complete feature. I don't think that this should be too hard. That way we can reduce the dependency on slow-reacting services like that one.

On the other side we might want to report those slow auto-completion engines, so we can work with them to get it improved or replaced. Anthony, would you mind to bring in the right contact here?
You're right, I was thinking to have this moved to the remote tests too.
I'll take care of that and then fill a bug to create our own auto-complete feature.

--
As an update of what I suggested above, the 2nd solution seems to work fine, no failure seen in 120 runs until now.
Failed today with Nightly it (20140323030203) on Windows 7 64 (mm-win-7-64-4)
http://mozmill-daily.blargon7.com/#/functional/report/97976110c92e838016d17f7fb21ab671
Failed again with Aurora it (20140324150430) on Ubuntu 12.04 64 (mm-ub-1204-64-4)
http://mozmill-daily.blargon7.com/#/functional/report/97976110c92e838016d17f7fb249aeb5

Release ja on Ubuntu 12.04 64 (2014-03-24_13-34-16)
http://mozmill-release.blargon7.com/#/functional/report/97976110c92e838016d17f7fb23967b4
(In reply to Henrik Skupin (:whimboo) from comment #21)
> Then we could figure out what we have to do to create our own search engine
> auto-complete feature. I don't think that this should be too hard. That way
> we can reduce the dependency on slow-reacting services like that one.
If we decide to go with this approach I would like to work on this. It failed a lot this couple of days.

> On the other side we might want to report those slow auto-completion
> engines, so we can work with them to get it improved or replaced. Anthony,
> would you mind to bring in the right contact here?
Adding need info for Anthony
Flags: needinfo?(anthony.s.hughes)
From what I understood this test is for checking that search-engine changes and the suggestions are not the same with the ones from previous one. We already have two engines in data directory, so I suggest we enhance them to offer different suggestions and check them.
For offering suggestion we can deliver an static json file with mock-ups or we could create a HttpServer, and register a path handler (registerPathHandler).
I'm open for any input here regarding what we should do.
I would go for the easier route of delivering static JSON responses.

As for remote vs local search engines, we might want to duplicate the existing ones and changing them to work fully offline. We might group them by subfolders like /data/search/local and /data/search/remote or maybe just append "_local" to their name.

If we only change them and enforce local use they won't work unless a local webserver is running.
Since they are also in the test-data repository we shouldn't break existing functionality.
If we have to go and fork the search engines to make them work wo/ a network, lets do that. I don't see why we have to be totally in sync with test-data. While having search engine results in our own hand, the automated tests have a way stronger requirement. So if we can make them local, lets do it! And yes, delivering static content is the right way to go here. Just make it a good list of entries, so they can be used in different tests.
(In reply to Henrik Skupin (:whimboo) from comment #29)
> If we have to go and fork the search engines to make them work wo/ a
> network, lets do that. I don't see why we have to be totally in sync with
> test-data. While having search engine results in our own hand, the automated
> tests have a way stronger requirement. So if we can make them local, lets do
> it! And yes, delivering static content is the right way to go here. Just
> make it a good list of entries, so they can be used in different tests.

If we don't have to be in sync with test-data then yes, lets just change our local search engines to work locally. Makes things easier!
I'm not sure how to help here. Making the engines local seems the right approach. We really aren't in the business of testing third-party search engines. We just want to make sure the mechanism built into Firefox is working.
Flags: needinfo?(anthony.s.hughes)
Whereby I would start to get the engines updated in test-data first. As of now we have a direct connection to mozqa.com, and retrieving data should mainly never fail, except the whole network is done and then even Jenkins will not work at all. So lets update the testcases on mozqa first, which might already be enough. If not we can still spend time on making really local tests out of it later.
Attached patch patch_v1.0[testcase-data] (obsolete) — — Splinter Review 
Thanks for input guys, here is the patch for updating the testcase-data.
Assignee: daniel.gherasim → cosmin.malutan
Attachment #8397729 - Flags: review?(andrei.eftimie)
Attachment #8397729 - Flags: review?(andreea.matei)
This is the mozmill-test component. Updates or new test files for testcase-data have to be filed in infrastructure.
Depends on: 988867
Comment on attachment 8397729 [details] [diff] [review]
patch_v1.0[testcase-data]

I filed bug 988867 for this.
Attachment #8397729 - Attachment is obsolete: true
Attachment #8397729 - Flags: review?(andrei.eftimie)
Attachment #8397729 - Flags: review?(andreea.matei)
Attached patch patch_v1.0 (obsolete) — — Splinter Review 
This patch is for feedback only until bug 988867 gets landed.
Here I install the two local engines and then I retrieve the suggestions for them as before. A thing here is that After I install the engine I have to add a sleep otherwise by time to time the first engine won't have any suggestion.
Attachment #8398587 - Flags: feedback?(andrei.eftimie)
Attachment #8398587 - Flags: feedback?(andreea.matei)
Comment on attachment 8398587 [details] [diff] [review]
patch_v1.0

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

::: data/search/mozsearch.xml
@@ +2,5 @@
>    <ShortName>mozqa.com</ShortName>
>    <Description>Mozqa.com search results</Description>
>    <InputEncoding>UTF-8</InputEncoding>
>    <Image width="16" height="16">data:image/x-icon;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8%2F9hAAAABGdBTUEAAK%2FINwWK6QAAABl0RVh0U29mdHdhcmUAQWRvYmUgSW1hZ2VSZWFkeXHJZTwAAAHWSURBVHjaYvz%2F%2Fz8DJQAggJiQOe%2Ffv2fv7Oz8rays%2FN%2BVkfG%2FiYnJfyD%2F1%2BrVq7ffu3dPFpsBAAHEAHIBCJ85c8bN2Nj4vwsDw%2F8zQLwKiO8CcRoQu0DxqlWrdsHUwzBAAIGJmTNnPgYa9j8UqhFElwPxf2MIDeIrKSn9FwSJoRkAEEAM0DD4DzMAyPi%2FG%2BQKY4hh5WAXGf8PDQ0FGwJ22d27CjADAAIIrLmjo%2BMXA9R2kAHvGBA2wwx6B8W7od6CeQcggKCmCEL8bgwxYCbUIGTDVkHDBia%2BCuotgACCueD3TDQN75D4xmAvCoK9ARMHBzAw0AECiBHkAlC0Mdy7x9ABNA3obAZXIAa6iKEcGlMVQHwWyjYuL2d4v2cPg8vZswx7gHyAAAK7AOif7SAbOqCmn4Ha3AHFsIDtgPq%2FvLz8P4MSkJ2W9h8ggBjevXvHDo4FQUQg%2FkdypqCg4H8lUIACnQ%2FSOBMYI8bAsAJFPcj1AAEEjwVQqLpAbXmH5BJjqI0gi9DTAAgDBBCcAVLkgmQ7yKCZxpCQxqUZhAECCJ4XgMl493ug21ZD%2BaDAXH0WLM4A9MZPXJkJIIAwTAR5pQMalaCABQUULttBGCCAGCnNzgABBgAMJ5THwGvJLAAAAABJRU5ErkJggg%3D%3D</Image>
> +  <Url type="application/x-suggestions+json" method="GET" template="http://localhost:43336/search/mozsearch_suggestions.json"/>

So the final patch would have this url point to mozqa.com, correct?

::: firefox/tests/functional/testSearch/testSearchSuggestions.js
@@ +11,5 @@
> +const BASE_URL = collector.addHttpResource("../../../../data/");
> +const SEARCH_ENGINES = [{
> +  name : "mozqa.com",
> +  url : BASE_URL + "search/mozsearch.html"
> +}, {

I'm not fond of this alignment.

@@ +14,5 @@
> +  url : BASE_URL + "search/mozsearch.html"
> +}, {
> +  name : "OpenSearch Test",
> +  url : BASE_URL + "search/opensearch.html"
> +}]

nit: missing semicolon

@@ +42,5 @@
> +    engineManager.installFromUrl(aEngine.name, aEngine.url, function () {
> +      var addButton = new elementslib.Name(controller.tabs.activeTab, "add");
> +      controller.click(addButton);
> +    });
> +    assert.waitFor(function () { return searchBar.isEngineInstalled(aEngine.name)},

Use the fat arrow function syntax please.

@@ +53,5 @@
> +    searchBar.selectedEngine = aEngine.name;
> +
> +    //TODO: I don't understand why but it seems I have to add a sleep after the
> +    // engine is installed otherwise the first search won't start every time
> +    controller.sleep(1000);

Do we have any events we could wait for?
Attachment #8398587 - Flags: feedback?(andrei.eftimie) → feedback-
Comment on attachment 8398587 [details] [diff] [review]
patch_v1.0

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

Nothing to add here, but I agree with Andrei that we need to figure out why we aren't getting suggestions sometimes. The sleep is just a workaround now.
Attachment #8398587 - Flags: feedback?(andreea.matei)
Attached patch patch_v2.0 (obsolete) — — Splinter Review 
I updated the patch, also I had to wait for an event after changing the engine to be sure it's ready before we try to type in.

http://mozmill-crowd.blargon7.com/#/functional/report/00bcee9ac333de165bbc47d9e8310b6f
http://mozmill-crowd.blargon7.com/#/functional/report/00bcee9ac333de165bbc47d9e830e9db
http://mozmill-crowd.blargon7.com/#/functional/report/00bcee9ac333de165bbc47d9e830fd51
Attachment #8398587 - Attachment is obsolete: true
Attachment #8399454 - Flags: review?(andrei.eftimie)
Attachment #8399454 - Flags: review?(andreea.matei)
Comment on attachment 8399454 [details] [diff] [review]
patch_v2.0

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

This is the bug for the mozmill test update, but not for testcase-data. Please check that you attach your patches to the correct bug.
Attachment #8399454 - Flags: review?(andrei.eftimie)
Attachment #8399454 - Flags: review?(andreea.matei)
Comment on attachment 8399454 [details] [diff] [review]
patch_v2.0

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

Well, I was wrong. But we should really wait with the test until the testcase data bug has been finished. I don't see why we have to do reviews on content (search engines) while their content has not been fully reviewed yet.
Comment on attachment 8399454 [details] [diff] [review]
patch_v2.0

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

::: firefox/lib/search.js
@@ +531,5 @@
>  
>        var engine = this.getElement({type: "engine", subtype: "id", value: name});
> +      var engineChanged = false;
> +      var observer = {
> +        observe: function(aSubject, aTopic, aData) {

Since you don't use them you can omit the arguments, and we can use the fat arrow syntax here.

@@ +537,5 @@
> +        }
> +      }
> +
> +      try {
> +        Services.obs.addObserver(observer, SEARCH_ENGINE_TOPIC, false);

This should be outside of the try block.

@@ +539,5 @@
> +
> +      try {
> +        Services.obs.addObserver(observer, SEARCH_ENGINE_TOPIC, false);
> +
> +        this._controller.waitThenClick(engine);

engine.waitThenClick()

@@ +542,5 @@
> +
> +        this._controller.waitThenClick(engine);
> +
> +        assert.waitFor(() => {
> +          return engineChanged;

You can remove both the curly braces and the `return` keyword.

::: firefox/tests/functional/testSearch/testSearchSuggestions.js
@@ +34,5 @@
>    var allSuggestions = [ ];
>    var suggestionsForEngine;
>    var searchEngines = [ ];
>  
> +  SEARCH_ENGINES.forEach(function (aEngine) {

aEngine => {

@@ +37,5 @@
>  
> +  SEARCH_ENGINES.forEach(function (aEngine) {
> +    engineManager.installFromUrl(aEngine.name, aEngine.url, function () {
> +      var addButton = new elementslib.Name(controller.tabs.activeTab, "add");
> +      controller.click(addButton);

addButton.click();

@@ +39,5 @@
> +    engineManager.installFromUrl(aEngine.name, aEngine.url, function () {
> +      var addButton = new elementslib.Name(controller.tabs.activeTab, "add");
> +      controller.click(addButton);
> +    });
> +    assert.waitFor(() => { return searchBar.isEngineInstalled(aEngine.name)},

Since this function contains a single return statement you can remove both the curly braces and the `return` keyword.
This still fails a lot:
http://mozmill-release.blargon7.com/#/functional/failure?app=All&branch=All&platform=All&from=2014-03-28&to=&test=%2FtestSearch%2FtestSearchSuggestions.js&func=testMultipleEngines

I believe we need to skip it since we also have a dependency bug and it might take a few days to get everything fixed.
Here is the skip patch.
Thanks
Attachment #8399861 - Flags: review?(andrei.eftimie)
Attachment #8399861 - Flags: review?(andreea.matei)
Comment on attachment 8399861 [details] [diff] [review]
942737[skip].patch

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

I updated the commit message, skipped down to esr24 which is not affected:
http://hg.mozilla.org/qa/mozmill-tests/rev/b971f51bf450 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/cf8bbd1a2fa9 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/264dd73f5ae4 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/5d2351761180 (release)
Attachment #8399861 - Flags: review?(andrei.eftimie)
Attachment #8399861 - Flags: review?(andreea.matei)
Attachment #8399861 - Flags: review+
status-firefox28: affected → disabled
status-firefox29: affected → disabled
status-firefox30: --- → disabled
status-firefox31: --- → disabled
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][mozmill-test-skipped]
Comment on attachment 8401921 [details] [diff] [review]
patch_v2.2

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

Small updates left.

::: firefox/lib/search.js
@@ +535,5 @@
> +        observe: () => { engineChanged = true; }
> +      }
> +
> +      try {
> +        Services.obs.addObserver(observer, SEARCH_ENGINE_TOPIC, false);

This can be outside the try-finally, as we use it in the other modules.

@@ +540,5 @@
> +
> +        engine.waitThenClick();
> +
> +        assert.waitFor(() =>  engineChanged,
> +                      "Search engine has been selected. Expected '" + name + "'");

One space shifted to the right please.

::: firefox/tests/functional/testSearch/testSearchSuggestions.js
@@ +36,5 @@
>    var searchEngines = [ ];
>  
> +  SEARCH_ENGINES.forEach((aEngine) => {
> +    engineManager.installFromUrl(aEngine.name, aEngine.url, function () {
> +      var addButton = new elementslib.Name(controller.tabs.activeTab, "add");

We should have this in a getElements() method.
Attachment #8401921 - Flags: review?(andrei.eftimie)
Attachment #8401921 - Flags: review?(andreea.matei)
Attachment #8401921 - Flags: review-
(In reply to Andreea Matei [:AndreeaMatei] from comment #47)
> > +    engineManager.installFromUrl(aEngine.name, aEngine.url, function () {
> > +      var addButton = new elementslib.Name(controller.tabs.activeTab, "add");
> 
> We should have this in a getElements() method.
This is an element in a custom page, I can add it in a library but I just don't know in which.
We use this locator in other test where we use this engine:
> https://hg.mozilla.org/qa/mozmill-tests/file/260ad8949f03/firefox/tests/functional/testSearch/testSearchSelection.js#l45
Can I live this as it is?
Oh, you're right, I thought it was from a dialog/popup to add the engine.
Attached patch patch_v2.3 (obsolete) — — Splinter Review 
I fixed the other nits. Thanks for review.
Attachment #8401921 - Attachment is obsolete: true
Attachment #8402615 - Flags: review?(andrei.eftimie)
Attachment #8402615 - Flags: review?(andreea.matei)
Comment on attachment 8402615 [details] [diff] [review]
patch_v2.3

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

Looks good to me. Adding Henrik for a final check.
Attachment #8402615 - Flags: review?(hskupin)
Attachment #8402615 - Flags: review?(andrei.eftimie)
Attachment #8402615 - Flags: review?(andreea.matei)
Attachment #8402615 - Flags: review+
Comment on attachment 8402615 [details] [diff] [review]
patch_v2.3

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

::: data/search/mozsearch.xml
@@ +3,5 @@
>    <ShortName>mozqa.com</ShortName>
>    <Description>Mozqa.com search results</Description>
>    <InputEncoding>UTF-8</InputEncoding>
>    <Image width="16" height="16">data:image/x-icon;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8%2F9hAAAABGdBTUEAAK%2FINwWK6QAAABl0RVh0U29mdHdhcmUAQWRvYmUgSW1hZ2VSZWFkeXHJZTwAAAHWSURBVHjaYvz%2F%2Fz8DJQAggJiQOe%2Ffv2fv7Oz8rays%2FN%2BVkfG%2FiYnJfyD%2F1%2BrVq7ffu3dPFpsBAAHEAHIBCJ85c8bN2Nj4vwsDw%2F8zQLwKiO8CcRoQu0DxqlWrdsHUwzBAAIGJmTNnPgYa9j8UqhFElwPxf2MIDeIrKSn9FwSJoRkAEEAM0DD4DzMAyPi%2FG%2BQKY4hh5WAXGf8PDQ0FGwJ22d27CjADAAIIrLmjo%2BMXA9R2kAHvGBA2wwx6B8W7od6CeQcggKCmCEL8bgwxYCbUIGTDVkHDBia%2BCuotgACCueD3TDQN75D4xmAvCoK9ARMHBzAw0AECiBHkAlC0Mdy7x9ABNA3obAZXIAa6iKEcGlMVQHwWyjYuL2d4v2cPg8vZswx7gHyAAAK7AOif7SAbOqCmn4Ha3AHFsIDtgPq%2FvLz8P4MSkJ2W9h8ggBjevXvHDo4FQUQg%2FkdypqCg4H8lUIACnQ%2FSOBMYI8bAsAJFPcj1AAEEjwVQqLpAbXmH5BJjqI0gi9DTAAgDBBCcAVLkgmQ7yKCZxpCQxqUZhAECCJ4XgMl493ug21ZD%2BaDAXH0WLM4A9MZPXJkJIIAwTAR5pQMalaCABQUULttBGCCAGCnNzgABBgAMJ5THwGvJLAAAAABJRU5ErkJggg%3D%3D</Image>
> +  <Url type="application/x-suggestions+json" method="GET" template="http://mozqa.com/data/firefox/search/mozsearch_suggestions_{searchTerms}.json"/>

Would be good to also have this served via localhost, but there might be a problem with determining the correct port. I think that once we have mozhttpd we can use that instead of mozqa.com.

::: firefox/lib/search.js
@@ +41,5 @@
>  const SEARCH_GO_BUTTON    = SEARCH_TEXTBOX  + '/anon({"class":"search-go-container"})' +
>                                                '/anon({"class":"search-go-button"})';
>  const SEARCH_AUTOCOMPLETE =  '/id("main-window")/id("mainPopupSet")/id("PopupAutoComplete")';
>  
> +const SEARCH_ENGINE_TOPIC = "browser-search-engine-modified";

Please always check other files what the correct prefix is. Here it is 'TOPIC_'. So it's not a suffix.

@@ +548,5 @@
>  
>        // Wait until the drop down has been closed
>        assert.waitFor(function () {
>          return !this.enginesDropDownOpen;
>        }, "Search engines drop down has been closed", undefined, undefined, this);

This waitFor() call is still necessary? Having another engine selected should have auto-closed the popup menu.

::: firefox/tests/functional/testSearch/testSearchSuggestions.js
@@ +40,5 @@
> +      var addButton = new elementslib.Name(controller.tabs.activeTab, "add");
> +      addButton.click();
> +    });
> +    assert.waitFor(() => searchBar.isEngineInstalled(aEngine.name),
> +                   "Engine " + aEngine.name + " has suggestions");

This waitFor() call is already part of installFromUrl(), so it is obsolete. As a note beside this also would wait for the engine to be installed, but not for suggestions. Is there no observer topic we could listen for?

@@ +47,1 @@
>      searchBar.clear();

This comment doesn't map at all.

@@ +47,4 @@
>      searchBar.clear();
>  
>      // Select search engine
> +    searchBar.selectedEngine = aEngine.name;

The comment seems to be superfluous.

@@ -56,5 @@
>      }
> -
> -    // Exit the for loop in case we have suggestions for 2 engines
> -    if (allSuggestions.length === 2)
> -      break;

Why has this if condition be removed?
Attachment #8402615 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #52)
> 
> Would be good to also have this served via localhost, but there might be a
> problem with determining the correct port. I think that once we have
> mozhttpd we can use that instead of mozqa.com.
> 
I used the localhost for testing but the port number might change if is not free.
> This waitFor() call is already part of installFromUrl(), so it is obsolete.
> As a note beside this also would wait for the engine to be installed, but
> not for suggestions. Is there no observer topic we could listen for?
Yes I found an topic for this event, but now I have to wait for a cache event too or to disable the cache for search engines:
"browser.search.cache.enabled".

> > -    // Exit the for loop in case we have suggestions for 2 engines
> > -    if (allSuggestions.length === 2)
> > -      break;
> 
> Why has this if condition be removed?
Because now we have exactly two engines with suggestions.
(In reply to Cosmin Malutan from comment #53)
> > This waitFor() call is already part of installFromUrl(), so it is obsolete.
> > As a note beside this also would wait for the engine to be installed, but
> > not for suggestions. Is there no observer topic we could listen for?
> Yes I found an topic for this event, but now I have to wait for a cache
> event too or to disable the cache for search engines:
> "browser.search.cache.enabled".

Lets file a follow-up bug for that. Nothing we have to cover here. Thanks.

> > > -    // Exit the for loop in case we have suggestions for 2 engines
> > > -    if (allSuggestions.length === 2)
> > > -      break;
> > 
> > Why has this if condition be removed?
> Because now we have exactly two engines with suggestions.

Oh, the for loop changed and we only test our own engines. So that's fine then.
Attached patch patch_v3.0 (obsolete) — — Splinter Review 
I added an observer to wait for cache event, this is more stable than disabling the cache. When I disabled the cache it still failed once in 20 runs, and when I waited for event it didn't. 
Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/7ab760e27012969ae02e3b9e41779396
http://mozmill-crowd.blargon7.com/#/functional/report/7ab760e27012969ae02e3b9e417cf6fd
http://mozmill-crowd.blargon7.com/#/functional/report/7ab760e27012969ae02e3b9e417ceed9
Attachment #8402615 - Attachment is obsolete: true
Attachment #8403934 - Flags: review?(hskupin)
Comment on attachment 8403934 [details] [diff] [review]
patch_v3.0

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

Cosmin, as I have said about 22h ago please file a new bug for the cache modifications. So I don't see why they have been put into an updated patch here. Those are too heavy for this bug.
Attachment #8403934 - Flags: review?(hskupin) → review-
Depends on: 994102
Attached patch patch_v3.1 (obsolete) — — Splinter Review 
I filed bug 994102 for that, I still had to add a sleep of one second I after I install the engines, otherwise it fails about twice in 20 runs.
http://mozmill-crowd.blargon7.com/#/functional/report/7ab760e27012969ae02e3b9e418e2099
http://mozmill-crowd.blargon7.com/#/functional/report/7ab760e27012969ae02e3b9e41896fe6
http://mozmill-crowd.blargon7.com/#/functional/report/7ab760e27012969ae02e3b9e418e06de
Attachment #8403934 - Attachment is obsolete: true
Attachment #8404050 - Flags: review?(hskupin)
There is no dependency here for bug 994102. It's totally separate.
No longer depends on: 994102
Comment on attachment 8404050 [details] [diff] [review]
patch_v3.1

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

::: firefox/lib/search.js
@@ +42,5 @@
>                                                '/anon({"class":"search-go-button"})';
>  const SEARCH_AUTOCOMPLETE =  '/id("main-window")/id("mainPopupSet")/id("PopupAutoComplete")';
>  
> +const SEARCH_ENGINE_ADDED = "engine-added";
> +const SEARCH_ENGINE_DEFAULT = "engine-default";

Can you please explain why you made this change? It was not requested and I don't see why we need those two constants, which are used only once in that library.

@@ -338,5 @@
> -
> -    // Check if the local engine has been installed
> -    var searchBarInstance = new searchBar(this._controller);
> -    assert.waitFor(function () {
> -      return searchBarInstance.isEngineInstalled(aName);

I also don't see why you have changed this. This might be exactly the problem why we return too early and the engine is not working properly. Also this change was not requested. Please really stay on focus with your changes to what has been requested.

@@ +543,5 @@
> +      var observer = {
> +        observe: (aSubject, aTopic, aData) => {
> +          if (aData === SEARCH_ENGINE_DEFAULT) {
> +            engineChanged = true;
> +          }

Here again. I don't see why we need that change.

@@ -532,5 @@
>  
> -      // Wait until the drop down has been closed
> -      assert.waitFor(function () {
> -        return !this.enginesDropDownOpen;
> -      }, "Search engines drop down has been closed", undefined, undefined, this);

Instead of refactoring everything in that patch, why you haven't simply removed this assert.waitFor()? As said last time, I don't see why it is necessary. I don't think it would break anything.
Attachment #8404050 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #59)
> > -    // Check if the local engine has been installed
> > -    var searchBarInstance = new searchBar(this._controller);
> > -    assert.waitFor(function () {
> > -      return searchBarInstance.isEngineInstalled(aName);
> 
> I also don't see why you have changed this. This might be exactly the
> problem why we return too early and the engine is not working properly. Also
> this change was not requested. Please really stay on focus with your changes
> to what has been requested.
I put this back, but this doesn't solve the issue, it doesn't fail constantly bot at least once in 10 runs. In comment 52, you asked if there is an event we could wait for instead, I thought I had to add it.
> @@ +543,5 @@
> > +      var observer = {
> > +        observe: (aSubject, aTopic, aData) => {
> > +          if (aData === SEARCH_ENGINE_DEFAULT) {
> > +            engineChanged = true;
> > +          }
> 
> Here again. I don't see why we need that change.
This is needed because on this topic we get different messages and I need to be sure that the default engine has been change an not that one has been installed/removed.
> @@ -532,5 @@
> >  
> > -      // Wait until the drop down has been closed
> > -      assert.waitFor(function () {
> > -        return !this.enginesDropDownOpen;
> > -      }, "Search engines drop down has been closed", undefined, undefined, this);
> 
> Instead of refactoring everything in that patch, why you haven't simply
> removed this assert.waitFor()? As said last time, I don't see why it is
> necessary. I don't think it would break anything.
I removed this, but as I said it will fail, with a delay after installation works every time. My guess is that the engine file get's blocked while is being opened and read to be cached. 
The cleanest way to workaround this would be to install the two engines in one loop, and check for suggestions in a different one, with this will work every time, and will look nice.
(In reply to Cosmin Malutan from comment #60)
> > I also don't see why you have changed this. This might be exactly the
> > problem why we return too early and the engine is not working properly. Also
> > this change was not requested. Please really stay on focus with your changes
> > to what has been requested.
> I put this back, but this doesn't solve the issue, it doesn't fail
> constantly bot at least once in 10 runs. In comment 52, you asked if there
> is an event we could wait for instead, I thought I had to add it.

At this time it was a question, so nothing you should immediately get implemented. Lets talk about things first, which will save us all time.

> > @@ +543,5 @@
> > > +      var observer = {
> > > +        observe: (aSubject, aTopic, aData) => {
> > > +          if (aData === SEARCH_ENGINE_DEFAULT) {
> > > +            engineChanged = true;
> > > +          }
> > 
> > Here again. I don't see why we need that change.
> This is needed because on this topic we get different messages and I need to
> be sure that the default engine has been change an not that one has been
> installed/removed.

You can also install engines by not setting it as the default engine. So this would certainly be the wrong observer topic. We would need something like ENIGNE_INSTALLED, engine=expected_name.

> > > -      // Wait until the drop down has been closed
> > > -      assert.waitFor(function () {
> > > -        return !this.enginesDropDownOpen;
> > > -      }, "Search engines drop down has been closed", undefined, undefined, this);
> > 
> > Instead of refactoring everything in that patch, why you haven't simply
> > removed this assert.waitFor()? As said last time, I don't see why it is
> > necessary. I don't think it would break anything.
> I removed this, but as I said it will fail, with a delay after installation
> works every time. My guess is that the engine file get's blocked while is
> being opened and read to be cached. 
> The cleanest way to workaround this would be to install the two engines in
> one loop, and check for suggestions in a different one, with this will work
> every time, and will look nice.

Lets use a different loop, which is fine for now. We can follow-up with a more in-depth fix for this situation.
(In reply to Henrik Skupin (:whimboo) from comment #61)
> > > @@ +543,5 @@
> > > > +      var observer = {
> > > > +        observe: (aSubject, aTopic, aData) => {
> > > > +          if (aData === SEARCH_ENGINE_DEFAULT) {
> > > > +            engineChanged = true;
> > > > +          }
> > > 
> > > Here again. I don't see why we need that change.
> > This is needed because on this topic we get different messages and I need to
> > be sure that the default engine has been change an not that one has been
> > installed/removed.
> 
> You can also install engines by not setting it as the default engine. So
> this would certainly be the wrong observer topic. We would need something
> like ENIGNE_INSTALLED, engine=expected_name.
This method is selectedEngine setter method, not the install, the message is sent after we set the engine as default. Because we get a lot of messages under the "browser-search-engine-modified" topic I have to check the message too.
Comment on attachment 8404599 [details] [diff] [review]
patch_v4.0

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

Looks good now. Please fix the small nit. r=me with that change.

::: firefox/lib/search.js
@@ +41,5 @@
>  const SEARCH_GO_BUTTON    = SEARCH_TEXTBOX  + '/anon({"class":"search-go-container"})' +
>                                                '/anon({"class":"search-go-button"})';
>  const SEARCH_AUTOCOMPLETE =  '/id("main-window")/id("mainPopupSet")/id("PopupAutoComplete")';
>  
> +const TOPIC_SEARCH_ENGINE = "browser-search-engine-modified";

Please make this name clearer. This could stand for everything but not only modified.
Attachment #8404599 - Flags: review?(hskupin) → review+
Not sure what's up with esr24.
status-firefox-esr24: --- → ?
Attached patch patch_v4.1 — — Splinter Review 
Thanks for review, I fixed the nit.
Attachment #8404599 - Attachment is obsolete: true
Attachment #8405318 - Flags: review?(hskupin)
Esr24 it supposed to be affected, it wasn't been disabled probably because it didn't failed.
status-firefox-esr24: ? → affected
Comment on attachment 8405318 [details] [diff] [review]
patch_v4.1

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

Why I'm getting asked for review here? Those were nits so Andreea and Andrei should be perfect as reviewer here. But anyway, this looks good now.

Andreea, could you land this please?
Attachment #8405318 - Flags: review?(hskupin)
Attachment #8405318 - Flags: review+
Attachment #8405318 - Flags: checkin?(andreea.matei)
Comment on attachment 8405318 [details] [diff] [review]
patch_v4.1

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

The review was requested by you in comment 64.
Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/d6197394e7d8 (default)

Cosmin, please check backporting.
Attachment #8405318 - Flags: checkin?(andreea.matei) → checkin+
Attached patch patch_v4.1[esr24] (obsolete) — — Splinter Review 
Previous patch applies cleanly on all branches except esr24. 
This is the patch for esr24.
Thanks
Attachment #8406133 - Flags: review?(andreea.matei)
Don't backport this yet, it failed on default, we have to wait for engine to be ready before we start typing.  
http://mozmill-daily.blargon7.com/#/functional/report/3b05997c57f8cb2e7551cc2d772fb0ce
Looks like I forgot to keep the change for disabling the search caching:
"browser.search.cache.enabled"

I will keep testing this and return with a follow up patch.
Attached patch patch_v4.2[follow-up] (obsolete) — — Splinter Review 
Just running in different loops is still not sufficient. It still failed after 150 runs. If I added a sleep after installation it didn't failed in 200 runs. I added the sleep and the bug reference.
Attachment #8406133 - Attachment is obsolete: true
Attachment #8406133 - Flags: review?(andreea.matei)
Attachment #8406223 - Flags: review?(andrei.eftimie)
Attachment #8406223 - Flags: review?(andreea.matei)
Comment on attachment 8406223 [details] [diff] [review]
patch_v4.2[follow-up]

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

::: firefox/tests/functional/testSearch/testSearchSuggestions.js
@@ +22,5 @@
>  
>    aModule.engineManager = new search.engineManager(aModule.controller);
>    aModule.searchBar = new search.searchBar(aModule.controller);
> +
> +  prefs.preferences.setPref(PREF_SEARCH_CACHE_ENABLED, false);

Sorry if I missed that, but can you explain or even comment here why this pref has to be set to false?

@@ +48,5 @@
>        addButton.click();
>      });
> +    // Bug 994102 - Search engines don't work right away after installation
> +    // TODO: remove this once the bug gets fixed
> +    controller.sleep(1000);

Never do the sleeps. So if the event is absolutely necessary follow up with that, but please no sleep calls.
Attachment #8406223 - Flags: review?(andrei.eftimie)
Attachment #8406223 - Flags: review?(andreea.matei)
Attachment #8406223 - Flags: review-
I kept the caching disabled, and I added two observers to wait until the engine is added and the metadata updated. I ran the test for 500 times and it didn't failed so I would say this is safe.
Attachment #8406223 - Attachment is obsolete: true
Attachment #8406849 - Flags: review?(andrei.eftimie)
Attachment #8406849 - Flags: review?(andreea.matei)
Comment on attachment 8406849 [details] [diff] [review]
942737[follow-up].patch

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

::: firefox/lib/search.js
@@ +340,5 @@
> +      observe: (aSubject, aTopic, aData) => {
> +        if (aData === "engine-added") {
> +          engineAdded = true;
> +        }
> +        else if (aData === "write-metadata-to-disk-complete") {

I'm not sure about these events since Gavin mentioned to never observe write-cache-to-disk-complete.

::: firefox/tests/functional/testSearch/testSearchSuggestions.js
@@ +25,5 @@
> +
> +  // Bug 994102 - Search engines don't work right away after installation
> +  // We disable the search caching here because it will open the engine file in
> +  // order to cache it and this will block the engine and it won't retrieve
> +  // suggestions right after installation

// Bug 994102
// In order to cache, it will open the engine file and this will
// block it from retrieving suggestions after installation
Attachment #8406849 - Flags: review?(andrei.eftimie)
Attachment #8406849 - Flags: review?(andreea.matei)
Attachment #8406849 - Flags: review-
Gavin, could you please tell us if "write-metadata-to-disk-complete" is safe to use? Thanks.
Flags: needinfo?(gavin.sharp)
No, you should not use "write-metadata-to-disk-complete". There are other search observer notifications ("browser-search-engine-modified") that should cover everything you need - if they don't, we need to understand why (and likely change the search service).

I am happy to help write robust/reliable search tests, but I don't have a lot of time to dig in and find all of the relevant code without help - so if you can point me to the exact code in question and explain what it's trying to do and what's failing, that would be helpful.
Flags: needinfo?(gavin.sharp)
Thanks Gavin, I don't know how familiar are you with mozmill-tests so I will give all the steps.
1 Download and unzip an environment from http://mozqa.com/mozmill-env/
2 Activate the environment, by running "run" file
3 Download a mozmill-tests repository:
>hg clone http://hg.mozilla.org/qa/mozmill-tests
4 Right now as a workaround I installed the engines in a loop and search for suggestions in a different one, that gave us some time between installation and actual searching. 
From mozmill-tests/firefox/tests/functional/testSearch/testSearchSuggestions.js remove lines 42-45 so you will have only one loop, and this will make test fail.
5 Ran the test, from environment type command:
>mozmill -t mozmill-tests/firefox/tests/functional/testSearch/testSearchSuggestions.js -b <path to a nightly>

What the test does:
- Install an engine.
- Clear the search-bar.
- Set the engine as default.
- Type in a word and wait for suggestions.
Will fail because it doesn't get any suggestions, if you add a delay of one second after installation (controller.sleep(1000)) it will pass, or if you apply the follow-up patch from this bug, in which I wait for "write-metadata-to-disk-complete" when installing (installFromUrl)
https://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/tests/functional/testSearch/testSearchSuggestions.js#l41

The topic to wait for I took it from link below, where it's the last step after installing an engine. 
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#4530
Gavin can you have a look at this? Also I checked the profile while running tests, if I wait for engine file to be written, and search-metadata.json, the test will pass, so it's clear that the engine is not working while writing those filles.
A wait similar to this is done in this tests:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/test_save_sorted_engines.js#73
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/head_search.js#70
Flags: needinfo?(gavin.sharp)
This sounds like the same or a similar issue as bug 994102. 

test_save_sorted_engines.js and the "afterCommit" helper should be removed from these tests.

How this is supposed to work is that you are supposed to listen for "browser-search-engine-modified" events for all of the relevant notifications.

Can you switch all of the problematic tests to using those notifications, and then we can debug why that isn't working?
Flags: needinfo?(gavin.sharp)
(That's what I was attempting to do in bug 994102 comment 6 as well.)
I would say we remove any related work to figure this out to bug 994102, and continue here once it has been fixed. It doesn't make sense to keep a discussion up on two different bugs. So Cosmin, please continue on bug 994102 with anything you have questions for. Thanks.
Depends on: 994102
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] → [mozmill-test-failure][mozmill-test-skipped][Blocked by bug 994102]
Lukas, this bug is in a different product/component then usual Firefox related bugs, so we drive those flags on our own. It's not decided yet, if that is a wontfix given that esr24 will still be around for quite a while.
status-firefox-esr24: wontfix → affected
Ok, it's around two more releases and FF31 (the next ESR base) is shipping in 3 weeks.  You'll still need to ask for uplift approval here if this has to land on the esr branch, but if not then totally understand you using the flags your own way for this one.
Lukas, I think you can remove the Mozmill Tests component from your queries. Mozmill tests exist outside of the Mozilla product branches. The automation team will usually file a Firefox bug any time there is a legitimate product/platform bug found via these tests.
Cosmin, we have to check which branches are affected given that it is still open and firefox 31 is marked as fixed. Please check that. We may have to re-prioritize the bug.
Flags: needinfo?(cosmin.malutan)
All branches are affected, inclusive 31 which at the time bug was filed was Nightly.
They got re-enabled with the landing of bug 988867, which revealed the issue from bug 994102.
On release it's disabled.
http://mozmill-daily.blargon7.com/#/functional/report/8e52cc909fb28910195c4e255e31fe42
http://mozmill-daily.blargon7.com/#/functional/report/c90eeeb0a5511695efc746255391804d
status-firefox28: disabled → ---
status-firefox29: disabled → ---
status-firefox31: fixed → affected
status-firefox32: --- → affected
status-firefox33: --- → affected
Flags: needinfo?(cosmin.malutan)
Can we get a temporary workaround for this test?
Maybe with a short sleep if waiting for the file be available on the disk is not something Gavin wants to wait for.

Add a comment above referencing bug 994102 for a proper fix.
URL: http://mozmill-daily.blargon7.com/#/f... → http://mozmill-daily.blargon7.com/#/f...
Flags: needinfo?(cosmin.malutan)
I checked if a delay would help us here, so first I tried with a delay  of 2s, it failed in the first 100 runs with 3s it failed twice in 5000 runs, I'll try with 5s now.
I think if this works we could add this fix and with an TODO comment, and wait for a proper fix in bug 994102, as suggested.
Flags: needinfo?(cosmin.malutan)
I'd really like to get this issue fixed in any capacity.

While on daily CI builds we've seen this 3 times in the last month:
http://mozmill-daily.blargon7.com/#/functional/failure?app=Firefox&branch=All&platform=All&from=2014-07-25&test=%2FtestSearch%2FtestSearchSuggestions.js&func=testMultipleEngines
(there are about 2 dozen failures in the month before)

I see this locally close to 100% of the time.
status-firefox34: --- → affected
status-firefox-esr24: affected → wontfix
status-firefox-esr31: --- → affected
Whiteboard: [mozmill-test-failure][mozmill-test-skipped][Blocked by bug 994102] → [mozmill-test-failure][Blocked by bug 994102]
This is also reproducible for me now by 100%, reason is it takes a while to load the resources from "http://mozqa.com" locally so getting the suggestions works better on CI nodes. If you add 
>controller.open("http://mozqa.com/data/firefox/search/mozsearch_suggestions_m.json");
>controller.waitForPageLoad();
>controller.open("http://mozqa.com/data/firefox/search/mozsearch_suggestions_mo.json");
>controller.waitForPageLoad();
>controller.open("http://mozqa.com/data/firefox/search/opensearch_suggestions_m.json");
>controller.waitForPageLoad();
>controller.open("http://mozqa.com/data/firefox/search/opensearch_suggestions_mo.json");
>controller.waitForPageLoad();
at the beginning of the test you'll see that the failure rate decreases a lot, this particular issue could be easily fixed once we have the wptserver running. 
Regarding the other dependant bug 994102, I can't fix it the best I can do is to make a patch to wait for "browser-search-engine-modified", but this won't help if we won't add a listener to wait for engines to be commited to disk (bug 994102) "write-metadata-to-disk-complete". 
My suggestion will be to listen for "write-metadata-to-disk-complete" with a TODO/Bug comment, to be removed once the bug is fixed.
Priority: P1 → P3
This didn't failed anymore since we landed bug 994102 and the follow-up in bug 678456.
The remaining issue here it will be to make the suggestion locally, but that would be only when we will have wpt-server in production. Until the the upcoming failures I expect to be network-related.

http://mozmill-release.blargon7.com/#/functional/failure?app=Firefox&branch=All&platform=All&from=2014-11-07&to=&test=%2FtestSearch%2FtestSearchSuggestions.js&func=testMultipleEngines
Priority: P2 → P3
I do not see this as an issue but a follow-up work in a different bug. If this test really doesn't fail anymore we should mark it as fixed based on the landings mentioned in comment 95.
Great.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: