559681 - [Windows, Packaged] xpcshell-tests: "Exception / NS_ERROR_FILE_NOT_FOUND / nsIProcess.init / test_largeOfflineStore.js :: run_test :: line 59". LargeOfflineStoreHelper.exe is not packaged

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1271363022.1271368276.2278.gz&fulltext=1
WINNT 5.2 comm-central-trunk debug test xpcshell on 2010/04/15 13:23:42
{
TEST-UNEXPECTED-FAIL | e:\builds\slave\comm-central-trunk-win32-debug-unittest-xpcshell\build\xpcshell\tests\test_imap\unit\test_largeOfflineStore.js | test failed (with xpcshell return code: 0), see following log:

WARNING: No valid default account found, just using first (FIXME): file e:/builds/slave/comm-central-trunk-win32-debug/build/mailnews/base/src/nsMsgAccountManager.cpp, line 772

TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIProcess.init]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: e:/builds/slave/comm-central-trunk-win32-debug-unittest-xpcshell/build/xpcshell/tests/test_imap/unit/test_largeOfflineStore.js :: run_test :: line 59"  data: no]
}

Comment 1

[Serge Gautherie (:sgautherie)]

Code is
{
56     helper.append("LargeOfflineStoreHelper.exe");
57     let helperProc = Cc["@mozilla.org/process/util;1"]
58                        .createInstance(Ci.nsIProcess);
59     helperProc.init(helper);
}

I wonder if the cause would be that LargeOfflineStoreHelper.exe is not packaged?
(Which is already an issue with some other test resources...)

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)

Until the more generic issue is fixed, my advise would be to postpone the packaging part and just add a try+catch with a todo().

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

If I'm reading this correctly, it's something that's breaking SeaMonkey tinderbox, but not Thunderbird.  Is that true?  If so, it's not clear to me why this would block Thunderbird 3.1.  Can you elaborate?

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)

> that's breaking SeaMonkey tinderbox, but not Thunderbird.  Is that true?

True, probably because of a 'bug 541235'-like issue.

> If so, it's not clear to me why
> this would block Thunderbird 3.1.  Can you elaborate?

Because this bug is a perma-orange but there's no 'MailNews Core' blocking flag...

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

OK, that's helpful.  I agree that there's a problem here, but I don't believe that the blocking-thunderbird3.1 flag is the right hammer for this nail. Generally speaking (from the experience of having tried to use them this way in the past), I've come to the conclusion that blocking flags rarely good hammers for test-only bugs, no matter what the product, because they just don't represent reality: if push comes to shove, the incentives for shipping pretty much always overwhelm the intent to block on things like this.

Now, that said, if we had reason to believe that this test failure represented a real bug that's likely to hurt a significant number of users, I think that would change the situation.

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)

> blocking flags rarely good hammers for test-only bugs

What is the good hammer then?

> a real bug that's likely to hurt a significant number of users

Perma-orange is hurting (== continuously loosing much time and missing other issues) all folks to which test suites matter...

Comment 7

[Rain [:rain1] (inactive)]

Well, why don't you package the file, then? I'd not be in favour of a try-catch just in case it breaks in Thunderbird in the future and we don't see it for a while.

Comment 8

[Mark Banner (:standard8)]

