RFR: 8214799: Add package declaration to each JTREG test case in the gc folder
Leo Korinth
leo.korinth at oracle.com
Thu Jan 17 15:58:34 UTC 2019
Hi,
I have added package name to newly added TestPeriodicLogMessages.java.
I would be happy for a review from a reviewer (for the whole change, not
only this test).
The updated test case has been tested.
/Leo
diff --git a/test/hotspot/jtreg/gc/g1/TestPeriodicLogMessages.java
b/test/hotspot/jtreg/gc/g1/TestPeriodicLogMessages.java
index 2cede583c45..bd02a6e1c0a 100644
--- a/test/hotspot/jtreg/gc/g1/TestPeriodicLogMessages.java
+++ b/test/hotspot/jtreg/gc/g1/TestPeriodicLogMessages.java
@@ -21,6 +21,8 @@
* questions.
*/
+package gc.g1;
+
/**
* @test TestPeriodicLogMessages
* @bug 8216490
@@ -29,6 +31,7 @@
* @library /test/lib /
* @modules java.base/jdk.internal.misc
* @modules java.management/sun.management
+ * @run main gc.g1.TestPeriodicLogMessages
*/
import jdk.test.lib.process.OutputAnalyzer;
On 14/01/2019 14:43, Leo Korinth wrote:
> 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
>>>>>
>>
More information about the hotspot-gc-dev
mailing list