Closed Bug 811404 Opened 7 years ago Closed 7 years ago

include C++ unit tests + harness in test package

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla25

People

(Reporter: ted, Assigned: dminor)

References

Details

(Whiteboard: [mentor=ted][lang=make])

Attachments

(1 file, 3 obsolete files)

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.
Blocks: 811409
This should be mostly just Makefile hacking.
Whiteboard: [mentor=ted][lang=make]
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!
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Attachment #761505 - Flags: review?(ted)
Blocks: 867828
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.
Attachment #761505 - Flags: review?(ted) → review+
Thanks for the review. Revised patch according to the suggestions.
Attachment #761505 - Attachment is obsolete: true
Keywords: checkin-needed
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.
Attachment #762752 - Attachment is obsolete: true
Attachment #766400 - Flags: review?(ted)
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.
Attachment #766400 - Flags: review?(ted) → review+
> +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.
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.
Sorry, my mistake. Patch revised as you suggested. Thanks.
Attachment #766400 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8c5be2044d17
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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.
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.
Depends on: 889078
You need to log in before you can comment on or make changes to this bug.