596782 - four Windows tests fail on Linux w/recent Firefox 4 builds

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

On Linux, with recent Firefox 4 builds (the latest nightly as well as 4.0b6), four Windows API tests fail.  The tests don't fail on Mac OS X, so this may be Linux only (haven't tested Windows yet).

Felipe, can you take a look at this?

Here's the console output:

error: fail: Non-browser windows aren't handled by this module (({get tabs () {var tabs = new TabModule(element);delete this.tabs;this.__defineGetter__("tabs", (function () tabs));return tabs;}, get title () element.document.title, close:(function (callback) {if (callback) {windowMap.addCloseCallback(element, callback);}element.close();})}) != null)
info: Traceback (most recent call last):
  File "resource://jetpack-core-jetpack-core-lib/timer.js", line 60, in notify
    this._callback.apply(null, this._params);
  File "resource://jetpack-core-jetpack-core-tests/test-windows.js", line 227, in nextStep
    testSteps.shift()();
  File "resource://jetpack-core-jetpack-core-tests/test-windows.js", line 194, in anonymous
    test.assertEqual(windows.activeWindow, null, "Non-browser windows aren't handled by this module");
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 227, in assertEqual
    this.fail(message);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 145, in fail
    console.trace();
error: fail: Correct active window - 2 ("Minefield" != "window 2 - Minefield")
info: Traceback (most recent call last):
  File "resource://jetpack-core-jetpack-core-lib/timer.js", line 60, in notify
    this._callback.apply(null, this._params);
  File "resource://jetpack-core-jetpack-core-tests/test-windows.js", line 227, in nextStep
    testSteps.shift()();
  File "resource://jetpack-core-jetpack-core-tests/test-windows.js", line 198, in anonymous
    test.assertEqual(windows.activeWindow.title, window2.title, "Correct active window - 2");
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 227, in assertEqual
    this.fail(message);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 145, in fail
    console.trace();
error: fail: Correct active window - 3 ("Minefield" != "window 3 - Minefield")
info: Traceback (most recent call last):
  File "resource://jetpack-core-jetpack-core-lib/timer.js", line 60, in notify
    this._callback.apply(null, this._params);
  File "resource://jetpack-core-jetpack-core-tests/test-windows.js", line 227, in nextStep
    testSteps.shift()();
  File "resource://jetpack-core-jetpack-core-tests/test-windows.js", line 202, in anonymous
    test.assertEqual(windows.activeWindow.title, window3.title, "Correct active window - 3");
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 227, in assertEqual
    this.fail(message);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 145, in fail
    console.trace();
error: fail: Non-browser windows aren't handled by this module ("Minefield" != "window 3 - Minefield")
info: Traceback (most recent call last):
  File "resource://jetpack-core-jetpack-core-lib/timer.js", line 60, in notify
    this._callback.apply(null, this._params);
  File "resource://jetpack-core-jetpack-core-tests/test-windows.js", line 227, in nextStep
    testSteps.shift()();
  File "resource://jetpack-core-jetpack-core-tests/test-windows.js", line 206, in anonymous
    test.assertEqual(windows.activeWindow.title, window3.title, "Non-browser windows aren't handled by this module");
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 227, in assertEqual
    this.fail(message);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 145, in fail
    console.trace();

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Yes, looking.

Does it consistently fail? I recall that Linux had worse timing for setting active windows (at least on my VM), and the tests would fail sometimes, but not always.
But if it always happen maybe something broke it on trunk recently.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Hmm, I thought it was failing consistently, but once I isolated the tests by only runnings tests for that one module, it became inconsistent, and when it happens, there is only a single test failure (rather than four).  Nevertheless, it still happens frequently.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Ok yeah, this points out to the .focus() response time being incosistent, and it also depends on the system state (trying to interact with other windows at the same time might make the test fail).

I saw on some other similar tests the approach "retry the condition X times before considering it a failure", which also helps on not having to have a very big setTimeout value by default. I'm thinking this would be a good idea to do here.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Felipe: can you put together a patch using that approach?

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: Make windows test more reliable

First I tried to use "focus" or "activate" listeners directly, but they weren't working 100% on Linux, so I had to grab the waitForFocus code from the mochitest harness to make it more reliable.

It looks like it's much more stable now. Can you test the patch and let me know if it fixes the problem? If it doesn't feel free to drop the review request.

Also included a bigger timeout for test-widget which should help here and bug 597202. If you prefer that I include that in a separate patch just let me know and I'll remove that modification from this one.

Also included an extra check on testOnOpenOnCloseListeners to make it easier to detect when the test fails due to open windows left by other tests.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 476074 [details] [diff] [review]
Make windows test more reliable

>+        childTargetWindow.removeEventListener("focus", arguments.callee, true);

Nit: we're slowly converting our code to ES5 strict, so arguments.callee will eventually become unavailable, and it'd be better to avoid using it.


> exports.testOnOpenOnCloseListeners = function(test) {
>   test.waitUntilDone();
>   let windows = require("windows").browserWindows;
> 
>+  test.assertEqual(windows.length, 1, "Only one window open");

I wish we could do this check after every test that might have opened windows. :-/


Otherwise, this looks great.  I do still see an intermittent timeout failure in testActiveWindow:

(jsdk)myk@myk:~/Projects/jsdk/packages/jetpack-core$ cfx test -F windows -b ~/Applications/firefox-trunk/firefox
Using binary at '/home/myk/Applications/firefox-trunk/firefox'.
Using profile at '/tmp/tmpm8tnsG.mozrunner'.

(firefox-bin:3801): GLib-WARNING **: g_set_prgname() called multiple times
........................error: TEST FAILED: test-windows.testActiveWindow (timed out)

24 of 25 tests passed.
FAIL
Total time: 12.230374 seconds
Program terminated unsuccessfully.


Nevertheless, this patch is still a big improvement, so I'm ok with landing it and then working on that intermittent failure in a separate bug.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

http://hg.mozilla.org/labs/jetpack-sdk/rev/c57cf845f179

I named the anonymous listeners to remove arguments.callee usage


>>+  test.assertEqual(windows.length, 1, "Only one window open");
>
> I wish we could do this check after every test that might have opened windows.
> :-/

That is a good idea, perhaps we could add something like that on the test harness

Comment 8

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".