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

[Omair Majid]

On 05/17/2011 09:31 AM, Jiri Vanek wrote:
>>> 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.

Well, 'make distcheck' is a commonly known target. In a source tree 
though, the meaning of tests/netx/dist/ is not clear.

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

If you can come up with something more descriptive than DIST, then 
please use that. Otherwise DIST should be fine for now.

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

I thought we were using the convention that *_SRCDIR indicates a source 
diretory and *_DIR indicates a build directory. Otherwise I think they 
are alright. I dont care too much what variables are called; I care much 
more about the source tree paths: /

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

Sorry if I wasn't clear: you have to build (all-local) to test of 
course. But I was suggesting that if you pass along all the right 
variables, that you could (possibly, have to double-check) run the 
just-built javaws binary. You might be able to skip having to install it 
to check. But it's not really that big a deal for now. You can ignore it :)

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

They are? Mea culpa then. As I said elsewhere in the email, I did not 
try and build this patch.

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

More typos ;)

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

Ok, that makes sense.

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

Sorry, I have a terrible memory :( Yes, of course it is sufficient.

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

If it works, no need to make life more complicated :)

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

That's actually a good idea. Can you please add a note in the README? 
(you probably want to do that as a separate patch).

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

Can you elaborate on why default package is a bad idea for test cases?

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

No, thank YOU for creating the patch and testing it and bearing with my 
very nit-picky reviews!

Cheers,
Omair