Closed Bug 613553 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: myk, Assigned: myk)

References

Details

Attachments

(1 file, 3 obsolete files)

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?
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
Assignee: nobody → dietrich
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 ?
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.
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?!
hm, looks like the tab-browser (jetpack-core) test pulls in tabs (addon-kit) in the testEventsAndLengthStayInModule test.
Attached patch v1 (obsolete) — Splinter Review
* 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
Attachment #492356 - Flags: review?(myk)
Hrm, on my 19th run I saw a failure:

..............error: TEST FAILED: test-tab-browser.testActiveTab (failure)
error: fail: activeTab element matches (({}) != ({}))
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.
And not only that. There's still a prior test that leaves a window open.
Attached patch v2 (obsolete) — Splinter Review
* fix about:blank loads better
* focus the window
Attachment #492356 - Attachment is obsolete: true
Attachment #492395 - Flags: review?(myk)
Attachment #492356 - Flags: review?(myk)
Comment on attachment 492395 [details] [diff] [review]
v2

canceling, still getting the wrong window sometimes.
Attachment #492395 - Flags: review?(myk)
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.
Attachment #492395 - Flags: review?(myk)
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();
Attachment #492395 - Flags: review?(myk) → review-
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
Assignee: dietrich → rFobic
Created work in progress patch https://github.com/mozilla/addon-sdk/pull/61
that removes dependencies from tab-browser.
Attached patch fixes tests (obsolete) — Splinter Review
(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>.
Attachment #492395 - Attachment is obsolete: true
Attachment #493928 - Flags: review?(dietrich)
Comment on attachment 493928 [details] [diff] [review]
fixes tests

Erm, the second change is correct, but the first one isn't.  Digging further...
Attachment #493928 - Attachment is obsolete: true
Attachment #493928 - Flags: review?(dietrich)
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.
Assignee: rFobic → myk
Status: NEW → ASSIGNED
Attachment #493946 - Flags: review?(dietrich)
Comment on attachment 493946 [details] [diff] [review]
patch v4: fixes tests

good find! r=me, and thanks for pulling in the other fixes.
Attachment #493946 - Flags: review?(dietrich) → review+
https://github.com/mozilla/addon-sdk/commit/456a9ff79b821b3633f17801ef0562cb88211a01
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Added some additional fixes to the myk's patch.
 https://github.com/mozilla/addon-sdk/pull/65
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: FIXED → ---
(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.
Status: UNCONFIRMED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: 0.10 → 1.0b1
You need to log in before you can comment on or make changes to this bug.