397227 - Reduce the effort needed to write C++ tests

Status: VERIFIED FIXED

(Firefox Build System :: General, defect)

Description

[Jeff Walden [:Waldo]]

I made a first good stab at this with xpcom/tests/TestPipe.cpp and xpcom/tests/TestPipe.cpp.  The current setup, however, requires a tad bit too much copying of Makefile goo.  If we add a special Makefile variable to set to the list of C++ test files, we can remove the manual work needed to run the tests, with assertions being fatal, and we can eliminate the need to change LOCAL_INCLUDES to include TestHarness.h.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

My goal here is to make it as dead simple as possible to write a test and have it run automatically.  The linking, including, compiling trickery sweeps too much under the rug for it to be used generally, but the goal's not purity or expressive power -- it's ease of use.

The testing section in config/rules.mk has to move up so that when the file's included, the testing stuff runs before the code that processes CPPSRCS and friends and causes the code to be compiled/linked.

The SIMPLE_PROGRAMS change is needed because =/:= evaluate lazily/immediately, respectively.  The change avoids adding tests to SIMPLE_PROGRAMS twice and getting console spew and maybe errors.

I plan to start converting existing C++ tests, both the non-automated ones and the fake-automated ones that don't communicate failure, to this framework as soon as possible after it's in the tree.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Updated to trunk, modifying more existing C++ unit test locations

Compiled-code tests are starting to be used more and more often, and the lack of a standardized way to report errors and status messages is starting to be a problem.  Best to solve this now, while there are relatively few locations to fix.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 305391 [details] [diff] [review]
Updated to trunk, modifying more existing C++ unit test locations

Punting since :bs says he wouldn't get to this for awhile...

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Updated to mozilla-central

The config/rules.mk move is necessary because the building rules are before the current test-harness code location; doing what the patch does in the current location happens too late.

Last I brought this up there were requests to move the programs out of dist and into the corresponding objdir directory, but as far as I could tell from my modifications that requires editing a ton of other different locations, and I couldn't get library linking to work correctly.  Since I'm not going to be able to work on this for awhile, I'd prefer that be punted to a separate bug -- the setup in this patch is no worse than the status quo.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 323762 [details] [diff] [review]
Updated to mozilla-central

+SIMPLE_PROGRAMS += $(CPP_UNIT_TESTS:.cpp=$(BIN_SUFFIX))

Does SIMPLE_PROGRAMS  require BIN_SUFFIX in there? I didn't think it did.

+
+# Copy TestHarness.h into its own module, for ease of setting up includes
+# correctly.
+libs::
+	$(INSTALL) $(srcdir)/TestHarness.h $(DIST)/include/testing

This ought to be in export::. Also, you might need to $(NSINSTALL) -D $(DIST)/include/testing first.

Good stuff, especially all the test refactoring you did there.

Comment 6

[Jeff Walden [:Waldo]]

