Thanks for the review Misha. can I get a LGTM from a Reviewer?
-- Igor > On Mar 14, 2019, at 3:53 PM, mikhailo.seledt...@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.seledt...@oracle.com >>> <mailto:mikhailo.seledt...@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.seledt...@oracle.com >>> <mailto:mikhailo.seledt...@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.hol...@oracle.com >>>>>> <mailto:david.hol...@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 >> >
