Closed
Bug 874690
Opened 11 years ago
Closed 11 years ago
mozmill tests involving test-nntp-helpers.js fail because MailServices is not defined
Categories
(Thunderbird :: Testing Infrastructure, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 25.0
People
(Reporter: tessarakt, Assigned: tessarakt)
References
Details
Attachments
(5 files, 2 obsolete files)
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.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Version: 5.0 → Trunk
Assignee | ||
Comment 2•11 years ago
|
||
My first wild guess is that a "Cu.import("resource:///modules/mailServices.js");" is missing. Will try that out now ...
Assignee | ||
Comment 3•11 years ago
|
||
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 ...
Assignee | ||
Comment 4•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #752458 -
Flags: review?(mbanner)
Assignee | ||
Comment 5•11 years ago
|
||
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?
Assignee | ||
Comment 6•11 years ago
|
||
Hmm. Is this possibly related to bug 720358?
Assignee | ||
Comment 7•11 years ago
|
||
What I don't understand is why the same issue does not occur in Tinderbox ...
Assignee | ||
Comment 8•11 years ago
|
||
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 ...
Assignee | ||
Comment 9•11 years ago
|
||
See also https://bugzilla.mozilla.org/show_bug.cgi?id=722187#c28 for a similar problem ...
Comment 10•11 years ago
|
||
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•11 years ago
|
||
(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.
Assignee | ||
Comment 12•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
(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).
Assignee | ||
Comment 15•11 years ago
|
||
When I put
> const URLCreator = folderDisplayHelper.MailServices.nntp.QueryInterface(Ci.nsIProtocolHandler);
into the function create_post, it works.
Assignee | ||
Updated•11 years ago
|
Attachment #752458 -
Flags: review?(mbanner)
Assignee | ||
Comment 16•11 years ago
|
||
> 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?
Assignee | ||
Comment 17•11 years ago
|
||
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 ...
Assignee | ||
Comment 18•11 years ago
|
||
Assignee | ||
Comment 19•11 years ago
|
||
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•11 years ago
|
||
Assignee | ||
Comment 21•11 years ago
|
||
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.
Assignee | ||
Comment 22•11 years ago
|
||
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.
Attachment #752458 -
Flags: review?(mbanner)
Comment 23•11 years ago
|
||
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?
Assignee: nobody → blog
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 24•11 years ago
|
||
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?
Assignee | ||
Comment 25•11 years ago
|
||
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:
Assignee | ||
Comment 26•11 years ago
|
||
See bug 876089
Comment 27•11 years ago
|
||
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.
Assignee | ||
Comment 28•11 years ago
|
||
Attachment #752458 -
Attachment is obsolete: true
Attachment #752458 -
Flags: review?(mbanner)
Attachment #754266 -
Flags: review?(mbanner)
Assignee | ||
Comment 29•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
Sorry, the change that removes the new error is:
- let nh = collector.getModule("test-nntp-helpers");
+ let nh = collector.getModule("nntp-helpers");
Comment 34•11 years ago
|
||
Comment on attachment 754266 [details] [diff] [review]
Add missing import to test-nntp-helpers.js
Jens, please fix the patch per comment 33.
Attachment #754266 -
Flags: review?(mbanner)
Comment 35•11 years ago
|
||
Fixed the patch for Jens as fixing tests it the priority now.
Attachment #754266 -
Attachment is obsolete: true
Attachment #767895 -
Flags: review?(mbanner)
Comment 36•11 years ago
|
||
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•11 years ago
|
||
That should result from the other bug.
This one could go in independently.
Comment 38•11 years ago
|
||
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.
Attachment #767895 -
Flags: review?(mbanner) → review+
Comment 39•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0
You need to log in
before you can comment on or make changes to this bug.
Description
•