(In reply to comment #6)
> (In reply to comment #5)
> 
> > blocking flags rarely good hammers for test-only bugs
> 
> What is the good hammer then?

Filing a bug, getting a clear issue of the actual failure and cc'ing the right people on it.

> > a real bug that's likely to hurt a significant number of users
> 
> Perma-orange is hurting (== continuously loosing much time and missing other
> issues) all folks to which test suites matter...

Yes, but in this bug it is clear what the issue is. The issue is that SeaMonkey is using packaged-tests and obviously the packaging of those tests is wrong (which you have already mentioned in comments 1 and 2).

Thunderbird doesn't run its tests in packaged style *yet*. So you can't necessarily expect Thunderbird devs to be concerned by a test-only bug that affects a configuration that Thunderbird doesn't run.

That doesn't mean to say we won't help, just that it is very hard to care about an issue that doesn't affect us at this moment in time.

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #2)
> (In reply to comment #1)
> 
> Until the more generic issue is fixed, my advise would be to postpone the
> packaging part and just add a try+catch with a todo().

Have you actually looked into the packaging issue?

I couldn't remember/work out how Firefox set up its packaged tests, so I downloaded one of the test packages and unpacked it.

A quick search for *.exe revealed some in the bin/ directory, some in the xpcshell/tests directories... finding those quickly pointed me at:

http://hg.mozilla.org/mozilla-central/annotate/c4bf13767790/xpcom/tests/Makefile.in#l169
and
http://hg.mozilla.org/mozilla-central/annotate/c4bf13767790/uriloader/exthandler/tests/Makefile.in#l72

so that's how Firefox does it, and the only thing I'd be concerned about is if the running the unit test in SeaMonkey would pick up the executable from the right place, looking at the history associated with the exthandler change there may be some test changes required, though I've not looked at the test code:

http://hg.mozilla.org/mozilla-central/rev/99930ad81298

Comment 10

[Serge Gautherie (:sgautherie)]

I confirmed locally that adding LargeOfflineStoreHelper.exe in the application directory fixes this error.
(Then I got another error, but we'll just ignore it ftb.)


(In reply to comment #7)
> Well, why don't you package the file, then?

Good question: why don't you?


(In reply to comment #9)

> Have you actually looked into the packaging issue?

What do you mean?

> A quick search for *.exe revealed some in the bin/ directory, some in the
> xpcshell/tests directories...

I know: that's why I was referring to bug 490223 and the like.

> the only thing I'd be concerned about is if the running the unit test
> in SeaMonkey would pick up the executable from the right place,

That would be the first issue:
test_largeOfflineStore.js is packaged from mailnews\imap\test\unit into test_imap\unit\,
but LargeOfflineStoreHelper.cpp is built from mailnews\test into mozilla\dist\bin,
and the Makefile.in there has no MODULE=....
I assume this setup should be fixed so the test loads the executable from its/a test directory.

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> > What is the good hammer then?
> 
> Filing a bug, getting a clear issue of the actual failure and cc'ing the right
> people on it.

I'm not convinced yet: bug 541235 had no actual solution (checked in) for 3 months, for example.

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> you can't
> necessarily expect Thunderbird devs to be concerned by a test-only bug that
> affects a configuration that Thunderbird doesn't run.

TB people don't fix packaging issues because TB doesn't run packaged tests,
but TB can't run packaged tests because the packaging is broken :-/
How do we get out of this situation?

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Copy Windows helper executable to xpcshell(-tests) directory, Use a cleanup callback, Unify dump()/do_throw() calls

(In reply to comment #9)
> http://hg.mozilla.org/mozilla-central/annotate/c4bf13767790/uriloader/exthandler/tests/Makefile.in#l72

Thanks! That's the right way to do it :-)

NB: I don't know why LargeOfflineStoreHelper.cpp is not in mailnews\imap\test, but I don't care atm.

Comment 14

[Rain [:rain1] (inactive)]

(In reply to comment #13)
> Created an attachment (id=439746) [details]
> (Av1) Copy Windows helper executable to xpcshell(-tests) directory, Use a
> cleanup callback, Unify dump()/do_throw() calls
> 
> (In reply to comment #9)
> > http://hg.mozilla.org/mozilla-central/annotate/c4bf13767790/uriloader/exthandler/tests/Makefile.in#l72
> 
> Thanks! That's the right way to do it :-)
> 
> NB: I don't know why LargeOfflineStoreHelper.cpp is not in mailnews\imap\test,
> but I don't care atm.

Because the plan is to use it for > 4GB POP tests too.

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)
> Because the plan is to use it for > 4GB POP tests too.

Then, I guess I should rather copy it to "xpcshell\tests\mailnews\".
What do you think?

Comment 16

[Rain [:rain1] (inactive)]

Yes, that sounds better.

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) Copy Windows helper executable to xpcshell(-tests) directory, Use a cleanup callback, Unify dump()/do_throw() calls

Av1, with comment 15 suggestion(s).

*****

I noticed locally this test randomly leaks:
nsAStreamCopier, nsPipe, nsRecyclingAllocatorImpl, nsSocketTransport, nsSocketTransportService, nsStringBuffer, nsThread, nsTimerImpl.

And sometimes, it even leaks the followings too:
nsDNSRecord, nsHostRecord.

Could it be caused by my changes? I didn't check the tinderbox logs...
That should be investigated later. Ftb, leaks don't cause failure.

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to comment #17)
> I noticed locally this test randomly leaks:

Oh, I can reproduce without my patch!
Is this random leak already filed?

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 439821 [details] [diff] [review]
(Av1a) Copy Windows helper executable to xpcshell(-tests) directory, Use a cleanup callback, Unify dump()/do_throw() calls


>+  do_register_cleanup(cleanupAfterTest);

If we are going to do this, please do this in a separate bug. If we're changing the style of how we do cleanup in these tests then we should do them all at once (or close as possible) and advertise the fact to developers. Doing it piecemeal will just lead to confusion.

>     if (exitValue == 1)
>     {
>+      do_check_false(false);
>       dump("On Windows, this test only works on NTFS volumes.\n");
>-      endTest();
>       return;

I still have no idea why do_check_false(false) is required. I seem to remember it is about some bug somewhere. However, again, please do not confuse bugs. If we need it, then we need to update everywhere at the same time and explain why/what for.

Assuming it is something like every test must have one check function call in it, then I'd also assert that the test harness should give us something like do_empty_test() and/or do_todo_test() - the first would just say "yes I did mean this to be an empty test" the second would give us todo functionality in unit tests. Checking false == false is bound to confuse devs and have them wondering why their tests are failing without it.

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #17)
> I noticed locally this test randomly leaks:

I filed bug 560420.

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)

> >+  do_register_cleanup(cleanupAfterTest);

I filed bug 560434.

> >+      do_check_false(false);
> 
> I still have no idea why do_check_false(false) is required.

See bug 496443.

> do_empty_test() and/or do_todo_test()

That's bug 468196. Feel free to file a followup bug if that one is not enough.

Comment 22

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Copy Windows helper executable to xpcshell(-tests) directory, Unify dump()/do_throw() calls [Checkin: Comment 23]

Av1a, with comment 19 suggestion(s).

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 440119 [details] [diff] [review]
(Av2) Copy Windows helper executable to xpcshell(-tests) directory, Unify dump()/do_throw() calls
[Checkin: Comment 23]


http://hg.mozilla.org/comm-central/rev/3bea8332b591

Comment 24

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1271787422.1271792082.14699.gz
WINNT 5.2 comm-central-trunk debug test xpcshell on 2010/04/20 11:17:02

V.Fixef