[RFC][icedtea-web] emma and testcoveragefor testsuites

Omair Majid omajid at redhat.com
Mon Nov 21 08:22:35 PST 2011 

Comments in-line below.

> diff -r 22db4e09bbc7 Makefile.am
> --- a/Makefile.am	Fri Nov 11 14:40:26 2011 +0100
> +++ b/Makefile.am	Sat Nov 19 00:07:45 2011 +0100
> @@ -25,6 +25,8 @@
>   KEYSTORE_NAME=teststore.ks
>
>   JUNIT_RUNNER_JAR=$(abs_top_builddir)/junit-runner.jar
> +UNIT_CLASS_NAMES = $(abs_top_builddir)/unit_class_names
> +REPRODUCERS_CLASS_NAMES = $(abs_top_builddir)/reproducers_class_names
>
>   # Build directories
>
> @@ -539,7 +541,11 @@
>   	mkdir -p stamps&&  \
>   	touch $@
>
> -run-netx-dist-tests: all-local stamps/netx.stamp stamps/junit-jnlp-dist-dirs stamps/netx-dist-tests-sign-some-reproducers.stamp \
> +run-netx-dist-tests: stamps/run-netx-dist-tests
> +

Files in the stamps dir have names that end with .stamp. Also, target 
aliases ($(foo): stamps/$(foo).stamp) are normally added to the end of 
the makefile.

> +stamps/run-netx-dist-tests: stamps/netx-dist.stamp extra-lib/about.jar stamps/plugin.stamp launcher.build/$(javaws) \
> + javaws.desktop stamps/docs.stamp launcher.build/$(itweb_settings) itweb-settings.desktop \
> + stamps/netx.stamp stamps/junit-jnlp-dist-dirs stamps/netx-dist-tests-sign-some-reproducers.stamp \
>    stamps/netx-dist-tests-compile.stamp stamps/netx-dist-tests-compile-testcases.stamp $(JUNIT_RUNNER_JAR) $(TESTS_DIR)/$(REPORT_STYLES_DIRNAME)
>   	cd $(JNLP_TESTS_ENGINE_DIR) ; \
>   	class_names= ; \
> @@ -548,7 +554,7 @@
>   	  class_name=`echo $$class_name | sed -e 's|/|.|g' ` ; \
>   	  class_names="$$class_names $$class_name" ; \
>   	done ; \
> -	echo $$class_names ; \
> +	echo $$class_names>  $(REPRODUCERS_CLASS_NAMES)	; \
>   	CLASSPATH=$(NETX_DIR)/lib/classes.jar:$(JUNIT_JAR):$(JUNIT_RUNNER_JAR):. \
>   	  $(BOOT_DIR)/bin/java -Dtest.server.dir=$(JNLP_TESTS_SERVER_DEPLOYDIR) -Djavaws.build.bin=$(DESTDIR)$(bindir)/javaws \
>   	 -Xbootclasspath:$(RUNTIME) CommandLine $$class_names \
> @@ -556,6 +562,7 @@
>   	 cat stdout.log ; \
>   	 cat stderr.log>&2
>   	-xsltproc $(TESTS_SRCDIR)/$(REPORT_STYLES_DIRNAME)/jreport.xsl $(JNLP_TESTS_ENGINE_DIR)/tests-output.xml>  $(TESTS_DIR)/index_reproducers.html
> +	touch $@
>
>   netx-unit-tests-source-files.txt:
>   	find $(NETX_UNIT_TEST_SRCDIR) -name '*.java' | sort>  $@
> @@ -590,13 +597,167 @@
>   	  class_name=`echo $$class_name | sed -e 's|/|.|g' ` ; \
>   	  class_names="$$class_names $$class_name" ; \
>   	done ; \
> -	echo $$class_names ; \
> +	echo $$class_names>  $(UNIT_CLASS_NAMES); \

Perhaps you should split this into a new target that generates 
$(UNIT_CLASS_NAMES), so this target and run-unit-test-code-coverage can 
depend on it?

>   	CLASSPATH=$(NETX_DIR)/lib/classes.jar:$(JUNIT_JAR):$(JUNIT_RUNNER_JAR):. \
>   	  $(BOOT_DIR)/bin/java -Xbootclasspath:$(RUNTIME) CommandLine $$class_names \
>   	>  stdout.log 2>  stderr.log ; \
>   	cat stdout.log ; \
> -	cat stderr.log>&2
> +	cat stderr.log>&2 ;
>   	-xsltproc $(TESTS_SRCDIR)/$(REPORT_STYLES_DIRNAME)/jreport.xsl $(NETX_UNIT_TEST_DIR)/tests-output.xml>  $(TESTS_DIR)/index_unit.html

Hm... perhaps we should add a configure check for xsltproc?

> +	touch $@
> +
> +$(NETX_UNIT_TEST_DIR)/coverage.es: run-unit-test-code-coverage
> +
> +run-unit-test-code-coverage: check

Would it be possible to add more specific dependencies?

> +	if test "$(EMMA_AVAILABLE)" = "true"; then \

We tend to do a makefile 'if' instead of a shell 'if'.

if WITH_EMMA
...
endif

As a bonus you will get rid of lots of "; \" :)

