613691 - "cfx test" after "cfx init" fails a test

Status: RESOLVED FIXED

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

Description

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

After running "cfx init" to create an addon, running "cfx test" fails a test:

(addon-sdk)myk@myk:~/Projects/addon-sdk/packages/foo$ cfx test
Using binary at '/home/myk/bin/firefox'.
Using profile at '/tmp/tmp9qNqAh.mozrunner'.

(firefox-bin:29341): GLib-WARNING **: g_set_prgname() called multiple times
Running tests on Firefox 4.0b8pre/Gecko 2.0b8pre ({ec8030f7-c20a-464f-9b0e-13a3a9e97384}) under Linux/x86-gcc3.
info: Jetpack sample addon running.
...error: TEST FAILED: test-main.test_open_tab (exception)
error: An exception occurred.
Traceback (most recent call last):
  File "resource://foo-jetpack-core-lib/timer.js", line 64, in notifyOnTimeout
    this._callback.apply(null, this._params);
  File "resource://foo-jetpack-core-lib/unit-test.js", line 223, in 
    timer.setTimeout(function() { onDone(self); }, 0);
  File "resource://foo-jetpack-core-lib/unit-test.js", line 248, in runNextTest
    self.start({test: test, onDone: runNextTest});
  File "resource://foo-jetpack-core-lib/unit-test.js", line 266, in start
    this.test.testFunction(this);
  File "resource://foo-jetpack-core-lib/unit-test-finder.js", line 57, in runTest
    test(runner);
  File "resource://foo-foo-tests/test-main.js", line 26, in 
    onOpen : function(tab){
TypeError: tabs.open is not a function

3 of 4 tests passed.
OK

Comment 1

[Julián Ceballos]

Ok, the fail was by:
const tabs = require("tabs"); 
that i written, i've changed to:
const tabs = require("tabs").tabs;

Now i receive this error:
(addon-sdk)Root:demo jceb$ cfx test
Using binary at '/Applications/Firefox.app/Contents/MacOS/firefox-bin'.
Using profile at '/var/folders/U0/U06LUiVJHCetNRivK2ZWt++++TI/-Tmp-/tmpFjSANS.mozrunner'.
Running tests on Firefox 4.0b7/Gecko 2.0b7 ({ec8030f7-c20a-464f-9b0e-13a3a9e97384}) under Darwin/x86_64-gcc3.
info: Jetpack sample addon running.
...error: TEST FAILED: test-main.test_open_tab (exception)
error: An exception occurred.
Traceback (most recent call last):
  File "resource://demo-jetpack-core-lib/timer.js", line 64, in notifyOnTimeout
    this._callback.apply(null, this._params);
  File "resource://demo-jetpack-core-lib/unit-test.js", line 223, in 
    timer.setTimeout(function() { onDone(self); }, 0);
  File "resource://demo-jetpack-core-lib/unit-test.js", line 248, in runNextTest
    self.start({test: test, onDone: runNextTest});
  File "resource://demo-jetpack-core-lib/unit-test.js", line 266, in start
    this.test.testFunction(this);
  File "resource://demo-jetpack-core-lib/unit-test-finder.js", line 57, in runTest
    test(runner);
  File "resource://demo-demo-tests/test-main.js", line 26, in 
    onReady: function(tab){
  File "resource://demo-addon-kit-lib/tabs.js", line 57, in open
    return browserWindows.activeWindow.tabs.open(options);
TypeError: browserWindows.activeWindow is null

3 of 4 tests passed.
FAIL
Total time: 3.275259 seconds
Program terminated unsuccessfully.

Do you know why?, my code in the unittest is:

exports.test_open_tab = function(test){
    const tabs = require("tabs").tabs;
    tabs.open({
        url: "http://www.mozilla.org",
        onReady: function(tab){
            test.assert(tab.location=="http://www.mozilla.org/");
            test.done();
        }
    });
    test.waitUntilDone(20000);
}

Comment 2

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

Hmm, I'm not sure why that is happening.  Perhaps the browser window isn't the active window when that code runs?

Comment 3

[Julián Ceballos]

Yes it Is, have you tried the test?

Comment 4

[Alexandre Poirot [:ochameau]]

There is various problems with cfx init, first, widgets and tabs API have changed. Then, there is this activeWindow being null due to the "Running tests ..." window, which is the activeWindow!

I propose two solutions:
1/ https://github.com/mozilla/addon-sdk/pull/57
   Only check browser window in browserWindows.activeWindow

2/ https://github.com/mozilla/addon-sdk/pull/58
   Or let browserWindows.activeWindow as before 
   and add browserWindows.activeBrowserWindow

The first proposal doesn't complexify the API but may not be clear with


Both pull request contains the same commit that fix widgets/tabs api change.

It's my first real commit/pull request and bugzilla try, so feel free to advise me best pratices.

Comment 5

[Alexandre Poirot [:ochameau]]

There is various problems with cfx init. First, widgets and tabs API have changed. Then, there is this activeWindow being null due to the "Running tests ..." window, which is the activeWindow!

I propose two solutions:
1/ https://github.com/mozilla/addon-sdk/pull/57
   Only check browser window in browserWindows.activeWindow

2/ https://github.com/mozilla/addon-sdk/pull/58
   Or let browserWindows.activeWindow as before 
   and add browserWindows.activeBrowserWindow (We may find a better name)

The first proposal doesn't complexify the API but may be confusing when we return a window that is not really active (when another window type is active).

Both pull request contains the same commit that fix widgets/tabs api change.

It's my first real commit/pull request and bugzilla try, 
so feel free to advise me best pratices.

Comment 6

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

(In reply to comment #5)
> There is various problems with cfx init. First, widgets and tabs API have
> changed. Then, there is this activeWindow being null due to the "Running tests
> ..." window, which is the activeWindow!
> 
> I propose two solutions:
> 1/ https://github.com/mozilla/addon-sdk/pull/57
>    Only check browser window in browserWindows.activeWindow
> 
> 2/ https://github.com/mozilla/addon-sdk/pull/58
>    Or let browserWindows.activeWindow as before 
>    and add browserWindows.activeBrowserWindow (We may find a better name)
> 
> The first proposal doesn't complexify the API but may be confusing when we
> return a window that is not really active (when another window type is active).
> 
> Both pull request contains the same commit that fix widgets/tabs api change.
> 
> It's my first real commit/pull request and bugzilla try, 
> so feel free to advise me best pratices.

Hi Alex, 

Thanks for looking into this! IMO 1st is expected behavior, specially since we don't expose any API for non-browser windows. It looks good, but you still need to create review for this change to get push authorization.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 57 - simplier solution

Comment 8

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

Comment on attachment 493699 [details] [diff] [review]
Pull request 57 - simplier solution

(In reply to comment #5)
> It's my first real commit/pull request and bugzilla try, 
> so feel free to advise me best pratices.

We're still working out a new set of best practices since the switch to Git, so it's hard to say exactly what to do, but what you did here, creating both a Bugzilla attachment and a GitHub pull request, works just fine for now!


(In reply to comment #6)
> Thanks for looking into this! IMO 1st is expected behavior, specially since we
> don't expose any API for non-browser windows. It looks good, but you still need
> to create review for this change to get push authorization.

Returning null when the active window isn't a browser window is actually the intended behavior.  However, after some further deliberation, I've come to the conclusion that always returning the frontmost browser window is indeed the better behavior.  It conflates the concepts of "active" and "frontmost" to some extent, but it is more likely to satisfy the common use cases of addon developers, and we can distinguish between "active" and "in front of all other windows, including non-browser windows" at a later date if it becomes important to do so.

Comment 9

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

Thanks for the fixes!

https://github.com/mozilla/addon-sdk/commit/45489bf1ca5d8a007fb848a8bfdec066ed524885

https://github.com/mozilla/addon-sdk/commit/8a1b288896eed67e09f7346946623d331134b834

https://github.com/mozilla/addon-sdk/commit/2293d851f15dc9853462fc77a385c4b9dac4092b

Comment 10

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

Note: this caused a test regression that I am fixing in my branch for bug 613553 <https://github.com/mykmelez/addon-sdk/commit/a9ef3f3b0848cdf5fd3ccf0d4274f9b7d70a86ff>.

Comment 11

[Julián Ceballos]

Thanks for fixes, right now i've continued with the update of cfx init to create the directory if it is necessary and to create a private/public keypair for addon. this topics are in the bugs 613604 and 613587.

Thanks again :).