834163 - Teardown code for /testSearch tests should not exercise ui path which can cause failures like: "Modal dialog has been found and processed"

Status: RESOLVED FIXED

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

Description

[Daniela Petrovici]

Started happening on Nightly FR and:

MAC OS 10.6.8:
http://mozmill-ci.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb512a9628

MAC OS 10.7.5:
http://mozmill-ci.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb512aa16d

MAC OS 10.8.2:
http://mozmill-ci.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb512a7804

This happens both in tearDown() -> restoreDefaultEngines() and also for openEngineManager() - waiting for modal dialog.

Comment 1

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

See bug 783197 for some of my comments which would apply here. I think there is no question that we shouldn't fail on those actions, but why are we exercising the UI path in teardown? That's something we did in the past and should change, so that we only rely on API calls. Otherwise we will not be able to successfully reset all the changes made during this test.

It only happened once for now but I will put it into the P1 bucket because it will affect multiple tests in a testrun. Lets get this focused next week.

Comment 3

[Andreea Matei [:AndreeaMatei]]

I was looking how to sort the engines through API (a workaround) since we don't have a sorting method in there. I've also seen that proposal in bug 556437, which remained untouched.

I figured I could use moveEngine() from the API, going through defaults list and the visible list (the one in the search drop down), compare the indexes and move it if they are not the same. Added also an observer, waiting for "browser-search-engine-modified" event. Somehow, the visible list is not getting updated though.
But if I use removeEngine() instead of moveEngine(), I see the visible list becoming empty.

So is there some other event I should wait for?

Please see:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIBrowserSearchService.idl
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js

Comment 4

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

I think it may be a good thing to ask Gavin about that.

Comment 5

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

What do you mean by "the visible list isn't being updated"? Do you have a code snippet that indicates the problem? I don't have enough context here to usefully provide input, it might help if you elaborate on what you're doing, what you're expecting, and what you're seeing.

Comment 6

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v1

I've managed to do it without the observer, being consistent now in removing and reordering engines.

Report:
http://mozmill-crowd.blargon7.com/#/functional/report/00ba6b3792451db672d192b36805362b

Comment 7

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

Comment on attachment 721189 [details] [diff] [review]
patch v1

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

Love it! There is only one thing to get fixed before landing the patch.

::: lib/search.js
@@ +787,3 @@
>      });
>  
>      // XXX: Bug 556477 - Restore default sorting

This comment needs to be updated.

Comment 8

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v1.1

Updated the bug number in the comment.

Comment 9

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

Comment on attachment 723868 [details] [diff] [review]
patch v1.1

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

I already gave r+ so you can carry it forward next time. Please land the patch.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Oh, I will. Felt weird to land my patches :)
http://hg.mozilla.org/qa/mozmill-tests/rev/0206124c904e (default)

I'll check backporting.

Comment 11

[Andreea Matei [:AndreeaMatei]]

For some reason, on all the other branches, the defaults list (Services.search.getDefaultEngines({ })) is different than for nightly, it has eBay on the last position instead on the 5th.
Although the visible list (the one in dropdown) and the installed engines list remains as on default.
So I need to rethink a bit the way we sort there.

Comment 12

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

Hm, not for me in my daily profile in Aurora. When I reset the order it's on the 5th position. Do you see that when doing it manually?

Comment 13

[Andreea Matei [:AndreeaMatei]]

It's strange, I'm testing Beta now, if you put 
dump(Services.search.getVisibleEngines({}).map(function (a) a.name) + "\n");
dump(Services.search.getEngines({}).map(function(a) a.name) + "\n");
dump(Services.search.getDefaultEngines({}).map(function(a) a.name) + "\n");

in the setup of a test, the outcome will be:

Google,Yahoo,Bing,Amazon.com,Twitter,Wikipedia (en),eBay

But visually on the browser are correctly sorted.

When running all testSearch folder with my patch, visible list gets updated correctly (Google,Yahoo,Bing,Amazon.com,eBay,Twitter,Wikipedia (en) as it is), but my patch will fail in testRestoreDefaults.js due to this sorting. 
Error: "message": "Engine has been restored at the correct position. - 'eBay' should equal 'Twitter'
Twitter is at the 5th position in Beta's defaults list.

Comment 14

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

