RFR: 8214799: Add package declaration to each JTREG test case in the gc folder

[Leonid Mesnik]

Hi

Great change. The only a couple of comments

1) I see that you added a lot of relative links to include test-root as library like:
32  * @library ../../..

I think you might want to use 
 @library /

or add to existing library test/lib like
@library /test/lib /

instead. You might find a lot of examples in existing compiler and gc tests.

It is the more common an unified approach  rather then using relative paths. Also it allows you to don't care when you move tests in different packages.


2) New test in http://cr.openjdk.java.net/~lkorinth/8214799/01/test/hotspot/jtreg/gc/epsilon/CriticalNativeStress.new.java.html <http://cr.openjdk.java.net/~lkorinth/8214799/01/test/hotspot/jtreg/gc/epsilon/CriticalNativeStress.new.java.html>
looks confusing to me. It is copied from http://cr.openjdk.java.net/~lkorinth/8214799/01/test/hotspot/jtreg/gc/stress/CriticalNativeStress.new.java.html <http://cr.openjdk.java.net/~lkorinth/8214799/01/test/hotspot/jtreg/gc/stress/CriticalNativeStress.new.java.html>

So now we have test CriticalNativeStress gc/epsilon/CriticalNativeStress.java and CriticalNativeStressEpsilon in gc/stress/CriticalNativeStress.java
The code in both files and jtreg tags looks pretty similar. Have you added it intentionally? Could you please explain a little bit purpose of this.
 
Other changes looks good.

Leonid

> On Dec 21, 2018, at 11:32 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
> 
> Hi again!
> 
> I am adding Leonid and Erik to the discussion to see if my package additions are in any way troublesome to the bigger picture. I believe they are not. We are already using Java packages in some tests; this change will just add them to more tests so that one can easily use an IDE.
> 
> Please ignore the old webrev as I had to rebase against the resent Shenandoah push. It is easier to just read this version (http://cr.openjdk.java.net/~lkorinth/8214799/01/). Unfortunately no incremental version is given as this is a rebase.
> 
> I have not had the time to update the test cases under the gc/shenandoah folder (I am also afraid to make this change any bigger), but I have tried to update the test cases that changed or was added by the Shenandoah push (some Epsilon and Shenandoah tests outside the gc/shenandoah folder).
> 
> This rebase basically applies the same kind of changes to the added test cases. One change that I would like you to look more carefully at is the addition of a helper class CriticalNative.java that uses JNI. Without this refactoring I could not have used the same c-file as it was used in different packages (it is used from three different test cases, and the package is part of the JNI export name). Please take an extra look at that change.
> 
> New webrev:
> http://cr.openjdk.java.net/~lkorinth/8214799/01/
> 
> Testing:
> I have run jtreg tests under gc/epsilon and I have run :hotspot_gc with Shenandoah in order not to break those test cases.
> 
> Also started new run of tier1-3 and :hotspot_gc
> 
> Thanks,
> Leo
> 
> On 05/12/2018 14:44, Leo Korinth wrote:
>> Hi,
>> I have added package declarations to all jtreg tests in the gc folder (and sub folders). Previously most tests were not in a package (though some where), making it extremely hard to work with jtreg tests in an IDE.
>> The main change consists of adding a package declaration that corresponds to the directory the test is located in. This also makes it necessary to change @run annotations. Some test cases uses "common" code that is not located in the test lib. Those test cases now need to have a @library annotation that references the jtreg test root.
>> A few test cases had package visible classes with the same name in the same directory. That did work before, but does not work anymore, so I have prefixed those package visible classes with the public class name.
>> class TestVerifySilentlyRunSystemGC
>> class TestVerifySubSetRunSystemGC
>> class TestEagerReclaimHumongousRegionsReclaimRegionFast
>> class TestEagerReclaimHumongousRegionsClearMarkBitsReclaimRegionFast
>> class TestEagerReclaimHumongousRegionsWithRefsReclaimRegionFast
>> Where classes have been referenced using string literals, I have tried to use a .class.getName() instead (where possible).
>> I have tried to update copyrights to all files (also adding a missing comma after the second year in some files), and I did add missing copyright to TestFromCardCacheIndex.
>> known cons:
>> - needs a package line in each java file
>> - needs an extra library line in the test if the test is using other files.
>> - sometimes needs a run tag
>> improvements:
>> + can use eclipse or other IDEs without creating a workspace per test (I guess it will be a significant improvement for users of emacs and vi as well)
>> + can use test files located in other directories
>> After this change, I have several big cleanups that are easy to do with a working IDE.
>> Enhancement:
>> https://bugs.openjdk.java.net/browse/JDK-8214799
>> Webrev:
>> http://cr.openjdk.java.net/~lkorinth/8214799/00/
>> Testing:
>> passed:
>> mach5 remote-build-and-test --build-profiles linux-x64,linux-x64-debug,macosx-x64,solaris-sparcv9,windows-x64 --test open/test/hotspot/jtreg/:hotspot_gc
>> now running:
>> mach5 remote-test --build-id 2018-12-05-1132377.leo.korinth.hs --build-profiles linux-x64-debug -j tier1,tier2,tier3
>> Thanks,
>> Leo

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181221/89921945/attachment.htm>