RFR(S) : 8219139 : move hotspot tests from test/jdk/vm

Thu Mar 14 22:53:07 UTC 2019

Looks good to me,

Thank you,

Misha

On 3/14/19 2:38 PM, Igor Ignatyev wrote:
> Hi Misha,
>
> thanks for your suggestions, I have moved all runtime tests into 
> subdirectories. here is the updated webrev: 
> http://cr.openjdk.java.net/~iignatyev//8219139/webrev.01/index.html 
> <http://cr.openjdk.java.net/%7Eiignatyev//8219139/webrev.01/index.html>
>
> Thanks,
> -- Igor
>
>> On Mar 4, 2019, at 1:57 PM, mikhailo.seledtsov at oracle.com 
>> <mailto:mikhailo.seledtsov at oracle.com> wrote:
>>
>> Hi Igor,
>>
>>   Sorry it took a while to get back to you on this one. See my 
>> comment below
>>
>>
>> On 2/22/19 10:35 AM,mikhailo.seledtsov at oracle.com 
>> <mailto:mikhailo.seledtsov at oracle.com>wrote:
>>> Overall the change looks good; thank you Igor for doing this. I have 
>>> couple of comments:
>>>
>>> - I am in favor of the approach where we move tests first under 
>>> corresponding sub-component directories in
>>> test/hotspot sub-tree, and then figure out whether to keep them. 
>>> Even though in general I am in favor
>>> of removing tests that are obsolete or of questionable value, this 
>>> requires time, consideration and discussions.
>>> Hence, I recommend filing an RFE for that, which can be addressed 
>>> after the migration.
>>>
>>> - Runtime normally avoids tests directly in test/hotspot/jtreg/runtime
>>> The tests should go into underlying sub-directories which best match 
>>> functional area or topic of that test.
>>> In most cases you did it in your change, but there are several tests 
>>> that your change places directly under
>>> test/hotspot/jtreg/runtime/:
>>>
>>> ExplicitArithmeticCheck.java
>>> MonitorCacheMaybeExpand_DeadLock.java
>>> ReflectStackOverflow.java
>>> ShiftTest.java - David commented this can go under compiler (a jit test)
>>> WideStrictInline.java
>> I have looked at the tests in more detail, and here are my 
>> recommendation of placements:
>>     ExplicitArithmeticCheck
>>         This test checks that ArithmeticException is thrown when 
>> appropriate
>>         I would recommend placing it under runtime/ErrorHandling
>> MonitorCacheMaybeExpand_DeadLock
>>         Existing folder: runtime/Thread (it does have a locking test)
>>         Or, alternatively, create a new folder: 'locking' or 'monitors'
>>     ReflectStackOverflow
>>         Uses recursive reflection attempting to provoke stack overflow
>>         Can go under: runtime/reflect
>>     WideStrictInline:
>>         checks for correct FP inlining by the interpreter
>>         I could not find existing sections; perhaps create 'interpreter'
>>         folder under 'runtime'
>>
>> Thank you,
>> Misha
>>>
>>> Since we plan (as discussed) to follow up this work with an RFE to 
>>> review and consider removal of old and
>>> not-that-useful tests, you could place them under 'misc' for now. 
>>> Alternatively, find the best match
>>> or create new sub-directories under runtime/ if necessary.
>>>
>>>
>>> Thank you,
>>> Misha
>>>
>>>
>>> On 2/21/19 11:53 AM, Igor Ignatyev wrote:
>>>>
>>>>> On Feb 21, 2019, at 12:11 AM, David Holmes 
>>>>> <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
>>>>>
>>>>> Hi Igor,
>>>>>
>>>>> On 21/02/2019 3:19 pm, Igor Ignatyev wrote:
>>>>>> http://cr.openjdk.java.net/~iignatyev//8219139/webrev.00/index.html 
>>>>>> <http://cr.openjdk.java.net/%7Eiignatyev//8219139/webrev.00/index.html>
>>>>>>> 40 lines changed: 17 ins; 2 del; 21 mod;
>>>>>> Hi all,
>>>>>> could you please review this small patch which moves tests from 
>>>>>> test/jdk/vm?
>>>>> I find some of these tests - the runtime ones at least - of 
>>>>> extremely dubious value. They either cover basic functionality 
>>>>> that is already well covered, or are regression tests for bugs in 
>>>>> code that hasn't existed for many many years!
>>>> as I wrote in another related email: "one of the reason I'm 
>>>> proposing this move is exactly questionable value of these tests, I 
>>>> want to believe that having these tests in hotspot/ test 
>>>> directories will bring more attention to them from corresponding 
>>>> component teams and hence they will be removed/reworked/re-whatever 
>>>> faster and better. and I also believe that one of the reason we got 
>>>> duplications exactly because these tests were located in jdk test 
>>>> suite."
>>>>
>>>>> BTW:
>>>>>
>>>>> test/hotspot/jtreg/runtime/ShiftTest.java
>>>>>
>>>>> is actually a jit test according to the test comment.
>>>> sure, I will move it to hotspot/compiler.
>>>>>> there are 16 tests in test/jdk/vm directory. all but 
>>>>>> JniInvocationTest are hotspot tests, so they are moved to 
>>>>>> test/hotspot test suite; JniInvocationTest is a launcher test
>>>>> No its a JNI invocation API test - nothing to do with our 
>>>>> launcher. It belongs in runtime/jni. But we already have tests in 
>>>>> runtime that use the JNI invocation API so this test adds no new 
>>>>> coverage.
>>>> this is actually was my first reaction, and I even have the webrev 
>>>> which moves it to runtime/jni, but then I looked at the associated 
>>>> bug and it is filed against tools/launcher. and I even got a false 
>>>> (as I know by now) memory that I saw JLI_ method being called from 
>>>> the test. there is actually another test 
>>>> (dk/tools/launcher/exeJliLaunchTest.c) associated w/ this bug which 
>>>> calls JLI_Launch. anyhow, I'll move this test to hotspot/runtime/jni.
>>>>
>>>>> I really think the value of these tests needs to be examined 
>>>>> before they are brought over.
>>>> I'd prefer to have follow-up RFEs/tasks, b/c the longer we keep 
>>>> jdk/vm directory the more tests can end up there and the more 
>>>> rotten these tests become.
>>>>
>>>> Thanks,
>>>> -- Igor
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> and hence it's moved to test/jdk/tools/launcher. as 
>>>>>> hotspot/compiler and hotspot/gc tests use packages, the tests 
>>>>>> moved there have been updated to have a package statement.
>>>>>> webrev: 
>>>>>> http://cr.openjdk.java.net/~iignatyev//8219139/webrev.00/index.html 
>>>>>> <http://cr.openjdk.java.net/%7Eiignatyev//8219139/webrev.00/index.html>
>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8219139
>>>>>> testing: tier[1-2] (in progress), all the touched tests locally
>>>>>> Thanks,
>>>>>> -- Igor
>