RFR: JDK-8075327: Merge jdk and hotspot test libraries

[Igor Ignatyev]

Alexander,

> Therefore, I would suggest reviewing the current patch as it is , since the changes are trivial and postpone refactoring to the new packages until some time later, since such refactoring is not trivial and the current patch does provide for the merge.
I agree that we should separate merge of test-libraries and refactoring 
them. however I do have a comment regarding the current patch.

besides classes in jdk.test.lib package, hotspot test-library contains 
other common libraries:
  * jdk.test.lib.cli -- a test framework for command line option tests
  * jdk.test.lib.dcmd -- aux classes to simplify work w/ jcmd
  * jdk.test.lib.dtrace -- a test framework for dtrace tests

 From my point of view, all these packages can be used in jdk tests as 
well. so I think these packages should be moved to top-level repository.

Another reason why these packages should be neither moved nor renamed 
(to smth like hotspot.test.lib) is ambiguity. If one see classes from 
jdk.test.lib package (or its subpackages), he/she will expect to find 
their source in <top>/test, but in your current version, they are placed 
in hotspot/test.
Even if we ok, we such ambiguity there is a risk that at some point, one 
will introduce another 'jdk.test.lib.cli' in top-level test directory. 
It will be really really hard to understand which libraries should be used.

That means that all other classes from both 
jdk/test/lib/testlibrary/jdk/testlibrary/ and 
/hotspot/test/testlibrary/jdk/test/lib should be moved to <top>/test.

PS I also noticed that RandomFactory[1] class kinda duplicates 
Utils::getRandomInstance[2], could you please merge them or file an RFE 
to do it?

Stuart,
could you please review jdk part of this patch?


[1] 
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/c1947d42537b/test/lib/testlibrary/jdk/testlibrary/RandomFactory.java
[2] 
http://hg.openjdk.java.net/jdk9/jdk9/hotspot/file/c25bfaaed7f2/test/testlibrary/jdk/test/lib/Utils.java#l367
--
Thanks,
Igor