(In reply to comment #5)
> Does SIMPLE_PROGRAMS  require BIN_SUFFIX in there? I didn't think it did.

Search results for <http://mxr.mozilla.org/mozilla-central/search?string=simple_program&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central> indicate this is correct.

I'm still "around" until tomorrow morning, but I don't have my computer, a checked-out tree, or a machine with make 3.78 or greater (!) on it which I could use to do a build of an updated patch, so I can't drive this the rest of the way into the tree -- much thanks to whoever eventually does that for me.

Comment 7

[Reed Loden [:reed]]

Tried what comment #5 said, but it still failed:

TestPlainTextSerializer.cpp
../../../dist/lib/libxpcomglue_s.a(nsStringAPI.o): In function `CompressWhitespace(nsAString&)':
/home/reed/mozilla/builds/hg/src/xpcom/glue/nsStringAPI.cpp:1047: undefined reference to `NS_IsAsciiWhitespace(unsigned short)'
/home/reed/mozilla/builds/hg/src/xpcom/glue/nsStringAPI.cpp:1053: undefined reference to `NS_IsAsciiWhitespace(unsigned short)'
../../../dist/lib/libxpcomglue_s.a(nsStringAPI.o): In function `ns_strnimatch':
/home/reed/mozilla/builds/hg/src/xpcom/glue/nsStringAPI.cpp:369: undefined reference to `NS_IsAscii(unsigned short)'
../../../dist/lib/libxpcomglue_s.a(nsStringAPI.o): In function `ns_strnmatch':
/home/reed/mozilla/builds/hg/src/xpcom/glue/nsStringAPI.cpp:355: undefined reference to `NS_IsAscii(unsigned short)'
../../../dist/lib/libxpcomglue_s.a(nsStringAPI.o): In function `nsAString::LowerCaseEqualsLiteral(char const*) const':
/home/reed/mozilla/builds/hg/src/xpcom/glue/nsStringAPI.cpp:316: undefined reference to `NS_IsAscii(unsigned short)'
../../../dist/lib/libxpcomglue_s.a(nsStringAPI.o): In function `nsAString::EqualsLiteral(char const*) const':
/home/reed/mozilla/builds/hg/src/xpcom/glue/nsStringAPI.cpp:300: undefined reference to `NS_IsAscii(unsigned short)'
../../../dist/lib/libxpcomglue_s.a(nsStringAPI.o): In function `nsAString::Equals(unsigned short const*, int (*)(unsigned short const*, unsigned short const*, unsigned int)) const':
/home/reed/mozilla/builds/hg/src/xpcom/glue/nsStringAPI.cpp:271: undefined reference to `NS_strlen(unsigned short const*)'
../../../dist/lib/libxpcomglue_s.a(nsStringAPI.o): In function `nsAString::Compare(unsigned short const*, int (*)(unsigned short const*, unsigned short const*, unsigned int)) const':
/home/reed/mozilla/builds/hg/src/xpcom/glue/nsStringAPI.cpp:235: undefined reference to `NS_strlen(unsigned short const*)'
../../../dist/lib/libxpcomglue_s.a(nsStringAPI.o): In function `NS_ToUpper(char)':
/home/reed/mozilla/builds/hg/src/xpcom/glue/nsCRTGlue.h:115: undefined reference to `nsLowerUpperUtils::kLower2Upper'
../../../dist/lib/libxpcomglue_s.a(nsStringAPI.o): In function `NS_ToLower(char)':
/home/reed/mozilla/builds/hg/src/xpcom/glue/nsCRTGlue.h:120: undefined reference to `nsLowerUpperUtils::kUpper2Lower'
/usr/bin/ld: TestPlainTextSerializer: hidden symbol `NS_strlen(unsigned short const*)' isn't defined
/usr/bin/ld: final link failed: Nonrepresentable section on output
collect2: ld returned 1 exit status
make[6]: *** [TestPlainTextSerializer] Error 1
make[6]: *** Waiting for unfinished jobs....
make[5]: *** [tools] Error 2
make[4]: *** [tools] Error 2
make[3]: *** [tools_tier_gecko] Error 2
make[2]: *** [tier_toolkit] Error 2
make[1]: *** [default] Error 2
make: *** [build] Error 2

Removing checkin-needed, as it needs more work.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Updated patch with comment addressed

I had to make some changes to accommodate xpcom/tests/windows, but they were straightforward.  If I'm going to land this now (but see below), the config/rules.mk touching is probably enough that I should at least get eyes back on this before doing so.

I can't reproduce the build error in the previous comment with any mozconfig I've tried on OS X.  I've also submitted this to the tryserver, and it passes all boxes.  We don't have enough tinderbox backlog for me to find which tinderbox failed with it (to determine its mozconfig) or how it failed.  I'm downloading Fedora 9 to install in a VM and try to reproduce there, but that's going to be a fairly long and tedious process, and I don't see how the error reported here is likely to be platform-specific; I'd really like to just land this just long enough to find which tinderbox and which mozconfig busts and then immediately revert, at a time that won't interfere with others wishing to push changes.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 347042 [details] [diff] [review]
Updated patch with comment addressed

Looks fine. Sorry for the wait. Did I ever file a bug on fixing the output from TestHarness.h to match our new unified unittest output? (If not, could you file one, or just fix it?)

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 347042 [details] [diff] [review]
Updated patch with comment addressed

Making it easier to write tests for compiled code is always a good thing!

Comment 11

[Jeff Walden [:Waldo]]

(In reply to comment #9)
> (From update of attachment 347042 [details] [diff] [review])
> Did I ever file a bug on fixing the output from TestHarness.h to match our new
> unified unittest output? (If not, could you file one, or just fix it?)

Don't think you have, will file and probably fix.

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Patch as landed on trunk, ready for 191ing

After a little fiddling I got the previous patch to apply to trunk and not turn anything orange or red.  This is the wrapup of all those changes and should be good to go in 191.

Comment 13

[(not currently active) Ted Mielczarek]

I don't think you should need approval for this test-only change, but that's just my opinion.

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 351423 [details] [diff] [review]
Patch as landed on trunk, ready for 191ing

Yeah, this isn't part of the Firefox build, so that seems reasonable (and the tree rules say not-part-of-Firefox-build is okay to land).  Will land momentarily and do some more tree-watching...

Comment 15

[(not currently active) Ted Mielczarek]

This is all checked in now, right?

Comment 16

[Jeff Walden [:Waldo]]

Yeah, I think so.  Unfortunately I don't think there are any new users of this yet, so it's possible there are still latent bugs in the automagic behavior, but it's not really possible to do anything about that -- just have to wait for them to happen naturally.

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #11)
> Don't think you have, will file and probably fix.

Filed as bug 468033.

Comment 18

[Tony Chung [:tchung]]

Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090404 Minefield/3.6a1pre 
and 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090406 Shiretoko/3.5b4pre

patch has tests.

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Remove leftover LOCAL_INCLUDES [Checkin: Comment 20 & 21]

Please, double check, but it looks like this can be removed too.

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 374679 [details] [diff] [review]
(Cv1) Remove leftover LOCAL_INCLUDES
[Checkin: Comment 20 & 21]


http://hg.mozilla.org/mozilla-central/rev/25533a905209

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 374679 [details] [diff] [review]
(Cv1) Remove leftover LOCAL_INCLUDES
[Checkin: Comment 20 & 21]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a5cd281a578a