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

Mon Nov 28 08:44:09 PST 2011

TYVM for review!

I have fixed most of the issues you suggested.
The one we talked about on IRC  - touching files in prefix/bin . I still think its the best aproach. Test coverage should be run in "test run" prefix, and with  configuration nearest to final one. I have found no better solution (copying prefix...) then this one to fulfil it more.

Except fixes you have suggested, there  are (inspired by your review) two mayor changes - final result is now in test.built instead of abs_top_build_dir and coverage targets have their own clean, instead of chaotic '-rm '  as it was before.

On 11/21/2011 05:22 PM, Omair Majid wrote:
>
> 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.
fixed, thanx!
>
>> +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?
done
>
>> 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?
I will do it later in separate patch, ok?
>
>> + 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?
Fixed.
All tests and coverage "top targets" now have are now stamped and have their not-stamped aliases, so this one depends on stamped unit and reproducers coverage-runs now.
>
>> + 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 "; \" :)
done
>
>> + 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?
It was already done in previous version, I forgot to change hints. I'm backuping original xml file, then renaming new one to _withEmma and restoring previous one. I have fixed  the hints, so this xhould be fixed.
>
>> + 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?
Yes. By some bug in emma, the first is setting encoding of output stream, and second is setting value of html tag encoding:-/
>
>> + -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?
Moved to Makefile.am comment
>
>> + 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?
I have added exit -5. Enough?
>
>> + 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:

According our discussion on IRC, I still think this is best idea.
Any more ideas more then welcomed.
> $ 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.
Yes, it was  required
>
>> 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

Best regards, and TYVM for review , J

-------------- next part --------------
A non-text attachment was scrubbed...
Name: testCoverageUponHead3.diff
Type: text/x-patch
Size: 18397 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20111128/10736391/testCoverageUponHead3.diff 