773581 - Failure in testGetMoreSearchEngines | Search engine 'IMDB' has been installed

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

It looks like this has recently started failing on Linux and Windows across several branches.

Comment 1

[Remus Pop (:RemusPop)]

Attachment: patch v1 (all branches)

Talked with Dave on IRC that we should get this test open a local page when clicking "Get more search engines". We should also remove the installing of a search engine because that is covered by testAddMozSearchProvider.

Comment 2

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

Comment on attachment 641876 [details] [diff] [review]
patch v1 (all branches)

>+  // Check that the right link has been opened
>+  var location = controller.window.content.location;
>+  expect.equal(location, SEARCH_ENGINE_URL,
>+               'Actual location must match "Get more search engines link"');

I think a better message would be "Get more search engines page has opened"

Otherwise, this looks good to me. Please ask Henrik for the next review as I'd like to see his thoughts before we proceed.

Comment 3

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

We need progress on this bug! If you cannot handle it Remus please talk to Vlad or Alex to finish it up. We can't wait longer. We will have to skip the test otherwise.

Comment 4

[Remus Pop (:RemusPop)]

Attachment: patch v2 (all branches)

Changed the message to the one Dave recommended.

Comment 5

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

Beside this fix do we know why this was/is broken? I don't want that we pave over a regression in Firefox or on AMO.

Comment 6

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

Comment on attachment 642576 [details] [diff] [review]
patch v2 (all branches)

>+  // Check that the right link has been opened
>+  var location = controller.window.content.location;
>+  expect.equal(location.toString(), SEARCH_ENGINE_URL,
>+               '"Get more search engines" page has opened');

I will update this patch so it makes use of utils.assertLoadedUrlEqual() here. Otherwise it looks fine.

Comment 7

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

Attachment: Patch v3

Comment 8

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

Pushed to default and aurora for now:
http://hg.mozilla.org/qa/mozmill-tests/rev/ce334305532a (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/5fcf97a36386 (aurora)

If no failure occurs anymore tomorrow, I will backport it to all other branches.

Comment 9

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

