[icedtea-web] reviewer needed - added engine for lunching reproducers
Omair Majid
omajid at redhat.com
Mon May 16 19:34:03 PDT 2011
Hi Jiri,
On 05/11/2011 11:37 AM, Jiri Vanek wrote:
> On 05/07/2011 01:32 AM, Omair Majid wrote:
>> Hi Jiri,
>>
>> On 05/05/2011 12:05 PM, Jiri Vanek wrote:
>>> Makefile - automated compile and copy for most of reproducers, with
>>> possibility to add more complex reproducers which will take care of
>>> compiling themselves (I have in this way also "repaired" your
>>> reproducers you send me, but because this patch is about engine itself
>>> they are not commited - but are working quite fine :) )
>>> When all reproducers are prepared, junit testsuite is lunched - after
>>> program is "installed". junit takes location of installed javaws and
>>> location of prepared reproducers (directory jars, jnlps and other
>>> resources).
>>>
>>> Inside junit suite virtual server is lunched and serves all resources.
>>> During the testsuite individual jnlps are executed, and their
>>> stderr/out/return code can be asserted. (There are helepers method for
>>> this in used virtual server which provides to tester exactly std out/err
>>> return code). Also possibility to timeout process is added.
>>> At the end the report - same as was for unit tests - is generated (if my
>>> previous patch is used :). There is also possibility of including
>>> stdout/err of tests into xml-report files.
>>>
>>
>> Sorry for taking so long to respond to this.
>>
>> I have got to admit, this is great! I have lots of little nitpicks [1],
>> but we can argue about those _after_ you commit an initial version.
>>
>> I only thing I dont like is how test information is spread out. I would
>> love if this was more jtreg-like: you place everything (jnlp file and
>> the associated java file (and possibly a makefile or other meta
>> information)) in one directory. I dont like that you have to add junit
>> test methods (to another location). You have a big thumbs up from me if
>> you can fix this.
>>
>> Thanks again for doing this!
>>
>> Cheers,
>> Omair
>>
>> [1] including file paths/names, variable names, spacing and other
>> details ;)
>
> So another round - partially based on attached communication.
> The heart of patch is not changed at all. It automatically compile and
> deploy individual reproducers. Still containing space for more complex
> reproducers which compile and(or) deploy themselves. Run testsuite
> inside junit, and contains small server (can run several instances on
> different port, and contains helping methods) as singleton and lunches
> by-make installed javaws.
>
> The crucial difference is in testing whether reproducer passed or
> failed. In previous patch this was included ind
> tests/dist/net/sourceforge/ (acording to tests/unit/net/sourceforge).
> Now (as previewd in simpletest 1 and 2 reproducer) each reproducer have
> except srcs and resources directory also testcases directory which
> contains javaclass which will be added to junit testsuit to be lunched
> and examined. (*** _I'm not sure about package hierarchy here_ ***).
> Those testcases have tests/dist on their classpath even during compile,
> so they can use helpers functions and server smoothly.
>
I am quite happy with this patch. I have a few recommendations below.
>
>
> diff -r 2b1a69f4c54b Makefile.am
> --- a/Makefile.am Tue May 10 11:16:17 2011 -0400
> +++ b/Makefile.am Wed May 11 13:52:03 2011 +0200
> @@ -12,11 +12,17 @@
> NETX_UNIT_TEST_SRCDIR=$(TESTS_SRCDIR)/netx/unit
> TESTS_STYLES_SRCDIR=$(TESTS_SRCDIR)/$(REPORT_STYLES_DIRNAME)
> JUNIT_RUNNER_SRCDIR=$(TESTS_SRCDIR)/junit-runner
> +NETX_DIST_TEST_SRCDIR=$(TESTS_SRCDIR)/netx/dist
> +NETX_JNLP_TEST_SRCDIR=$(TESTS_SRCDIR)/jnlp_tests
>
> TESTS_DIR=$(abs_top_builddir)/tests.build
> NETX_UNIT_TEST_DIR=$(TESTS_DIR)/netx/unit
> TESTS_STYLES_DIR=$(TESTS_DIR)/$(REPORT_STYLES_DIRNAME)
> JUNIT_RUNNER_DIR=$(TESTS_DIR)/junit-runner
> +NETX_DIST_TEST_DIR=$(TESTS_DIR)/netx/dist
> +NETX_DIST_TESTCASES_DIR=$(TESTS_DIR)/netx/dist_testcases
> +TESTS_SERVER_DIR=$(TESTS_DIR)/test_server
> +TESTS_JNLP_DIR=$(TESTS_DIR)/jnlp_tests
>
I would suggest renaming these directories (and variables). I am not
sure what 'dist' really signifies. The jnlp tests should probably in
tests/netx/jnlp/ - they are for netx only and not for the plugin.
>
> JUNIT_RUNNER_JAR=$(abs_top_builddir)/junit-runner.jar
> @@ -459,6 +465,68 @@
> @junit-runner-source-files.txt&& \
> $(BOOT_DIR)/bin/jar cf $@ -C $(JUNIT_RUNNER_DIR) .
>
> +junit-jnlp-dist-dirs.txt:
> + mkdir -p $(TESTS_SERVER_DIR)
> + mkdir -p $(TESTS_JNLP_DIR)
> + mkdir -p $(NETX_DIST_TEST_DIR)
> + cd $(NETX_JNLP_TEST_SRCDIR)/simple/ ; \
> + find . -maxdepth 1 -mindepth 1 | sed "s/.\/*//"> $(abs_top_builddir)/$@
> +
> +stamps/netx-dist-tests-prepare-reproducers.stamp: junit-jnlp-dist-dirs.txt
> + simpleReproducers=(`cat $(abs_top_builddir)/junit-jnlp-dist-dirs.txt `); \
> + for dir in "$${simpleReproducers[@]}" ; do \
> + mkdir -p $(TESTS_JNLP_DIR)/$$dir ; \
> + $(BOOT_DIR)/bin/javac -d $(TESTS_JNLP_DIR) $(NETX_JNLP_TEST_SRCDIR)/simple/$$dir/srcs/$$dir/* ; \
> + d=`pwd` ; \
> + cd $(TESTS_JNLP_DIR) ; \
> + $(BOOT_DIR)/bin/jar cf $(TESTS_SERVER_DIR)/$$dir.jar $$dir/ ; \
> + cd $$d ; \
> + cp -R $(NETX_JNLP_TEST_SRCDIR)/simple/$$dir/resources/* $(TESTS_SERVER_DIR)/ ; \
> + done ; \
> + mkdir -p stamps&& \
> + touch $@
> +
> +netx-dist-tests-source-files.txt:
> + find $(NETX_DIST_TEST_SRCDIR) -name '*.java' | sort> $@
> +
> +stamps/netx-dist-tests-compile.stamp: stamps/netx.stamp \
> + junit-jnlp-dist-dirs.txt netx-dist-tests-source-files.txt
> + $(BOOT_DIR)/bin/javac $(IT_JAVACFLAGS) \
> + -d $(NETX_DIST_TEST_DIR) \
> + -classpath $(JUNIT_JAR):$(NETX_DIR)/lib/classes.jar \
> + @netx-dist-tests-source-files.txt&& \
> + mkdir -p stamps&& \
> + touch $@
> +
> +stamps/netx-dist-tests-compile-testcases.stamp: stamps/netx.stamp junit-jnlp-dist-dirs.txt\
> + netx-dist-tests-source-files.txt stamps/netx-dist-tests-compile.stamp
> + simpleReproducers=(`cat $(abs_top_builddir)/junit-jnlp-dist-dirs.txt `); \
> + for dir in "$${simpleReproducers[@]}" ; do \
> + $(BOOT_DIR)/bin/javac $(IT_JAVACFLAGS) \
> + -d $(NETX_DIST_TEST_DIR) \
> + -classpath $(JUNIT_JAR):$(NETX_DIR)/lib/classes.jar:$(NETX_DIST_TEST_DIR) \
> + $(NETX_JNLP_TEST_SRCDIR)/simple/$$dir/testcases/* ; \
> + done ; \
> + mkdir -p stamps&& \
> + touch $@
> +
> +run-netx-dist-tests: all-local stamps/netx.stamp junit-jnlp-dist-dirs.txt stamps/netx-dist-tests-prepare-reproducers.stamp \
> + stamps/netx-dist-tests-compile.stamp stamps/netx-dist-tests-compile-testcases.stamp $(JUNIT_RUNNER_JAR) $(TESTS_DIR)/index.html
> + cd $(NETX_DIST_TEST_DIR) ; \
> + class_names= ; \
> + for test in `find -type f` ; do \
> + class_name=`echo $$test | sed -e 's|\.class$$||' -e 's|^\./||'` ; \
> + class_name=`echo $$class_name | sed -e 's|/|.|g' ` ; \
> + class_names="$$class_names $$class_name" ; \
> + done ; \
> + echo $$class_names ; \
> + CLASSPATH=$(NETX_DIR)/lib/classes.jar:$(JUNIT_JAR):$(JUNIT_RUNNER_JAR):. \
> + $(BOOT_DIR)/bin/java -Dtest.server.dir=$(TESTS_SERVER_DIR) -Djavaws.build.bin=$(DESTDIR)$(bindir)/javaws \
> + -Xbootclasspath:$(RUNTIME) CommandLine $$class_names \
I just realized (sometimes I can be slow ;) ) that if you are passing
all these java properties, you can also pass along the values of
-Dicedtea-web.bin.name and -Dicedtea-web.bin.path and avoid having to
run these tests only after 'make install'.
> + > stdout.log 2> stderr.log ; \
> + cat stdout.log ; \
> + cat stderr.log>&2
> +
> netx-unit-tests-source-files.txt:
> find $(NETX_UNIT_TEST_SRCDIR) -name '*.java' | sort> $@
>
> @@ -496,7 +564,7 @@
> cat stdout.log ; \
> cat stderr.log>&2
>
> -clean-netx-tests: clean-netx-unit-tests clean-junit-runner
> +clean-netx-tests: clean-netx-unit-tests clean-junit-runner clean-netx-dist-tests
> if [ -e $(TESTS_DIR)/netx ]; then \
> rmdir $(TESTS_DIR)/netx ; \
> fi
> @@ -513,6 +581,16 @@
> rm -rf $(NETX_UNIT_TEST_DIR)
> rm -f stamps/netx-unit-tests-compile.stamp
>
> +clean-netx-dist-tests:
> + rm -f netx-dist-tests-source-files.txt
> + rm -rf $(TESTS_JNLP_DIR)
> + rm -rf $(TESTS_SERVER_DIR)
> + rm -rf $(NETX_DIST_TEST_DIR)
> + rm -f stamps/netx-dist-tests-compile.stamp
> + rm -f stamps/netx-dist-tests-prepare-reproducers.stamp
> + rm -f stamps/netx-dist-tests-compile-testcases.stamp
> + rm -f junit-jnlp-dist-dirs.txt
> +
> # plugin tests
I cant see a way to run these tests using a standard 'make' target.
>
> if ENABLE_PLUGIN
> diff -r 2b1a69f4c54b tests/jnlp_tests/README
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/jnlp_tests/README Wed May 11 13:52:03 2011 +0200
> @@ -0,0 +1,1 @@
> +Each file in simple must follows naming convention and is compiled/jared automaticky into server's working directory and content of resources likewise. The name of jnlp is independent, ant there can be even more jnlps for each future jar.
> diff -r 2b1a69f4c54b tests/jnlp_tests/simple/simpletest1/resources/simpletest1.jnlp
Please fix the spelling error(s?).
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/jnlp_tests/simple/simpletest1/resources/simpletest1.jnlp Wed May 11 13:52:03 2011 +0200
Why the '/simple/' in the path?
> @@ -0,0 +1,16 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<jnlp spec="1.0" href="simpletest1.jnlp" codebase=".">
If you are starting a simple http server, then codebase value should
point to the server, not '.'.
> +<information>
> +<title>simpletest1</title>
> +<vendor>NetX</vendor>
> +<homepage href="http://jnlp.sourceforge.net/netx/"/>
> +<description>simpletest1</description>
> +<offline/>
> +</information>
> +<resources>
> +<j2se version="1.4+"/>
> +<jar href="simpletest1.jar"/>
> +</resources>
> +<application-desc main-class="simpletest1.SimpleTest1">
> +</application-desc>
> +</jnlp>
> diff -r 2b1a69f4c54b tests/jnlp_tests/simple/simpletest1/srcs/simpletest1/SimpleTest1.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/jnlp_tests/simple/simpletest1/srcs/simpletest1/SimpleTest1.java Wed May 11 13:52:03 2011 +0200
> @@ -0,0 +1,8 @@
Please add a license here too. Well, I hope you are not committing this
trivial test. But just to give an idea. And thanks for adding a concrete
example to show how actual tests would be like.
> +package simpletest1;
> +
> +public class SimpleTest1{
> +
> + public static void main(String[] args){
> + System.out.println("Good simple javaws exapmle");
> + }
> +}
Is there an advantage to placing this class in a separate package?
> diff -r 2b1a69f4c54b tests/jnlp_tests/simple/simpletest1/testcases/SimpleTest1Test.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/jnlp_tests/simple/simpletest1/testcases/SimpleTest1Test.java Wed May 11 13:52:03 2011 +0200
> @@ -0,0 +1,82 @@
> +/* ParserMalformedXml.java
> +Copyright (C) 2011 Red Hat, Inc.
> +
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or
> +modify it under the terms of the GNU General Public License as published by
> +the Free Software Foundation, version 2.
> +
> +IcedTea is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING. If not, write to
> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library. Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module. An independent module is a module which is not derived from
> +or based on this library. If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so. If you do not wish to do so, delete this
> +exception statement from your version.
> + */
> +package net.sourceforge.jnlp;
> +
> +import java.io.File;
> +import java.io.FileFilter;
> +import java.net.URL;
> +import org.junit.AfterClass;
> +import org.junit.Assert;
> +
> +import org.junit.BeforeClass;
> +import org.junit.Test;
> +
> +public class SimpleTest1Test {
> +
> + private static ServerAccess server = new ServerAccess();
> +
> +
> +
> + @Test
> + public void testSimpletest1lunchOk() throws Exception {
> + System.err.println("connecting simpletest1 request");
> + ServerAccess.ProcessResult pr=server.executeJavawsHeadless(null,"/simpletest1.jnlp");
> + System.err.println(pr.stdout);
> + System.err.println(pr.stderr);
> + Assert.assertTrue(pr.stdout.contains("Good simple javaws exapmle"));
> + Assert.assertFalse(pr.stderr.contains("xception"));
> + //Assert.assertEquals(0, pr.process.exitValue());
> + }
> +
> +
> +
> +
> +
> + }
> +
Please remove this extra whitespace and fix the indentation.
I dont see any other issues, but I will build and test out this patch as
soon as I get some extra time.
Cheers,
Omair
More information about the distro-pkg-dev
mailing list