539340 - [SeaMonkey 2.0] xpcshell-tests: test_autoconfigUtils.js fails (and leaks) after bug 538568 landing

Status: VERIFIED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Serge Gautherie (:sgautherie)]

All 3 platforms:
{
TEST-UNEXPECTED-FAIL | e:\builds\slave\comm-1.9.1-win32-unittest\build\objdir\mozilla\_tests\xpcshell\test_mailnewsbase\unit\test_autoconfigUtils.js | test failed (with xpcshell return code: 3), see following log:

uncaught exception: ContentLength not available (not a local URL?)

== BloatView: ALL (cumulative) LEAK STATISTICS
}

Comment 1

[Serge Gautherie (:sgautherie)]

Blake, Mark, any hint?

Comment 2

[Blake Winton (:bwinton) (:☕️)]

I took a look, but can't figure out why it would do that for SM, but not TB.

(To be fair, I haven't built SeaMonkey yet, and don't really have the time to until mid-February.)

Does it stop failing and/or stop leaking if you comment out the call to "test_copying_readFromXML();" on line 355 of mailnews/base/test/unit/test_autoconfigUtils.js ?

Thanks,
Blake.

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)

> I took a look, but can't figure out why it would do that for SM, but not TB.

I would guess because SM does not have the "accountcreation" feature (yet)?

> (To be fair, I haven't built SeaMonkey yet, and don't really have the time to
> until mid-February.)

You can use a packaged tests SM tinderbox build :-)

> Does it stop failing and/or stop leaking if you comment out the call to
> "test_copying_readFromXML();" on line 355 of
> mailnews/base/test/unit/test_autoconfigUtils.js ?

Same error.
The error is caused by any of the 4 |loader.loadSubScript("chrome://messenger/content/accountcreation/*.js"|.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Okay, so if you're not using the autoconfig, does it make any sense to run this test in the first place?

And if not, can we remove it only when SeaMonkey runs the test suite?

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)

I don't know much about this very feature, but:
it looks like it was meant to live in MNCore.
I think the usual way is to check whether it's enabled or not when trying to test it.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Sounds good.  If it's still not fixed by the time I get some free time, I'll work on that.

Thanks,
Blake.

Comment 7

[Mark Banner (:standard8)]

Well according to mailnews/jar.mn there's an ifdef in there for MOZ_THUNDERBIRD for those js files. So the test just isn't going to be able to load them.

Dunno why, I expect the reason is lost in the midst of time. Doesn't matter anyway, we just need to take account of it in the test somehow.

Serge: maybe try adding a try/catch and then something in run_test to return early?

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Rough handling for SeaMonkey case, Let the log be somewhat more explicit.

Per comment 7. I'm not sure if we can do any better ftb :-|

Comment 9

[Justin Wood (:Callek)]

Comment on attachment 422273 [details] [diff] [review]
(Av1) Rough handling for SeaMonkey case, Let the log be somewhat more explicit.

Whitespace... and why the extra do_check's that really "do" nothing?

