[fyi][icedtea-web]Refactoring of reproducers as agreed in April

Fri Jun 29 08:58:50 PDT 2012

On 06/29/2012 08:50 AM, Jiri Vanek wrote:
> Hi! It looks to me that we have agreed on first five patches (2x hg mv,
> extracting of tests and inner clases, adapted makefile and added
> headers). I would like to push them (it is getting hard to maintain
> those set of changsets).

Fair enough. Just one request, could you ensure that every changeset is
buildable? You should probably combine the one doing the hg rename with
the ones doing the makefile changes to update the directory name. It
doesnt really matter for now whether each changeset is buildable, if you
pull all at once, but if we ever have to do a 'hg bisect' (or otherwise
dig through history to identify a problem), a changeset that wont build
can be painful. You don't have to post a separate patch review for this.

> Your next two concerns (netx on classapth and renaming) I would like to
> provide as two  separate changsets, as it is not critically bounded. Se
> my comments lower to the renaming patch as you wanted and get rid of
> unnecessary netx classapth entry.

Hm... In general, I am fine with pushing as separate changesets. It
feels like the renaming build dirs bit belongs in this patch though (we
are renaming the src dirs, so the build dirs with similar names should
be updated too). But it's your call.

> On 06/28/2012 06:18 PM, Omair Majid wrote:
>> On 06/28/2012 11:44 AM, Jiri Vanek wrote:
>>> On 06/28/2012 05:20 PM, Omair Majid wrote:
>> ....
>>>>
>>>> Could you rename the variable so they are more accurate? Something like
>>>> JNLP_TESTS_ENGINE_SRCDIR ->   TEST_EXTENSIONS_SRCDIR makes more
>>>> sense to
>>>
>>> sure. Give sens.. Will do tomorrow morning as new changeset (will send
>>> to review together with those already existing 5) for you.
>>>
>>>> me. Also, can you update the build dir names to match the src dir
>>>> names?
>>>
>>> Please no - my tools are depending on those directory names. I really
>>> donot want to change them, because sudenly there will be necessary two
>>> tryes to find files to compare - one in old name and second in new.
>>
>> Perhaps you can add symlinks for compatiblity? I would really like to
>> see consistent names. Otherwise it will be harder for developer to do
>> the mapping from source dir names to build dir names.
>>
>>>>> @@ -634,8 +637,24 @@
>>>>>        mkdir -p stamps&&   \
>>>>>        touch $@
>>>>>
>>>>> +netx-dist-tests-tests-source-files.txt:
>>>>
>>>> s/netx-dist-tests-tests-source-files/test-extensions-sources-files/
>>> 1
>>>>
>>>>> +    find $(JNLP_TESTS_ENGINE_TESTS_SRCDIR) -name '*.java' |
>>>>> sort>   $@
>>>>> +
>>>>> +stamps/netx-dist-tests-tests-compile.stamp: stamps/netx.stamp \
>>> 2
>>>
>>>>
>>>> A clearer name for this stamp too, please.
>>>
>>> hmh.. they are not so bad.
>>>
>>> If you insists, then first will become
>>> test-extensions-tests-sources-files.
>>>
>>> Th rest is derived from top level target:
>>
>> We might want to consider better names for top-level targets too (and
>> leave older targets for compatibility)
>>
>>> run-netx-dist-tests =>
>>> netx-dist-tests-source-files
>>> netx-dist-tests-tests-source-files
>>>
>>> But if you insists I will chnage  both netx-dist-tests-source-files and
>>> netx-dist-tests-tests-source-files to something more reliable to new
>>> names.
>>>
>>
>> Please do.
> 
> Ok. So I have renamed variables, create one backward symlink and renamed
> not-top level targets. Does the new name give sense to you?
>>
>>>>
>>>>> + stamps/junit-jnlp-dist-dirs netx-dist-tests-tests-source-files.txt \
>>>>> + stamps/netx-dist-tests-compile.stamp
>>>>> +    mkdir -p $(JNLP_TESTS_ENGINE_TESTS_DIR);
>>>>> +    $(BOOT_DIR)/bin/javac $(IT_JAVACFLAGS) \
>>>>> +     -d $(JNLP_TESTS_ENGINE_TESTS_DIR) \
>>>>> +     -classpath
>>>>> $(JUNIT_JAR):$(NETX_DIR)/lib/classes.jar:$(JNLP_TESTS_ENGINE_DIR) \
>>>>
>>>> The tests for the test-extensions depend on netx to build? Seems rather
>>>> weird.
>>>
>>> I will check this before tomorrow morning new-set-of-patches
>>
>> Thanks!
> 
> Checked. NJot necessary indedd. Although this particular one change i
> have included into "adapted makefile" I'm not going to push it if you
> agree with my initial recommendation  This issue is little bit spread. I
> would like to investiagte where it is safe to remove (right now I'm sure
> in three cases).

