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

[Igor Ignatyev]

Thanks for the review Misha.

can I get a LGTM from a Reviewer?

-- Igor

> On Mar 14, 2019, at 3:53 PM, mikhailo.seledtsov at oracle.com wrote:
> 
> 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 <https://bugs.openjdk.java.net/browse/JDK-8219139>
>>>>>>> testing: tier[1-2] (in progress), all the touched tests locally
>>>>>>> Thanks,
>>>>>>> -- Igor
>> 
>