[icedtea-web] reviewer needed - added engine for lunching reproducers

Tue May 17 06:31:58 PDT 2011

On 05/17/2011 04:34 AM, Omair Majid wrote:
> Hi Jiri,
>
>...snip...
>> 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_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.
You yourself suggested 'make check-dist' - so I took this word to signify its belongings.
ok -  tests/netx/jnlp/ will be more suitable

NETX_JNLP_TEST_SRCDIR  contains sources of reproducers applications, NETX_DIST_TEST_SRCDIR contains helping classes and virtual server sources.
NETX_DIST_TEST_DIR  contains compiled  helping classes, server etc. NETX_DIST_TESTCASES_DIR have been removed.
TESTS_SERVER_DIR - contains reproducers jars, resources... And is root directory for server. TESTS_JNLP_DIR contains compiled reproducers

So since teh testcases_dir is removed, i think the names are ok.

Or you can vote for:
NETX_JNLP_TEST_SRCDIR -> JNLP_REPRODUCERS_SRCDIR
NETX_DIST_TEST_SRCDIR - >  JNLP_TESTS_HELPINGCLASSES_SRCDIR
NETX_DIST_TEST_DIR -> JNLP_TESTS_HELPINGCLASSES_BUILDDIR
TESTS_JNLP_DIR  -> JNLP_REPRODUCERS_BUILDDIR
TESTS_SERVER_DIR ->  JNLP_TESTS_SERVER_DIR

>>
>> JUNIT_RUNNER_JAR=$(abs_top_builddir)/junit-runner.jar
...snip...
>> + 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'.

Hmm.. Do not understand this very much.  Is there something useful to run this tests without all-local? Where comes values of this variables from? Where are they from? From where will they come when no make all-local done?

>> + > 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.

But they are running this way now (or NOT????? -confused-)
Is there any issue I can't see?

>
>>
>> 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?).

Done..

Each file in simple must follows naming convention and is compiled/jared automatically 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.

Sometimes I wish to be native speaker...
>
>> --- /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?

As mentioned before (and ^^ :) all reproducers in simple directory will be  compiled and deployed automaticly. When some reproducer is more complex and needs manual work, then it have to be  placed somewhere else (eg under sibling /complex/ or maybe as sibling itself).

>
>> @@ -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 '.'.

I have already mentioned this - when "." Is included, then as codebase is (should be!) used url from which the jnlp was obtained. And this is quite sufficient, isn't it?
If you really want to "hardcode path" Then I will need to sed all jnlp files - as port where server is runing is chosen as random-empty-one. So you can expect something like

codebase="[@]address[@]" , which will then be changed in java tests or the port will be handled outside junit-run and so it will be sedded.. somwehow.

>
>> +<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.

Licence - yes!
Those trivial test are included exactly and mainly  as proof of concept. But I was thinking about commitng them - as guide others and as warning  that something goes very very bad (with java, javaws or testsuite or with whatever) when even those two  fails.

>
>> +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?

Sure! I believe that classes should never be in "default" (== empty) package. In this case it also helps me to jar them clearly.
>
>> 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
... snip...
>> +
>> +
>> +
>> + }
>> +
>
> Please remove this extra whitespace and fix the indentation.

yy

>
> I dont see any other issues, but I will build and test out this patch as soon as I get some extra time.
Thank you very much for review and hints.
J.
>
> Cheers,
> Omair