860382 - Test failure "Search engines drop down has been closed" in testSearchSuggestions.js

Status: RESOLVED FIXED

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

Description

[Andreea Matei [:AndreeaMatei]]

Started today with Nightly, on Linux so far.

I can reproduce this locally, most likely is also a regression. I see the dropdown from search bar being opened, but no engine is selected, it timeouts and fails.

Comment 1

[u279076]

(In reply to Andreea Matei [:AndreeaMatei] from comment #0)
> Started today with Nightly, on Linux so far.
> 
> I can reproduce this locally, most likely is also a regression. I see the
> dropdown from search bar being opened, but no engine is selected, it
> timeouts and fails.

When you say "locally" do you mean you can reproduce the failure in a local Mozmill testrun or that you can reproduce a Firefox regression?

If this is a Firefox regression please file a bug immediately, providing steps to reproduce and a regression window, and CC me so I can escalate further. 

Thank you

Comment 2

[Andreea Matei [:AndreeaMatei]]

I meant reproduce the failure by running the test. I saw some changes for search engines in todays pushlog, so we have to check what caused this behavior on our test.

Comment 3

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

If it turns out that it will also cause massive failures in our CI we should also get this one skipped and fixed as best this week.

Comment 4

[Daniela Petrovici]

The pushlog for the change is: 
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9db46ddfb517&tochange=ee5ca214e87c 

I used tinderbox builds to get the pushlog. I am building Firefox now to see what change caused this issue.

Comment 5

[Daniela Petrovici]

Attachment: patch v1.0

I haven't found the changeset responsible for this issue yet, but I was able to fix this issue by using mouseDown and mouseUp instead of click. Report for Linux is here: http://mozmill-crowd.blargon7.com/#/functional/report/cb9e33213cf3d2f37d7b0f8dcc8eb07c
I am still running reports for Mac and Windows and will update this issue with them as soon as they are done.

Comment 6

[Daniela Petrovici]

Report for MAC is: http://mozmill-crowd.blargon7.com/#/functional/report/cb9e33213cf3d2f37d7b0f8dcc8fd13d
Report for Windows is: http://mozmill-crowd.blargon7.com/#/functional/report/cb9e33213cf3d2f37d7b0f8dcc8ffb63

Comment 7

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 736216 [details] [diff] [review]
patch v1.0

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

If this stops the failures, I'm fine with it for now (ran it 20 times on Linux and OS X and haven't failed), but I'd be curious to know what changed in firefox that made waitThenClick() or click() to not work for that search dropdown. Can you please find the changeset?
Thanks.

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

Comment 8

[Daniela Petrovici]

This is the responsible changeset: http://hg.mozilla.org/mozilla-central/rev/3c55e92d87a6
Bug 738818 is the one for this changeset and is also responsible for bug 860330

Comment 9

[Andreea Matei [:AndreeaMatei]]

Hm, could this line be the cause? Google is set to be the default/current engine and we try to set it again:
http://hg.mozilla.org/mozilla-central/rev/3c55e92d87a6#l9.129

Comment 10

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

Wasn't Google the default engine before too? I don't think that this has been changed. Or do I miss something?

Comment 11

[Daniela Petrovici]

Yes, it was the default engine before, too, but I tested and these lines are the issue (please see below). 

Steps to test if this is the issue or not:
- hg checkout 3c55e92d87a6
- built Firefox
- verified that the issue reproduces with old repo (before the changes from this bug)

- removed the if from this line: http://hg.mozilla.org/mozilla-central/rev/3c55e92d87a6#l9.129 (file: /toolkit/components/search/nsSearchService.js)
- built Firefox
- RESULT: ran the test and the issue does not reproduces anymore

Comment 12

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

(In reply to Daniela Petrovici from comment #11)
> - removed the if from this line:
> http://hg.mozilla.org/mozilla-central/rev/3c55e92d87a6#l9.129 (file:
> /toolkit/components/search/nsSearchService.js)
> - built Firefox
> - RESULT: ran the test and the issue does not reproduces anymore

That likely means that the test was relying on the "engine-current" notification firing when setting the current engine to its existing value. That stopped happening thanks to those lines. So the test (or possibly code that the test is checking) should stop relying on that.

Comment 13

[Daniela Petrovici]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #12)
> (In reply to Daniela Petrovici from comment #11)
> > - removed the if from this line:
> > http://hg.mozilla.org/mozilla-central/rev/3c55e92d87a6#l9.129 (file:
> > /toolkit/components/search/nsSearchService.js)
> > - built Firefox
> > - RESULT: ran the test and the issue does not reproduces anymore
> 
> That likely means that the test was relying on the "engine-current"
> notification firing when setting the current engine to its existing value.
> That stopped happening thanks to those lines. So the test (or possibly code
> that the test is checking) should stop relying on that.

We are relying on the state of the searchbar-popup. After clicking on the default search engine, the state for the element does not change to closed. I have looked on mxr, but I did not find any information indicating that the state of the popup is dependent on the engine-current notification.

NOTE: This is the line from the mozmill test: http://hg.mozilla.org/qa/mozmill-tests/file/c0370a31d3bb/lib/search.js#l462

Comment 14

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

I think you need to debug further, because as you say the change you reference does not have any direct impact on the popup.

Comment 15

[Daniela Petrovici]

Attachment: patch v2.0

