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

[mikhailo.seledtsov at oracle.com]

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 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> 
>>> wrote:
>>>
>>> Hi Igor,
>>>
>>> On 21/02/2019 3:19 pm, Igor Ignatyev wrote:
>>>> http://cr.openjdk.java.net/~iignatyev//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
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8219139
>>>> testing: tier[1-2] (in progress), all the touched tests locally
>>>> Thanks,
>>>> -- Igor
>