RFR(S) : 8219139 : move hotspot tests from test/jdk/vm
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? 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 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
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! BTW: test/hotspot/jtreg/runtime/ShiftTest.java is actually a jit test according to the test comment.
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. I really think the value of these tests needs to be examined before they are brought over. 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
On Feb 21, 2019, at 12:11 AM, David Holmes <david.holmes@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
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@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
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@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@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
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.seledtsov@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@oracle.com <mailto:mikhailo.seledtsov@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@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
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@oracle.com <mailto:mikhailo.seledtsov@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@oracle.com <mailto:mikhailo.seledtsov@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@oracle.com <mailto:david.holmes@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 testing: tier[1-2] (in progress), all the touched tests locally Thanks, -- Igor
Thanks for the review Misha. can I get a LGTM from a Reviewer? -- Igor
On Mar 14, 2019, at 3:53 PM, mikhailo.seledtsov@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@oracle.com <mailto:mikhailo.seledtsov@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@oracle.com <mailto:mikhailo.seledtsov@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@oracle.com <mailto:david.holmes@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
Hi Igor, This all seems fine to me. Thanks, David On 15/03/2019 7:38 am, 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
Thanks, -- Igor
On Mar 4, 2019, at 1:57 PM, mikhailo.seledtsov@oracle.com <mailto:mikhailo.seledtsov@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@oracle.com <mailto:mikhailo.seledtsov@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@oracle.com <mailto:david.holmes@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
On 21/02/2019 05:19, 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?
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 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
The tools/launcher directory is the tests for the java launcher, I don't think it's the right place for tests for the JNI invocation interface - I think that one is asking to be moved to be with the other JNI tests. -Alan
On Feb 21, 2019, at 12:19 AM, Alan Bateman <Alan.Bateman@oracle.com> wrote: On 21/02/2019 05:19, 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?
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 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
The tools/launcher directory is the tests for the java launcher, I don't think it's the right place for tests for the JNI invocation interface - I think that one is asking to be moved to be with the other JNI tests. Alan, you are right, this test is a JNI test and should be moved to hotspot/runtime/jni. more details in my answer to the same comment in David's email. in two words, I accidentally looked at another test.
-- Igor
-Alan
On 21/02/2019 19:55, Igor Ignatyev wrote:
: Alan, you are right, this test is a JNI test and should be moved to hotspot/runtime/jni. more details in my answer to the same comment in David's email. in two words, I accidentally looked at another test.
Can you double check that it is actually using the JNI invocation interface directly? I don't think we were able to find anyone in Eclipse to explain what their launcher on macOS is doing. I suspect it may be directly (or indirectly) reading the CFBundleExecutable property from Info.plist and calling the JNI_CreateJavaVM function in libjli (not libjvm). We probably need more tests in this area and also a bit more archaeology to see whether this was a supported interface in Apple's JDK. -Alan
Hi Alan, I double checked, the test is linked w/ -ljli, and calls JNI_CreateJavaVM from libjli.so. hence I'll leave JniInvocationTest in jdk/tools/launcher. -- Igor
On Feb 21, 2019, at 12:17 PM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 21/02/2019 19:55, Igor Ignatyev wrote:
: Alan, you are right, this test is a JNI test and should be moved to hotspot/runtime/jni. more details in my answer to the same comment in David's email. in two words, I accidentally looked at another test.
Can you double check that it is actually using the JNI invocation interface directly? I don't think we were able to find anyone in Eclipse to explain what their launcher on macOS is doing. I suspect it may be directly (or indirectly) reading the CFBundleExecutable property from Info.plist and calling the JNI_CreateJavaVM function in libjli (not libjvm). We probably need more tests in this area and also a bit more archaeology to see whether this was a supported interface in Apple's JDK.
-Alan
On 14/03/2019 21:33, Igor Ignatyev wrote:
Hi Alan,
I double checked, the test is linked w/ -ljli, and calls JNI_CreateJavaVM from libjli.so. hence I'll leave JniInvocationTest in jdk/tools/launcher.
Thanks for checking, in which case I think we need to find a better name for this test as JniInvocationTest is a misleading name when it's a JLI rather than JNI test. -Alan.
participants (4)
-
Alan Bateman
-
David Holmes
-
Igor Ignatyev
-
mikhailo.seledtsov＠oracle.com