874690 - mozmill tests involving test-nntp-helpers.js fail because MailServices is not defined

Status: RESOLVED FIXED

(Thunderbird :: Testing Infrastructure, defect)

Description

[Jens Müller (:tessarakt)]

Attachment: mozmill-account-values.log

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:23.0) Gecko/20130516 Firefox/23.0 (Nightly/Aurora)
Build ID: 20130516004016

Steps to reproduce:

In the object directory, I did
1. make SOLO_TEST=folder-widget/test-message-filters.js mozmill-one
2. make SOLO_TEST=account/test-account-values.js mozmill-one


Actual results:

Both tests failed, with this exception:

SUMMARY-UNEXPECTED-FAIL | test-nntp-helpers.js | test-nntp-helpers.js::<TOP_LEVEL>
  EXCEPTION: MailServices is not defined
    at: nonesuch line 129

Full logs will follow.


Expected results:

Tests should complete successfully.

Comment 1

[Jens Müller (:tessarakt)]

Attachment: Output from make SOLO_TEST=folder-widget/test-message-filters.js mozmill-one

Comment 2

[Jens Müller (:tessarakt)]

My first wild guess is that a "Cu.import("resource:///modules/mailServices.js");" is missing. Will try that out now ...

Comment 3

[Jens Müller (:tessarakt)]

With this very minimal change:

Cu.import("resource://gre/modules/Services.jsm");
+Cu.import("resource:///modules/mailServices.js");

(first line was already there, second line (without "+") added)

in mail/test/mozmill/shared-modules/test-nntp-helpers.js, both Mozmill tests now work. I guess a bunch of others did not work before as well, and will now work ...

Comment 4

[Jens Müller (:tessarakt)]

Attachment: Add missing import to test-nntp-helpers.js

Comment 5

[Jens Müller (:tessarakt)]

Some questions out of curiosity:

Are the Mozmill tests automatically run? Are the results published somewhere?

Is passing them not a requirement for patches to be included? If yes, how could this break so severely?

Comment 6

[Jens Müller (:tessarakt)]

Hmm. Is this possibly related to bug 720358?

Comment 7

[Jens Müller (:tessarakt)]

What I don't understand is why the same issue does not occur in Tinderbox ...

Comment 8

[Jens Müller (:tessarakt)]

test-nntp-helpers.js was changed here: http://hg.mozilla.org/comm-central/diff/0d9c225b2a8b/mail/test/mozmill/shared-modules/test-nntp-helpers.js - see bug 852690 ...

Comment 9

[Jens Müller (:tessarakt)]

See also https://bugzilla.mozilla.org/show_bug.cgi?id=722187#c28 for a similar problem ...

Comment 10

[:aceman]

Yes, this is strange. I am the author of the test-account-values.js test and I have seen no such failure. I think mailServices is imported into this test via folder-display-helpers.

And it also does not happen on the TB try server: https://tbpl.mozilla.org/?tree=Thunderbird-Trunk . I think Services and MailServices should be imported in some global init file (if there is one) so that it is not needed to import into each test file. But it seems it is still the case today, many tests import it themselves: http://mxr.mozilla.org/comm-central/search?string=mailServices.js&find=%2Fmail%2Ftest%2Fmozmill&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
So if you add it to nntp-helpers that seems to be the right thing for now.

Comment 11

[Magnus Melin [:mkmelin]]

