Failure in testFocusAndSearch | Disconnect Error: Application unexpectedly closed

RESOLVED FIXED

Status

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

People

(Reporter: RemusPop, Assigned: vladmaniac)

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][qa-], URL)

Attachments

(5 attachments, 7 obsolete attachments)

917 bytes, patch
whimboo
: review+
Details | Diff | Splinter Review
6.56 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
832 bytes, patch
whimboo
: review+
Details | Diff | Splinter Review
7.13 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
6.28 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
This seems to be an intermittent issue appearing on Nightly and Aurora. Windows and Linux are affected for now.
It's more and more happening now. Please skip this test for now asap.
Severity: normal → critical
(Reporter)

Updated

5 years ago
Assignee: nobody → remus.pop
(Reporter)

Comment 2

5 years ago
Created attachment 630876 [details] [diff] [review]
skip test v1 (default, aurora, beta) [checked-in]

Skips the test.
Attachment #630876 - Flags: review?(hskupin)
(Reporter)

Updated

5 years ago
Attachment #630876 - Attachment description: patch v1 (default, aurora) → skip test v1 (default, aurora)
Comment on attachment 630876 [details] [diff] [review]
skip test v1 (default, aurora, beta) [checked-in]

Pushed:
http://hg.mozilla.org/qa/mozmill-tests/rev/5261453b6744 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/8584638c969f (aurora)

Remus, please disable those litmus tests on the aurora branch.
Attachment #630876 - Attachment description: skip test v1 (default, aurora) → skip test v1 (default, aurora) [checked-in]
Attachment #630876 - Flags: review?(hskupin) → review+
Flags: in-litmus?
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][mozmill-test-skipped]
(Reporter)

Comment 4

5 years ago
Disabled in Litmus: https://litmus.mozilla.org/show_test.cgi?id=15706
Flags: in-litmus? → in-litmus-
Remus, are you able to reproduce this problem on your local boxes on any platform? Would you mind to run some tests?
(Reporter)

Comment 6

5 years ago
I have this testrun from a Ubuntu 12.04 machine:
http://mozmill-crowd.blargon7.com/#/functional/report/87961186c2b807b7747aa7e6d400cb48
No failure.
I'll try to run them on 11.04 and 11.10.
(Reporter)

Comment 7

5 years ago
This is the report on 11.04:
http://mozmill-crowd.blargon7.com/#/functional/report/87961186c2b807b7747aa7e6d401c3a3
This is happening across platforms. So please try out each of those. If you can't reproduce we probably have to try to reproduce on the CI master.
(Reporter)

Comment 9

5 years ago
No luck in getting a disconnect error.
This happened yesterday with the next upcoming Firefox 14 beta build:
http://mozmill-ci.blargon7.com/#/functional/report/e67171ea696231bb192f565615000625

Probably try with such a build.
So we got this failure across all platforms for the Firefox 14.0b8 release candidate. I was running a single testrun on OS X but nothing has been shown. So I get the impression that this is somewhat related to the cpu load. I will trigger an on-demand testrun now so we can check if it happens in such a case.
status-firefox-esr10: --- → unaffected
status-firefox13: --- → unaffected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
No failures when I triggered the ondemand job:
http://mozmill-ondemand.blargon7.com/#/functional/reports?branch=14.0&platform=All&from=2012-06-20&to=2012-06-20

So I'm going to disable the test on beta too:
http://hg.mozilla.org/qa/mozmill-tests/rev/710f4d67b0bb

Remus, please update the Litmus tests if not done automatically yet. Thanks.
(Reporter)

Updated

5 years ago
Attachment #630876 - Attachment description: skip test v1 (default, aurora) [checked-in] → skip test v1 (default, aurora, beta) [checked-in]
(Reporter)

Comment 13

5 years ago
Disabled Litmus id 64053.
We cannot disable this one Litmus test but we have to remove the Mozmill group from all of them across branches.
Flags: in-litmus- → in-litmus?(remus.pop)
(Assignee)

Comment 15

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #14)
> We cannot disable this one Litmus test but we have to remove the Mozmill
> group from all of them across branches.

