811404 - include C++ unit tests + harness in test package

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[(not currently active) Ted Mielczarek]

Currently we only run the C++ unit tests as part of "make check". Moving them to be run on the test machines has a few nice advantages:
1) Less time spent on the build slave
2) Ability to run the tests on mobile

Step 1 to making this happen is to package the bits needed to run them in the test package.

Comment 1

[(not currently active) Ted Mielczarek]

This should be mostly just Makefile hacking.

Comment 2

[Sarup Banskota]

Hi Ted,
I'm new to Mozilla/Open Source. I'm decent at Python and C++. Could you assign me the bug (as long as it doesn't involve me downloading over 100MB of content, I have a slightly unstable connection currently).

And of course, I'll need lots of help getting started :)

Thanks!

Comment 3

[Dan Minor [:dminor]]

Attachment: Patch to stage cpptests as part of package-tests.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 761505 [details] [diff] [review]
Patch to stage cpptests as part of package-tests.

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

r=me with a couple of nit fixes.

::: config/rules.mk
@@ +167,5 @@
>  INCLUDES += -I$(DIST)/include/testing
>  LIBS += $(XPCOM_GLUE_LDOPTS) $(NSPR_LIBS) $(MOZ_JS_LIBS) $(if $(JS_SHARED_LIBRARY),,$(MOZ_ZLIB_LIBS))
>  
> +libs:: $(CPP_UNIT_TESTS:.cpp=$(BIN_SUFFIX))
> +	$(NSINSTALL) -D $(DIST)/cpptests

You could instead stick $(call mkdir_deps,$(DIST)/cpptests) in the prerequisites.

@@ +168,5 @@
>  LIBS += $(XPCOM_GLUE_LDOPTS) $(NSPR_LIBS) $(MOZ_JS_LIBS) $(if $(JS_SHARED_LIBRARY),,$(MOZ_ZLIB_LIBS))
>  
> +libs:: $(CPP_UNIT_TESTS:.cpp=$(BIN_SUFFIX))
> +	$(NSINSTALL) -D $(DIST)/cpptests
> +	$(NSINSTALL) $(subst .cpp,$(BIN_SUFFIX),$(CPP_UNIT_TESTS)) $(DIST)/cpptests

You should probably just make an intermediate variable for "CPP_UNIT_TESTS as binaries" like:
CPP_UNIT_TEST_BINS := $(CPP_UNIT_TESTS:.cpp=$(BIN_SUFFIX))

Then use that in the three places we have the substitution.

Comment 5

[Dan Minor [:dminor]]

Attachment: Patch to stage cpptests as part of package-tests.

Thanks for the review. Revised patch according to the suggestions.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02fad60002a9

Comment 7

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5d8b98db928e - PGO seemed displeased

https://tbpl.mozilla.org/php/getParsedLog.php?id=24183846&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=24183747&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=24183690&tree=Mozilla-Inbound

Comment 8

[Dan Minor [:dminor]]

Attachment: Patch to stage cpptests as part of package-tests.

This fixes the PGO builds. Here is a green try run with PGO forced on which packages the cppunittests successfully: https://tbpl.mozilla.org/?tree=Try&rev=e54b84b94695.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 766400 [details] [diff] [review]
Patch to stage cpptests as part of package-tests.

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

::: config/rules.mk
@@ +168,5 @@
>  LIBS += $(XPCOM_GLUE_LDOPTS) $(NSPR_LIBS) $(MOZ_JS_LIBS) $(if $(JS_SHARED_LIBRARY),,$(MOZ_ZLIB_LIBS))
>  
> +ifdef MOZ_PROFILE_GENERATE
> +libs:: $(call mkdir_deps,$(DIST)/cppunittests)
> +else

Just make this ifndef MOZ_PROFILE_GENERATE and drop the empty libs rule in the current if block.

Comment 10

[Dan Minor [:dminor]]

> +ifdef MOZ_PROFILE_GENERATE
> +libs:: $(call mkdir_deps,$(DIST)/cppunittests)
> +else

This was to work around a problem I hit on try last week: https://tbpl.mozilla.org/?tree=Try&rev=f613698441a8 where it appears to attempt to package things during the MOZ_PROFILE_GENERATE stage:

/builds/slave/try-osx64-00000000000000000000/build/obj-firefox/i386/config/nsinstall -D ./dist/test-package-stage/cppunittests
/builds/slave/try-osx64-00000000000000000000/build/obj-firefox/i386/config/nsinstall /builds/slave/try-osx64-00000000000000000000/build/testing/runcppunittests.py ./dist/test-package-stage/cppunittests
/builds/slave/try-osx64-00000000000000000000/build/obj-firefox/i386/config/nsinstall /builds/slave/try-osx64-00000000000000000000/build/testing/remotecppunittests.py ./dist/test-package-stage/cppunittests
cp -RL ./dist/cppunittests ./dist/test-package-stage
cp: ./dist/cppunittests: No such file or directory
make[4]: *** [stage-cppunittests] Error 1
make[3]: *** [postflight_all] Error 2
make[2]: *** [realbuild] Error 2
make[1]: *** [profiledbuild] Error 2
make: *** [build] Error 2

I could move this to be a dependency for stage-cppunittests in testsuite-targets.mk or check for directory existence before the call to cp if that would be a better spot for it.

Comment 11

[(not currently active) Ted Mielczarek]

That's on a Mac universal build, which we do not do in practice. We discussed this on IRC, this was just fallout from you using a patch that forced PGO everywhere.

Comment 12

[Dan Minor [:dminor]]

Attachment: Patch to stage cpptests as part of package-tests.

Sorry, my mistake. Patch revised as you suggested. Thanks.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c5be2044d17

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8c5be2044d17

Comment 15

[Nick Thomas [:nthomas] (UTC+12)]

The size of the test archive has increased significantly on some platforms:
  linux32:  82 --> 220MB
  linux64:  85 --> 230MB
  mac:      69 --> 118MB
  win32:    77 -->  78MB

That has knock-on effects for traffic on ftp.m.o, overhead unpacking tests on every test run, and so on. Please let RelEng know about these sorts of changes before they land so we can make sure the infra will be OK.

That said, the cppunittests directory seem not to be stripped on linux. Could it be ? That would bring the zip back to 104MB.

Comment 16

[(not currently active) Ted Mielczarek]

Yes, we could (and should) strip those files. Sorry for missing that. We should be able to do that in the test package staging step.