[If mark r+'es ignore my r- of course]

Comment 10

[Justin Wood (:Callek)]

(In reply to comment #9)
> (From update of attachment 422273 [details] [diff] [review])
> Whitespace... 

By this I mean indents where you braced should exist.

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)

> Whitespace... 

Sure, I could re-indent both blocks if you want: it just happens that some times (as an exception) we don't...

> and why the extra do_check's that really "do" nothing?

To get a trace/count in the log: see bug 468196.

Comment 12

[Justin Wood (:Callek)]

(In reply to comment #11)
> (In reply to comment #9)
> > and why the extra do_check's that really "do" nothing?
> 
> To get a trace/count in the log: see bug 468196.

If you mean because of: // These will trigger the 'deprecated' (harness) warning.

I'm still not even sure on why your adding them here. Wouldn't the other parts of the test still generate something useful relating to that?

If you mean something else, can you be a bit more specific.

Besides I don't see anything here that depends on 468196 or vice-versa (why not leave any "other" features of this to a new patch. Properly explained in its own bug?

Comment 13

[Robert Kaiser]

(In reply to comment #7)
> Well according to mailnews/jar.mn there's an ifdef in there for MOZ_THUNDERBIRD
> for those js files.
> Dunno why, I expect the reason is lost in the midst of time.

We decided that the account autoconfig is a bit too experimental for SeaMonkey 2.0, which already had enough other stuff we needed to get finished up and working well enough. We should revisit and possibly change that for 2.1, though.

Just as a note.

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to comment #12)

This test currently logs nothing about its checks except in case of failure: |do_throw(aWhy);|.
Adding logs let know what actually happened ... and be sure that it did not just skip everything for example: that's how reftests and mochitests work.

Bug 468196 dependency is just a reminder that this test badly needs more detailed messages: 400 times the same log is the best that can be done ftb, but is hardly user friendly.

I'm doing it all at once because the test author doesn't care about the perma-orange and the log changes are trivial.

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #13)
> We should revisit and possibly change that for 2.1, though.

Ftr, this is bug 507841.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 422273 [details] [diff] [review]
(Av1) Rough handling for SeaMonkey case, Let the log be somewhat more explicit.

>+
>+try {
>+
> var loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
>                        .getService(Components.interfaces.mozIJSSubScriptLoader);
> loader.loadSubScript("chrome://messenger/content/accountcreation/util.js",
>                      xmlReader);
> loader.loadSubScript("chrome://messenger/content/accountcreation/accountConfig.js",
>                      xmlReader);
> loader.loadSubScript("chrome://messenger/content/accountcreation/sanitizeDatatypes.js",
>                      xmlReader);
> loader.loadSubScript("chrome://messenger/content/accountcreation/readFromXML.js",
>                      xmlReader);
>

This needs correct indentation.
 
>+} catch (ex) {
>+  // The "accountcreation" files are not available in SeaMonkey (yet).
>+  do_check_false(false);

One issue per bug please. If we're doing this for bug 468196 then a) this bad check needs a clear explanation of why it is being added (IMHO there should be some sort of "info" option if we need a check such as this that badly) and b) we need to have proper checkin comment for blame.

> function assert(aBeTrue, aWhy)
> {
>-  if (!aBeTrue)
>+  if (aBeTrue)
>+    do_check_true(true);
>+  else
>     do_throw(aWhy);

Ditto, we don't need this change to fix this issue.

>+
>+  if (xmlReader) {
>+
>   test_copying_readFromXML();
> 
>+  }

There's no excuse not to indent that.

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Rough handling that "accountcreation" files are not available in SeaMonkey (yet) [Checkin: See comment 19 & 20]

Av1, with comment 16 suggestion(s).

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 423558 [details] [diff] [review]
(Av2) Rough handling that "accountcreation" files are not available in SeaMonkey (yet)
[Checkin: See comment 19 & 20]

>+  if (xmlReader) {
>+    test_copying_readFromXML();
>+  }

nit: (sorry only just realised this) no brackets necessary here.

r=Standard8 with that fixed.

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 423558 [details] [diff] [review]
(Av2) Rough handling that "accountcreation" files are not available in SeaMonkey (yet)
[Checkin: See comment 19 & 20]


http://hg.mozilla.org/comm-central/rev/3a0de036bf83
Av2, with comment 18 suggestion(s).

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 423558 [details] [diff] [review]
(Av2) Rough handling that "accountcreation" files are not available in SeaMonkey (yet)
[Checkin: See comment 19 & 20]


http://hg.mozilla.org/releases/comm-1.9.1/rev/2ce707a354cd

Comment 21

[Serge Gautherie (:sgautherie)]

V.Fixed, per SeaMonkey 2.0 tinderboxes.

Comment 22

[Mark Banner (:standard8)]

Verifying as fixed for 3.0.2 based on the fact that these are test-only patches and the last 5 runs from MozMill/unit tests have been green with no failures relating to these patches.