(In reply to Henrik Skupin (:whimboo) from comment #5)
> Beside this fix do we know why this was/is broken? I don't want that we pave
> over a regression in Firefox or on AMO.

Alex, this is still an open question. You said you will take care of it a couple hours ago. So can you please followup with your investigation?

Comment 10

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

http://hg.mozilla.org/qa/mozmill-tests/rev/e34f5b097e6b (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/7391c8f971d1 (release)

Alex, please followup with a backport for the esr10 branch.

Comment 11

[Alex Lakatos[:AlexLakatos]]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> (In reply to Henrik Skupin (:whimboo) from comment #5)
> > Beside this fix do we know why this was/is broken? I don't want that we pave
> > over a regression in Firefox or on AMO.
> 
> Alex, this is still an open question. You said you will take care of it a
> couple hours ago. So can you please followup with your investigation?
I backed out Remus's patch locally, but I can't reproduce the failure. Based on the error message, I'm guessing we were not waiting enough for the search engine to be registered. Seeing as how it was running in a VM, that may have been the issue.

Comment 12

[Alex Lakatos[:AlexLakatos]]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> http://hg.mozilla.org/qa/mozmill-tests/rev/e34f5b097e6b (beta)
> http://hg.mozilla.org/qa/mozmill-tests/rev/7391c8f971d1 (release)
> 
> Alex, please followup with a backport for the esr10 branch.
Working at the backport I saw a guideline from our style guide was not followed. As well, the setup, teardown and test functions were rewrited not to be anonymous, but not the handler one. Should I backport the patch as is, or change it?

Comment 13

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v3.0[esr]

patch for esr branch, as is, without edits

Comment 14

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

Landed on esr10 branch:
http://hg.mozilla.org/qa/mozmill-tests/rev/435bf99257e3

Not sure who owns the search component in Litmus right now, but we should check if we should also update the manual test. Alex, can you please take care of it?

Comment 15

[Alex Lakatos[:AlexLakatos]]

The litmus tests have you as an author. The only other author in Litmus for Search Manager is Zach Lipton, so adding him to the cc, for feedback. Adding Simona as well, as the current owner.

Comment 16

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

We transferred ownership from me to someone else I can't remember of right now by last year. It could be Simona. Zach is not involved here.

Comment 17

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #16)
> We transferred ownership from me to someone else I can't remember of right
> now by last year. It could be Simona. Zach is not involved here.

Sorry to interfere but Simona just took this ownership. Could not be from last year so I'm pretty sure we are talking about someone else.

Comment 18

[Simona Badau, Desktop QA]

I currently own the search component and I took a look over this test in Litmus.
https://litmus.mozilla.org/show_test.cgi?id=15702

IMO this Litmus test case should remain as it is and shouldn't suffer other modifications because it verifies that a Search Engine can be installed from AMO and that after installation the newly installed search tool is located in the search bar drop down menu.

Test https://litmus.mozilla.org/show_test.cgi?id=15114 is also related to the Search Tools but this test does not necessarily takes you to an add-on that after installation is located in the search bar drop down menu. In this case the newly added search tool could be located in the Add-on Manager. 

Moreover the install dialog in  test testGetMoreSearchEngines is different than the install dialog from the test mentioned above.

This test https://litmus.mozilla.org/show_test.cgi?id=15699 does not verify that the search engine is installed from AMO.

Comment 19

[Alex Lakatos[:AlexLakatos]]

This seems to have fallen in nirvana. Henrik, can we get your feedback regarding Simona's comment here?

Comment 20

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

(In reply to Simona B [QA] from comment #18)
> I currently own the search component and I took a look over this test in
> Litmus.
> https://litmus.mozilla.org/show_test.cgi?id=15702
> 
> IMO this Litmus test case should remain as it is and shouldn't suffer other
> modifications because it verifies that a Search Engine can be installed from
> AMO and that after installation the newly installed search tool is located
> in the search bar drop down menu.

I thought there is another Litmus test already which is doing exactly the same. That's why we also have a separate Mozmill test for. If we are doing the same operation twice we have a >100% coverage of the underlaying code which is not necessary.

> Test https://litmus.mozilla.org/show_test.cgi?id=15114 is also related to
> the Search Tools but this test does not necessarily takes you to an add-on
> that after installation is located in the search bar drop down menu. In this
> case the newly added search tool could be located in the Add-on Manager. 

This is not a functional test in Litmus but an remote test.

> This test https://litmus.mozilla.org/show_test.cgi?id=15699 does not verify
> that the search engine is installed from AMO.

Well, installing an engine from AMO specifically should not be a requirement for our functional test. The thing which is important here is that we make use of a trusted source. If it is on AMO or on mozqa.com doesn't make a difference. But for the latter we have the control over which is by far better. If you want to test that a search engine can be installed from AMO, a test should really be added under the addons.mozill.org group of Litmus.

Comment 21

[Simona Badau, Desktop QA]

I can see your point, and since there is another Litmus test case (https://litmus.mozilla.org/show_test.cgi?id=15700) that verifies that a new search engine is added to the search bar drop down menu (that pretty much covers the extra steps from this particular test case), I decided to edit the Litmus test case in order to match the Mozmill one.

I updated the test cases for the following branches:
- Aurora: https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=15702
- Release: https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=64049
- Esr: https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=40800
- Beta - there are no test runs in Litmus for Firefox 15 (the test runs for Firefox 15 were done in MozTrap and this particular test case is not added there yet).

Comment 22

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

(In reply to Simona B [QA] from comment #21)
> I can see your point, and since there is another Litmus test case
> (https://litmus.mozilla.org/show_test.cgi?id=15700) that verifies that a new
> search engine is added to the search bar drop down menu (that pretty much
> covers the extra steps from this particular test case), I decided to edit
> the Litmus test case in order to match the Mozmill one.

Keep in mind that mozSearch and open search engines are different types. But beside the above Litmus test the following will cover the first one: https://litmus.mozilla.org/show_test.cgi?id=15699

> I updated the test cases for the following branches:
> - Aurora: https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=15702
> - Release: https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=64049
> - Esr: https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=40800
> - Beta - there are no test runs in Litmus for Firefox 15 (the test runs for
> Firefox 15 were done in MozTrap and this particular test case is not added
> there yet).

That's reasonable. Thanks a lot!

Comment 23

[Alex Lakatos[:AlexLakatos]]

https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=15702
https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=64049
https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=40800

Edited the litmus testcases to point to the right file in our repo.