831474 - Update revision of Jetpack tests used in Firefox tests once bug 825471 lands

Status: RESOLVED FIXED

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

Description

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Once the pull request in bug 825471 lands, we need to update the jetpack revision used in mozilla-central's tests to get them testing the new stuff, and to green up the tree a bit.

Comment 1

[(no longer active)]

Does this apply to Aurora as well?  Do you expect this to fix the Jetpack perma-oranges on the Birch branch?

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to :Ehsan Akhgari from comment #1)
> Does this apply to Aurora as well?  Do you expect this to fix the Jetpack
> perma-oranges on the Birch branch?

If we could get approval-aurora+, that'd be nice. But this only affects tests, not actual addon code, so it's not really urgent.

The failure on Birch isn't related to private-browsing, but something with the garbage collector:

error: addon-sdk: TEST FAILED: test-disposable.test disposables are GC-able (failure)
error: addon-sdk: fail: GC removed dispose listeners - 0 == 1
info: addon-sdk: Traceback (most recent call last):
  File "resource://71bdea69-0efe-4ff3-ad65-ed54f22894c6-at-jetpack/addon-sdk/lib/sdk/timers.js", line 31, in notify
    callback.apply(null, args);
  File "resource://71bdea69-0efe-4ff3-ad65-ed54f22894c6-at-jetpack/addon-sdk/tests/test-disposable.js", line 121, in exports["test disposables are GC-able"]/<
    assert.equal(disposals, 0, "GC removed dispose listeners");
  File "resource://71bdea69-0efe-4ff3-ad65-ed54f22894c6-at-jetpack/addon-sdk/lib/sdk/test/assert.js", line 116, in equal
    operator: "=="
  File "resource://71bdea69-0efe-4ff3-ad65-ed54f22894c6-at-jetpack/addon-sdk/lib/sdk/test/assert.js", line 80, in fail
    this._log.fail(message);
  File "resource://71bdea69-0efe-4ff3-ad65-ed54f22894c6-at-jetpack/addon-sdk/lib/sdk/deprecated/unit-test.js", line 75, in fail
    this.console.trace();

Odd that this isn't showing up anywhere else, though. Irakli wrote those tests, maybe he knows?

Comment 3

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

Yeah it that's test actually is really tricky it tries to make GC collect listener
and see if listener is still invoked. Although I suspect on that specific setup GC does not actually claims listener and it's still called.

Have no idea how to actually force GC collection though :(

Comment 4

[(no longer active)]

(In reply to comment #2)
> (In reply to :Ehsan Akhgari from comment #1)
> > Does this apply to Aurora as well?  Do you expect this to fix the Jetpack
> > perma-oranges on the Birch branch?
> 
> If we could get approval-aurora+, that'd be nice. But this only affects tests,
> not actual addon code, so it's not really urgent.

If it's test-only, you can land it with a=test-only.  No need to get approval.

> The failure on Birch isn't related to private-browsing, but something with the
> garbage collector:
> 
> error: addon-sdk: TEST FAILED: test-disposable.test disposables are GC-able
> (failure)
> error: addon-sdk: fail: GC removed dispose listeners - 0 == 1
> info: addon-sdk: Traceback (most recent call last):
>   File
> "resource://71bdea69-0efe-4ff3-ad65-ed54f22894c6-at-jetpack/addon-sdk/lib/sdk/timers.js",
> line 31, in notify
>     callback.apply(null, args);
>   File
> "resource://71bdea69-0efe-4ff3-ad65-ed54f22894c6-at-jetpack/addon-sdk/tests/test-disposable.js",
> line 121, in exports["test disposables are GC-able"]/<
>     assert.equal(disposals, 0, "GC removed dispose listeners");
>   File
> "resource://71bdea69-0efe-4ff3-ad65-ed54f22894c6-at-jetpack/addon-sdk/lib/sdk/test/assert.js",
> line 116, in equal
>     operator: "=="
>   File
> "resource://71bdea69-0efe-4ff3-ad65-ed54f22894c6-at-jetpack/addon-sdk/lib/sdk/test/assert.js",
> line 80, in fail
>     this._log.fail(message);
>   File
> "resource://71bdea69-0efe-4ff3-ad65-ed54f22894c6-at-jetpack/addon-sdk/lib/sdk/deprecated/unit-test.js",
> line 75, in fail
>     this.console.trace();
> 
> Odd that this isn't showing up anywhere else, though. Irakli wrote those tests,
> maybe he knows?

Thanks for the explanation.  Do I need to file another bug for that?

Comment 5

[(no longer active)]

(In reply to comment #3)
> Yeah it that's test actually is really tricky it tries to make GC collect
> listener
> and see if listener is still invoked. Although I suspect on that specific setup
> GC does not actually claims listener and it's still called.
> 
> Have no idea how to actually force GC collection though :(

Is that test orange everywhere or is it just Birch?

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to :Ehsan Akhgari from comment #4)
> Thanks for the explanation.  Do I need to file another bug for that?

I just filed bug 831517 for it, and just pushed a one-line patch for it which seemed to fix it for me, testing locally. So once we get the pwpb stuff landed, and then land this bug on aurora, it should green up Birch on the following m-a->birch merge.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to :Ehsan Akhgari from comment #5)
> (In reply to comment #3)
> > Yeah it that's test actually is really tricky it tries to make GC collect
> > listener
> > and see if listener is still invoked. Although I suspect on that specific setup
> > GC does not actually claims listener and it's still called.
> > 
> > Have no idea how to actually force GC collection though :(
> 
> Is that test orange everywhere or is it just Birch?

I'm only seeing it on Birch at the moment.

Comment 8

[(no longer active)]

(In reply to comment #6)
> (In reply to :Ehsan Akhgari from comment #4)
> > Thanks for the explanation.  Do I need to file another bug for that?
> 
> I just filed bug 831517 for it, and just pushed a one-line patch for it which
> seemed to fix it for me, testing locally. So once we get the pwpb stuff landed,
> and then land this bug on aurora, it should green up Birch on the following
> m-a->birch merge.

Thanks a lot!  :-)

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Landed this: http://hg.mozilla.org/integration/mozilla-inbound/rev/3ee6b0c9bab9
It doesn't include the new pwpb stuff, but it does green up most of our tests, so landing this now. 

Lets leave this open so we can get the pwpb stuff in once it lands.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/3ee6b0c9bab9

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Bug 825471 landed a few weeks ago, but it didn't get marked as fixed until tonight. I landed Jetpack test updates to mozilla-central and mozilla-aurora last week, so I'm calling this fixed.