I have changed the selectedEngine method to not select the engine in case it is the default one. This is working properly on Linux, MAC and Windows (please see reports below). I have also changed back from mouseDown and mouseUp to waitThenClick as it was initially. It seem that not selecting again the default engine fixes the issue.

REPORTS:
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/c8094a822ef568b588c5eddaca0288b2
http://mozmill-crowd.blargon7.com/#/functional/report/c8094a822ef568b588c5eddaca0318c6
MAC:
http://mozmill-crowd.blargon7.com/#/functional/report/c8094a822ef568b588c5eddaca056811
http://mozmill-crowd.blargon7.com/#/functional/report/c8094a822ef568b588c5eddaca0288b2
http://mozmill-crowd.blargon7.com/#/functional/report/8ec48e7ab0431a61b624e36d31fccd0b
http://mozmill-crowd.blargon7.com/#/functional/report/8ec48e7ab0431a61b624e36d31fc6a12

NOTE: The previous patch was just an workaround in order to quickly fix the issue.

Comment 16

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

Comment on attachment 739019 [details] [diff] [review]
patch v2.0

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

::: lib/search.js
@@ +447,5 @@
>     * @param {string} name
>     *        Name of the search engine to select
>     */
>    set selectedEngine(name) {
> +    if (name !== Services.search.defaultEngine.name)  {

What if we want to select the default search engine, perhaps after first switching to an alternative engine? It looks like this would just silently ignore the request.

Comment 17

[Daniela Petrovici]

Attachment: patch v2.1

Changed the function to use the current search engine. In case the engine we want to select is already the current engine we are not doing anything. Reports:
Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/c8094a822ef568b588c5eddaca1527f3
http://mozmill-crowd.blargon7.com/#/functional/report/c8094a822ef568b588c5eddaca140f3f
MAC:
http://mozmill-crowd.blargon7.com/#/functional/report/c8094a822ef568b588c5eddaca16e7dc
http://mozmill-crowd.blargon7.com/#/functional/report/c8094a822ef568b588c5eddaca16f36c
Windows:
http://mozmill-crowd.blargon7.com/#/functional/report/c8094a822ef568b588c5eddaca1700a6
http://mozmill-crowd.blargon7.com/#/functional/report/c8094a822ef568b588c5eddaca16eb17

Comment 18

[Andreea Matei [:AndreeaMatei]]

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

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

Other than the comments above, this change makes sense, why should we select it again if it's already there?

::: lib/search.js
@@ +443,3 @@
>  
>    /**
>     * Select the search engine with the given name

We can now add here "if is not already selected"

@@ +447,5 @@
>     * @param {string} name
>     *        Name of the search engine to select
>     */
>    set selectedEngine(name) {
> +    if (name !== Services.search.currentEngine.name)  {

You have an extra space added after the if condition. No need for the extra blank line either.

@@ +461,5 @@
> +        return !this.enginesDropDownOpen;
> +      }, "Search engines drop down has been closed", undefined, undefined, this);
> +
> +      assert.waitFor(function () {
> +        return this.selectedEngine === name

Can you please add a semicolon here? Not sure why is missing but since we're here..

Comment 19

[Daniela Petrovici]

Attachment: patch v2.2

Modified patch based on review. Reports: 
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/c8094a822ef568b588c5eddaca82bf61
MAC: http://mozmill-crowd.blargon7.com/#/functional/report/c8094a822ef568b588c5eddaca864cbb
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/c8094a822ef568b588c5eddaca88f875

Comment 20

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 740241 [details] [diff] [review]
patch v2.2

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

It needs a small update due to landing of patch from bug 848200, related to TIMEOUT removal.

Comment 21

[Daniela Petrovici]

Attachment: patch v2.3

Modified patch to remove TIMEOUT. Reports are:
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1159c07
MAC: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde11601f0
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1182f9c

Comment 22

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 741234 [details] [diff] [review]
patch v2.3

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

Thanks, landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/d551d6336962 (default)

Giving that it would be an improvement to select the engine only if is not selected already (there's no point, right?), I think it will be good to backport it.

Comment 23

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

Yes, sounds good to me. Even we haven't seen failures on older branches yet, the patch could be bullet-proof any upcoming regression. So if we stay green, lets get it backported.

Comment 24

[Daniela Petrovici]

Attachment: patch v1.0 for Aurora

The patch for default does not apply cleanly on Aurora due to the fact that bug 848200 was not backported on Aurora. The patch for Aurora also does not apply cleanly on Beta due to this line: http://hg.mozilla.org/qa/mozmill-tests/file/beta/lib/search.js#l504
I will provide a new one for Beta branch.

Reports for Aurora:
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde15e18cd
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde15e5973
MAC: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde15df08d

Comment 25

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 741873 [details] [diff] [review]
patch v1.0 for Aurora

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

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

Comment 26

[Daniela Petrovici]

Attachment: patch v1.0 for Beta, release and esr17

Patch for Beta that applies cleanly on ESR17 and Release.
1) Reports for Beta
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde17597c1
MAC: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde175874c
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde166c95c

2) Release:
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1752219
MAC: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1752137
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1753ffa

3) ESR17
MAC: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1755a2e
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1754ab8
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1755ee3

Comment 27

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 742262 [details] [diff] [review]
patch v1.0 for Beta, release and esr17

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

Transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/27f618841e1b (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/4943bf853dc7 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/0fdd759a1209 (esr17)