On 06/04/2015 05:23 PM, Alexander Kulyakhtin wrote:
> Stefan,
>
> There are some issues which may make it difficult to implement the grouping of the common test library files into the new packages as JDK-8075327 proposes.
>
> The changes in the current patch are trivial and made by a script (although they spread over many files).
> They are trivial because the names of the packages are left intact (except fo renaming of jdk.testlibrary to jdk.test.lib in the jdk repo).
>
> However, if we introduce the new packages per the CR description we will end up with
>
> import jdk.test.lib.*;
> import jdk.test.lib.vm.*;
> import jdk.test.lib.processtools.*;
> import jdk.test.lib.misc.*;
>
> where previously was just a single line
>
> import jdk.test.lib.*;
>
> The same goes for @build and @run directives where we end up with
>
> @build jdk.test.lib.* jdk.test.lib.vm.* jdk.test.lib.processtools.* jdk.test.lib.misc.*
>
> instead of
>
> @build jdk.test.lib.*
>
> There are many tests having import jdk.test.lib.* and @build jdk.test.lib.* and similar constructions.
>
> If we then want to deduce the minimal required dependencies for each and every jdk and hotspot test, then it will not be as trivial task as what we have currently done. It will, probably, require a significant review effort from both jdk and hotspot teams.
> The benefits of that effort are not quite evident for me though I, probably, might be missing something here.
>
> The current patch does already provide for the merge of the common part of the jdk and hotspot libraries and do eliminate the duplicated files.
>
> Therefore, I would suggest reviewing the current patch as it is , since the changes are trivial and postpone refactoring to the new packages until some time later, since such refactoring is not trivial and the current patch does provide for the merge.
>
> Best regards,
> Alexander
>
> ----- Original Message -----
> From: alexander.kulyakhtin at oracle.com
> To: stefan.sarne at oracle.com
> Cc: jdk9-dev at openjdk.java.net, yekaterina.kantserova at oracle.com, hotspot-dev at openjdk.java.net
> Sent: Wednesday, June 3, 2015 3:33:22 PM GMT +03:00 Iraq
> Subject: Re: RFR: JDK-8075327: Merge jdk and hotspot test libraries
>
>
>
> Hi Stefan,
>
> Thank you very much for the review. I'll update the patch accordingly, shortly.
>
> Best regards,
> Alexander
>
> ----- Original Message -----
> From: stefan.sarne at oracle.com
> To: alexander.kulyakhtin at oracle.com, jdk9-dev at openjdk.java.net, hotspot-dev at openjdk.java.net
> Cc: yekaterina.kantserova at oracle.com
> Sent: Wednesday, June 3, 2015 12:08:33 PM GMT +03:00 Iraq
> Subject: Re: RFR: JDK-8075327: Merge jdk and hotspot test libraries
>
>
>
> Hi Alexander,
>
> I think we want to group the utilities into packages directly.
>
> One reason for grouping is to support discovery, to clarify what belongs together. For example both the OutputAnalyzer and the StreamPumper belong together in a processtools package, while Asserts doesn't.
>
> Another reason is to simplify imports and reduce the impact of wild card includes, since for example an import of process tools should not have to bother with declaring usage of sun.misc.Unsafe in the modules world, just because Utils happen to be in the same package.
>
> Here are some examples - also available in the jira case:
>
> Package jdk.test.lib.processtools is the package for utilities for launching processes and analyzing the output.
> <jdk9 root>/test/lib/share/classes/jdk/test/lib/processtools/OutputAnalyzer.java
> <jdk9 root>/test/lib/share/classes/jdk/test/lib/processtools/OutputBuffer.java
> <jdk9 root>/test/lib/share/classes/jdk/test/lib/processtools/ProcessTools.java
> <jdk9 root>/test/lib/share/classes/jdk/test/lib/processtools/StreamPumper.java
> <jdk9 root>/test/lib/share/classes/jdk/test/lib/processtools/JDKToolFinder.java
> <jdk9 root>/test/lib/share/classes/jdk/test/lib/processtools/JDKToolLauncher.java
>
> <jdk9 root>/test/lib-test/jdk/test/lib/processtools/OutputAnalyzerTest.java
>
>
> Asserts can stay at top level.
> <jdk9 root>/test/lib/share/classes/jdk/test/lib/Asserts.java
> <jdk9 root>/test/lib-test/jdk/test/lib/AssertsTest.java
>
>
> For VM specific info, it would have vm package. Note that the whitebox API moves here.
> <jdk9 root>/test/lib/share/classes/jdk/test/lib/vm/InputArguments.java
> <jdk9 root>/test/lib/share/classes/jdk/test/lib/vm/Platform.java
>
>
> Ok, so then there are the stuff which just is a bucket of stuff and fluff.
> A later exercise could be to break this class into better named and placed classes, but this is a start:
> <jdk9 root>/test/lib/share/classes/jdk/test/lib/misc/Utils.java
>
> Thanks,
> Stefan
>
>
> Alexander Kulyakhtin skrev den 2015-06-02 13:18:
>
>
> Hi,
>
> Could you, please, review the following test-only changes to the 'jdk', 'hotspot' and the common 'test' repository http://cr.openjdk.java.net/~akulyakh/8075327/wr_hs/webrev.01/index.html http://cr.openjdk.java.net/~akulyakh/8075327/wr_jdk/webrev.01/index.html http://cr.openjdk.java.net/~akulyakh/8075327/wr_test/webrev.01/ The changes are to move test library files common to both jdk and hotspot to the upper-level test repository.
>
> The following has been done:
> 1) Common files (Asserts.java, OutputAnalyzer.java etc) from the jdk and hotspot repositories,  have been merged and moved to the upper-level common test repository, to test/lib/share/classes.
> 2) Package jdk.testlibrary in the jdk repository have been renamed to jdk.test.lib to have the same name as in the hotspot repository
> 2) Upper level test/lib/share/classes library references have been added to the @library statements
> 3) Some packages previously located at upper level test repo at /test/lib have been moved to /test/lib/share/classes for consistency
>
> Best regards,
> Alexander
>