728651 - [SeaMonkey] "browser_bug557956.js | Should have seen 4 still incompatible items - Got 7, expected 4"

Status: VERIFIED FIXED

(SeaMonkey :: Testing Infrastructure, defect, P2)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1329624302.1329629638.17173.gz
OS X 10.6 comm-central-trunk debug test mochitest-other on 2012/02/18 20:05:02
+
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1329580439.1329585873.28060.gz
Linux comm-aurora debug test mochitest-other on 2012/02/18 07:53:59
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug557956.js | Should have seen 4 still incompatible items - Got 7, expected 4
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug557956.js | Should have seen 4 still incompatible items - Got 7, expected 4
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug557956.js | Test timed out
[and more: I assume the test is leaking to the next ones :-/]
}

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Beta/1329446868.1329450672.14816.gz
WINNT 5.2 comm-beta debug test mochitest-other on 2012/02/16 18:47:48
doesn't seem affected.

***

This is an issue we already faced a long time ago with some other test(s):
this test doesn't account for ChatZilla + DOMi + Venkman, which are bundled with SeaMonkey.
(Or would it be these bundled addons that are not configured correctly?)

Locally, if I click on "Don't check", then the test completes, with a 3rd failure:
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug557956.js | Should have seen 1 still incompatible items - Got 4, expected 1
}

Fwiw, if I comment out
"Services.prefs.setBoolPref(PREF_STRICT_COMPAT, true);"
then the test passes.
(I know this is not the wanted solution.)

***

(Also locally, Modern theme is randomly reported too :-|
 But that's ftr only, for now.)

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

The actual issue is that the maxVersions of the three bundled add-ons (CZ, DOMI and Venkman) have not been updated, and with disabled compatibility, they actually *are* incompatible on trunk (but not on Beta, because their current maxVersions are 11.x (FF/TB/Toolkit) / 2.8.x (SM).

IOW, the test fails for good reason and doesn't need to be changed at all. I checked that the complete test suite succeeds once the three add-ons have been version-bumped. I have the corresponding patches locally now and will file bugs accordingly: I think if a user chooses to disable compatible-by-default, the shipped add-ons should/must stay compatible.

For now I've just updated the compatibility of all three add-ons on AMO (but only for SeaMonkey because I don't know if it's OK for me to update the compatibility for the other applications). That doesn't fix the test, but it was needed anyway. ;-)

What we /should/ do however (IMHO) is create a SM-specific test (under suite/ in comm-central) which checks that we have no incompatible shipped add-ons. Basically a test that does what fails here (disable compatible-by-default), but with better error messages (including the names of problematic add-ons).

Suggesting INVALID.

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #1)

> The actual issue is that the maxVersions of the three bundled add-ons (CZ,
> DOMI and Venkman) have not been updated

Oh :-/ (I missed to actually check that.)
One would think that would be part of SeaMonkey cycle update (human) process by now :-(

> IOW, the test fails for good reason and doesn't need to be changed at all. I
> checked that the complete test suite succeeds once the three add-ons have

That is good news :-)

> been version-bumped. I have the corresponding patches locally now and will
> file bugs accordingly: I think if a user chooses to disable

Make them block this one.

> compatible-by-default, the shipped add-ons should/must stay compatible.

Agreed.

> For now I've just updated the compatibility of all three add-ons on AMO (but
> only for SeaMonkey because I don't know if it's OK for me to update the
> compatibility for the other applications). That doesn't fix the test, but it
> was needed anyway. ;-)

(Good.)

> What we /should/ do however (IMHO) is create a SM-specific test (under
> suite/ in comm-central) which checks that we have no incompatible shipped
> add-ons. Basically a test that does what fails here (disable
> compatible-by-default), but with better error messages (including the names
> of problematic add-ons).

An automatic check seems a very good idea!
Unless that could be (automatically) done at build or release time, and that we would prefer that.

> Suggesting INVALID.

I would rather morph the bug once the test passes...

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

Since we'll probably morph this bug, here's some sample code we can use, which currently results in a single "DOM Inspector 2.0.11pre" alert:

Components.utils.import("resource://gre/modules/Services.jsm");
Components.utils.import("resource://gre/modules/AddonManager.jsm");

