600877 - unable to focus on browser while running browser-chrome tests, we timeout and fail

Status: RESOLVED DUPLICATE of bug 607018

(Testing :: Mochitest, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

while running browser-chrome tests on maemo gtk the device has focus on the browser-chrome test harness window instead of the browser!  This causes us to fail because the browser-chrome tests depend on focus (lots of clicking, typing) and without focus we fail.

here is the code that launches the browser-chrome harness window upon startup:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#27

here is the code that tries to do the focus (and does so successfully on linux desktop fennec):
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-harness.xul#237

I have does a nsIWindowMediator enumeration of the windows and verified that we are trying to set focus on the correct window.  In addition, I switch the focus call to a EventUtils.synthesizeMouse() which does a mouse click.  I did this on the favicon and it worked as expected on linux desktop, but not on maemo.  

If I touch the screen of my device outside of the browser-chrome harness window, I will get focus on my browser and tests will work as expected.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

steps to reproduce:
1) build fennec (or download from ftp) with --enable-tests
2) install fennec on n900
3) copy over tests.zip file device and unpack
4) cd mochitest directory
5) run python script:
python runtests.py --browser-chrome --appname=<full_path>/fennec --certificate-path=../certs --utility-path=../bin --test-path=mobile --autorun

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: add option for --status-window, otherwise don't display xul controller window (1.0)

in working with :dougt on this, it became obvious that the maemo windowing system doesn't support switching between windows as other operating systems do.  The obvious solution here is to remove the XUL window that is acting as a controller and status for the browser-chrome tests.

This patch does that by default and does --autorun/--close-when-done as well since there is no clear way to start or known when the tests are finished.  If you want to run locally, you can add the --status-window option to the commandline and browser-chrome tests will function as they have in the past.

In general, this is a cut and paste of the code from browser-harness.xul into browser-test.js.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: add option for --status-window, otherwise don't display xul controller window (2.0)

updated patch to work better with windows on desktop Firefox.  There were issues with us assuming we were running in the controller window and trying to close all the other windows/tabs.  This is conditionally run if we show the --status-window.  Also make the testStart stuff more robust for dealing with/out the status-window.

running on try right now.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

this has passed on try server (twice now).  Consider this patch ready for review and if r+ ready for checkin.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 481870 [details] [diff] [review]
add option for --status-window, otherwise don't display xul controller window (2.0)

Haven't been able to do a full review, but here are some quick things I noticed:
- we should do the window state checking regardless of whether or not we have a status window - it's still useful for catching extra windows that have been inadvertently left open by tests
- why do you need to include all of browser-test.js in browser-harness.xul? It seems like the approach should be to move all of the test code into the main browser overlay, and have browser-harness.xul interact with it as little as possible (beyond calling "Tester.start();" and receiving results to display). That should avoid the need for "gStatusWindow", I think?

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: add option for --status-window, otherwise don't display xul controller window (3.0)

updated patch to address comments:
 - check for all windows that leak
 - remove global variable that was added as a state check.  

running on try atm, passes local tests with and without status-window.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

this patch is looking good on try.  A lot of orange on various tests, but nothing new for browser-chrome or other tests for that matter.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 484322 [details] [diff] [review]
add option for --status-window, otherwise don't display xul controller window (3.0)

>diff --git a/testing/mochitest/browser-test-overlay.xul b/testing/mochitest/browser-test-overlay.xul
>--- a/testing/mochitest/browser-test-overlay.xul
>+++ b/testing/mochitest/browser-test-overlay.xul
>@@ -35,10 +35,15 @@
>    - and other provisions required by the LGPL or the GPL. If you do not delete
>    - the provisions above, a recipient may use your version of this file under
>    - the terms of any one of the MPL, the GPL or the LGPL.
>    -
>    - ***** END LICENSE BLOCK ***** -->
> 
> <overlay id="browserTestOverlay"
>          xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+<window>
>+  <script src="chrome://mochikit/content/tests/SimpleTest/MozillaFileLogger.js"/>
>+  <script src="chrome://mochikit/content/tests/SimpleTest/quit.js"/>
>+  <script src="chrome://mochikit/content/chrome-harness.js"/>
>   <script type="application/javascript" src="chrome://mochikit/content/browser-test.js"/>
>+</window>

