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

Mon Jan 14 13:43:08 UTC 2019

On 11/01/2019 18:30, Leonid Mesnik wrote:
> Hi
> 
> The changes look good to me. Thanks for fixing all this.  Please note that I am not a 'R'eviewer.
> 
> Leonid

Thank you Leonid for your (committer) review and suggestions for 
improvement.

/Leo

>> On Jan 11, 2019, at 8:54 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
>>
>> Hi again!
>>
>> New webrev, no incremental this time either as I have changed all copyright years, all library annotations, and several new tests have been rebased.
>>
>> Changes since last time:
>> 1) The bad rebase artifact (bad rebase of moved file) CriticalNativeStress.java is now removed. I still left the small refactoring to a Java JNI "mirror" class as I think it is nicer than the old solution with several Java files in different packages mapping to the same JNI file.
>> 2) All relative @library lines now uses the absolute "/" that will improve future refactoring. I still have them on separate lines if several paths are used -- my reason is that they diff better.
>> 3) All new test cases in the new directory nvdimm have been fixed
>> 4) A new test in directory epsilon has been fixed
>> 5) All (Oracle) copyright years have been updated to 2019, I did not bother to add Oracle copyrights to the Epsilon/Shenandoah sources as my contribution is insignificant.
>> 6) Shenandoah has landed, but I will not update all those test cases, I made this webrev too large already. I have tried to update all Shenandoah test cases that are located outside the shenendoah folder though.
>>
>> (New) Webrev:
>> http://cr.openjdk.java.net/~lkorinth/8214799/02/
>>
>> Testing:
>> open/test/hotspot/jtreg/:hotspot_gc
>>
>> Thanks,
>> Leo
>>
>>
>> On 22/12/2018 12:10, Leo Korinth wrote:
>>> Hi!
>>> On 21/12/2018 21:50, Leonid Mesnik wrote:
>>>> 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.
>>>>
>>> I will try that, it is certainly better. I think I will use a new @library line though, it is easier to see the change.
>>>>
>>>> 2) New test in 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
>>>>
>>>> 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.
>>> Many thanks for finding this fault of mine. The file was moved with the Shenandoah push, and I must have rebased it badly although the move of the corresponding c-file went through, strange...
>>>> Other changes looks good.
>>>>
>>> Thanks for taking a look at it!
>>> I will create a new webrev after the holiday.
>>> /Leo
>>>> Leonid
>>>>> On Dec 21, 2018, at 11:32 AM, Leo Korinth <leo.korinth at oracle.com
>>>>> <mailto: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
>>>>
> 