632429 - mailnews cpp tests don't build (TestImapFlagAndUidState TestImapHdrXferInfo TestMimeCrash TestMsgStripRE)

Status: RESOLVED WORKSFORME

(MailNews Core :: Testing Infrastructure, defect)

Description

[David :Bienvenu]

We have a few .cpp tests that can't currently be xpcshell tests (i.e., can't be written in js). These broke when we switched to libxul builds.

Comment 1

[Joshua Cranmer [:jcranmer]]

Possible workarounds:

1. Static libxul.a, aka, let's kill your disk space!
2. Conditionally export required test things on ENABLE_TESTS
3. Unconditionally eliminate hidden symbols on ENABLE_TESTS
4. Modify the C++ test framework to add tests to libxul.so.

Comment 2

[(not currently active) Ted Mielczarek]

Note that:
2&3. ENABLE_TESTS is the default
4. We also want to build and run tests for release builds, so this won't work.

In other bugs I've argued that we ought to be figuring out an easy way to let C++ unit tests link directly to the object files they need. Now that we don't actually build static libs, and just use object files everywhere, this is probably feasible.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Build TestMimeCrash

Fortunately, TestMimeCrash can be built.

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 592056 [details] [diff] [review]
Build TestMimeCrash

Review of attachment 592056 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/mime/Makefile.in
@@ +46,5 @@
>  
>  PARALLEL_DIRS	= public src emitters cthandlers
>  
>  ifdef ENABLE_TESTS
> +TOOL_DIRS += test

Interesting: not using PARALLEL_DIRS here seems to make sense.
In that case, there should be some other cases to fix in
http://mxr.mozilla.org/comm-central/search?string=PARALLEL_DIRS.*test&regexp=on&case=1&find=%2FMakefile

::: mailnews/mime/test/Makefile.in
@@ +47,5 @@
>  XPCSHELL_TESTS = unit
>  
> +CPP_UNIT_TESTS = \
> +               TestMimeCrash.cpp \
> +               $(NULL)

Nit: I suggest to use a single tab instead.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 592056 [details] [diff] [review]
Build TestMimeCrash

If we're not doing all of them in this bug, can we do this one in a separate bug so that we're not going to end up with a bug in a limbo state where its part fixed and part not. That's just confusing.

Comment 8

[Wayne Mery (:wsmwk)]

obsolete?

Comment 9

[Magnus Melin [:mkmelin]]

Yes we removed those.