613553 - test-tab-browser tests fail after Tabs API EventEmitter/e10s-compatibility changes

Status: RESOLVED FIXED

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

Description

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

After the Tabs API EventEmitter/e10s-compatibility changes landed in bug 598981, three test-tab-browser tests started failing on both nightly builds and Firefox 4.0b7:

--------------------------------------------------------------------------------
error: TEST FAILED: test-tab-browser.testActiveTab (failure)
error: fail: activeTab element matches (({}) != ({}))
info: Traceback (most recent call last):
  File "resource://testpkgs-jetpack-core-lib/tab-browser.js", line 138, in null
    require("errors").catchAndLog(function(e) options.onLoad(e))(e);
  File "resource://testpkgs-jetpack-core-lib/errors.js", line 49, in null
    return callback.apply(this, arguments);
  File "resource://testpkgs-jetpack-core-lib/tab-browser.js", line 138, in null
    require("errors").catchAndLog(function(e) options.onLoad(e))(e);
  File "resource://testpkgs-jetpack-core-tests/test-tab-browser.js", line 282, in null
    test.assertEqual(browser.tabContainer.getItemAtIndex(tabIndex), tabBrowser.activeTab, "activeTab element matches");
  File "resource://testpkgs-jetpack-core-lib/unit-test.js", line 195, in assertEqual
    this.fail(message);
  File "resource://testpkgs-jetpack-core-lib/unit-test.js", line 113, in fail
    console.trace();
..error: fail: Correct number of events fired from all windows (0 != 5)
info: Traceback (most recent call last):
  File "resource://testpkgs-jetpack-core-lib/timer.js", line 64, in notifyOnTimeout
    this._callback.apply(null, this._params);
  File "resource://testpkgs-jetpack-core-lib/tab-browser.js", line 572, in null
    require("errors").catchAndLog(function(tab) options.onOpen(tab))(tab);
  File "resource://testpkgs-jetpack-core-lib/errors.js", line 49, in null
    return callback.apply(this, arguments);
  File "resource://testpkgs-jetpack-core-lib/tab-browser.js", line 572, in null
    require("errors").catchAndLog(function(tab) options.onOpen(tab))(tab);
  File "resource://testpkgs-jetpack-core-tests/test-tab-browser.js", line 319, in null
    test.assertEqual(counterTabs, 5, "Correct number of events fired from all windows");
  File "resource://testpkgs-jetpack-core-lib/unit-test.js", line 195, in assertEqual
    this.fail(message);
  File "resource://testpkgs-jetpack-core-lib/unit-test.js", line 113, in fail
    console.trace();
..error: fail: Correct number of tabs in all windows ((void 0) != NaN)
info: Traceback (most recent call last):
  File "resource://testpkgs-jetpack-core-lib/timer.js", line 64, in notifyOnTimeout
    this._callback.apply(null, this._params);
  File "resource://testpkgs-jetpack-core-lib/tab-browser.js", line 572, in null
    require("errors").catchAndLog(function(tab) options.onOpen(tab))(tab);
  File "resource://testpkgs-jetpack-core-lib/errors.js", line 49, in null
    return callback.apply(this, arguments);
  File "resource://testpkgs-jetpack-core-lib/tab-browser.js", line 572, in null
    require("errors").catchAndLog(function(tab) options.onOpen(tab))(tab);
  File "resource://testpkgs-jetpack-core-tests/test-tab-browser.js", line 322, in null
    test.assertEqual(tabs.length, 5 + startingTabs, "Correct number of tabs in all windows");
  File "resource://testpkgs-jetpack-core-lib/unit-test.js", line 195, in assertEqual
    this.fail(message);
  File "resource://testpkgs-jetpack-core-lib/unit-test.js", line 113, in fail
    console.trace();
--------------------------------------------------------------------------------

The first test doesn't fail if one runs only test-tab-browser tests rather than all tests.  But the other two fail regardless.

Irakli: can you take a look at this?

Comment 1

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

I also see this error:

