491376 - Restore make -C mailnews/ check/xpcshell-tests functionality to mailnews.

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Mark Banner (:standard8)]

Attachment: The fix

When I reviewed bug 485736 I didn't fully realise the implications of the patch. It changed the build system so that you can only do "make xpcshell-tests" at the top-level (where it uses a manifest) or in a "test" dir, e.g. mailnews/addrbook/tests.

This means the old make -C mailnews check (which would now be make -C mailnews xpcshell-tests) functionality is broken. This is especially painful for mailnews developers when we don't touch core.

A discussion with Ted on irc tells me the change was intentional (because of the difference between the top-level manifest and running dirs), so our best option is to provide a xpcshell-tests target in mailnews/Makefile.in.

The added bonus with this is that we can add mailnews/base back in, such that developers only have to run in mailnews/ rather than the previous mailnews/base and mailnews/.

I'd like to get quick review on this if possible as its already hurting devs.

Comment 1

[Mark Banner (:standard8)]

Robert told me over irc that:

TEST_PATH=mailnews make xpcshell-tests

should work. However, on 1.9.1 this runs all tests and on trunk, this doesn't run any tests.

Comment 2

[Robert Kaiser]

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

Hmm, this looks hackish to me, I'd prefer a more general solution on the global xpcshell-tests level, if possible. We can take that workaround in the meantime but it shouldn't be long-lived...

Comment 3

[Mark Banner (:standard8)]

I found a way to make TEST_PATH work:

TEST_PATH=test_addbook/unit make xpcshell-tests

So this requires both knowledge of TEST_PATH and the individual test output directories in the objdir. TEST_PATH is what the path ends with. Whilst we could put them all into test_mailnews/unit I think that's not too logical.

I wonder if it would be acceptable to make TEST_PATH be matched on part of a path, so that we could just do TEST_PATH=mailnews and then put all our mailnews tests into test_mailnews*/unit.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> TEST_PATH=mailnews make xpcshell-tests
> However, on 1.9.1 this runs all tests and on trunk, this doesn't
> run any tests.

1.9.1: Expected, as you tested before the "TEST_PATH" patch landed...

1.9.2: Correct, as 'mailnews' doesn't match
|if testPath and not testdir.endswith(testPath):|


(In reply to comment #2)
> (From update of attachment 375702 [details] [diff] [review])
> Hmm, this looks hackish to me

Given the current Core target/code, this patch seems correct,
except I would suggest to use a list of dirs and a 'for' loop.

Another way to do it would be to create a "mailnews" manifest then run it with:
|make EXTRA_TEST_ARGS=--manifest=./_tests/xpcshell/mailnews.list -C objdir xpcshell-tests|
(or the like...).


(In reply to comment #3)
> I wonder if it would be acceptable to make TEST_PATH be matched on part of a
> path, so that we could just do TEST_PATH=mailnews and then put all our mailnews
> tests into test_mailnews*/unit.

I guess we could (even go all the way and) match against a regex instead of using endswith(), though I'm not sure that we want/need to do that (at all) :-|

Comment 5

[Robert Kaiser]

(In reply to comment #4)
> 1.9.2: Correct, as 'mailnews' doesn't match
> |if testPath and not testdir.endswith(testPath):|

Ugh. It always should test for the directory (relative to topsrcdir) _starting_ with the given path, not _ending_ in it, that sounds really crazy.

> (In reply to comment #2)
> > (From update of attachment 375702 [details] [diff] [review] [details])
> > Hmm, this looks hackish to me
> 
> Given the current Core target/code, this patch seems correct,
> except I would suggest to use a list of dirs and a 'for' loop.

Makefile don't have loops, AFAIK. We're not based on pymake yet, where we probably could use the still crappy but somewhat better language of python instead of (GNU) make syntax.

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)

> It always should test for the directory (relative to topsrcdir) _starting_
> with the given path, not _ending_ in it, that sounds really crazy.

This (bug 485736) code is mine and is (only) a more flexible version of the initial (bug 482084) python code from Ted.
I was simply explaining what the current situation is.

I'm not sure what was the exact behavior of the previous shell scripts.
If you think there is a regression or that this behavior should be changed, you can file a bug about it...

> Makefile don't have loops, AFAIK.

Isn't 'for' provided at the shell level?
As in
http://mxr.mozilla.org/mozilla-central/search?string=++for+&regexp=on&case=on&find=%2FMakefile%5C.in%24
for example.

Comment 7

[Mark Banner (:standard8)]

Attachment: Better fix

This one is a better one - it uses a for loop and is based largely on PARALLEL_DIRS. I could have probably done a recursive search, but didn't want to make the logic too complicated.

Comment 8

[Robert Kaiser]

Comment on attachment 376211 [details] [diff] [review]
Better fix

>+	    make -C $$dir/test xpcshell-tests; \

Nit: never use plain "make" in Makefiles, use $(MAKE) instead, which allows make to optimize calls to itself (like e.g. communicating variables to the sub-make or doing the right thing for some options like -t).

Comment 9

[Mark Banner (:standard8)]

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