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

[mikhailo.seledtsov at oracle.com]

   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

      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