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.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
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
