Last Comment Bug 761984 - Failure in testFocusAndSearch | Disconnect Error: Application unexpectedly closed
: Failure in testFocusAndSearch | Disconnect Error: Application unexpectedly cl...
Status: RESOLVED FIXED
[mozmill-test-failure][qa-]
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: P1 critical (vote)
: ---
Assigned To: Maniac Vlad Florin (:vladmaniac)
:
:
Mentors:
http://mozmill-ci.blargon7.com/#/func...
Depends on:
Blocks: 780892
  Show dependency treegraph
 
Reported: 2012-06-06 04:38 PDT by Remus Pop (:RemusPop)
Modified: 2012-09-13 00:47 PDT (History)
7 users (show)
hskupin: in‑litmus? (vlad.mozbugs)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed


Attachments
skip test v1 (default, aurora, beta) [checked-in] (917 bytes, patch)
2012-06-07 00:30 PDT, Remus Pop (:RemusPop)
hskupin: review+
Details | Diff | Splinter Review
re enable test patch 1.0 (908 bytes, patch)
2012-07-20 05:50 PDT, Maniac Vlad Florin (:vladmaniac)
dave.hunt: review-
Details | Diff | Splinter Review
patch v1.0 (3.37 KB, patch)
2012-08-01 08:21 PDT, Maniac Vlad Florin (:vladmaniac)
alex.lakatos.qa: review-
Details | Diff | Splinter Review
patch v1.1 (4.16 KB, patch)
2012-08-02 04:00 PDT, Maniac Vlad Florin (:vladmaniac)
dave.hunt: review-
Details | Diff | Splinter Review
patch v1.2 (5.60 KB, patch)
2012-08-03 05:10 PDT, Maniac Vlad Florin (:vladmaniac)
dave.hunt: review-
Details | Diff | Splinter Review
patch v1.3 (5.05 KB, patch)
2012-08-06 06:02 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v1.4 (5.09 KB, patch)
2012-08-07 07:44 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v1.5 (6.03 KB, patch)
2012-08-09 02:34 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v1.6 [checked-in] (6.56 KB, patch)
2012-08-10 06:54 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review+
Details | Diff | Splinter Review
update manifest v1.0 [checked-in] (832 bytes, patch)
2012-08-13 05:58 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review+
Details | Diff | Splinter Review
backport for aurora, beta, release (7.13 KB, patch)
2012-08-13 07:01 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review+
Details | Diff | Splinter Review
backport for esr (6.28 KB, patch)
2012-08-13 23:03 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review+
Details | Diff | Splinter Review

Description Remus Pop (:RemusPop) 2012-06-06 04:38:36 PDT
This seems to be an intermittent issue appearing on Nightly and Aurora. Windows and Linux are affected for now.
Comment 1 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-06-06 14:08:04 PDT
It's more and more happening now. Please skip this test for now asap.
Comment 2 Remus Pop (:RemusPop) 2012-06-07 00:30:50 PDT
Created attachment 630876 [details] [diff] [review]
skip test v1 (default, aurora, beta) [checked-in]

Skips the test.
Comment 3 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-06-07 06:58:18 PDT
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.
Comment 4 Remus Pop (:RemusPop) 2012-06-07 07:03:36 PDT
Disabled in Litmus: https://litmus.mozilla.org/show_test.cgi?id=15706
Comment 5 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-06-07 07:08:21 PDT
Remus, are you able to reproduce this problem on your local boxes on any platform? Would you mind to run some tests?
Comment 6 Remus Pop (:RemusPop) 2012-06-07 07:13:56 PDT
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.
Comment 7 Remus Pop (:RemusPop) 2012-06-07 07:36:29 PDT
This is the report on 11.04:
http://mozmill-crowd.blargon7.com/#/functional/report/87961186c2b807b7747aa7e6d401c3a3
Comment 8 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-06-07 07:54:20 PDT
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.
Comment 9 Remus Pop (:RemusPop) 2012-06-12 01:08:37 PDT
No luck in getting a disconnect error.
Comment 10 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-06-12 23:07:17 PDT
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.
Comment 11 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-06-20 02:12:43 PDT
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.
Comment 12 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-06-20 02:38:33 PDT
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.
Comment 13 Remus Pop (:RemusPop) 2012-06-20 03:00:51 PDT
Disabled Litmus id 64053.
Comment 14 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-07-05 00:37:47 PDT
We cannot disable this one Litmus test but we have to remove the Mozmill group from all of them across branches.
Comment 15 Maniac Vlad Florin (:vladmaniac) 2012-07-19 04:11:18 PDT
(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)
Comment 16 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-07-19 21:33:01 PDT
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.
Comment 17 Maniac Vlad Florin (:vladmaniac) 2012-07-20 04:57:36 PDT
(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
Comment 18 Maniac Vlad Florin (:vladmaniac) 2012-07-20 05:49:38 PDT
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
Comment 19 Maniac Vlad Florin (:vladmaniac) 2012-07-20 05:50:16 PDT
Created attachment 644272 [details] [diff] [review]
re enable test patch 1.0

Re enables the test
Comment 20 Maniac Vlad Florin (:vladmaniac) 2012-07-20 05:50:38 PDT
Comment on attachment 644272 [details] [diff] [review]
re enable test patch 1.0

adding Dave
Comment 21 Dave Hunt (:davehunt) 2012-07-20 07:26:44 PDT
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)
Comment 22 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-07-22 13:01:47 PDT
Relanded skip patch because it's failing across all platforms:

http://hg.mozilla.org/qa/mozmill-tests/rev/31f2e3267ae6
Comment 23 Dave Hunt (:davehunt) 2012-07-23 04:43:26 PDT
Thanks Henrik. What's the next action for this if it can't be replicated locally?
Comment 24 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-07-23 05:20:12 PDT
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.
Comment 25 Maniac Vlad Florin (:vladmaniac) 2012-08-01 05:03:14 PDT
(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.
Comment 26 Maniac Vlad Florin (:vladmaniac) 2012-08-01 05:25:44 PDT
The local test page which installs a custom search engine is http://mozqa.com/data/firefox/search/opensearch.html
Comment 27 Dave Hunt (:davehunt) 2012-08-01 06:10:35 PDT
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.
Comment 28 Maniac Vlad Florin (:vladmaniac) 2012-08-01 06:41:25 PDT
(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
Comment 29 Dave Hunt (:davehunt) 2012-08-01 08:21:10 PDT
(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.
Comment 30 Maniac Vlad Florin (:vladmaniac) 2012-08-01 08:21:11 PDT
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
Comment 31 Alex Lakatos[:AlexLakatos] 2012-08-01 08:55:02 PDT
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.
Comment 32 Maniac Vlad Florin (:vladmaniac) 2012-08-02 04:00:01 PDT
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)
Comment 33 Dave Hunt (:davehunt) 2012-08-02 04:56:58 PDT
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.
Comment 34 Maniac Vlad Florin (:vladmaniac) 2012-08-03 05:10:12 PDT
Created attachment 648669 [details] [diff] [review]
patch v1.2

* we now have a local search engine
* corrected a mistake in search.js module
Comment 35 Dave Hunt (:davehunt) 2012-08-05 10:14:19 PDT
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.
Comment 36 Maniac Vlad Florin (:vladmaniac) 2012-08-06 06:02:55 PDT
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
Comment 37 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-07 06:16:56 PDT
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.
Comment 38 Maniac Vlad Florin (:vladmaniac) 2012-08-07 07:39:09 PDT
(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
Comment 39 Maniac Vlad Florin (:vladmaniac) 2012-08-07 07:44:01 PDT
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
Comment 40 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-08 05:39:15 PDT
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.
Comment 41 Maniac Vlad Florin (:vladmaniac) 2012-08-08 07:25:40 PDT
> 
> 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
Comment 42 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-08 13:52:18 PDT
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.
Comment 43 Maniac Vlad Florin (:vladmaniac) 2012-08-09 02:34:59 PDT
Created attachment 650478 [details] [diff] [review]
patch v1.5

Adding this patch based on the clarifications on iRC on both sides
Comment 44 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-10 06:46:13 PDT
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.
Comment 45 Maniac Vlad Florin (:vladmaniac) 2012-08-10 06:54:14 PDT
Created attachment 650872 [details] [diff] [review]
patch v1.6 [checked-in]

fixed to use expect calls
Comment 46 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-13 03:37:06 PDT
Comment on attachment 650872 [details] [diff] [review]
patch v1.6 [checked-in]

Looks good. Lets see how it works with todays testrun.
Comment 47 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-13 03:39:05 PDT
Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/1ce47d3e8032
Comment 48 Maniac Vlad Florin (:vladmaniac) 2012-08-13 05:58:28 PDT
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
Comment 49 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-13 06:12:59 PDT
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.
Comment 50 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-13 06:14:30 PDT
Follow-up landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/3aa79883fb2c (default)
Comment 51 Maniac Vlad Florin (:vladmaniac) 2012-08-13 07:01:07 PDT
Created attachment 651369 [details] [diff] [review]
backport for aurora, beta, release

* patch for backporting which also includes the updated manifest file
Comment 52 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-13 15:31:05 PDT
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.
Comment 53 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-13 15:35:09 PDT
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?
Comment 54 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-13 15:37:31 PDT
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.
Comment 55 Maniac Vlad Florin (:vladmaniac) 2012-08-13 23:03:16 PDT
Created attachment 651643 [details] [diff] [review]
backport for esr

* This should apply cleanly now on esr
Comment 56 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-14 03:06:56 PDT
http://hg.mozilla.org/qa/mozmill-tests/rev/56cb661b2b4d
Comment 57 Maniac Vlad Florin (:vladmaniac) 2012-08-14 05:37:35 PDT
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+
Comment 58 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-14 06:51:09 PDT
This only enables the test for the keyboard shortcut but not for the mouse focus one. Litmus updates aren't done yet.
Comment 59 Maniac Vlad Florin (:vladmaniac) 2012-08-14 06:54:21 PDT
(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.
Comment 60 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-15 02:24:14 PDT
Vlad, you should always post all links to litmus tests we would have to update, not only the ones you have updated.
Comment 61 Maniac Vlad Florin (:vladmaniac) 2012-08-15 23:12:25 PDT
(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.
Comment 62 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-08-15 23:23:53 PDT
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.
Comment 63 Maniac Vlad Florin (:vladmaniac) 2012-08-15 23:27:02 PDT
(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.
Comment 64 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-09-13 00:47:14 PDT
Vlad, the Litmus request is still open. Please get the tests updated.

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