Removed the Mozmill group from the test across branches: 

https://litmus.mozilla.org/show_test.cgi?id=40804 (esr)
https://litmus.mozilla.org/show_test.cgi?id=55699 (Firefox 13)
https://litmus.mozilla.org/show_test.cgi?id=64053 (Firefox 14)
https://litmus.mozilla.org/show_test.cgi?id=15706 (Aurora)
(Assignee)

Updated

5 years ago
Flags: in-litmus?(remus.pop) → in-litmus+
Given that we fixed a lot of those application disconnect errors lately, I wonder if we could re-enable this test. Vald, could you try with a loaded system before we backout the skip patch? Thanks.
Assignee: remus.pop → vlad.mozbugs
Status: NEW → ASSIGNED
(Assignee)

Comment 17

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #16)
> Given that we fixed a lot of those application disconnect errors lately, I
> wonder if we could re-enable this test. Vald, could you try with a loaded
> system before we backout the skip patch? Thanks.

My plan here is to try with win XP, since this system has a history of giving us the most failures. I'll just flood the VMs with extra tasks and come back with the results shortly
(Assignee)

Comment 18

5 years ago
So I do not see this test failing again: 
Ran on Ubuntu 11.04 and a win XP heavy loaded VM with both mozmill 1.5.13 and mozmill 1.5.15 

http://mozmill-crowd.blargon7.com/#/functional/report/89726f6b98208a209e7ce2df1035edda
http://mozmill-crowd.blargon7.com/#/functional/report/89726f6b98208a209e7ce2df10359e32

I think we can re-enalbe the test
(Assignee)

Comment 19

5 years ago
Created attachment 644272 [details] [diff] [review]
re enable test patch 1.0

Re enables the test
Attachment #644272 - Flags: review?(hskupin)
(Assignee)

Comment 20

5 years ago
Comment on attachment 644272 [details] [diff] [review]
re enable test patch 1.0

adding Dave
Attachment #644272 - Flags: review?(dave.hunt)
Comment on attachment 644272 [details] [diff] [review]
re enable test patch 1.0

Patch was not necessary - the original patch to disable the test has been backed out:
http://hg.mozilla.org/qa/mozmill-tests/rev/36c5cf88e466 (default)
Attachment #644272 - Flags: review?(hskupin)
Attachment #644272 - Flags: review?(dave.hunt)
Attachment #644272 - Flags: review-
Relanded skip patch because it's failing across all platforms:

http://hg.mozilla.org/qa/mozmill-tests/rev/31f2e3267ae6
status-firefox13: unaffected → disabled
status-firefox14: affected → disabled
status-firefox15: affected → disabled
status-firefox16: affected → disabled
status-firefox17: --- → disabled
Attachment #644272 - Attachment is obsolete: true
Thanks Henrik. What's the next action for this if it can't be replicated locally?
The first thing we have to determine is which search engine is used and which web page gets opened when the test gets run on our box in MV. We do not use a local page yet, which could fix it at all.
(Assignee)

Comment 25