This worries me a little bit in that we're loading a whole bunch of extra code into browser.xul now, and that makes it a lot easier to step on the toes of browser code or browser test. I don't have a great idea of how to fix it though, short of migrating all this stuff to .jsms, which is a much larger refactoring.

>diff --git a/testing/mochitest/browser-test.js b/testing/mochitest/browser-test.js
>--- a/testing/mochitest/browser-test.js
>+++ b/testing/mochitest/browser-test.js
>@@ -13,24 +13,33 @@ function testOnLoad() {
>+/*
>+ * Code from browser-harness.xul, this builds the list of tests
>+ * and runs each test while collecting results.
>+ *
>+ */
>+
>+    if (Cc === undefined) {
>+      var Cc = Components.classes;
>+      var Ci = Components.interfaces;
>+    }

The indentation on this code looks wrong compared to the code immediately preceding your comment.

>diff --git a/testing/mochitest/server.js b/testing/mochitest/server.js
>--- a/testing/mochitest/server.js
>+++ b/testing/mochitest/server.js
>@@ -547,17 +547,18 @@ function linksToTableRows(links, recursi
>                           A({href: bug_url}, "Bug " + bug_num)));
>       }
>     }
>   }
>   return response;
> }
> 
> function arrayOfTestFiles(linkArray, fileArray, testPattern) {
>-  for (var [link, value] in linkArray) {
>+  for (var link in linkArray) {
>+    var value = linkArray[link];

I know we discussed this on IRC, but it would be good to figure out why this wasn't working for you.

Looks fine to me otherwise. I guess the default is no status window, then? Maybe nobody will care, since you auto-run them, and "make mochitest-browser-chrome" will spit the logs to stdout anyway.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

is there a difference from having the additional <script src=''> calls in the overlay vs the window.open('browser-harness.xul')?  We have the same code, it is just in the overlay instead of the popup window.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

The overlay is applied to the main browser window, so the scripts get loaded into the global browser scope. That means there's potential for conflicts where there wasn't before (when things were loaded in the popup window scope). I'd really like to actually refactor this properly and make it a JSM - I think it might not be too hard.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

I took a first stab at this and found that pushing browser-test.js (from the patch) didn't work (I did a few hacks to get the obvious fixes out of the way).  When we loadSubScript(packed.js), it fails to find this._document.  With the way mochikit works, I would think we need to load the mochikit stuff from the browser context, not a raw javascript context.

With that said, maybe we could refactor some of the chrome-harness.js and original browser-test.js code into a .jsm.  For the other files (mozillafilelogger.js and quit.js), we might be able to do something.  That would require more refactoring because mochikit.jar (used for ipc and test packaging on remote devices) relies on this as well.  

Any advice or direction would be appreciated!

Comment 12

[:Gavin Sharp [email: [e-mail]]]

jmaher: I'm hoping bug 607018 will solve your maemo test issues without the need to take this patch. We should still refactor eventually, but easier to do that when there's less time pressure to get tests working.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

I don't think that patch will solve the problem.  I tried a variety of things on the window open screen and was unsuccessful.  I removed the 'dialog', but never did 'dialog=no'.

Comment 14

[Dave Townsend [:mossop]]

(In reply to comment #13)
> I don't think that patch will solve the problem.  I tried a variety of things
> on the window open screen and was unsuccessful.  I removed the 'dialog', but
> never did 'dialog=no'.

The chrome feature implies dialog so you have to explicitly do dialog=no to change it.

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

oh sweet, this seems to work.  In this case, lets keep this bug as a browsr-chrome overall harness refactor.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 484322 [details] [diff] [review]
add option for --status-window, otherwise don't display xul controller window (3.0)

I think we want to just use a JS module for this when we do it (to avoid the scope pollution issues Ted mentioned). Thankfully that other bug has solved the main problem this was looking to solve.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Going to dupe this over, I filed bug 611548 for a more general cleanup.