482768 - xpcshell tests broken on trunk due to mozilla-central changes

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Mark Banner (:standard8)]

Bug 482084 changed how xpcshell is run/generated, we need to find a way to sync comm-central to those changes for trunk builds.

At the moment we haven't got coverage for mailnews/ tests in comm-central trunk, however as we have coverage on 1.9.1 for those tests I'm not too concerned at the moment.

Comment 1

[(not currently active) Ted Mielczarek]

As I suggested in #developers, you just need to fix rules.mk to use the new harness. However, since you want to be compat with 1.9.1 and trunk, you could, for the time being, just import m-c/testing/xpcshell into c-c and change your rules.mk to reference that. That way you will be able to ignore changes until everything lands on 1.9.1, then you can remove the imported code in c-c and switch back.

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Support both shell/1.9.1 and python/1.9.2 [Checkin: Comment 4]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090312 SeaMonkey/2.0b1pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/6f6d47027297
 +http://hg.mozilla.org/comm-central/rev/be064d80219d)

I copied m-c lines, then did s/topsrcdir/MOZILLA_SRCDIR/g on them.

I didn't build with this patch, but |make check| works yet.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 367155 [details] [diff] [review]
(Av1) Support both shell/1.9.1 and python/1.9.2
[Checkin: Comment 4]

This is probably just delaying the inevitable but it'll do for now. Thanks for doing this.

r=Standard8

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 367155 [details] [diff] [review]
(Av1) Support both shell/1.9.1 and python/1.9.2
[Checkin: Comment 4]


http://hg.mozilla.org/comm-central/rev/bd9dc914d935

Comment 5

[(not currently active) Ted Mielczarek]

I probably just broke you again since I landed a few other things. I can just try to get everything landed on 1.9.1 ASAP so you're back to normal, if that's cool with you.

Comment 6

[Serge Gautherie (:sgautherie)]

c-c/1.9.1 does not seem broken atm.
c-c/1.9.2 (= TB/1.9.2 boxes at least) does.

Iiuc, I'd like you to land on 1.9.1 everything till your last changes (= bug 482085 and probably bug 421611) asap.
Then wait (a little) before landing the latters till we are ready to handle them.
Braking c-c/1.9.1 would not be good.

On a general note, I do look forward to having/keeping the same (improved) test infrastructure on 1.9.1 as on 1.9.2 :-)

Comment 7

[Serge Gautherie (:sgautherie)]

Confirming that SM/1.9.2 has the same failure as TB/1.9.2.

Working on it...

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Port bug 482085 to c-c

This synchronizes rules.mk only.