5 years ago
(In reply to Henrik Skupin (:whimboo) [away 07/27 - 08/05] from comment #24)
> The first thing we have to determine is which search engine is used and
> which web page gets opened when the test gets run on our box in MV. We do
> not use a local page yet, which could fix it at all.

I can't say about the box in MV, but the test uses Google search engine and opens a google search page. We might want to use a custom search engine but I think that implies installing it first in the setupModule as a test prerequisite.
(Assignee)

Comment 26

5 years ago
The local test page which installs a custom search engine is http://mozqa.com/data/firefox/search/opensearch.html
Yes, as Henrik points out, we need to switch to using a local page. The custom search engine mentioned in comment 26 is not local but certainly an improvement over Google. Please work on a patch, which ideally would add a search engine via a backend API based on a local page, or failing that, uses the hosted litmus-data page.
(Assignee)

Comment 28

5 years ago
(In reply to Dave Hunt (:davehunt) from comment #27)
> Yes, as Henrik points out, we need to switch to using a local page. The
> custom search engine mentioned in comment 26 is not local but certainly an
> improvement over Google. Please work on a patch, which ideally would add a
> search engine via a backend API based on a local page, or failing that, uses
> the hosted litmus-data page.

Dave, the page is available also in our /data folder under mozmill-tests
This makes it local so I am going forward with the idea from comment 25, comment 26 and comment 27 and come up with a patch
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #28)
> Dave, the page is available also in our /data folder under mozmill-tests
> This makes it local so I am going forward with the idea from comment 25,
> comment 26 and comment 27 and come up with a patch

The page is local, but the search engine it installs is not - it is hard-coded to hg.mozilla.org.
(Assignee)

Comment 30

5 years ago
Created attachment 647958 [details] [diff] [review]
patch v1.0

* Updated the test to comply with code standards
* Removed unnecessary data in the test (linked litmus tests) 
* Added code to make the test use a local search engine
Attachment #647958 - Flags: review?(alex.lakatos)
Comment on attachment 647958 [details] [diff] [review]
patch v1.0

>+function testClickAndSearch() {
>+  searchBar.selectedEngine = SEARCH_ENGINE_NAME;
This should be done in the setupModule.

>+function testShortcutAndSearch() {
>+  searchBar.selectedEngine = SEARCH_ENGINE_NAME;
This should be done in the setupModule.

> /**
>- * Map test functions to litmus tests
>+ * Install a local search engine
>  */
>-// testClickAndSearch.meta = {litmusids : [8241]};
>-// testShortcutAndSearch.meta = {litmusids : [8242]};
>+function installSearchEngine() {
>+  controller.open(LOCAL_TEST_PAGE);
>+  controller.waitForPageLoad();
> 
>-setupModule.__force_skip__ = "Bug 761984 - Failure in testFocusAndSearch |" +
>-                             "Disconnect Error: Application unexpectedly closed";
>+  var addEngine = new elementslib.Name(controller.tabs.activeTab, "add");
>+  var md = new modalDialog.modalDialog(controller.window);
>+  
>+  md.start(handleSearchInstall);
>+  controller.click(addEngine);
>+  md.waitForDialog(TIMEOUT_INSTALL_DIALOG);
>+}
This does not need a separate function. Move the code to the setupModule. But first try and do this using back-end code and not the ui.
Attachment #647958 - Flags: review?(alex.lakatos) → review-
(Assignee)

Comment 32

5 years ago
Created attachment 648303 [details] [diff] [review]
patch v1.1

* using backend API to install a search engine 
* opening a local test page to get access to search engine URI
* using a litmus data search engine rather than Google 
* test is modified to comply with our coding standards 
* unnecessary code was killed 
* search.js has a minor update - instantiation of the search service globally to have access of the backend API in tests (see places.js for a working example)
Attachment #647958 - Attachment is obsolete: true
Attachment #648303 - Flags: review?(dave.hunt)
Comment on attachment 648303 [details] [diff] [review]
patch v1.1


> /**
>+ *  Instance of the search service to gain access to the search API
>+ *
>+ *  @see http://mxr.mozilla.org/mozilla-central (nsIBrowserSearchService.idl)
>+ */
>+var browserSearchService = Cc["@mozilla.org/browser/search-service;1"]
>+                              .getService(Ci.nsIBrowserSearchService);

This is already used by searchBar, so we should add a method there for adding a search engine via the API.

>+exports.browserSearchService = browserSearchService;

We shouldn't export this.

>+const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/');
>+const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 'search/opensearch.html';

The local test page should be the search results (see comments below)

>+  installSearchEngine();

We don't need a separate method here, include this in the setupModule

>+ * Install a local search engine
>  */
>+function installSearchEngine() {
>+  controller.open(LOCAL_TEST_PAGE);
>+  controller.waitForPageLoad();
>+  var engineURL = LOCAL_TEST_PAGE.replace(".html", ".xml");
>+  var type = Components.interfaces.nsISearchEngine.DATA_XML;
>+  
>+  // Adding empty string instead of an icon url
>+  search.browserSearchService.addEngine(engineURL, type, "", false);

This still uses the opensearch.xml, which is hardcoded to point to hg.mozilla.org. Please add a method to searchBar for adding an engine using the addEngineWithDetails API call. See https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIBrowserSearchService#addEngineWithDetails%28%29 for details. I have successfully tested this locally. You will also need to set the current search engine.
Attachment #648303 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 34

5 years ago
Created attachment 648669 [details] [diff] [review]
patch v1.2

* we now have a local search engine
* corrected a mistake in search.js module
Attachment #648303 - Attachment is obsolete: true
Attachment #648669 - Flags: review?(dave.hunt)
Comment on attachment 648669 [details] [diff] [review]
patch v1.2

>     var domainRegex = /[^\.]+\.([^\.]+)\..+$/gi;
>     var targetDomainName = targetUrl.host.replace(domainRegex, "$1");
>-    var currentDomainName = currentUrl.host.replace(domainRegex, "$1");
>+    var currentDomainName = currentUrl.hostname.replace(domainRegex, "$1");

Could you explain the reason for this change, and why it would not apply to targetDomainName also?

>   /**
>+   * Install search engine (API call)
>+   * 
>+   * @param {string} aName
>+   *         The search engine's name. Must be unique. Must not be null.
>+   *
>+   * @param {string} aIconURL
>+   *        Optional: A URL string pointing to the icon to be used to represent
>+   *        the engine.
>+   *
>+   * @param {string} aAlias
>+   *        Optional: A unique shortcut that can be used to retrieve the
>+   *        search engine.
>+   *
>+   * @param {string} aDescription
>+   *        Optional: a description of the search engine.
>+   *
>+   * @param aMethod
>+   *        The HTTP request method used when submitting a search query.
>+   *        Must be a case insensitive value of either "get" or "post".
>+   *
>+   * @param {string} aURL
>+   *        The URL to which search queries should be sent.
>+   *        Must not be null.
>+   * @param {boolean} selected
>+   *        If true select the newly installed search engine
>+   *        Must be set to either 'true' or 'false'
>+   */

I don't think we need to map all the arguments. I would omit the optional arguments, and may also be tempted to make the request method optional and default to "get" to simplify the method call for our tests.

>+  searchBar.installEngine(ENGINE.name, null, null, null, 
>+                          "GET", ENGINE.url, true);

I think we can just use the engine name and URL constant directly here. There's no need to create an ENGINE object.
Attachment #648669 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 36

5 years ago
Created attachment 649246 [details] [diff] [review]
patch v1.3

* Dave - to answer your question: whomever added that check in search.js assumed that all time the host == hostname. It can happen but not always. host may also contain ports or other data, not just the name, for example you can have 

   http://localhost as hostname
   http://localhost:43336 as host 

The other item checked can be both host or hostname

* other change requests are addressed 
* refactor installEngine method accordingly
Attachment #648669 - Attachment is obsolete: true
Attachment #649246 - Flags: review?(dave.hunt)
(Assignee)

Updated

5 years ago
Attachment #649246 - Flags: review?(hskupin)
Comment on attachment 649246 [details] [diff] [review]
patch v1.3

>     var targetDomainName = targetUrl.host.replace(domainRegex, "$1");
>-    var currentDomainName = currentUrl.host.replace(domainRegex, "$1");
>+    var currentDomainName = currentUrl.hostname.replace(domainRegex, "$1");

I agree with Dave. Lets be consistent here.

>+   * Install search engine (API call)
>+   */
>+  installEngine: function searchBar_installEngine(aName, aURL, aSpec) {

I wish we wouldn't mix up back-end and ui code in the same class. But right now there is nothing else we can do until the big refactoring. So it's fine.

>+    if (selected)
>+      aEngine = this._bss.getEngineByName(aName);
>+      this._bss.currentEngine = aEngine;    

Missing brackets.

>+function testClickAndSearch() {
>   searchBar.focus({type: "click"});
>   searchBar.search({text: "Firefox", action: "returnKey"});
> }
> 
> /**
>  * Use the keyboard shortcut to focus the search bar and start a search
>  */
>-var testShortcutAndSearch = function()
>-{
>+function testShortcutAndSearch() {
>   searchBar.focus({type: "shortcut"});
>   searchBar.search({text: "Mozilla", action: "goButton"});
> }

Right now we have two test functions in this module. Please make sure that we separate them into two modules. This can be a follow-up bug.

>-/**
>- * Map test functions to litmus tests
>- */
>-// testClickAndSearch.meta = {litmusids : [8241]};
>-// testShortcutAndSearch.meta = {litmusids : [8242]};

Please leave this in for existent tests. It would at least help us to find appropriate even outdated litmus tests.
Attachment #649246 - Flags: review?(hskupin)
Attachment #649246 - Flags: review?(dave.hunt)
Attachment #649246 - Flags: review-
(Assignee)

Comment 38

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #37)
> Comment on attachment 649246 [details] [diff] [review]
> patch v1.3
> 
> >     var targetDomainName = targetUrl.host.replace(domainRegex, "$1");
> >-    var currentDomainName = currentUrl.host.replace(domainRegex, "$1");
> >+    var currentDomainName = currentUrl.hostname.replace(domainRegex, "$1");
> 
> I agree with Dave. Lets be consistent here.

Sorry I know I've said we can do this but looking further targetUrl is a URI which does not have a hostname. In this case, URI.host behaves exactly like location.hostname (you can test this easy in the web console). More, we cannot cannot change currentUrl.hostname back to currentUrl.host because of the reason explained in comment 36. So I'm afraid we'll have to live with this
(Assignee)

Comment 39

5 years ago
Created attachment 649638 [details] [diff] [review]
patch v1.4

* changes addressed except replacing targetUrl.host with targetUrl.hostname which as previously said, does not apply
Attachment #649246 - Attachment is obsolete: true
Attachment #649638 - Flags: review?(hskupin)
(Assignee)

Updated

5 years ago
Attachment #649638 - Flags: review?(dave.hunt)
(Assignee)

Updated

5 years ago
Blocks: 780892
Comment on attachment 649638 [details] [diff] [review]
patch v1.4

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #38)
> (In reply to Henrik Skupin (:whimboo) from comment #37)
> > Comment on attachment 649246 [details] [diff] [review]
> > patch v1.3
> > >     var targetUrl = this._bss.currentEngine.getSubmission(searchTerm, null).uri;
> > 
> > >     var targetDomainName = targetUrl.host.replace(domainRegex, "$1");
> > >-    var currentDomainName = currentUrl.host.replace(domainRegex, "$1");
> > >+    var currentDomainName = currentUrl.hostname.replace(domainRegex, "$1");
> > 
> > I agree with Dave. Lets be consistent here.
> 
> Sorry I know I've said we can do this but looking further targetUrl is a URI
> which does not have a hostname. In this case, URI.host behaves exactly like
> location.hostname (you can test this easy in the web console). More, we
> cannot cannot change currentUrl.hostname back to currentUrl.host because of
> the reason explained in comment 36. So I'm afraid we'll have to live with
> this.

No, we do not want to live with this. First the variable is called Url which would also be wrong, but as said we should be consistent. So please check how to get the url from a nsIURI instance and use it so we can use the hostname.

> 
>     this._controller.assert(function () {
>       return currentDomainName === targetDomainName;
>     }, "Current domain name matches target domain name - got '" +
>       currentDomainName + "', expected '" + targetDomainName + "'");
> 
>     // Check if search term is listed in URL
>     this._controller.assert(function () {

At the same time please replace those this._controller.assert() calls so we make use of the assertion module.
Attachment #649638 - Flags: review?(hskupin)
Attachment #649638 - Flags: review?(dave.hunt)
Attachment #649638 - Flags: review-
(Assignee)

Comment 41

5 years ago
> 
> No, we do not want to live with this. First the variable is called Url which
> would also be wrong, but as said we should be consistent. So please check
> how to get the url from a nsIURI instance and use it so we can use the
> hostname.

Variable called Url - that is indeed wrong 
Getting the Url is possible, here is the code snippet 

var targetUri = this._bss.currentEngine.getSubmission(searchTerm, null).uri;
var targetUrl = targetUri.QueryInterface(Components.interfaces.nsIURL).spec;

On the other hand, "hostname" is a property from nsIDOMLocation
It is not possible to reach nsIDOMLocation from nsIURI, therefore targetUrl can never 
have "hostname". 

If we still want to be consistent, I can just convert currentUrl to a uri, and then access "host" property for both objects instead of "hostname". Please let me know if this meets your expectations so I can upload a patch
Please read my comment 40 closely. We do not want to use nsIURI for our checks but the url, so that hostname can be used.
(Assignee)

Comment 43

5 years ago
Created attachment 650478 [details] [diff] [review]
patch v1.5

Adding this patch based on the clarifications on iRC on both sides
Attachment #649638 - Attachment is obsolete: true
Attachment #650478 - Flags: review?(hskupin)
Comment on attachment 650478 [details] [diff] [review]
patch v1.5

>-    this._controller.assert(function () {
>-      return currentDomainName === targetDomainName;
>-    }, "Current domain name matches target domain name - got '" +
>-      currentDomainName + "', expected '" + targetDomainName + "'");
>+    assert.equal(currentDomainName, targetDomainName, 
>+                 "Current domain name matches target domain name");
[..]
>+    assert.ok(currentUrl.href.toLowerCase().indexOf(searchTerm.toLowerCase()) != -1, 
>+              "Current URL contains the search term '" + 
>+              searchTerm.toLowerCase() + "'");

I'm fairly sure that both should be expect.* calls. Tests should still continue and not be aborted.

Otherwise looks fine now.
Attachment #650478 - Flags: review?(hskupin) → review-
(Assignee)

Comment 45

5 years ago
Created attachment 650872 [details] [diff] [review]
patch v1.6 [checked-in]

fixed to use expect calls
Attachment #650478 - Attachment is obsolete: true
Attachment #650872 - Flags: review?(hskupin)
Comment on attachment 650872 [details] [diff] [review]
patch v1.6 [checked-in]

Looks good. Lets see how it works with todays testrun.
Attachment #650872 - Flags: review?(hskupin) → review+
Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/1ce47d3e8032
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox17: disabled → fixed
Resolution: --- → FIXED
(Assignee)

Comment 48

5 years ago
Created attachment 651340 [details] [diff] [review]
update manifest v1.0 [checked-in]

* This patch updates the manifest file correctly. Sorry I missed it in the previous versions
Attachment #651340 - Flags: review?(hskupin)
(Assignee)

Updated

5 years ago
Attachment #651340 - Flags: review?(dave.hunt)
Comment on attachment 651340 [details] [diff] [review]
update manifest v1.0 [checked-in]

Please come up with a follow-up patch we can use for backporting.
Attachment #651340 - Flags: review?(hskupin)
Attachment #651340 - Flags: review?(dave.hunt)
Attachment #651340 - Flags: review+
Follow-up landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/3aa79883fb2c (default)
(Assignee)

Comment 51

5 years ago
Created attachment 651369 [details] [diff] [review]
backport for aurora, beta, release

* patch for backporting which also includes the updated manifest file
Attachment #651369 - Flags: review?(hskupin)
(Assignee)

Updated

5 years ago
Attachment #651369 - Flags: review?(dave.hunt)
Attachment #650872 - Attachment description: patch v1.6 → patch v1.6 [checked-in]
Attachment #651340 - Attachment description: update manifest v1.0 → update manifest v1.0 [checked-in]
status-firefox-esr10: unaffected → affected
Comment on attachment 651369 [details] [diff] [review]
backport for aurora, beta, release

r+ only because the code is doing what I shall do. Otherwise I would have given a r- because there are still a dozen of whitespace failures in the patch. Sadly those are too in the version for default which already has been landed. To keep all versions identical I will push the patch with the same whitespace mistakes to the other branches.

We have called out issues like those more than a dozen of times. I want to stop on those. So please get those really fixed on your side.
Attachment #651369 - Flags: review?(hskupin)
Attachment #651369 - Flags: review?(dave.hunt)
Attachment #651369 - Flags: review+
Comment on attachment 651369 [details] [diff] [review]
backport for aurora, beta, release

This patch does not apply to the esr branch. Have you tested it?
Attachment #651369 - Attachment description: backport for aurora, beta, release, esr → backport for aurora, beta, release
http://hg.mozilla.org/qa/mozmill-tests/rev/f320a0d03162 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/692010c8f43d (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/dd7f7c2329d3 (release)

Once it has been landed on esr10 we have to update the litmus tests.
Flags: in-litmus+ → in-litmus?(vlad.mozbugs)
status-firefox13: disabled → ---
status-firefox14: disabled → fixed
status-firefox15: disabled → fixed
status-firefox16: disabled → fixed
(Assignee)

Comment 55

5 years ago
Created attachment 651643 [details] [diff] [review]
backport for esr

* This should apply cleanly now on esr
Attachment #651643 - Flags: review?(hskupin)
(Assignee)

Updated

5 years ago
Attachment #651643 - Flags: review?(dave.hunt)
Attachment #651643 - Flags: review?(hskupin)
Attachment #651643 - Flags: review?(dave.hunt)
Attachment #651643 - Flags: review+
http://hg.mozilla.org/qa/mozmill-tests/rev/56cb661b2b4d
status-firefox-esr10: affected → fixed
(Assignee)

Comment 57

5 years ago
Some majority of the litmus tests were not updated in the first place when this test was skipped, so will remain the same in conclusion. 

There were some updates made for: 
https://litmus.mozilla.org/show_test.cgi?id=15706
https://litmus.mozilla.org/show_test.cgi?id=40804
https://litmus.mozilla.org/show_test.cgi?id=64053
to point to the mozmill subgroup

With this, in-litmus+
Flags: in-litmus?(vlad.mozbugs) → in-litmus+
Priority: -- → P1
This only enables the test for the keyboard shortcut but not for the mouse focus one. Litmus updates aren't done yet.
Flags: in-litmus+ → in-litmus?(vlad.mozbugs)
(Assignee)

Comment 59

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #58)
> This only enables the test for the keyboard shortcut but not for the mouse
> focus one. Litmus updates aren't done yet.

The mouse focus one is from BTS, the shortcut one is from FFT. They were not updated correctly in the first place, that's why its kinda messy. 

When I checked, the mouse focus one were ok. Did you check and got other results? 
As I said in comment 57, some were not updated when the test was skipped so now remain the same. 

Still, if the mouse focus test appears not up to date to you, please paste me a link.
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] → [mozmill-test-failure][mozmill-test-skipped][qa-]
Vlad, you should always post all links to litmus tests we would have to update, not only the ones you have updated.
(Assignee)

Comment 61

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #60)
> Vlad, you should always post all links to litmus tests we would have to
> update, not only the ones you have updated.

Alright then - are any of these policies documented somewhere? It seems I miss them. If so, mind directing me to the appropriate location of those so that in the future these matter would not block us.
Why policies? I don't see a reason for one. If we fix a failure across branches I also put all the changeset links in the bug just to let others know and they don't have to query. For Dave and me it will simply help a lot to not have to cross-check ourselves what has not been changed.
Whiteboard: [mozmill-test-failure][mozmill-test-skipped][qa-] → [mozmill-test-failure][qa-]
(Assignee)

Comment 63

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #62)
> Why policies? I don't see a reason for one. If we fix a failure across
> branches I also put all the changeset links in the bug just to let others
> know and they don't have to query. For Dave and me it will simply help a lot
> to not have to cross-check ourselves what has not been changed.

It would be good to have our workflow better documented somewhere so that I don't have to guess all the time which would be the good practice, that's what I meant.
Vlad, the Litmus request is still open. Please get the tests updated.
You need to log in before you can comment on or make changes to this bug.