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

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: davehunt, Assigned: AlexLakatos)

Tracking

unspecified
Bug Flags:
in-litmus +

Firefox Tracking Flags

(firefox14 fixed, firefox15 fixed, firefox16 fixed, firefox17 fixed, firefox-esr10 fixed)

Details

(Whiteboard: [mozmill-test-failure], URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
It looks like this has recently started failing on Linux and Windows across several branches.
Created attachment 641876 [details] [diff] [review]
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.
Assignee: nobody → remus.pop
Status: NEW → ASSIGNED
Attachment #641876 - Flags: review?(dave.hunt)
(Reporter)

Updated

5 years ago
status-firefox-esr10: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
(Reporter)

Comment 2

5 years ago
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.
Attachment #641876 - Flags: review?(dave.hunt) → review-
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.
Created attachment 642576 [details] [diff] [review]
patch v2 (all branches)

Changed the message to the one Dave recommended.
Attachment #641876 - Attachment is obsolete: true
Attachment #642576 - Flags: review?(hskupin)
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 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.
Attachment #642576 - Flags: review?(hskupin) → review-
Created attachment 642688 [details] [diff] [review]
Patch v3
Attachment #642576 - Attachment is obsolete: true
Attachment #642688 - Flags: review?(dave.hunt)
(Reporter)

Updated

5 years ago
Attachment #642688 - Flags: review?(dave.hunt) → review+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox16: affected → fixed
status-firefox17: --- → fixed
Resolution: --- → FIXED
(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?
Assignee: remus.pop → alex.lakatos
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.
status-firefox14: --- → fixed
status-firefox15: affected → fixed
(Assignee)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
(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?
(Assignee)

Comment 13

5 years ago
Created attachment 643274 [details] [diff] [review]
patch v3.0[esr]

patch for esr branch, as is, without edits
(Assignee)

Updated

5 years ago
Attachment #643274 - Flags: review?(hskupin)
Attachment #643274 - Flags: review?(dave.hunt)
Attachment #643274 - Flags: review?(hskupin)
Attachment #643274 - Flags: review?(dave.hunt)
Attachment #643274 - Flags: review+
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?
status-firefox-esr10: affected → fixed
Flags: in-litmus?(alex.lakatos)
(Assignee)

Comment 15

5 years ago
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.
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.
(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.
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.
(Assignee)

Comment 19

5 years ago
This seems to have fallen in nirvana. Henrik, can we get your feedback regarding Simona's comment here?
(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.
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).
(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!
Flags: in-litmus?(alex.lakatos) → in-litmus+
(Assignee)

Comment 23

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.