LGTM
--alex
On 04/13/2020 10:35, Igor Ignatyev wrote:
sure.
@alias, can I get a 2nd review for this patch?
Thanks,
-- Igor
On Apr 9, 2020, at 9:45 PM, Chris Plummer <[email protected]
<mailto:[email protected]>> wrote:
Hi Igor,
I think it's best to get another review.
thanks,
Chris
On 4/9/20 8:49 PM, Igor Ignatev wrote:
Thanks Chris! Would you consider this trivial or should I wait for
another review?
— Igor
On Apr 9, 2020, at 8:35 PM, Chris Plummer <[email protected]>
wrote:
Hi Igor,
The changes looks good.
thanks,
Chris
On 4/9/20 4:13 PM, Igor Ignatyev wrote:
Hi Chris,
I looked what transport got skipped on Windows, and it's dt_shmem,
as I didn't see any reasons why this test should not be run w/ this
transport, I looked deeper and I found the place which needs to be
updated to make the test work w/ dt_shmem. so now the test doesn't
skip any transports (and thus the removed warning isn't even
partially true) and only skips connector other than
c.s.j.RawCommandLineLaunch, to make it cleaner the test now logs
the skipped connectors, e.g. on my mac, I'm getting the following
in .jtr
skipping com.sun.jdi.CommandLineLaunch (defaults:
home=/Users/iignatye/ws/jdk/jdk/build/macosx-x64/images/jdk,
options=, main=, suspend=true, quote=", vmexec=java)
I've retested the patch on the same platforms as before and now it
passes on windows as well.
incremental webrev:
http://cr.openjdk.java.net/~iignatyev//8242471/webrev.0-1/index.html
full webrev:
http://cr.openjdk.java.net/~iignatyev//8242471/webrev.01/index.html
Thanks,
-- Igor
On Apr 9, 2020, at 3:15 PM, Chris Plummer
<[email protected] <mailto:[email protected]>> wrote:
Hi Igor,
89 log.display("Connector's transport is: " +
c.transport().name());
Would be nice if you printed each transport's name (even the
skipped ones). That way when reading the log after getting
SkippedException you can see which transports were attempted.
You removed the following:
50 * ================================================
51 * WARNING:
52 * Temporarily the test is prepared only for
53 * Sparc.Solaris.dt_socket-transport of RawCommandLineLaunch
54 * connector
55 * ================================================
Isn't this still somewhat true? It's still requires dt_socket, but
no longer solaris-sparc. This is part of the reason I asked above
to print which transports are attempted. I'm curious what the
skipped transport is on Windows.
thanks,
Chris
On 4/9/20 2:43 PM, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev/8242471/webrev.00/
38 lines changed: 8 ins; 23 del; 7 mod;
Hi all,
could you please review this small patch for
nsk/jdi/Argument/value/value004 test?
from JBS:
nsk/jdi/Argument/value/value004 test has restrictions to be run
only on solaris-sparc and w/ dt_socket transport, solaris-sparc
restriction seems to be not valid any more. the test also should
be updated to use skipped exception if there are no suitable
connectors available.
JBS: https://bugs.openjdk.java.net/browse/JDK-8242471
testing: the changed test on {linux,windows,macosx}-x64 and
solaris-sparcv9, w/ the test being skipped on windows-x64
webrev: http://cr.openjdk.java.net/~iignatyev/8242471/webrev.00/
Thanks,
-- Igor