const PREF_STRICT_COMPAT = "extensions.strictCompatibility";
Services.prefs.setBoolPref(PREF_STRICT_COMPAT, true);

AddonManager.getAllAddons(function(aAddons) {
  var addons = aAddons.filter(function(a) {
    return a.type != "plugin" && !a.isCompatible;
    // or a.appDisabled, cf. <https://developer.mozilla.org/en/Addons/Add-on_Manager/Addon#Required_properties>
  });
  addons.forEach(function(a) {
    alert(a.name + " " + a.version);
  });
});

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

Attachment: new SM test

I just checked in patches for CZ and DOMI; the Venkman patch is still pending review.

The attached is a new test for SM that should allow us to identify such issues earlier/better in the future. Of course we also need to get better at bumping the compatibility of the bundled add-ons in time. ;-)

Comment 5

[[e-mail]]

Comment on attachment 600812 [details] [diff] [review]
new SM test

>+  info("If the next test fails, Toolkit test browser_bug557956.js will fail, too.");
But of course you can't hg copy across repos, can you ;-)

>+    let ias = []; 
>+    aAddons.forEach(function(a) {
>+      if (a.type != "plugin" && !a.isCompatible)
>+        ias.push(a.name + " " + a.version);
>+    });
>+    is(ias.length, 0, "Found incompatible add-ons: " + ias.join(", "));
Wouldn't it make sense to do something like this:
aAddons.forEach(function(a) {
  if (a.type != "plugin") // we don't care about plugins
    ok(a.isCompatible, a.name + " " + a.version + " should be compatible");
});

>+    Services.prefs.setBoolPref(PREF_STRICT_COMPAT, false);
clearUserPref perhaps?

>\ No newline at end of file
Nit

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 600812 [details] [diff] [review]
new SM test

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

::: suite/browser/test/browser/browser_bug728651.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +// Test that the all bundled add-ons are compatible.

Nit: remove 'the'.

@@ +3,5 @@
> + */
> +
> +// Test that the all bundled add-ons are compatible.
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");

Isn't Services.jsm already available (in browser-chrome)?

@@ +4,5 @@
> +
> +// Test that the all bundled add-ons are compatible.
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/AddonManager.jsm");

If AddonManager.jsm is needed, then I assume it should be loaded through a tempScope variable (not to leak).

@@ +22,5 @@
> +    aAddons.forEach(function(a) {
> +      if (a.type != "plugin" && !a.isCompatible)
> +        ias.push(a.name + " " + a.version);
> +    });
> +    is(ias.length, 0, "Found incompatible add-ons: " + ias.join(", "));

As Neil suggested, I too would prefer to always have a detailed list of all add-ons reported, so we can compare to it.

Nonetheless, you could still use a simpler
|forEach( ... if (...) ok(false, "As this test failed, Toolkit test browser_bug557956.js should be failing too."); break;|

@@ +24,5 @@
> +        ias.push(a.name + " " + a.version);
> +    });
> +    is(ias.length, 0, "Found incompatible add-ons: " + ias.join(", "));
> +
> +    Services.prefs.setBoolPref(PREF_STRICT_COMPAT, false);

As Neil suggested, use clearUserPref().
Fwiw, there is also SpecialPowers.pushPrefEnv()...

@@ +25,5 @@
> +    });
> +    is(ias.length, 0, "Found incompatible add-ons: " + ias.join(", "));
> +
> +    Services.prefs.setBoolPref(PREF_STRICT_COMPAT, false);
> +    is(AddonManager.strictCompatibility, false, "Strict compatibility should be disabled");

I don't think we care to check that, at this point.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v1a [Checkin: Comments 12 and 14]

(In reply to [e-mail] from comment #5)
> Wouldn't it make sense to do something like this:

Well, I guess since it's a test it's OK, but I generally follow a crontab-compatible approach: Only report something if an error occurs. ;-)

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 600989 [details] [diff] [review]
patch v1a [Checkin: Comments 12 and 14]

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

