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/~iignatyev//8219139/webrev.01/index.html>
Thanks, -- Igor > On Mar 4, 2019, at 1:57 PM, 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> 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