Gavin might hopefully know what's going on here and if there was a change in Nightly which changed the behavior for the better.

Comment 15

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

Comment on attachment 723868 [details] [diff] [review]
patch v1.1

>diff --git a/lib/search.js b/lib/search.js

>+    // XXX: Bug 556437 - Restore default sorting
>+    Services.search.getEngines().forEach(function (aEngine, aIndex) {
>+      if (defaults.indexOf(aEngine) !== aIndex) {
>+        Services.search.moveEngine(aEngine, defaults.indexOf(aEngine));
>+      }
>     });

This code isn't guaranteed to restore the original search order, since subsequent moveEngine calls can re-order items that were previously moved, depending on the order the engine list was in initially. You probably want something more like:

for (let i = 0; i < defaults.length; i++)
  Services.search.moveEngine(defaults[i], i);

But really we should avoid adding this code here, and fix bug 556437 instead. If you write that patch I can land it!

Comment 16

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

Thanks Gavin! Ah, one of my old filed bugs. :) A fix would work for Nightly but we would need this workaround for all the other branches in our tests down to esr17. So we could temporarily fix our tests and hope that someone could fix bug 556437. Andreea feel free to do so if you want!

Comment 17

[Andreea Matei [:AndreeaMatei]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #15)
> Comment on attachment 723868 [details] [diff] [review]
> patch v1.1
> 
> >diff --git a/lib/search.js b/lib/search.js
> 
> >+    // XXX: Bug 556437 - Restore default sorting
> >+    Services.search.getEngines().forEach(function (aEngine, aIndex) {
> >+      if (defaults.indexOf(aEngine) !== aIndex) {
> >+        Services.search.moveEngine(aEngine, defaults.indexOf(aEngine));
> >+      }
> >     });
> 
> This code isn't guaranteed to restore the original search order, since
> subsequent moveEngine calls can re-order items that were previously moved,
> depending on the order the engine list was in initially. 

Thanks for your reply.
I will take that bug as soon as I can, but I feel the issue is somewhere else.

The code I wrote works fine for Nightly, the issue comes at other branches that have a difference between defaults list and the visible list in the dropdown from search bar.
API call defaults is this: Google,Yahoo,Bing,Amazon.com,Twitter,Wikipedia (en),eBay
Visible is: Google,Yahoo,Bing,Amazon.com,eBay,Twitter,Wikipedia (en)

When we restore defaults from the UI button (Manager Search Engine), the result is still
Google,Yahoo,Bing,Amazon.com,eBay,Twitter,Wikipedia (en)

Do you know why these differences? Doesn't seem right.
Nightly has Google,Yahoo,Bing,Amazon.com,eBay,Twitter,Wikipedia (en) for all.

We need to change clicking the button in the UI by using API calls, but those are giving different results..

Comment 18

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

What are your exact STR for reproducing the issue, from scratch? It's difficult for me to look into strange behavior you're seeing without any such context.

Comment 19

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

Services.search.getVisibleEngines({}).map(function (a) a.name) returns the same list as what I see in the dropdown in an Aurora build for me. So I suspect what you're seeing is specific to the specific mozmill environment (or something the test does).

If you fix the issue I mention in comment 15, do you still see the strange behavior?

Comment 20

[Andreea Matei [:AndreeaMatei]]

Attachment: minimized testcase

Gavin, could you please check with Beta also?
I did a minimized testcase, just opening the browser and dumping that Services.search* call and indeed for Aurora those are shown as in default, but for beta, release and esr17, eBay is at the end of the list.

But then I manually opened the dropdown list right after the browser opened, and that made the dump for visible and installed list to be correctly listed. See the two behaviours above:

andreeamatei@P4986:~/clean$ mozmill -t tests/functional/testSearch/testcase.js --show-errors -b ~/Builds/ESR17/firefox/firefox
TEST-START | /home/andreeamatei/clean/tests/functional/testSearch/testcase.js | setupModule
visible Google,Yahoo,Bing,Amazon.com,Twitter,Wikipedia (en),eBay
installed Google,Yahoo,Bing,Amazon.com,Twitter,Wikipedia (en),eBay
defaults Google,Yahoo,Bing,Amazon.com,Twitter,Wikipedia (en),eBay
TEST-PASS | /home/andreeamatei/clean/tests/functional/testSearch/testcase.js | testcase.js::setupModule
INFO Passed: 1
INFO Failed: 0
INFO Skipped: 0

Here I clicked the drop down button:
andreeamatei@P4986:~/clean$ mozmill -t tests/functional/testSearch/testcase.js --show-errors -b ~/Builds/ESR17/firefox/firefox
TEST-START | /home/andreeamatei/clean/tests/functional/testSearch/testcase.js | setupModule
visible Google,Yahoo,Bing,Amazon.com,eBay,Twitter,Wikipedia (en)
installed Google,Yahoo,Bing,Amazon.com,eBay,Twitter,Wikipedia (en)
defaults Google,Yahoo,Bing,Amazon.com,Twitter,Wikipedia (en),eBay
TEST-PASS | /home/andreeamatei/clean/tests/functional/testSearch/testcase.js | testcase.js::setupModule
INFO Passed: 1
INFO Failed: 0
INFO Skipped: 0

Couldn't get the outcome through the browser console, as it's not recognizing Components.classes.

Your idea from comment 15 is not working, after a few tests are ran, I get "moveEngine: Can't move a hidden engine!". Not sure why the engine gets changed to hidden.

Comment 21

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #20)
> andreeamatei@P4986:~/clean$ mozmill -t
> tests/functional/testSearch/testcase.js --show-errors -b

What profile does this use? What setup code (if any) does this run on that profile before the test runs? I don't know anything about how mozmill works.

When I run a current beta build with a new profile, I get the expected output:

> Services.search.getVisibleEngines({}).map(function (e) e.name);
Google,Yahoo,Bing,Amazon.com,eBay,Twitter,Wikipedia (en)

Comment 22

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #20)
> Your idea from comment 15 is not working, after a few tests are ran, I get
> "moveEngine: Can't move a hidden engine!". Not sure why the engine gets
> changed to hidden.

You should call Services.search.restoreDefaultEngines(); before re-ordering, to make sure there are no hidden engines.

Comment 23

[Andreea Matei [:AndreeaMatei]]

Attachment: screeshot

Mozmill starts the browser with a clean profile, each time. 
How can I get that output, what are the steps in the browser? I'm not able to do it through the Web console as it's not letting me import Component.classes. I want to test against the build more, without Mozmill.

Tried with restore before reordering, its restoring defaults but in the way I've mentioned, with eBay at the end. 
Attaching screenshot with visible list right when the browser starts (on the left) and in the right is after restoreDefaults(), it's putting eBay at the end because that's how it is in Services.search.getDefaultEngines();

Dave, Henrik:
get engines() is returning the defaults array, with eBay at the end:
http://hg.mozilla.org/qa/mozmill-tests/file/mozilla-beta/lib/search.js#l72

Then this one is returning the visible array, as it normally is with eBay 5th:
http://hg.mozilla.org/qa/mozmill-tests/file/mozilla-beta/lib/search.js#l480

This is where we fail, because the arrays are not identical:
http://hg.mozilla.org/qa/mozmill-tests/file/ca996a686b45/tests/functional/testSearch/testRestoreDefaults.js#l36

Comment 24

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #23)
> Created attachment 727194 [details]
> screeshot
> 
> Mozmill starts the browser with a clean profile, each time. 
> How can I get that output, what are the steps in the browser? I'm not able
> to do it through the Web console as it's not letting me import
> Component.classes. I want to test against the build more, without Mozmill.

You can use top.opener.Services.search.* in the Error Console.

> Tried with restore before reordering, its restoring defaults but in the way
> I've mentioned, with eBay at the end. 

Using my suggested reordering method?

Comment 25

[Andreea Matei [:AndreeaMatei]]

Indeed when I use top.opener.Services.search.* in the browser I get the correct list. Also when I open the browser through mozmill and I manually do the same in console error.
But when I run this testcase:

Cu.import("resource://gre/modules/Services.jsm");
function setupModule() {
  controller = mozmill.getBrowserController();
  controller.sleep(10000);

  dump("defaults " + Services.search.getDefaultEngines().map(function(a) a.name) + "\n");
}

During the sleep I manually opened error console, list is still correct, but the dump line returned eBay at the end.

Is very strange and I'm not sure what's going on here.

I used your reordering method, but the output seems about right since it takes the wrong list (eBay at the end) from the beginning. If I can figure out why is that, your method would work, I test it over Nightly and it does the job.

Comment 26

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I can replicate this with the minimised test case. If this behavior changed between Beta and Aurora we should be able to find the nightly where it changed. This might help us to determine what has caused it.

Comment 27

[Andreea Matei [:AndreeaMatei]]

So I've found that the build from February 12 dumps
defaults Google,Yahoo,Bing,Amazon.com,Twitter,Wikipedia (en),eBay

And the one from February 13:
defaults Google,Yahoo,Bing,Amazon.com,eBay,Twitter,Wikipedia (en)

This is the pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2013-02-12&enddate=2013-02-14

These 2 bugs have changes related to search, but I don't see anything related to reordering: bug 587780 and bug 590068.

Still, in browser error console it's still dumped correctly:
Google,Yahoo,Bing,Amazon.com,eBay,Twitter,Wikipedia (en)

Not sure why through Mozmill run test is different.
I'm building to find a closer regression range since the tinderbox builds are no longer available from that date.

Comment 28

[Andreea Matei [:AndreeaMatei]]

Unfortunately the bisect ended with a changeset that dumps eBay at the end (meaning before the change):
12 febr - 36525224b14e - good
	3344115d1cbc - ebay - good
	bca5bd2cd0e1 - ebay - good
	3b0b1092f9b4 - ebay - good
	bca5bd2cd0e1 - wiki - bad
	98b4c33fc377 - ebay - good
	9a0846d180af - wiki - bad
	e9f2886c0179 - wiki - bad
	244daeec3393 - wiki - bad
	a356cb75873f - wiki - bad
	93ba23f414ff - ebay - good
	0100ad335f92 - ebay - good
	35eda1ce617e - ebay - good
	c3a4d688b26f - ebay - good
	bb8d5da0bf70 - wiki - bad
	14da2eb23b19 - ebay - good
	b8c19a26a1d2 - ebay - good

13 febr - 161a347bda5b - bad

This is the last bad in the list: http://hg.mozilla.org/mozilla-central/rev/bb8d5da0bf70

Comment 29

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

Sounds like perhaps the result is timing-dependent, in which case you can't really bisect.

Comment 30

[Dave Hunt [:davehunt] [he/him] ⌚BST]

How do you mean timing dependent? Looking through the bisect log (with the exception of bca5bd2cd0e1 which is both bad and good*) it would appear that the list changes after changeset bb8d5da0bf70.

* Andreea has since re-run with this changeset and confirms that it is good.

Comment 31

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

If the bug depends on a race condition, hg bisect won't necessarily produce reliable results. It sounds like maybe that's what was happening here. If it isn't, that's great!

Comment 32

[Andreea Matei [:AndreeaMatei]]

I have ran each build for 20 times with the testcase, to make sure the array is not changing from run to run. It remains the same. Otherwise I think today's nightly could and should fail as well by now, but it's not.

How to proceed now? Have a separate sort for the other branches until all will have this changeset?

Comment 33

[Dave Hunt [:davehunt] [he/him] ⌚BST]

If it's consistently different with the older branches, then I don't see an immediate issue with using a different expected sort order on those. What do you think, Henrik?

Comment 34

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

(In reply to Dave Hunt (:davehunt) from comment #33)
> If it's consistently different with the older branches, then I don't see an
> immediate issue with using a different expected sort order on those. What do
> you think, Henrik?

Sounds fine to me. Lets ride the trains... So we can close this bug now given that the fix landed for 22.0 and hasn't shown a problem?

Comment 35

[Andreea Matei [:AndreeaMatei]]

It's still available for the older branches (beta/release/esr17) although no related failures were seen on those. 
If we want to change the sorting there (as it is on  Mozmill, cause in the browser alone is the same, as Gavin and I showed), we should leave the bug open and I will come up with an update for that. Otherwise, that will be done with future merges, like it was with Aurora.

Comment 36

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

The question here is if our test is valid on those branches. If it is I'm fine with letting the changes ride the train. Otherwise we should make sure to update the test, to test the real behavior.

Comment 37

[Andreea Matei [:AndreeaMatei]]

I feel it's valid, it works fine with aurora and default and we're also safe on the other branches because it did not fail as it is there. The error first reported in January was affecting default only.