Okay, this can be done later, then.

> Anyway -  Do you think it is worthy? I can imagine few cases when
> reproducer(testcase,extension) will would like to know something from
> netx.jar... Although it do not need now, it can in future.. So it will
> be work to add it.

I am fairly sure testcases need netx.jar on classpath to build. As for
test extensions...perhaps we may add jnlp or applet specific extensions
there in the future. I am no longer sure my original suggestion was a
good idea :)

> Anyway(2:) - I would like to remove those dependencies as separate
> changset - Right after we agree on renaming as you proposed.

> # HG changeset patch
> # User Jiri Vanek <jvanek at redhat.com>
> # Date 1340971958 -7200
> # Node ID c913c1fd1138c833df13146da42c9d0a3512d7fd
> # Parent  5b088a10b659082df3abe9629cc8dae9cc453dd3
> Renamed make variables to correspond with new directory structure
> 
> diff --git a/Makefile.am b/Makefile.am
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -12,19 +12,21 @@
>  export TESTS_DIR=$(abs_top_builddir)/tests.build
>  
>  export NETX_UNIT_TEST_SRCDIR=$(TESTS_SRCDIR)/netx/unit
> -export NETX_UNIT_TEST_DIR=$(TESTS_DIR)/netx/unit
> +export NETX_TEST_DIR=$(TESTS_DIR)/netx
> +export NETX_UNIT_TEST_DIR=$(NETX_TEST_DIR)/unit
>  
>  export JUNIT_RUNNER_DIR=$(TESTS_DIR)/junit-runner
>  export JUNIT_RUNNER_SRCDIR=$(TESTS_SRCDIR)/junit-runner
>  
>  
> -export JNLP_TESTS_ENGINE_SRCDIR=$(TESTS_SRCDIR)/test-extensions
> -export JNLP_TESTS_ENGINE_TESTS_SRCDIR=$(TESTS_SRCDIR)/test-extensions-tests
> -export JNLP_TESTS_SRCDIR=$(TESTS_SRCDIR)/reproducers
> -export JNLP_TESTS_ENGINE_DIR=$(TESTS_DIR)/jnlp_testsengine
> -export JNLP_TESTS_ENGINE_TESTS_DIR=$(TESTS_DIR)/netx/jnlp_testsengine_tests
> -export JNLP_TESTS_SERVER_DEPLOYDIR=$(TESTS_DIR)/jnlp_test_server
> -export JNLP_TESTS_DIR=$(TESTS_DIR)/jnlp_tests
> +export TEST_EXTENSIONS_SRCDIR=$(TESTS_SRCDIR)/test-extensions
> +export TEST_EXTENSIONS_TESTS_SRCDIR=$(TESTS_SRCDIR)/test-extensions-tests
> +export REPRODUCERS_TESTS_SRCDIR=$(TESTS_SRCDIR)/reproducers
> +export TEST_EXTENSIONS_DIR=$(TESTS_DIR)/test-extensions
> +export TEST_EXTENSIONS_COMPATIBILITY_SYMLINK=$(TESTS_DIR)/netx/jnlp_testsengine
> +export TEST_EXTENSIONS_TESTS_DIR=$(TESTS_DIR)/test-extensions-tests
> +export REPRODUCERS_TESTS_SERVER_DEPLOYDIR=$(TESTS_DIR)/reproducers_test_server_deploydir

I think just REPRODUCERS_DEPLOYDIR would be sufficient, but it's up to you.

Looks fine to me.

Cheers,
Omair