896738 - Lots of TEST-UNEXPECTED-FAIL | Error console says ... in xpcshell tests

Status: RESOLVED FIXED

(Thunderbird :: Testing Infrastructure, defect)

Description

[Mark Banner (:standard8)]

Attachment: Fix strict warnings part 1

This started since bug 889911 landed. What is happening, is that our tests are checking for failures logged on the console, and that bug added a lot more output, especially with respect to strict failures.

I've started working through them, although some are m-c as well which will require a separate bug. The first batch are attached for some of the ones I'm fairly sure about.

Comment 2

[:aceman]

Comment on attachment 779438 [details] [diff] [review]
Fix strict warnings part 1

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

::: mailnews/base/search/src/nsMsgTraitService.js
@@ +119,4 @@
>    {
>      let proIndices = [];
>      let antiIndices = [];
> +    for (let id in _traits)

Please fix also the other occurence of this as said in bug 896909.

::: mailnews/test/fakeserver/imapd.js
@@ -765,5 @@
>      return this._dispatchCommand(command, args);
>    },
> -  onServerFault: function (e) {
> -    return this._tag + " BAD internal server error: " + e;
> -  },

I think you remove the wrong occurence of this function, see bug 896904. Decide if you fix it here and dupe 896904 here or drop this part from this patch.

Comment 3

[Mark Banner (:standard8)]

Attachment: [checked in] Fix strict warnings part 1 v2

With additional fixes for the areas that aceman highlighted.

Comment 5

[:aceman]

Comment on attachment 779684 [details] [diff] [review]
[checked in] Fix strict warnings part 1 v2

Nice, thanks.

Comment 6

[Mark Banner (:standard8)]

Although I did the start patch, I'm not quite sure I'm going to have time to do all of it in a short amount of time, so others should feel free to chip in.

Comment 7

[:aceman]

Error console says [stackFrame assignment to undeclared variable gCustomSearchTermSubject] - See following stack:
https://tbpl.mozilla.org/php/getParsedLog.php?id=25606003&tree=Thunderbird-Trunk#error89

probably needs to add 'var' to http://mxr.mozilla.org/comm-central/source/mail/base/test/unit/test_viewWrapper_virtualFolderCustomTerm.js#26

Comment 8

[Mark Banner (:standard8)]

Looks like it, fancy trying some patches?

Comment 9

[:aceman]

Yes, I may in some hours (if my build actually succeeds after recent breakage in m-c), just posting if anybody wants to try it sooner.

Comment 10

[:aceman]

For the "in strict mode code, functions may be declared only at top level or immediately within another function" error, I have found this suspicious function declaration:

http://hg.mozilla.org/comm-central/file/22a2433d1c66/mailnews/test/resources/messageGenerator.js#l1141

Comment 11

[Joshua Cranmer [:jcranmer]]

Comment on attachment 779684 [details] [diff] [review]
[checked in] Fix strict warnings part 1 v2

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

Ugh, we really need to just use testing-only modules for half this stuff.