error: An exception occurred.
Traceback (most recent call last):
  File "resource://testpkgs-jetpack-core-lib/timer.js", line 64, in notifyOnTimeout
    this._callback.apply(null, this._params);
  File "resource://testpkgs-addon-kit-lib/windows.js", line 109, in 
    this._initWindowTabTracker();
  File "resource://testpkgs-jetpack-core-lib/windows/tabs.js", line 79, in _initWindowTabTracker
    for each (let tabContainer in this._tabContainers) {
  File "resource://testpkgs-jetpack-core-lib/windows/tabs.js", line 71, in 
    Array.slice(this._window.document.getElementsByTagName(TAB_BROWSER))
TypeError: this._window.document is null

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I don't think it's realize blocker though. I mentioned in the the Bug-612676 that tests for selection API are the only consumers for a tab-browser. IMO we should remove that dependency and tab-browser along with it's tests instead of trying to fix this.

Thoughts ?

Comment 3

[Dietrich Ayala (:dietrich)]

This is failing tests 100% of the time afaict, which absolutely makes it a release blocker. If it was failing tests at the time, it shouldn't have been checked in at all.

Regarding the approach, here are the dependencies:

http://mxr.mozilla.org/labs-central/search?string=%28%22tab-browser%22%29&find=%2Fjetpack-sdk%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=labs-central

I'm looking into why it's failing. Need to figure out if it's easier to remove tab-browser, or just fix the test.

Comment 4

[Dietrich Ayala (:dietrich)]

Looks like it's a poorly written test (probably by me). Basically, test.done() was being called before the tests themselves were run. Possibly something in the e10s changes caused this to surface?!

Comment 5

[Dietrich Ayala (:dietrich)]

hm, looks like the tab-browser (jetpack-core) test pulls in tabs (addon-kit) in the testEventsAndLengthStayInModule test.

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: v1

* fix a typo
* fix a couple of places where it was possible for tests to end prematurely, causing leftover windows and tabs to screw up subsequent tests
* remove the addon-kit dependency from the jetpack-core test

Comment 7

[Dietrich Ayala (:dietrich)]

Hrm, on my 19th run I saw a failure:

..............error: TEST FAILED: test-tab-browser.testActiveTab (failure)
error: fail: activeTab element matches (({}) != ({}))

Comment 8

[Dietrich Ayala (:dietrich)]

Looking like the load handler wasn't taking into account about:blank loads. current theory is that by the time the "userland" load listener is called, the browser has finished filling in the bits for the actual page that's being loaded. sometimes.

Comment 9

[Dietrich Ayala (:dietrich)]

And not only that. There's still a prior test that leaves a window open.

Comment 10

[Dietrich Ayala (:dietrich)]

Attachment: v2

* fix about:blank loads better
* focus the window

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 492395 [details] [diff] [review]
v2

canceling, still getting the wrong window sometimes.

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 492395 [details] [diff] [review]
v2

Just ran the test 50x without a fail. I propose to move forward with the fixes we have so far, and file a followup for the intermittent failure.

Comment 13

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

Comment on attachment 492395 [details] [diff] [review]
v2

With this patch, I no longer see failures when running only jetpack-core tests.  But if I run tests for multiple packages, I still see failures, even if I only run tests for tab-browser and one other module in another package, and even if the tab-browser tests run first (and thus should not be affected by changes made by any other tests).

For example, the following commands all result in failures:

  cfx testall
  cfx testpkgs

  # jetpack-core/tab-browser and addon-kit/tabs
  cfx testpkgs -F tab

  # jetpack-core/tab-browser and addon-kit/windows
  cfx testpkgs -F 'tab-browser|windows'


The failures look a little different, however:

error: TEST FAILED: test-tab-browser.testActiveTab (failure)
error: fail: url1 still matches ("data:text/html,bar" != "data:text/html,foo")
info: Traceback (most recent call last):
  File "resource://testpkgs-jetpack-core-lib/tab-browser.js", line 140, in null
    require("errors").catchAndLog(function(e) options.onLoad(e))(e);
  File "resource://testpkgs-jetpack-core-lib/errors.js", line 49, in null
    return callback.apply(this, arguments);
  File "resource://testpkgs-jetpack-core-lib/tab-browser.js", line 140, in null
    require("errors").catchAndLog(function(e) options.onLoad(e))(e);
  File "resource://testpkgs-jetpack-core-tests/test-tab-browser.js", line 312, in null
    test.assertEqual(tabURL(tabBrowser.activeTab), url1, "url1 still matches");
  File "resource://testpkgs-jetpack-core-lib/unit-test.js", line 195, in assertEqual
    this.fail(message);
  File "resource://testpkgs-jetpack-core-lib/unit-test.js", line 113, in fail
    console.trace();
error: fail: activeTab element matches (({}) != ({}))
info: Traceback (most recent call last):
  File "resource://testpkgs-jetpack-core-lib/tab-browser.js", line 140, in null
    require("errors").catchAndLog(function(e) options.onLoad(e))(e);
  File "resource://testpkgs-jetpack-core-lib/errors.js", line 49, in null
    return callback.apply(this, arguments);
  File "resource://testpkgs-jetpack-core-lib/tab-browser.js", line 140, in null
    require("errors").catchAndLog(function(e) options.onLoad(e))(e);
  File "resource://testpkgs-jetpack-core-tests/test-tab-browser.js", line 314, in null
    test.assertEqual(tabAtIndex, tabBrowser.activeTab, "activeTab element matches");
  File "resource://testpkgs-jetpack-core-lib/unit-test.js", line 195, in assertEqual
    this.fail(message);
  File "resource://testpkgs-jetpack-core-lib/unit-test.js", line 113, in fail
    console.trace();

Comment 14

[Dietrich Ayala (:dietrich)]

Looking into removing tab-browser now, since I can't reproduce the test failures anymore, so getting diminishing returns there.

Better mxr search for tab-browser usage: http://bit.ly/ec6X8K

Comment 15

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Created work in progress patch https://github.com/mozilla/addon-sdk/pull/61
that removes dependencies from tab-browser.

Comment 16

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

Attachment: fixes tests

(In reply to comment #15)
> Created work in progress patch https://github.com/mozilla/addon-sdk/pull/61
> that removes dependencies from tab-browser.

I haven't done a thorough review, but at first glance, there are two problems with this patch.  First, a process issue: it makes huge changes at the tail end of a development cycle.  Second, a substantive issue: it exposes an additional and unnecessary selection API in order to simplify the implementation, whereas we should be prioritizing API simplicity.


There are two failing test functions: testActiveTab and testEventsAndLengthStayInModule.

testActiveTab is failing because it opens two tabs, compares the first one to the tab at the first document's index, and then compares the second one to the tab at the first document's index instead of the second document's index.

testEventsAndLengthStayInModule is failing for an unknown reason, but what is known is that it tests both the jetpack-core.tab-browser and the addon-kit.tabs modules, whereas it should only be testing the former, as jetpack-core should not depend on addon-kit, and test-tabs.testTabsIteratorAndLength does exactly the same tests.

Here's a patch that resolves both problems accompanied by pull request 62 <https://github.com/mozilla/addon-sdk/pull/62>.

Comment 17

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

Comment on attachment 493928 [details] [diff] [review]
fixes tests

Erm, the second change is correct, but the first one isn't.  Digging further...

Comment 18

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

Attachment: patch v4: fixes tests

Ok, here's a patch that really does fix the tests.

It turns out that the test harness evaluates all test files before executing any tests, so if you run the addon-kit/windows and jetpack-core/tab-browser tests, test-windows.js gets evaluated before test-tab-browser.js gets run.  And test-windows does require("windows"), which instantiates the windows module, which starts tracking tabs.

But the windows module assumes that all tabs are being opened through the open() method of its tabs getter (or the open method of the tabs module), and it relies on that fact to activate tabs if inBackground isn't set.  Tabs can be opened via other means, however, including by tab-browser and by core Firefox code.  So the windows module can't assume tabs are being opened through the open() method.  This patch makes it not assume that.

It also includes Dietrich's earlier fixes as well as a fix for a regression from the fix for bug 613691.

The pull request remains the same: https://github.com/mozilla/addon-sdk/pull/62.

Comment 19

[Dietrich Ayala (:dietrich)]

Comment on attachment 493946 [details] [diff] [review]
patch v4: fixes tests

good find! r=me, and thanks for pulling in the other fixes.

Comment 20

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

https://github.com/mozilla/addon-sdk/commit/456a9ff79b821b3633f17801ef0562cb88211a01

Comment 21

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Added some additional fixes to the myk's patch.
 https://github.com/mozilla/addon-sdk/pull/65

Comment 22

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

(In reply to comment #21)
> Added some additional fixes to the myk's patch.
>  https://github.com/mozilla/addon-sdk/pull/65

Note: we're going to address these additional fixes in bug 615674.