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

RESOLVED FIXED in Thunderbird 5.0b1

Status

MailNews Core
Build Config
--
blocker
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
Thunderbird 5.0b1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

7 years ago
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

7 years ago
Please ping with any additional requirements to manifestparser that you might desire
(Assignee)

Updated

7 years ago
Depends on: 658671
(Assignee)

Updated

7 years ago
Severity: normal → blocker
Summary: Tidy up port of bug 616999 / switching of xpcshell to manifests → Comm-central xpcshell-tests busted due to bug 616999 / switching xpcshell to manifests
(Assignee)

Comment 2

7 years ago
Created attachment 534129 [details] [diff] [review]
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

7 years ago
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...
Attachment #534129 - Attachment description: Partial fix → Partial fix - checked in.
Attachment #534129 - Flags: review+

Comment 4

7 years ago
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

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

Comment 6

7 years ago
http://hg.mozilla.org/comm-central/rev/6591bfe0dac0 pushed for windows and linux build bustage (nomac -> notmac)
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

7 years ago
(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.
(Assignee)

Comment 9

7 years ago
(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

7 years ago
I did re-open the tree.

Updated

7 years ago
Blocks: 658878

Comment 11

7 years ago
> - 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
(Assignee)

Updated

7 years ago
Assignee: nobody → dbienvenu
Target Milestone: --- → Thunderbird 3.3a4
(Assignee)

Comment 12

7 years ago
Created attachment 539156 [details] [diff] [review]
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).
Attachment #539156 - Flags: review?(kairo)
Attachment #539156 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 13

7 years ago
(and I'll take the review of whoever reviews it first).
(Assignee)

Updated

7 years ago
Assignee: dbienvenu → mbanner

Comment 14

7 years ago
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.
Attachment #539156 - Flags: review?(kairo) → review+
(Assignee)

Updated

7 years ago
Attachment #539156 - Flags: review?(bugspam.Callek)
(Assignee)

Updated

7 years ago
Attachment #539156 - Attachment description: Actually run core tests → Actually run core tests - checked in
(Assignee)

Comment 15

6 years ago
I just filed bug 718408 on the central mailnews xpcshell.ini file, which covers the remaining part of this. Hence, resolving this as fixed.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
No longer depends on: 658671
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.