658666 - Comm-central xpcshell-tests busted due to bug 616999 / switching xpcshell to manifests

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Mark Banner (:standard8)]

I'm just working on committing the changes for Thunderbird for porting bug 616999, I'll comment with more details of the landing and follow-ups required in a moment.

Comment 1

[Jeff Hammel]

Please ping with any additional requirements to manifestparser that you might desire

Comment 2

[Mark Banner (:standard8)]

Attachment: Partial fix - checked in.

Ok, I've done the hard work, but someone else is going to have to finish fixing this up - I'm going to be moving house for the next few days, and I won't be able to get much time until our next merge point (just 4 days!), by which time we need to have landed some other patches.

This patch:
- defines manifest files for xpcshell tests that affects Thunderbird
- gets it partially working.

Hacks:
- We copy xpcshell.ini via mail/app/Makefile, because overriding testing/xpcshell/Makfile.in is difficult, and mail/app is about the last thing the build does.
- We have to duplicate everything that is mozilla/ because of two reasons. First manifest doesn't seem to support nested includes (bug 658671), second, we probably can't share the core xpcshell.ini anyway as there's tests we don't want to run (e.g. browser).

Outstanding blocker:
- Despite the fact that the mozilla/ tests are listed in the manifest file. None of them get run even though running xpcshell-tests passes.

If we can fix that issue, then I'd be happy for this to land with a green run on try (with approximately correct xpcshell-test totals) and rs=me, and we can do the rest in follow-ups.

Other issues (can do in follow-ups):
- SeaMonkey and maybe Calendar need to patch their part of the build so that it has the relevant manifest files.
- We should fix doing make xpcshell-tests in mailnews/. This would probably be easier if nested includes worked as we could share the manifest.
- We should sort out some of our tests which are platform specific and are currently testing via what interfaces are available. Currently I believe the manifest format doesn't support doing run-if.os = xxx or skip-if.os = xxx on individual tests, but directories. I'm not sure if that's a bug or not. It would probably be annoying to have to create lots of extra directories just for OS specific tests.

Comment 3

[David :Bienvenu]

Comment on attachment 534129 [details] [diff] [review]
Partial fix - checked in.

This gets mailnews and mail (and ldap) xpcshell tests working, which Standard8 and I agreed was sufficient to re-open the tree, especially since we're getting to a branch point in 4 days. Marking r=me, for what that's worth...

Comment 4

[David :Bienvenu]

this patch didn't turn any of the tinderboxes green (in fact, the errors seem to be the same as before I landed, except for a new error on linux). Maybe I screwed up landing the patch, but the diff looks OK...

Comment 5

[David :Bienvenu]

Needless to say, I'm not re-opening the tree anytime soon.

Comment 6

[David :Bienvenu]

http://hg.mozilla.org/comm-central/rev/6591bfe0dac0 pushed for windows and linux build bustage (nomac -> notmac)

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

I see you edit config/rules.mk, do you add in build/xpccheck.py?  I am not too familiar with comm-central, but that is needed for the changes that were added to config/rules.mk.

Comment 8

[David :Bienvenu]

(In reply to comment #7)
> I see you edit config/rules.mk, do you add in build/xpccheck.py?  I am not
> too familiar with comm-central, but that is needed for the changes that were
> added to config/rules.mk.

If Standard8 changed that file, he didn't include it in his diff. I can look at that. My last change has made tinderbox a bit greener, though.

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #7)
> I see you edit config/rules.mk, do you add in build/xpccheck.py?  I am not
> too familiar with comm-central, but that is needed for the changes that were
> added to config/rules.mk.

Our version of config/rules.mk will pick up the build/xpccheck.py from the mozilla/ sub directory, i.e. the mozilla-central version of it.

Comment 10

[David :Bienvenu]

I did re-open the tree.

Comment 11

[Philip Chee]

> - We should sort out some of our tests which are platform specific and are 
> currently testing via what interfaces are available. Currently I believe the 
> manifest format doesn't support doing run-if.os = xxx or skip-if.os = xxx on 
> individual tests, but directories.

Hmm. In Bug 658928 Comment 0:

> xpcshell manifests (bug 616999) allow us to disable tests based on platform 
> and other things like whether we're in a debug build or not, e.g.:
> 
> [test_foo.js]
> skip-if.os = mac
> skip-if.config = debug

Comment 12

[Mark Banner (:standard8)]

Attachment: Actually run core tests - checked in

This gets the core xpcshell-tests running again. This worked for Thunderbird on try server, although there is one new orange in the core tests, which we can deal with separately (I'd rather get them mostly running again).

Basically, we use the core xpcshell.ini for the core tests, copied to a different location and included from our own. This works because if you're including an xpcshell-test file, then the file locations are relative to the directory you're in. Also, due to the separate build systems, there's no checks for what we're doing, so we get away with it for now at least.

We can't currently do this hack for including a mailnews xpcshell.ini file, as the build system doesn't cope correctly (see bug 658671 comment 3).

Comment 13

[Mark Banner (:standard8)]

(and I'll take the review of whoever reviews it first).

Comment 14

[Robert Kaiser]

Comment on attachment 539156 [details] [diff] [review]
Actually run core tests - checked in

Looks good to me, given you tested it. Things can only get better using that.

Comment 15

[Mark Banner (:standard8)]

I just filed bug 718408 on the central mailnews xpcshell.ini file, which covers the remaining part of this. Hence, resolving this as fixed.