(In reply to Jens Müller from comment #5)
> Some questions out of curiosity:
> 
> Are the Mozmill tests automatically run? Are the results published somewhere?

Yes, they are run continously - results are here: https://tbpl.mozilla.org/?tree=Thunderbird-Trunk

> Is passing them not a requirement for patches to be included? 

yes.

Comment 12

[Jens Müller (:tessarakt)]

(In reply to :aceman from comment #10)
> Yes, this is strange. I am the author of the test-account-values.js test and
> I have seen no such failure. I think mailServices is imported into this test
> via folder-display-helpers.

Indeed.

folder-display-helper.js contains 
> Cu.import("resource:///modules/mailServices.js");
and the symbol MailServices is not mentioned in DO_NOT_EXPORT ...

test-nntp-helpers.js contains
>  const MODULES_REQUIRES = ['folder-display-helpers', 'window-helpers'];

So it seems it should be exported ...

The crucial question now is why it isn't on my system. It's probably not a good idea to just cover this up ...

Comment 13

[:aceman]

(In reply to Jens Müller from comment #12)
> (In reply to :aceman from comment #10)
> > Yes, this is strange. I am the author of the test-account-values.js test and
> > I have seen no such failure. I think mailServices is imported into this test
> > via folder-display-helpers.
> 
> Indeed.
> 
> folder-display-helper.js contains 
> > Cu.import("resource:///modules/mailServices.js");
> and the symbol MailServices is not mentioned in DO_NOT_EXPORT ...
> 
> test-nntp-helpers.js contains
> >  const MODULES_REQUIRES = ['folder-display-helpers', 'window-helpers'];
> 
> So it seems it should be exported ...

No, the real import command in test-nntp-helpers is this:
folderDisplayHelper = collector.getModule('folder-display-helpers');

Then all objects from it are referenced as folderDisplayHelper.* . So maybe folderDisplayHelper.MailServices.* would work there.

Is it possible that the test only fails when it is run alone? If it is run as part of all the tests in the directory, then maybe somehow the services get imported and preserved from other test? E.g. like objects in TB profile are preserved between tests in the same dir. Can you try it?

Comment 14

[:aceman]

(In reply to Jens Müller from comment #0)
> In the object directory, I did
> 1. make SOLO_TEST=folder-widget/test-message-filters.js mozmill-one
> 2. make SOLO_TEST=account/test-account-values.js mozmill-one

I have tried this now and do not see any such failure (on linux).

Comment 15

[Jens Müller (:tessarakt)]

When I put
> const URLCreator = folderDisplayHelper.MailServices.nntp.QueryInterface(Ci.nsIProtocolHandler);
into the function create_post, it works.

Comment 16

[Jens Müller (:tessarakt)]

> const URLCreator = MailServices.nntp.QueryInterface(Ci.nsIProtocolHandler);
works too, as long as it is inside create_post.

For the record: Apparently we have some strange differences in evaluation order between my system and all others ...

Apparently, on my machine, "const URLCreator = MailServices.nntp.QueryInterface(Ci.nsIProtocolHandler);" (i.e., test_nntp_helpers.js) is evaluated before the other modules which eventually import the symbol "MailServices" into the global object ... -- But why?

Comment 17

[Jens Müller (:tessarakt)]

Quoting aceman: "you have a hell fast machine and some parallel Javascript engine that happens to process the MailServices line before the other includes ;)"

I added some dump()s to mail/test/resources/mozmill/mozmill/extension/resource/modules/frame.js

I have attached the logfile ... One possibility now is to compare it with the output of someone else. And to go through the files one by one to see where a Cu.import("resource:///modules/mailServices.js"); imports MailServices into the (shared) global object ...

Comment 18

[Jens Müller (:tessarakt)]

Attachment: dump output showing the order in which .js files were loaded

Comment 19

[Jens Müller (:tessarakt)]

If I put
> Cu.import("resource:///modules/mailServices.js");
into test-windows-helpers.js, it works.

So maybe someone else can do this:
> var loadFile = function(path, collector) {
>   dump("loadFile called for: " + path + "\n");>
> ...

so that we can compare the order in which files are loaded ...

Comment 20

[:aceman]

Attachment: dump of "make SOLO_TEST=folder-widget mozmill-one"

Comment 21

[Jens Müller (:tessarakt)]

OK, got it.

In Mozmill modules (test-something.js files), one defines the dependencies in MODULE_REQUIRES. But those are not filenames, but module names. To find the desired modules, Mozmill loads all test-xyz.js files in RELATIVE_ROOT.

This is done in the function Collector.prototype.initTestDirectory, in the order the files are returned by os.listDirectory.

That might depend on the operating system or file system, and on such factors as whether it is a fresh clone or was updated from time to time.

Mystery solved. The conclusion now is to make sure that files define by themselves the symbols they expect in the global namespace on initialization time. Symbols used inside functions are a wholly different issue.

Comment 22

[Jens Müller (:tessarakt)]

Comment on attachment 752458 [details] [diff] [review]
Add missing import to test-nntp-helpers.js

I'm now pretty convinced that this is a proper way to solve the problem. Requesting review again.

Comment 23

[:aceman]

Yes, this seems to be a bandaid for this specific occurrence. Maybe you would like to file a bug to fix all the tests universally so that they do not depend on symbols randomly included from other files?

Comment 24

[Jens Müller (:tessarakt)]

Yes, after / together with fixing the apparent bug in the Mozmill module loading code. Is the copy in c-c the authoritative one, or is Mozmill coming from somewhere upstream?

Comment 25

[Jens Müller (:tessarakt)]

The second answer in http://stackoverflow.com/questions/15179265/how-do-i-use-the-targetobj-parameter-to-loadsubscript-in-firefox-extensions explains the problem:

> So while variables and functions of the subscript are defined in its scope, it still has access to the scope of the script that loaded it - undeclared variables will still be created in the outer scope and the outer scope will be searched for variables that cannot be resolved in the subscript scope.

> If you really want to isolate the subscript then you should use a "proper" global object like a window object or a sandbox:

Comment 26

[Jens Müller (:tessarakt)]

See bug 876089

Comment 27

[:aceman]

While there, could you strip the "test-" prefix from the module name of test-nntp-helpers? It seems no other module in shared-modules has it. The prefix should only be in the filename.

Comment 28

[Jens Müller (:tessarakt)]

Attachment: Add missing import to test-nntp-helpers.js

Comment 29

[Jens Müller (:tessarakt)]

(In reply to :aceman from comment #27)
> While there, could you strip the "test-" prefix from the module name of
> test-nntp-helpers? It seems no other module in shared-modules has it. The
> prefix should only be in the filename.

Done.

Comment 30

[:aceman]

Suyash, in your mozmill run you also get this failure multiple times:
Test Failure: MailServices is not defined
TEST-UNEXPECTED-FAIL | /home/suyash/comm-central/mail/test/mozmill/shared-modules/test-nntp-helpers.js | test-nntp-helpers.js::<TOP_LEVEL>

Can you try applying the patch here to see if it fixes those?

Comment 31

[:aceman]

Tessarakt, for you information, Suyash also uses a 64bit linux and he sees the bug. I use 32bit and I do not see it. It is still strange why the 64bit linux build on Thunderbird-Trunk does not see it.

Comment 32

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #30)
> Suyash, in your mozmill run you also get this failure multiple times:
> Can you try applying the patch here to see if it fixes those?

Sure.

The patch removes MailServices error.
But this error slips in:

SUMMARY-UNEXPECTED-FAIL | test-message-filters.js | test-message-filters.js::setupModule
  EXCEPTION: nh is undefined
    at: test-message-filters.js line 26

Making this change fixes this error as well,

mail/test/mozmill/folder-widget/test-message-filters.js
@@ -4,30 +4,30 @@
-                       "test-nntp-helpers", "address-book-helpers",
+                       "nntp-helpers", "address-book-helpers",

And both the tests pass.

Comment 33

[Suyash Agarwal (:sshagarwal)]

Sorry, the change that removes the new error is:

-  let nh = collector.getModule("test-nntp-helpers");
+  let nh = collector.getModule("nntp-helpers");

Comment 34

[:aceman]

Comment on attachment 754266 [details] [diff] [review]
Add missing import to test-nntp-helpers.js

Jens, please fix the patch per comment 33.

Comment 35

[:aceman]

Attachment: Add missing import to test-nntp-helpers.js v2

Fixed the patch for Jens as fixing tests it the priority now.

Comment 36

[Mark Banner (:standard8)]

Looking at try, there's another failure as a result of these two bugs:

TEST-START | /home/cltbld/talos-slave/test/build/mozmill/message-reader/test-bug594646.js | setupModule
Test Failure: os is not defined
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/message-reader/test-bug594646.js | test-bug594646.js::setupModule

Comment 37

[:aceman]

That should result from the other bug.
This one could go in independently.

Comment 38

[Mark Banner (:standard8)]

Comment on attachment 767895 [details] [diff] [review]
Add missing import to test-nntp-helpers.js v2

Ok, looks good. I've pushed it to the tree; even though it doesn't fix any bustage on the tree it seems to fix things locally for people, and it'll also trigger a new cset on the tree which should hopefully clear the picture up a bit as I know there are some fixes landed in m-c that should help us.

Comment 39

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/849aa4cab973