Last Comment Bug 773581 - Failure in testGetMoreSearchEngines | Search engine 'IMDB' has been installed
: Failure in testGetMoreSearchEngines | Search engine 'IMDB' has been installed
Status: RESOLVED FIXED
[mozmill-test-failure]
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Alex Lakatos[:AlexLakatos]
:
Mentors:
http://mozmill-ci.blargon7.com/#/func...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-13 03:12 PDT by Dave Hunt (:davehunt)
Modified: 2012-08-07 08:01 PDT (History)
5 users (show)
hskupin: in‑litmus+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed


Attachments
patch v1 (all branches) (5.85 KB, patch)
2012-07-13 07:30 PDT, Remus Pop (:RemusPop)
dave.hunt: review-
Details | Diff | Splinter Review
patch v2 (all branches) (5.86 KB, patch)
2012-07-16 07:23 PDT, Remus Pop (:RemusPop)
hskupin: review-
Details | Diff | Splinter Review
Patch v3 (5.70 KB, patch)
2012-07-16 13:02 PDT, Henrik Skupin (:whimboo)
dave.hunt: review+
Details | Diff | Splinter Review
patch v3.0[esr] (5.86 KB, patch)
2012-07-18 00:18 PDT, Alex Lakatos[:AlexLakatos]
hskupin: review+
Details | Diff | Splinter Review

Description Dave Hunt (:davehunt) 2012-07-13 03:12:15 PDT
It looks like this has recently started failing on Linux and Windows across several branches.
Comment 1 Remus Pop (:RemusPop) 2012-07-13 07:30:22 PDT
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.
Comment 2 Dave Hunt (:davehunt) 2012-07-13 07:53:04 PDT
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) 2012-07-16 06:39:22 PDT
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) 2012-07-16 07:23:45 PDT
Created attachment 642576 [details] [diff] [review]
patch v2 (all branches)

Changed the message to the one Dave recommended.
Comment 5 Henrik Skupin (:whimboo) 2012-07-16 12:54:18 PDT
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) 2012-07-16 12:59:16 PDT
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) 2012-07-16 13:02:48 PDT
Created attachment 642688 [details] [diff] [review]
Patch v3
Comment 8 Henrik Skupin (:whimboo) 2012-07-16 15:27:04 PDT
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) 2012-07-17 07:57:23 PDT
(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) 2012-07-17 08:02:44 PDT
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] 2012-07-17 08:15:47 PDT
(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] 2012-07-17 10:48:27 PDT
(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] 2012-07-18 00:18:37 PDT
Created attachment 643274 [details] [diff] [review]
patch v3.0[esr]

patch for esr branch, as is, without edits
Comment 14 Henrik Skupin (:whimboo) 2012-07-18 07:00:20 PDT
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] 2012-07-18 08:51:01 PDT
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) 2012-07-18 08:57:50 PDT
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) 2012-07-18 09:00:41 PDT
(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 B [:simonab ] -PTO- back Sept 5th 2012-07-19 07:18:27 PDT
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] 2012-08-06 01:42:45 PDT
This seems to have fallen in nirvana. Henrik, can we get your feedback regarding Simona's comment here?
Comment 20 Henrik Skupin (:whimboo) 2012-08-06 15:36:45 PDT
(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 B [:simonab ] -PTO- back Sept 5th 2012-08-07 04:38:52 PDT
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) 2012-08-07 04:46:49 PDT
(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!

Note You need to log in before you can comment on or make changes to this bug.