::: suite/browser/test/browser/browser_bug728651.js
@@ +11,5 @@
> +
> +  Services.prefs.setBoolPref(PREF_STRICT_COMPAT, true);
> +  is(AddonManager.strictCompatibility, true, "Strict compatibility should be enabled");
> +
> +  info("If one of the next tests fails, Toolkit test browser_bug557956.js will fail, too.");

All good.
Though I would prefer this line to be
|forEach( ... if (...) ok(false, "As this test failed, Toolkit test browser_bug557956.js should be failing too."); break;|
and after the following forEach().

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #7)
> Only report something if an error occurs. ;-)


(In reply to Serge Gautherie (:sgautherie) from comment #8)
> |forEach( ... if (...) ok(false, "As this test failed, Toolkit test
> browser_bug557956.js should be failing too."); break;|
> and after the following forEach().

Or, probably better:

>+  AddonManager.getAllAddons(function(aAddons) {
      let allCompatible = true;
>+    aAddons.forEach(function(a) {
>+      if (a.type != "plugin") // we don't care about plugins
        {
>+        ok(a.isCompatible, a.name + " " + a.version + " should be compatible");
          allCompatible = allCompatible && a.isCompatible;
        }
>+    });
      if (!allCompatible)
        ok(false, "As this test failed, Toolkit test browser_bug557956.js should be failing too.");

Comment 10

[[e-mail]]

Comment on attachment 600989 [details] [diff] [review]
patch v1a [Checkin: Comments 12 and 14]

>+  info("If one of the next tests fails, Toolkit test browser_bug557956.js will fail, too.");
Nit: should say "following tests".

On my build three extensions failed as expected, presumably because I haven't pulled the latest version updates yet ;-)

Comment 11

[[e-mail]]

(In reply to Serge Gautherie from comment #9)
>         ok(false, "As this test failed, Toolkit test browser_bug557956.js should be failing too.");
Nit: I'd prefer "should fail too" ("should have failed too" if it runs first).

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 600989 [details] [diff] [review]
patch v1a [Checkin: Comments 12 and 14]

http://hg.mozilla.org/comm-central/rev/6658e84a7d40
with Serge's ok() instead of the info()

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

Leaving open until Venkman is fixed and both tests pass.

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 600989 [details] [diff] [review]
patch v1a [Checkin: Comments 12 and 14]

http://hg.mozilla.org/releases/comm-aurora/rev/ad79480a62c3
http://hg.mozilla.org/releases/comm-beta/rev/94b36719e8df

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #11)
> Nit: I'd prefer "should fail too" ("should have failed too" if it runs
> first).

Ftr, the /toolkit test runs after our added /suite test.


(In reply to Jens Hatlak (:InvisibleSmiley) from comment #12)
> http://hg.mozilla.org/comm-central/rev/6658e84a7d40

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1330480610.1330484794.28875.gz
Linux comm-central-trunk debug test mochitest-other on 2012/02/28 17:56:50
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_bug728651.js | JavaScript Debugger 0.9.88.2 should be compatible
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_bug728651.js | As this test failed, Toolkit test browser_bug557956.js should fail, too.
}

Thanks for the explicit test!
V.Fixed


(In reply to Jens Hatlak (:InvisibleSmiley) from comment #13)
> Leaving open until Venkman is fixed and both tests pass.

Fixing Venkman is bug 730691 let's assume that will complete this bug.

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #14)

> http://hg.mozilla.org/releases/comm-aurora/rev/ad79480a62c3

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1330503347.1330507688.898.gz
OS X 10.6 comm-aurora debug test mochitest-other on 2012/02/29 00:15:47
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_bug728651.js | DOM Inspector 2.0.10 should be compatible
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_bug728651.js | ChatZilla 0.9.88 should be compatible
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_bug728651.js | JavaScript Debugger 0.9.88.2 should be compatible
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_bug728651.js | As this test failed, Toolkit test browser_bug557956.js should fail, too.
}

> http://hg.mozilla.org/releases/comm-beta/rev/94b36719e8df

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Beta/1330489986.1330493973.10985.gz
OS X 10.6 comm-beta debug test mochitest-other on 2012/02/28 20:33:06
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_bug728651.js | DOM Inspector 2.0.10 should be compatible
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_bug728651.js | As this test failed, Toolkit test browser_bug557956.js should fail, too.
}

seamonkey2.9 and seamonkey2.8: verified.

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Rename and improve browser_bug728651.js [Checked in: See comment 19]

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 602392 [details] [diff] [review]
(Bv1) Rename and improve browser_bug728651.js
[Checked in: See comment 19]

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

Hmm, AFAICS this is just bikeshedding (and quite a lot of it), so it doesn't qualify for branch landing (in case you considered that). Otherwise it's fine with me, provided that you checked (or will check) that changing the order of tests does not regress anything (in theory there could be interdependencies, which I didn't check).

Note: This initially failed to apply locally, probably due to me using qimportbz. It worked after I first converted the existing test to Unix line ends. I'll leave it to you whether you also do that upon check-in or not (no idea whether we have a line end policy or something).

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 602392 [details] [diff] [review]
(Bv1) Rename and improve browser_bug728651.js
[Checked in: See comment 19]

http://hg.mozilla.org/comm-central/rev/1983f27227be


(In reply to Jens Hatlak (:InvisibleSmiley) from comment #18)

> Hmm, AFAICS this is just bikeshedding (and quite a lot of it), so it doesn't
> qualify for branch landing (in case you considered that). Otherwise it's

No, I meant this additional patch for Trunk only.

> fine with me, provided that you checked (or will check) that changing the
> order of tests does not regress anything (in theory there could be
> interdependencies, which I didn't check).

I tested my patch locally, single test ... or whole sub-directory now.

Ftr, this (unsorted) Makefile list should be used for building/packaging only: it should have no impact on which order the tests are run.
If the latter order can change (on a filesystem that doesn't sort files?), and an interdependency is found, then let it be filed so we can fix it.

> Note: This initially failed to apply locally, probably due to me using
> qimportbz. It worked after I first converted the existing test to Unix line
> ends. I'll leave it to you whether you also do that upon check-in or not (no
> idea whether we have a line end policy or something).

Yes, we have: Unix/LF is the rule.
Done:
http://hg.mozilla.org/comm-central/rev/2a0bb870310b

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #19)
> http://hg.mozilla.org/comm-central/rev/1983f27227be

Ftr,
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1330742797.1330747356.24459.gz&fulltext=1
Linux comm-central-trunk debug test mochitest-other on 2012/03/02 18:46:37
{
TEST-START | chrome://mochitests/content/browser/suite/browser/test/browser_checkAddonCompatibility.js
TEST-PASS | [...] | Strict compatibility should be enabled
TEST-PASS | [...] | ChatZilla 0.9.88 should be compatible
TEST-PASS | [...] | DOM Inspector 2.0.11pre should be compatible
TEST-PASS | [...] | JavaScript Debugger 0.9.88.2 should be compatible
TEST-PASS | [...] | Mochitest 1.0 should be compatible
TEST-PASS | [...] | SeaMonkey Debug and QA UI 1.0pre should be compatible
TEST-PASS | [...] | SeaMonkey Default Theme 2.10a1 should be compatible
TEST-PASS | [...] | SeaMonkey Modern 2.10a1 should be compatible
TEST-PASS | [...] | Special Powers 2010.07.23 should be compatible
TEST-PASS | [...] | WorkerTest 2011.10.10 should be compatible
TEST-PASS | [...] | WorkerTestBootstrap 2011.10.10 should be compatible
INFO TEST-END | chrome://mochitests/content/browser/suite/browser/test/browser_checkAddonCompatibility.js | finished in 220ms
}

Comment 21

[Frank Wein [:mcsmurf]]

Just making sure: So it's normal for this test
 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug557956.js | Should have seen 4 still incompatible items - Got 7, expected 4
to fail because of the SeaMonkey-specific add-ons?

Comment 22

[Justin Wood (:Callek)]

(In reply to Frank Wein [:mcsmurf] from comment #21)
> Just making sure: So it's normal for this test
>  TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/
> browser_bug557956.js | Should have seen 4 still incompatible items - Got 7,
> expected 4
> to fail because of the SeaMonkey-specific add-ons?

Actually no, we need to bump compat directive for the addons, and we need to get better at that.