::: mailnews/test/resources/messageGenerator.js
@@ +1138,5 @@
>    for (let [name, ubfunc] in Iterator(aObj)) {
>      // the variable binding needs to get captured...
>      let realFunc = ubfunc;
> +    delete aObj[name];
> +    aObj.__defineGetter__(name, function getterFunc() {

Looking at some JS docs, this should probably be something like:
Object.defineProperty(aObj, name, {
  get: function getterFunc() {
    return realFunc.bind(this);
}});

Comment 12

[:aceman]

(In reply to :aceman from comment #10)
> http://hg.mozilla.org/comm-central/file/22a2433d1c66/mailnews/test/resources/
> messageGenerator.js#l1141

Yeah and this spot seems to be handled in the patch, sorry.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 779684 [details] [diff] [review]
[checked in] Fix strict warnings part 1 v2

https://hg.mozilla.org/comm-central/rev/fc759fc258a3

Comment 14

[Mark Banner (:standard8)]

I'm currently looking at the testing-only modules stuff.

Comment 15

[:aceman]

Attachment: [checked in] declare gCustomSearchTermSubject

This fixes warning from comment 7. I could not completely verify the test as I still get many of the "function only at top level" error in this test (even after applying all the available patches).

Comment 16

[Mark Banner (:standard8)]

Attachment: Change mailDirService to be a testing module

This moves mailDirService.js to be a testing module, so that we no longer have to worry about it being included more than once.

There's a couple of other fixes in here, like adding a line to rules.mk so that check-one actually works (check interactive).

This probably doesn't fix any more tests yet - we need to do mailTestUtils.js for that as well - which does make it hard to know if we've improved anything or not, but for the files I've examined, it seems to work.

The returning and assigning gProfileDir, is because the module can't access do_get_profile, and if you can't change a variable value after its initial export from a module - you can only change the properties within it.

Comment 17

[Mark Banner (:standard8)]

Attachment: [checked in] Change mailDirService to be a testing module v2

Now without the debug print statement, and recursive remove of directories fixed.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 779919 [details] [diff] [review]
[checked in] declare gCustomSearchTermSubject

Looks good.

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 779919 [details] [diff] [review]
[checked in] declare gCustomSearchTermSubject

https://hg.mozilla.org/comm-central/rev/e3079c599ff6

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 779994 [details] [diff] [review]
[checked in] Change mailDirService to be a testing module v2

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

This makes sense to me, for the most part. I'm not currently in a position to test it though, so I'll assume you did. Thanks Mark!

::: mailnews/test/resources/mailDirService.js
@@ +7,4 @@
>   * For xpcshell tests, the "profile" directory will be <profile_dir>/mailtest/
>   */
>  
> +const EXPORTED_SYMBOLS = ["profileDir"];

Generally speaking, at least in my experience, exports are capitalized. If you're into it, I'd prefer ProfileDir.

@@ +22,4 @@
>  var Cr = Components.results;
>  var CC = Components.Constructor;
>  
> +var profileDir = {

let or const, not var.

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 779994 [details] [diff] [review]
[checked in] Change mailDirService to be a testing module v2

Yep, tested it the best I could considering we've still got many tests failing. Landed with comments addressed:

https://hg.mozilla.org/comm-central/rev/45cc1abe255a

Comment 22

[Mark Banner (:standard8)]

Attachment: WIP mailTestUtils.js switch

This is a WIP I started for mailTestUtils.js. Combined with the previous patch it was getting a bit big, so I split it out. I'm not too keen on the export of the object that replaces gLocalIncomingServer and co, but it seems to be the only way to do it.

I guess there's going to be some automated replacements coming, however, feedback welcome.

Comment 23

[:aceman]

https://tbpl.mozilla.org/php/getParsedLog.php?id=25651884&tree=Thunderbird-Trunk#error82:
Error console says [stackFrame reference to undefined property msgHdr[aKeyOrValueGetter]]

This seems caused by this interesting construct:
http://mxr.mozilla.org/comm-central/source/mailnews/base/test/unit/test_nsMsgDBView.js#345:

let valueGetter = (typeof(aKeyOrValueGetter) == "string") ?
   function(msgHdr) { return msgHdr[aKeyOrValueGetter]; } : aKeyOrValueGetter;

Comment 24

[:aceman]

Standard8, didn't the mailDirService.js change cause the xpcshell failures on Windows (https://tbpl.mozilla.org/php/getParsedLog.php?id=25653163&tree=Thunderbird-Trunk)?
Couldn't recursive remove directory: [Exception... "Component returned failure code: 0x8052000e (NS_ERROR_FILE_IS_LOCKED) [nsIFile.remove]"  nsresult: "0x8052000e (NS_ERROR_FILE_IS_LOCKED)"  location: "JS frame :: resource://testing-common/mailnews/mailDirService.js :: ProfileDir._recursiveRemove :: line 118"  data: no]Trying automatically
System JS : ERROR resource://testing-common/mailnews/mailDirService.js:92
                     NS_ERROR_FILE_IS_LOCKED: Component returned failure code: 0x8052000e (NS_ERROR_FILE_IS_LOCKED) [nsIFile.remove]
WARNING: nsExceptionService ignoring thread destruction after shutdown: file e:/builds/moz2_slave/tb-c-cen-w32-d-000000000000000/build/mozilla/xpcom/base/nsExceptionService.cpp, line 168
System JS : ERROR C:\talos-slave\test\build\xpcshell\head.js:247
                     TypeError: _fakeIdleService is undefined

Comment 25

[:aceman]

Attachment: fixes in mailnews/test/fakeserver/*

After applying the mailTestUtils patch, the tests moved forward and uncovered these problems (when running mail/base/test). I do not convert maild.js to a module yet, I just defined the functions as variables for now. So I could find the other more interesting bugs.

Comment 26

[:aceman]

Comment on attachment 780010 [details] [diff] [review]
WIP mailTestUtils.js switch

When I converted some tests use this module they started to work (after applying also the other fixes). So this does work.

Comment 27

[:aceman]

Attachment: [checked in] Declare 'msg' variable in testing/xpcshell/head.js

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 780010 [details] [diff] [review]
WIP mailTestUtils.js switch

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

Yeah, this is OK. Like I mention below, I think I'd prefer is mailTestUtils brought in a single item as opposed to a whole family of functions and items. But I guess that's just personal preference.

::: mailnews/test/resources/mailTestUtils.js
@@ +2,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +const EXPORTED_SYMBOLS = ["loadLocalMailAccount",

I suppose this is OK...I'm usually pretty freaked out by JSM's that suddenly bring in a ton of stuff (as opposed to a single object with many properties). But I guess this is for testing only, so my resolve is lessened. :)

Comment 29

[:aceman]

Comment on attachment 780010 [details] [diff] [review]
WIP mailTestUtils.js switch

It seems these imports are no longer visible to the tests including mailTestUtils.js . You would need to include them into EXPORTED_SYMBOLS.

// we would like for everyone to have fixIterator and toXPComArray
Components.utils.import("resource:///modules/iteratorUtils.jsm");
// exposes component loader's btoa impl
Components.utils.import("resource:///modules/IOUtils.js");
// JS ctypes, needed for a few native functions
Components.utils.import("resource://gre/modules/ctypes.jsm");
// Services
Components.utils.import("resource://gre/modules/Services.jsm");
// MailServices
Components.utils.import("resource:///modules/mailServices.js");

Comment 30

[:aceman]

Comment on attachment 782026 [details] [diff] [review]
[checked in] Declare 'msg' variable in testing/xpcshell/head.js

Thanks Ted.

Comment 31

[Mark Banner (:standard8)]

Comment on attachment 782018 [details] [diff] [review]
fixes in mailnews/test/fakeserver/*

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

I think this may be alright, but I'm not convinced about the maild changes, and I'd also like Joshua to take a look at the imap changes, so redirecting review.

Comment 32

[Mark Banner (:standard8)]

Comment on attachment 782026 [details] [diff] [review]
[checked in] Declare 'msg' variable in testing/xpcshell/head.js

https://hg.mozilla.org/integration/mozilla-inbound/rev/a18191f49ae6

Comment 33

[:aceman]

(In reply to Mark Banner (:standard8) from comment #31)
> I think this may be alright, but I'm not convinced about the maild changes,
> and I'd also like Joshua to take a look at the imap changes, so redirecting
> review.

Yes, I heard he is converting those fakeserver files to modules so then these changes are not needed. I can drop those from the patch and keep only the changes I do inside functions (undeclared variables and access to non-existent properties)

Comment 34

[Mark Banner (:standard8)]

Comment on attachment 782026 [details] [diff] [review]
[checked in] Declare 'msg' variable in testing/xpcshell/head.js

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 889911
User impact if declined: None
Testing completed (on m-c, etc.): landed on inbound
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

This regressed because bug 889911 was block-approved as part of another bug to land on aurora. Unfortunately, that causes Thunderbird many strict errors and warnings, which we are now tidying up. This patch tides up javascript strict errors in mozilla-central.

Comment 35

[(not currently active) Ted Mielczarek]

I think you can just land this with a=test-only.

Comment 36

[Joshua Cranmer [:jcranmer]]

(In reply to :aceman from comment #33)
> (In reply to Mark Banner (:standard8) from comment #31)
> > I think this may be alright, but I'm not convinced about the maild changes,
> > and I'd also like Joshua to take a look at the imap changes, so redirecting
> > review.
> 
> Yes, I heard he is converting those fakeserver files to modules so then
> these changes are not needed. I can drop those from the patch and keep only
> the changes I do inside functions (undeclared variables and access to
> non-existent properties)

That I filed as bug 900519.

Comment 37

[Mark Banner (:standard8)]

(In reply to :aceman from comment #29)
> Comment on attachment 780010 [details] [diff] [review]
> WIP mailTestUtils.js switch
> 
> It seems these imports are no longer visible to the tests including
> mailTestUtils.js . You would need to include them into EXPORTED_SYMBOLS.

We should just include them where necessary in the head files. Many of them are already imported where necessary.

Comment 38

[Mark Banner (:standard8)]

Attachment: Load*ToString moves in mailTestUtils.js (diff -w)

Ok, I'm splitting this up into pieces, based on previous suggestions. If we export just one symbol - mailTestUtils, and have everything a sub-part of that, we can progressively move towards a state where we have something ready for a sub-module (as a side effect, we actually manage to start getting rid of some of the warnings as they land).

This first section removes the unnecessary atob and btoa suggestions, and moves loadFileToString and loadMessageToString to a module.

Although loadFileToString is now available in IOUtils, I've left it in for the one case where we need to specify the charset, that IOUtils doesn't support.

Comment 39

[Mark Banner (:standard8)]

Attachment: [checked in] Load*ToString moves in mailTestUtils.js (normal)

Comment 40

[Joshua Cranmer [:jcranmer]]

Comment on attachment 782018 [details] [diff] [review]
fixes in mailnews/test/fakeserver/*

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

Drop the maild.js changes altogether.

::: mailnews/test/fakeserver/imapd.js
@@ +948,5 @@
>  
>      var func = this._kAuthSchemeStartFunction[scheme];
>      if (!func || typeof(func) != "function")
>        return "BAD I just pretended to implement AUTH " + scheme + ", but I don't";
> +    return func.call(this, (args.length > 1) ? args[1] : null);

Hmm... func.apply(this, args.slice(1)), if I remember my JS syntax correctly?

@@ +1077,5 @@
>        }
>      }
>      // check for optional list return options argument used by LIST-EXTENDED
>      // and other related RFCs
> +    if ((args.length > 2 && args[2] == "RETURN") ||

The LIST code has really gotten out of control, but I think this should work.

Comment 41

[Mark Banner (:standard8)]

Attachment: [checked in] firstMsgHdr moves into mailTestUtils

Moving firstMsgHdr into mailTestUtils object

Comment 42

[Joshua Cranmer [:jcranmer]]

Comment on attachment 784429 [details] [diff] [review]
Load*ToString moves in mailTestUtils.js (diff -w)

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

I was about to ask you to reindent the source code until I realized this was a diff -w :-)

Comment 43

[Mark Banner (:standard8)]

Attachment: [checked in] Move large file related functions into mailTestUtils

Comment 44

[Mark Banner (:standard8)]

Attachment: [checked in] Moves other misc functions to mailTestUtils

This completes all the functions, apart from those relating to the local mail folder & server setup.

Comment 45

[Joshua Cranmer [:jcranmer]]

Comment on attachment 784451 [details] [diff] [review]
[checked in] firstMsgHdr moves into mailTestUtils

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

I get the feeling, looking at all these changes, that we need a msgTestUtils.loadFirstMessageToString(folder) helper.

::: mailnews/compose/test/unit/head_compose.js
@@ -171,5 @@
> -  let enumerator = folder.msgDatabase.EnumerateMessages();
> -  if (enumerator.hasMoreElements())
> -    return enumerator.getNext().QueryInterface(Ci.nsIMsgDBHdr);
> -  return null;
> -}

Huh.

Comment 46

[Joshua Cranmer [:jcranmer]]

Comment on attachment 784455 [details] [diff] [review]
[checked in] Move large file related functions into mailTestUtils

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

I didn't look over this too carefully, so I'm assuming that a diff -w shows minimal changes.

::: mailnews/test/resources/mailTestUtils.js
@@ +239,4 @@
>      // Win32 type and other constants.
>      const BOOL = ctypes.int32_t;
> +    const MAX_PATH = 260;
> +  

Trailing whitespace.

@@ +410,5 @@
> +                               bytesReturned.address(), null)) {
> +            throw new Error("Unable to mark file as sparse, error " +
> +                            ctypes.winLastError);
> +          }
> +        

Trailing whitespace.

Comment 47

[Mark Banner (:standard8)]

Attachment: Make mailTestUtils into a module; move local account functions to a separate file

This makes mailTestUtils.js its own module, and moves the local account functions out to a separate file. Logically, I think this makes more sense, especially to help with maintenance.

Its possible I've missed adding some imports around the place, or that I've broken a couple of tests. I think with the general state of things, we'll land this and do clean up following it, as it'll be easier for more folks to look at the errors we've got outstanding.

Comment 48

[:aceman]

Attachment: [checked in] small JS-strict fixes in mailnews/test/fakeserver/* v2 [ready for checkin]

Thanks, carrying over r=jcranmer.

Comment 49

[Mark Banner (:standard8)]

Attachment: [checked in] Make mailTestUtils into a module; move local account functions to a separate file v2

Now with localAccountUtils.js

Comment 50

[Joshua Cranmer [:jcranmer]]

Comment on attachment 784518 [details] [diff] [review]
Make mailTestUtils into a module; move local account functions to a separate file

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

Hmm, can you add the --testing-modules line to mailnews/Makefile.in's xpcshell-tests run?

::: mailnews/compose/test/unit/head_compose.js
@@ -17,5 @@
>  
>  // Import the smtp server scripts
> -load("../../../fakeserver/maild.js")
> -load("../../../fakeserver/auth.js")
> -load("../../../fakeserver/smtpd.js")

This is going to bitrot hurt :-)

::: mailnews/news/test/unit/head_server_setup.js
@@ +15,4 @@
>  load("../../../fakeserver/nntpd.js");
>  
>  // Generic mailnews resource scripts
> +load("../../../resources/localAccountUtils.js");

Does NNTP actually need these?

Comment 51

[Joshua Cranmer [:jcranmer]]

Comment on attachment 784544 [details] [diff] [review]
[checked in] Make mailTestUtils into a module; move local account functions to a separate file v2

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

::: mailnews/test/resources/localAccountUtils.js
@@ +19,5 @@
> +{
> +  // This function is idempotent
> +  if (_localAccountInitialized)
> +    return;
> +  

Trailing whitespace :)

Comment 52

[Mark Banner (:standard8)]

Comment on attachment 784430 [details] [diff] [review]
[checked in] Load*ToString moves in mailTestUtils.js (normal)

https://hg.mozilla.org/comm-central/rev/b7cfccc838e8

Comment 53

[Mark Banner (:standard8)]

Comment on attachment 784451 [details] [diff] [review]
[checked in] firstMsgHdr moves into mailTestUtils

https://hg.mozilla.org/comm-central/rev/5a2676f7a59e

Comment 54

[Mark Banner (:standard8)]

Comment on attachment 784455 [details] [diff] [review]
[checked in] Move large file related functions into mailTestUtils

https://hg.mozilla.org/comm-central/rev/2482c00d7ebc

Comment 55

[Mark Banner (:standard8)]

Comment on attachment 784469 [details] [diff] [review]
[checked in] Moves other misc functions to mailTestUtils

https://hg.mozilla.org/comm-central/rev/b26a399e5ded

Comment 56

[Mark Banner (:standard8)]

Comment on attachment 784544 [details] [diff] [review]
[checked in] Make mailTestUtils into a module; move local account functions to a separate file v2

https://hg.mozilla.org/comm-central/rev/3d83e962924d

Comment 57

[Mark Banner (:standard8)]

Comment on attachment 784531 [details] [diff] [review]
[checked in] small JS-strict fixes in mailnews/test/fakeserver/* v2 [ready for checkin]

https://hg.mozilla.org/comm-central/rev/65c33aac5dfb

Comment 58

[Mark Banner (:standard8)]

Landed a follow-up to fix compose tests:

https://hg.mozilla.org/comm-central/rev/7cbfcea11dc9

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a18191f49ae6

Comment 60

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #58)
> Landed a follow-up to fix compose tests:
> 
> https://hg.mozilla.org/comm-central/rev/7cbfcea11dc9

Got mailnews xpcshell working, and found a few more bits that were missed:

https://hg.mozilla.org/comm-central/rev/f7cbf04c3704

Comment 61

[Mark Banner (:standard8)]

Attachment: [checked in] Fix more issues

This fixes more issues generally that I picked up just by scanning the logs. It fixes 12 tests on my machine, and sets up a bunch of others (imap I think) ready for passing once we fix the imap pump.

Comment 62

[Joshua Cranmer [:jcranmer]]

Comment on attachment 785038 [details] [diff] [review]
[checked in] Fix more issues

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

::: mailnews/test/fakeserver/imapd.js
@@ +333,4 @@
>    this.uid = uid;
>    this.size = 0;
>    this.flags = new Array;
> +  for each (let flag in flags)

Change this to for (let flag of flags) ?

Comment 63

[Mark Banner (:standard8)]

Comment on attachment 785038 [details] [diff] [review]
[checked in] Fix more issues

https://hg.mozilla.org/comm-central/rev/099f9cfcdec5

Comment 64

[Joshua Cranmer [:jcranmer]]

Attachment: [checked-in] Fix all but 2 IMAP tests on Linux

Comment 65

[Joshua Cranmer [:jcranmer]]

Attachment: [checked in] Fix some base and MIME tests

The imapd.js changes are basically moving a custom parser to use the main IMAP parser instead.

Comment 66

[Joshua Cranmer [:jcranmer]]

Comment on attachment 785157 [details] [diff] [review]
[checked-in] Fix all but 2 IMAP tests on Linux

remote:   https://hg.mozilla.org/comm-central/rev/4006332617d1

Comment 67

[Joshua Cranmer [:jcranmer]]

Comment on attachment 785185 [details] [diff] [review]
[checked in] Fix some base and MIME tests

remote:   https://hg.mozilla.org/comm-central/rev/e7c9fcd02462

Comment 68

[Mark Banner (:standard8)]

Apart from one possible remaining issue on bug 904162, this is fixed now. Thanks to all those that have been helping.