helpwanted:
(All) The "mailnews" tests need to be updated.
And at least mailnews/test/resources/* need to be explicitly copied to the objdir.

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Port bug 421611 to c-c

This synchronizes rules.mk only.

helpwanted:
I don't know if anything else needs to be updated too. (probably ?)

***

PS:
Patches Bv1 and Cv1 could land now, I guess.
But, obviously, landing the (to-be-)updated tests will have to be synchronized with harness landing to m-1.9.1. (See comment 6)

Comment 10

[(not currently active) Ted Mielczarek]

You can't land the latter one of those without the harness changes, as it stops passing the srcdir parameter to runxpcshelltests.py, which would break things. For the test changes, it was time consuming for me, but not very hard. You can see the exhaustive list of changes here:
http://hg.mozilla.org/mozilla-central/rev/a17552ca8f97
and here:
http://hg.mozilla.org/mozilla-central/rev/99930ad81298

They basically break down to:
* removing all instances of do_import_script, either replacing them with load('relative/path.js') or do_load_httpd_js() to load that specific script
* fixing all instances of do_get_file() to use relative paths
* as part of the above, moving any files referenced by the tests to be inside the XPCSHELL_TESTS dirs, so they'll be copied to the test dir in the objdir. You'll also note a little bit of Makefile fiddling to copy some files to the test dirs from other places, like:
http://hg.mozilla.org/mozilla-central/diff/a17552ca8f97/netwerk/test/Makefile.in
http://hg.mozilla.org/mozilla-central/diff/a17552ca8f97/toolkit/mozapps/extensions/test/Makefile.in

Once those were done, and "make check" passed, I then tried running the tests from a packaged build, and hit a few other issues that needed to be fixed, like the intl/strres/tests tests that were loading a .properties file via resource: URL. The .properties files were not packaged with the build, so I changed them to instead be inside the unit dir and be loaded via a file: URL:
http://hg.mozilla.org/mozilla-central/diff/99930ad81298/intl/strres/tests/Makefile.in
http://hg.mozilla.org/mozilla-central/diff/99930ad81298/intl/strres/tests/unit/test_bug378839.js

There were also a few tests that relied on finding test binaries from the app dir, which weren't in the packaged build. I added some makefile rules to install the binaries to the test dir:
http://hg.mozilla.org/mozilla-central/diff/99930ad81298/uriloader/exthandler/tests/Makefile.in
http://hg.mozilla.org/mozilla-central/diff/99930ad81298/xpcom/tests/Makefile.in

and fixed the tests accordingly:
http://hg.mozilla.org/mozilla-central/diff/99930ad81298/uriloader/exthandler/tests/unit/test_punycodeURIs.js
http://hg.mozilla.org/mozilla-central/diff/99930ad81298/xpcom/tests/unit/test_nsIProcess.js

jcranmer mentioned on IRC that he thought that fakeserver might be a problem for c-c unit tests, since many tests expect to be able to load that file from various directories. I think we could hack the harness to provide a do_load_fakeserver_js() just like the do_load_httpd_js() just to simplify tests.

I'd rather not have to fix all the c-c unit tests myself, so hopefully someone else can do this, but I'm certainly happy to help out with advice. I won't land the harness changes on 1.9.1 without making sure that c-c is ready, however, so hopefully we can get this together quickly.

Comment 11

[Mark Banner (:standard8)]

Taking...

Comment 12

[Mark Banner (:standard8)]

Attachment: Partial Patch

WIP patch. This makes mailnews/base and mailnews/imap pass "make check". It touches a few other places, but I haven't fixed those in full yet.

The general idea is:

- Put items from mailnews/test/ into <objdir>/mozilla/_tests/xpcshell/mailnews
- Move resource scripts to mailnews/test/resources where they are shared by more than one test-subsystem.
- Adjust paths to compenstate.

The disadvantage of putting the mailnews/test files into <objdir>/mozilla/_tests/xpcshell/mailnews is that to load them our tests need to load from the relative directory ../../mailnews/data. I think we've got several options here:

a) live with it
b) copy mailnews/test into each of the xpcshell/$(MODULE)/unit directories
c) reduce mailnews unit test modules to just one directory in the objdir, i.e. leave the files where they are, but all tests go into xpcshell/test_mailnews/unit or something like that

I'm sort of thinking any of these is a possibility and I'm not sure which I like most, though a is probably not the best option.

Thoughts?

Comment 13

[(not currently active) Ted Mielczarek]

I would probably suggest going with a) for now, as it seems like the lowest effort. Once we have everything in place and working you can always refactor later. I don't think that copying common files around a lot is really harmful here, as long as there's no manual work involved, but. In any case, I think you should optimize for making it as easy as possible to write new tests.

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to comment #12)
> a) live with it

Maybe "overload" load() and do_get_file() +/- like Ted's do_load_httpd_js() ?

> b) copy mailnews/test into each of the xpcshell/$(MODULE)/unit directories

Don't like too much duplicating files.

> c) reduce mailnews unit test modules to just one directory in the objdir, i.e.
> leave the files where they are, but all tests go into
> xpcshell/test_mailnews/unit or something like that

Wouldn't want this, as it's handy to be able to run one test directory only.

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #14)
> (In reply to comment #12)
> > a) live with it
> 
> Maybe "overload" load() and do_get_file() +/- like Ted's do_load_httpd_js() ?

Yeah I expect this is what we'll do, though maybe not in quite the same way. The short term priority is to fix this bug though and I'll think about an improvement later.

> > c) reduce mailnews unit test modules to just one directory in the objdir, i.e.
> > leave the files where they are, but all tests go into
> > xpcshell/test_mailnews/unit or something like that
> 
> Wouldn't want this, as it's handy to be able to run one test directory only.

That's a good point, I'd forgotten about that.

Comment 16

[Mark Banner (:standard8)]

Attachment: The fix

This patch now does all the necessary changes to make mailnews/ pass with the new xpcshell test system. I have checked it on comm-central with mozilla-central, I'm assuming that when we get the core patches on mozilla-1.9.1 it will work there as well.

Most of the patch is adjusting paths. There are some removals of 1.9.1 versus trunk sections. I've also moved some directories/files where it seems to make sense, i.e. <module>/test/resources to <module>/test/unit/resources so that the files get copied as part of the xpcshell test installation process, rather than needed a separate makefile step.

When it comes to landing, I'll tie up with Ted on irc so we can land patches at the same time.

I'll do any bustage fixes as necessary as I wouldn't be surprised if there are some due to newly added tests etc (depending on how long it takes us to land this).

Comment 17

[Robert Kaiser]

Comment on attachment 369244 [details] [diff] [review]
The fix

> endef # do not remove the blank line!

That line sounds interesting, curious what it is ;-)

In any case, we should try to find something that abstracts away the "../../mailnews/", but that can be done in a followup.

I trust you tested it works, I have no trunk tree that can run tests right now.

r=me by code inspection.

Comment 18

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/90ad0990c8a6

This has improved the comm-central + trunk situation (c-c + 1.9.1 still passes).

I'll file follow up bugs and sort out the other issues in a few hours.