694806 - 19 of 21 tests pass for cfx test -f window-utils

Status: RESOLVED FIXED

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

Description

[Erik Vold]

When I run cfx test within the api-utils folder I get "2693 of 2693 tests passed." but when I run cfx test -f window-utils I get "19 of 21 tests passed.".

Comment 1

[Erik Vold]

When I run cfx test -f window-utils --times 5 I get "103 of 105 tests passed."

Comment 2

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

Does it say which two tests are failing?

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Wait for end of test runner windows loading.

When we launch this test only, the test runner windows is still loading when we are running testActiveWindow. It ends up messing with focus changes.

In this patch I've added some code that wait before executing this test, only if the test windows is still loading.

Comment 4

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

Comment on attachment 569048 [details] [diff] [review]
Wait for end of test runner windows loading.

Review of attachment 569048 [details] [diff] [review]:
-----------------------------------------------------------------

Alex, this patch looks ok, but I'm not sure about approach. I think it would be much better to put this into test runner itself to defer test execution till test window is loaded, as I can imagine that there are other tests that will have similar problems.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Wait for harnesss window to be ready before loading tests

Here is a new patch that waits for harness window to be fully loaded/focused before running any tests. So I moved this code to run-tests.js so that all tests will benefit from this!

All tests pass! It may fix some other intermittent failure.

myk: I'm requesting your review instead of irakli, as I don't really know when he will be back.

Comment 6

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

Comment on attachment 571364 [details] [diff] [review]
Wait for harnesss window to be ready before loading tests

This fixes the test failure but triggers new warnings in SDK code that look fairly problematic:

--------------------------------------------------------------------------------
(addon-sdk)mykbook:api-utils myk$ cfx test -f window-utils
Using binary at '/Applications/Nightly.app/Contents/MacOS/firefox-bin'.
Using profile at '/var/folders/bN/bN4Z5w6UEdC60cFgLX32NU+++TI/-Tmp-/tmpkMcxBp.mozrunner'.
Running tests on Firefox 10.0a1/Gecko 10.0a1 ({ec8030f7-c20a-464f-9b0e-13a3a9e97384}) under darwin/x86.
.......console: [JavaScript Warning: "({'test suite':{Assert:(function Assert() {"use strict";return Object.create(AssertBase.apply(null, arguments), assertDescriptor);}), 'test that custom assertor is passed to test function':(function (assert) {"use strict";assert.ok("foo" in assert, "custom assertion function `foo` is defined");assert.foo("custom assertion function `foo` is called");}), 'test sub suite':{'test that `Assert` is inherited by sub suits':(function (assert) {"use strict";assert.ok("foo" in assert, "assertion function `foo` is not defined");}), 'test sub sub suite':{Assert:(function Assert() {"use strict";return Object.create(AssertBase.apply(null, arguments), assertDescriptor);}), 'test that custom assertor is passed to test function':(function (assert) {"use strict";assert.ok("bar" in assert, "custom assertion function `bar` is defined");assert.bar("custom assertion function `bar` is called");}), 'test that `Assert` is not inherited by sub sub suits':(function (assert) {"use strict";assert.ok(!("foo" in assert), "assertion function `foo` is not defined");})}}}}) is not extensible" {file: "resource://30ebeff5-d2b3-4e0c-a074-d5571d4deaf6-at-jetpack-api-utils-lib/cuddlefish.js -> resource://30ebeff5-d2b3-4e0c-a074-d5571d4deaf6-at-jetpack-api-utils-lib/test.js" line: 99}]
console: [JavaScript Warning: "reference to undefined property desc1.get" {file: "resource://30ebeff5-d2b3-4e0c-a074-d5571d4deaf6-at-jetpack-api-utils-lib/cuddlefish.js -> resource://30ebeff5-d2b3-4e0c-a074-d5571d4deaf6-at-jetpack-api-utils-lib/traits/core.js" line: 54}]
console: [JavaScript Warning: ""Assert" is read-only" {file: "resource://30ebeff5-d2b3-4e0c-a074-d5571d4deaf6-at-jetpack-api-utils-lib/cuddlefish.js -> resource://30ebeff5-d2b3-4e0c-a074-d5571d4deaf6-at-jetpack-api-utils-lib/test.js" line: 99}]
console: [JavaScript Warning: ""Assert" is read-only" {file: "resource://30ebeff5-d2b3-4e0c-a074-d5571d4deaf6-at-jetpack-api-utils-lib/cuddlefish.js -> resource://30ebeff5-d2b3-4e0c-a074-d5571d4deaf6-at-jetpack-api-utils-lib/test.js" line: 99}]
console: [JavaScript Warning: "({'test custom constructor and inherited toString':(function (assert) {"use strict";
function Type() {return Object.create(Type.prototype);}

Type.prototype = Trait({method: function method() {return 2;}}).create(Object.freeze(Type.prototype));var fixture = Type();assert.equal(fixture.constructor, Type, "must override constructor");assert.equal(fixture.toString(), "[object Type]", "must inherit toString");}), 'test custom toString and inherited constructor':(function (assert) {"use strict";
function Type() {return Object.create(Type.prototype);}

Type.prototype = Trait({toString: function toString() {return "<toString>";}}).create();var fixture = Type();assert.equal(fixture.constructor, Trait, "must inherit constructor Trait");assert.equal(fixture.toString(), "<toString>", "Must override toString");}), 'test custom toString and constructor':(function (assert) {"use strict";
function Type() {return TypeTrait.create(Type.prototype);}

Object.freeze(Type.prototype);var TypeTrait = Trait({toString: function toString() {return "<toString>";}});var fixture = Type();assert.equal(fixture.constructor, Type, "constructor is provided to create");assert.equal(fixture.toString(), "<toString>", "toString was overridden");}), 'test resolve constructor':(function (assert) {"use strict";
function Type() {}

var T1 = Trait({constructor: Type}).resolve({constructor: "_foo"});var f1 = T1.create();assert.equal(f1._foo, Type, "constructor was resolved");assert.equal(f1.constructor, Trait, "constructor of prototype is inherited");assert.equal(f1.toString(), "[object Trait]", "toString is inherited");}), 'test compose read-only':(function (assert) {"use strict";
function Type() {}

Type.prototype = Trait.compose(Trait({}), {constructor: {value: Type}, a: {value: "b", enumerable: true}}).resolve({a: "b"}).create({a: "a"});var f1 = new Type;assert.equal(Object.getPrototypeOf(f1), Type.prototype, "inherits correctly");assert.equal(f1.constructor, Type, "constructor was overridden");assert.equal(f1.toString(), "[object Type]", "toString was inherited");assert.equal(f1.a, "a", "property a was resolved");assert.equal(f1.b, "b", "property a was renamed to b");assert.ok(!Object.getOwnPropertyDescriptor(Type.prototype, "a"), "a is not on the prototype of the instance");var proto = Object.getPrototypeOf(Type.prototype);var dc = Object.getOwnPropertyDescriptor(Type.prototype, "constructor");var db = Object.getOwnPropertyDescriptor(Type.prototype, "b");var da = Object.getOwnPropertyDescriptor(proto, "a");assert.ok(!dc.writable, "constructor is not writable");assert.ok(!dc.enumerable, "constructor is not enumerable");assert.ok(dc.configurable, "constructor inherits configurability");assert.ok(!db.writable, "a -> b is not writable");assert.ok(db.enumerable, "a -> b is enumerable");assert.ok(!db.configurable, "a -> b is not configurable");assert.ok(da.writable, "a is writable");assert.ok(da.enumerable, "a is enumerable");assert.ok(da.configurable, "a is configurable");})}) is not extensible" {file: "resource://30ebeff5-d2b3-4e0c-a074-d5571d4deaf6-at-jetpack-api-utils-lib/cuddlefish.js -> resource://30ebeff5-d2b3-4e0c-a074-d5571d4deaf6-at-jetpack-api-utils-lib/test.js" line: 99}]
console: [JavaScript Warning: "variable window redeclares argument" {file: "resource://30ebeff5-d2b3-4e0c-a074-d5571d4deaf6-at-jetpack-api-utils-lib/cuddlefish.js -> resource://30ebeff5-d2b3-4e0c-a074-d5571d4deaf6-at-jetpack-api-utils-lib/tab-browser.js" line: 736 column: 6 source: "  var window = tabEl.linkedBrowser.contentWindow;
"}]
console: [JavaScript Warning: "anonymous function does not always return a value" {file: "resource://30ebeff5-d2b3-4e0c-a074-d5571d4deaf6-at-jetpack-api-utils-lib/cuddlefish.js -> resource://30ebeff5-d2b3-4e0c-a074-d5571d4deaf6-at-jetpack-api-utils-lib/windows/loader.js" line: 119 column: 4 source: "    return window;
"}]
.............warning: 7 warnings or errors were logged to the platform's nsIConsoleService, which could be of no consequence; however, they could also be indicative of aberrant behavior.

20 of 20 tests passed.
Total time: 2.610338 seconds
Program terminated successfully.
--------------------------------------------------------------------------------


>+    // Finally, we have to run test on next cycle, otherwise XPCOM components
>+    // are not correctly updated.

I don't understand this comment; can you explain further?  And is there a platform bug here that we should file and reference?

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to Myk Melez [:myk] from comment #6)

I've seen these new warning messages. I wasn't able to fix errors in test.js.
I identified what is failing. We are trying to modify a frozen object, but I don't know exactly why it is frozen. And even if I avoid modifying it with if (!Object.isFrozen(obj)), it still prints this warning.
I'll need to ping Irakli about this, it is some code related to CommonJS tests.

Having said that, I don't think this patch causes these errors. I think it just exposes them, they may just have been hidden for an unknown reason!

> Comment on attachment 571364 [details] [diff] [review] [diff] [details] [review]
> >+    // Finally, we have to run test on next cycle, otherwise XPCOM components
> >+    // are not correctly updated.
> 
> I don't understand this comment; can you explain further?  And is there a
> platform bug here that we should file and reference?

If we do not wait one additional cycle after focus event, we get this exception:
"Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIFocusManager.getFocusedElementForWindow]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"

on the following line:
  fm.getFocusedElementForWindow(targetWindow, true, childTargetWindow);
with targetWindow being our test runner window.

Comment 8

[Alexandre Poirot [:ochameau]]

Irakli: can you try applying this patch and see why test.js can't set test.Assert attribute?
test.js:100
  test.Assert = test.Assert || Assert;
It fails when `test` is equal to `exports` object from asserts.js
And the manir reason why I asked for your help is that, even if I add
  if (!("Assert" in test) && !Object.isFrozen(test))
The previous line keeps throwing strict warning message from comment 6.

Comment 9

[Alexandre Poirot [:ochameau]]

Just in case someone face the same issue ...
We have the same problem with addon-kit module with:
  cfx test -v -f test-windows
  > 20 of 25 tests passed.

This patch solve this issue too.

Comment 10

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

I believe that fails because `exports` are frozen immediately after module body is invoked. This is intentional though.

Comment 11

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

what about doing `if (!('Assert' in test)) test.Assert = Assert` instead:

https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/test.js#L99

Comment 12

[Alexandre Poirot [:ochameau]]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #11)
> what about doing `if (!('Assert' in test)) test.Assert = Assert` instead:
> 
> https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/test.
> js#L99

Can you look at comment 8. I already tried that without success.
I wouldn't have bug you about this if it was that easy :p

Whatever I add, I keep having this error:

console: [JavaScript Warning: "({'test suite':{Assert:(function Assert() {"use s
trict";return Object.create(AssertBase.apply(null, arguments), assertDescriptor)
;}), 'test that custom assertor is passed to test function':(function (assert) {
"use strict";assert.ok("foo" in assert, "custom assertion function `foo` is defi
ned");assert.foo("custom assertion function `foo` is called");}), 'test sub suit
e':{'test that `Assert` is inherited by sub suits':(function (assert) {"use stri
ct";assert.ok("foo" in assert, "assertion function `foo` is not defined");}), 't
est sub sub suite':{Assert:(function Assert() {"use strict";return Object.create
(AssertBase.apply(null, arguments), assertDescriptor);}), 'test that custom asse
rtor is passed to test function':(function (assert) {"use strict";assert.ok("bar
" in assert, "custom assertion function `bar` is defined");assert.bar("custom as
sertion function `bar` is called");}), 'test that `Assert` is not inherited by s
ub sub suits':(function (assert) {"use strict";assert.ok(!("foo" in assert), "as
sertion function `foo` is not defined");})}}}}) is not extensible" {file: "resou
rce://f9826cb2-afc9-4e75-a471-502d5a89d96f-at-jetpack-api-utils-lib/cuddlefish.j
s -> resource://f9826cb2-afc9-4e75-a471-502d5a89d96f-at-jetpack-api-utils-lib/te
st.js" line: 99}]

Comment 13

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #6)
> Comment on attachment 571364 [details] [diff] [review] [diff] [details] [review]
> Wait for harnesss window to be ready before loading tests
> 
> This fixes the test failure but triggers new warnings in SDK code that look
> fairly problematic:
> 

Myk I have filled new Bug 701075 that addresses this warnings, also while this patch triggers this warnings, it's not related to the change. Unless you have any other reason to r- this, I would r+ this but land only after 701075.

Comment 14

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

Comment on attachment 571364 [details] [diff] [review]
Wait for harnesss window to be ready before loading tests

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #13)
> Myk I have filled new Bug 701075 that addresses this warnings, also while
> this patch triggers this warnings, it's not related to the change. Unless
> you have any other reason to r- this, I would r+ this but land only after
> 701075.

Good idea!  r=myk

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Fix for FF8

This patch was failing on Firefox8!
For some reason, if we try to call window.focus() before our test harness window has finished loading ... it is now failing like without this patch!
So I've updated the patch in order to call focus() after `load` event, and I've added some comments regarding myk's comment 6 about xpcom.

Comment 16

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

Comment on attachment 573509 [details] [diff] [review]
Fix for FF8

Review of attachment 573509 [details] [diff] [review]:
-----------------------------------------------------------------

::: packages/test-harness/lib/run-tests.js
@@ +71,5 @@
> +  // We have to wait for this window to be fully loaded *and* focused
> +  // in order to avoid it to mess with our various window/focus tests.
> +  // We are first waiting for our window to be fully loaded before ensuring
> +  // that it will take the focus, and then we wait for it to be focused.
> +  window.addEventListener("focus", function onfocus() {

How do you know that's your focus call and not something happening before ? Maybe it's better to do that after onload ?

Comment 17

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

Comment on attachment 573509 [details] [diff] [review]
Fix for FF8

Review of attachment 573509 [details] [diff] [review]:
-----------------------------------------------------------------

Alex, told on IRC he will write another patch addressing my comment.

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: Wait for load before listening for focus

Yes, it makes sense to wait for load before listening,
it may fix some other corner cases where developers play with windows while test runs!

Comment 19

[[github robot]]

Commit pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/11adb5ac797ae117bc900563eba0e85b6fab0fd4
fix bug 694806 - 19 of 21 tests pass for cfx test -f window-utils - by waiting for the test window to be loaded/focused before running tests; r=@gozala