MODULE: tests/functional/testSearch/testSearchSelection.js TEST: testGetMoreEngines FAIL: TimeoutError("controller.waitForEval: Timeout exceeded for 'subject.selectedTabIndex == subject.expectedIndex'")@mozmill-tests/tests/functional/testSearch/testSearchSelection.js:83 BRANCH: mozilla-aurora, default

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/20968059

Changed waitForEval to waitFor

Comment on attachment 574287 [details] [diff] [review] patch fix v1.0 - initial proposed fix >+const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/'); >+const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 'layout/mozilla_mission.html'; > > const TIMEOUT = 5000; > >-const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/'); >-const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 'layout/mozilla_mission.html'; >- >-var setupModule = function() { >+function setupModule() { These changes are unnecessary. Please only make changes which directly fix the failure. Making these changes now breaks test consistency and will impede broad-sweep refactoring efforts. The same goes for any following code which you "refactored" for style. >+ controller.waitFor(function () { >+ return (tabs.selectedIndex === tabIndex); >+ }, "Selected tab: '" + tabIndex + "'"); Can you refactor the message to read: "Selected tab X, expected tab Y" >+ controller.waitFor(function () { >+ return (tabs.selectedIndex === tabIndex + 1); >+ }, "Selected tab: '" + (tabIndex + 1) + "'"); Same here.

This test is currently failing on latest Aurora and latest Nightly because those builds do not open a tab in background anymore. I am uploading a mozmill work patch to demonstrate this. If you want to test manually, do it with following STR: 1. Open Firefox Aurora or Nightly 2. Navigate to google.com and search "Firefox" 3. Go to suggestions and mouse select the word "Firefox" from anywhere on the page 4. Right click and use the context menu to search (Search "ebay" for "word") A new tab is opened in foreground, even if the pref 'browser.tabs.loadInBackground' is set to true. The question is - Do we want this? If this is an expected behavior we'll change the test, if not, we have ourselves a Firefox bug. What do you think ?

Please apply the patch locally to test on, and watch the console for dumping messages. Compare the results with a firefox release or beta build and a firefox aurora or nightly build.

Vlad, the behaviour you got now is the expected one as of bug 695482. Also see bug 696747 for reference to get the wanted behaviour when that lands.

(In reply to Remus Pop (:RemusPop) from comment #6) > Vlad, the behaviour you got now is the expected one as of bug 695482. > > Also see bug 696747 for reference to get the wanted behaviour when that > lands. Even if bug 695482 is landed reading the comments shows that people have second opinions on this still. Bug 696747 has an r- patch so still needs work. We can definitely fix this failing test but my question is now: - Do we provide a separate fix for aurora and nightly and just fix beta and release at merges time? (can be tricky) - How do we proceed? My personal idea is to disable the test on aurora and nightly and keep them for beta and release which are more important to have this test, and then after 12 weeks make the change on all branches.

Update the test so it aligns with the new behavior and watch the other bug so we can update the test later. Same applies to the Litmus test which needs an update.

release and beta branch do not need a fix yet.

(In reply to Henrik Skupin (:whimboo) from comment #8) > Update the test so it aligns with the new behavior and watch the other bug > so we can update the test later. Same applies to the Litmus test which needs > an update. Updated litmus test for aurora branch https://litmus.mozilla.org/show_test.cgi?id=15707 We don't have the default (nightly) branch there so no update in litmus for that. Hope I didn't do anything stupid.

Comment on attachment 575450 [details] [diff] [review] patch v1:default/aurora [backed-out] Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/b475c4b5a1a0 (default) http://hg.mozilla.org/qa/mozmill-tests/rev/097dd432a52d (mozilla-aurora)

Please mark VERIFIED and update the spreadsheet after checking tomorrow's results.

Verified http://mozmill-release.brasstacks.mozilla.com/#/functional/top?branch=All&platform=All&from=2011-11-23&to=2011-11-23

As given on the dependent bug the new feature has been backed out. That means that we should also backout this patch from default and aurora.

Comment on attachment 575450 [details] [diff] [review] patch v1:default/aurora [backed-out] Backed out: http://hg.mozilla.org/qa/mozmill-tests/rev/2e459e48fd4d (default) http://hg.mozilla.org/qa/mozmill-tests/rev/7176167b1648 (mozilla-aurora) We will need to revisit this once bug 695482 is resolved.

Vlad, please submit a patch immediately which marks this test disabled.

Block should be the other way around...this bug depends on the other bug.

skip patch as requested - we'll need to address a fix once the dependency is fixed

Comment on attachment 580027 [details] [diff] [review] disable test (default/aurora) [checked-in] Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/f084f96f5f89 (default) http://hg.mozilla.org/qa/mozmill-tests/rev/7a3bcd18d167 (mozilla-aurora)

The dependency is shown as fixed, will investigate if the fix allows me to provide a fix patch for this test.

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #20) > The dependency is shown as fixed, will investigate if the fix allows me to > provide a fix patch for this test. The dependency patch fixes our failure for nightly, still the other branches are still affected and the test should remain disabled for aurora, beta and firefox release branch. I suggest reopening this test once bug 592275 is fixed. Adding dependency

Lets enable the test again on default.

(In reply to Henrik Skupin (:whimboo) from comment #22) > Lets enable the test again on default. Feel free to go on and enable it on default branch

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #23) > (In reply to Henrik Skupin (:whimboo) from comment #22) > > Lets enable the test again on default. > > Feel free to go on and enable it on default branch Forget I said that - needs a little more investigation. I'll follow up shortly

It's time to get this test back in. This patch fixes our failures for the default branch

Enable the test as it is on mozilla-aurora branch The new pref which was causing our failure is not landed for mozilla-aurora.

Comment on attachment 605688 [details] [diff] [review] [mozilla-aurora][mozilla-beta][mozilla-release] enable test v1.0 The patch is eligible and tested also on mozilla-beta and mozilla-release branch. We can enable also on those branch. Dependencies were fixed so we can re-use this test

Fixed commit message to inform about the changes in all branches, not just in aurora

The fix patch passes locally on the functional testrun for nightly http://mozmill-crowd.blargon7.com/#/functional/report/e438d6e3916b2b636037d77445c613bd

Comment on attachment 605684 [details] [diff] [review] [default] fix v1.0 [landed:default] Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/9f645bb2a217 (default)

Comment on attachment 605691 [details] [diff] [review] [aurora,beta,release] enable v1.1 [landed] Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/4c8f7ea76716 (mozilla-aurora) http://hg.mozilla.org/qa/mozmill-tests/rev/5d530e52dac9 (mozilla-beta) http://hg.mozilla.org/qa/mozmill-tests/rev/8b81cf754fb1 (mozilla-release)

Please verify that the test passes on all branches and reopen if not.

This is appearing in aurora too: http://mozmill-ci.blargon7.com/#/functional/failure?branch=13.0&platform=All&from=2012-03-20&to=2012-03-27&test=%2FtestSearch%2FtestSearchSelection.js&func=testSearchSelection.js%3A%3AtestSearchSelectionViaContextMenu So just flipping a pref will do the trick both for aurora and nightly.

Before we flip a pref I would suggest that we use the default behavior of Firefox to test it.

Changed the pref and also moved it to a constant

Comment on attachment 609664 [details] [diff] [review] fix v2 (aurora) >+const PREF_LOAD_IN_BACKGROUND = "browser.search.context.loadInBackground"; > const TIMEOUT = 5000; Looks good but please separate both from each other. Those constants are from different areas.

Comment on attachment 609664 [details] [diff] [review] fix v2 (aurora) ++

Comment on attachment 609664 [details] [diff] [review] fix v2 (aurora) As I have mentioned in my feedback please get the patch updated. It removes one unnecessary round of reviews.

Added a empty line for delimiting the new constant.

Comment on attachment 610068 [details] [diff] [review] patch v3 (aurora) [landed] So on which branches do I have to land this patch? Default and Aurora? the patch description is misleading here.

This fixes the teardownModule which was not clearing the right preference. I also added a pref constant as in the aurora fix.

Landed both patches: http://hg.mozilla.org/qa/mozmill-tests/rev/5d3977fcdef5 (master) http://hg.mozilla.org/qa/mozmill-tests/rev/3de6f2b2129a (aurora)

Thanks Henrik. Remus, please verify this is fixed with tomorrow's testruns.