> +	  echo "warning, this can rewrite tests.build/netx/unit/tests-output.xml, but not coresponding html file"; \
> +	  echo "xml results run from emma sandbox, however, can be wrong" ; \

Perhaps we should address this by either writing to another file rather 
than tests-output.xml or making sure the emma results are coorect?

> +	  mv $(NETX_UNIT_TEST_DIR)/tests-output.xml  $(NETX_UNIT_TEST_DIR)/tests-output.xml_noEmma ; \
> +	  cd $(NETX_UNIT_TEST_DIR) ; \
> +	  class_names=`cat $(UNIT_CLASS_NAMES)` ; \
> +	  $(BOOT_DIR)/bin/java -Xbootclasspath:$(RUNTIME) -cp $(EMMA_JAR) -Demma.report.html.out.encoding=UTF-8 emmarun \
> +	   -Dreport.html.out.encoding=UTF-8 \

Are the properties duplicated on purpose?

> +	   -raw \
> +	   -sp $(NETX_SRCDIR) \
> +	   -sp $(NETX_UNIT_TEST_SRCDIR) \
> +	   -sp $(JUNIT_RUNNER_SRCDIR) \
> +	   -r html \
> +	   -r xml \
> +	   -cp $(NETX_DIR)/lib/classes.jar \
> +	   -cp $(JUNIT_JAR) \
> +	   -cp $(JUNIT_RUNNER_JAR) \
> +	   -cp $(BOOT_DIR)/jre/lib/rt.jar \
> +	   -cp $(BOOT_DIR)/jre/lib/jsse.jar \
> +	   -cp $(RHINO_RUNTIME) \
> +	   -cp . \
> +	   -ix "-org.junit.*" \
> +	   -ix "-junit.*" \
> +	   CommandLine $$class_names ;  \
> +	echo "you can add -ix "-*Test*" -ix "-*test*"  to ignore all test cases from statistics." ; \

I would rather avoid these echo commands telling a user how to modify 
the makefile. Perhaps a comment might be useful instead?

> +	mv $(NETX_UNIT_TEST_DIR)/tests-output.xml  $(NETX_UNIT_TEST_DIR)/tests-output_withEmma.xml ; \
> +	mv $(NETX_UNIT_TEST_DIR)/tests-output.xml_noEmma  $(NETX_UNIT_TEST_DIR)/tests-output.xml ; \
> +	else \
> +	  echo "Sorry, coverage report cant be run without emma installed. Try install emma or specify with-emma value" ; \

Perhaps you might want to invoke false here so "make 
run-unit-test-code-coverage" fails?

> +	fi
> +	touch $@
> +
> +$(JNLP_TESTS_ENGINE_DIR)/coverage.es: run-reproducers-test-code-coverage
> +
> +run-reproducers-test-code-coverage: stamps/run-netx-dist-tests
> +	if test "$(EMMA_AVAILABLE)" = "true"; then \
> +	  echo "warning, this can rewrite tests.build/netx/jnlp_testsengine/tests-output.xml, but not coresponding html file" ; \
> +	  echo "xml results run from emma sandbox, however, can be wrong" ; \
> +	  mv $(JNLP_TESTS_ENGINE_DIR)/tests-output.xml  $(JNLP_TESTS_ENGINE_DIR)/tests-output.xml_noEmma ; \
> +	  echo "backuping javaws and netx.jar in $(DESTDIR)" ; \
> +	  netx_backup=$(DESTDIR)$(datadir)/$(PACKAGE_NAME)/netx_backup.jar ; \
> +	  javaws_backup=$(DESTDIR)$(bindir)/javaws_backup ; \
> +	  mv $(DESTDIR)$(bindir)/javaws $$javaws_backup ;  \
> +	  mv $(DESTDIR)$(datadir)/$(PACKAGE_NAME)/netx.jar $$netx_backup ; \

I dont think playing with files inside $(bindir) and/or $(datadir) is a 
good idea. The files may be read-only at this point if the user does 
something like this:
$ make
# make install
$ make run-reproducers-test-code-coverage

> @@ -611,6 +772,9 @@
>   clean-netx-unit-tests: clean_tests_reports
>   	rm -f netx-unit-tests-source-files.txt
>   	rm -rf $(NETX_UNIT_TEST_DIR)
> +	rm -f $(UNIT_CLASS_NAMES)
> +	-rm -f run-unit-test-code-coverage
> +	-rm -f run-netx-unit-tests

Is the "-" in "-rm" required? -f means force so rm should not fail if 
the file is missing.

>   	rm -f stamps/netx-unit-tests-compile.stamp
>
>   clean_tests_reports:
> @@ -629,7 +793,10 @@
>   	rm -f stamps/netx-dist-tests-sign-some-reproducers.stamp
>   	rm -f junit-jnlp-dist-simple.txt
>   	rm -f junit-jnlp-dist-signed.txt
> +	rm -f $(REPRODUCERS_CLASS_NAMES)
>   	rm -f $(abs_top_builddir)/$(KEYSTORE_NAME)
> +	-rm -f run-reproducers-test-code-coverage
> +	rm -f stamps/run-netx-dist-tests
>
>   # plugin tests
>

Cheers,
Omair

More information about the distro-pkg-dev mailing list