Ping: RFR: JDK-8209604: [TEST] rewrite com/sun/jdi shell tests to java version - step2

[JC Beyler]

Hi Alexey,

I noticed:
http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev.01/test/jdk/com/sun/jdi/lib/jdb/JdbCommand.java.udiff.html

- Should the enum types be in lower case? Other exampes in the vmTestBase
and in jdk/com/sun seem to put them in upper case, no?

Apart from that, it looks good to me,
Jc

On Tue, Aug 28, 2018 at 2:03 PM Alex Menkov <alexey.menkov at oracle.com>
wrote:

> Need one more reviewer for the fix.
>
> --alex
>
> On 08/22/2018 10:18, serguei.spitsyn at oracle.com wrote:
> > Hi Alex,
> >
> > This looks good.
> > Thank you for the update!
> >
> > Thanks,
> > Serguei
> >
> >
> > On 8/21/18 17:10, Alex Menkov wrote:
> >> Hi guys,
> >>
> >> Updated webrev:
> >> http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev.01/
> >>
> >> changes (vs initial fix):
> >> - Eval*.java tests are updated, common methods are moved into JdbTest;
> >> - $classname substitution is dropped in EvalArgs.java;
> >> - comment are reintroduced;
> >> - line separator logic was dropped from JdbCommand (it's not needed as
> >> JdbCommand contains static factory methods to create commands).
> >>
> >> Update of String.split to use "\\R" is done in other fix:
> >> JDK-8209605: com/sun/jdi/BreakpointWithFullGC.java fails with ZGC
> >>
> >> --alex
> >>
> >> On 08/20/2018 19:12, David Holmes wrote:
> >>> Picking up on one point ...
> >>>
> >>> On 21/08/2018 9:40 AM, serguei.spitsyn at oracle.com wrote:
> >>>> Hi Alex,
> >>>>
> >>>> It looks good in general.
> >>>> Some minor comments below.
> >>>>
> >>>>
> >>>>
> http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/lib/jdb/JdbCommand.java.frames.html
> >>>>
> >>>>
> >>>>   132     private static final String ls =
> >>>> System.getProperty("line.separator");
> >>>>
> >>>>    Minor: Replace variable name "ls" with "LINE_SEP" (in upper case
> >>>> as it is a static final).
> >>>
> >>> There have been bugs in the past where tests use the wrong
> >>> line-separator because there is confusion as to which output comes
> >>> via the OS (and so has platform line-separators) and which comes more
> >>> directly (e.g. via socket) and so has the language line-separator.
> >>> Are we sure we will always be dealing with the platform
> >>> line-separator here?
> >>>
> >>> For String.split use of the regex \\R for the line-separators seems
> >>> the best solution.
> >>>
> >>> Thanks,
> >>> David
> >>>
> >>>>
> >>>>
> >>>>
> http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalArgs.java.frames.html
> >>>>
> >>>>
> >>>>
> >>>>    Just wanted to double-check that this check:
> >>>> 262 jdbFailIfPresent "Arguments match no method"
> >>>>
> >>>>    works in each call of the verifyNoArgumentsMatchMethod()
> >>>>    instead of just once as it was originally.
> >>>>
> >>>> 214 private void verifyNoArgumentsMatchMethod(String expr) {
> >>>> 215 List<String> reply =
> >>>> jdb.command(JdbCommand.eval(expr.replace("$classname",
> >>>> DEBUGGEE_CLASS)));
> >>>> 216 new
> >>>>
> OutputAnalyzer(reply.stream().collect(Collectors.joining(lineSeparator)))
> >>>>
> >>>> 217 .shouldNotContain("Arguments match no method");
> >>>> 218 }
> >>>>
> >>>>   Minor: It feels like the method name need a tweak.
> >>>>          Something like: verifyNoArgumentsMatchNoMethod
> >>>>
> >>>>
> >>>> 292 // These overload calls should fail because ther Minor: A
> >>>> suggestion is to fix a typo at the end of comment.
> >>>>
> >>>>
> >>>>
> http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalArraysAsList.java.frames.html
> >>>>
> >>>>
> >>>>    The comment was not converted:
> >>>>
> >>>> 31 # The test checks if evaluation of the expression
> >>>> java.util.Arrays.asList(null, "a")
> >>>> 32 # works normally and does not throw an IllegalArgumentException.
> >>>>
> >>>>
> >>>>
> >>>>
> http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalInterfaceStatic.java.frames.html
> >>>>
> >>>>
> >>>>    The comment was not converted:
> >>>>
> >>>> 33 # The test exercises the ability to invoke static methods on
> >>>> interfaces.
> >>>> 34 # Static interface methods are a new feature added in JDK8.
> >>>> 35 #
> >>>> 36 # The test makes sure that it is, at all, possible to invoke an
> >>>> interface
> >>>> 37 # static method and that the static methods are not inherited by
> >>>> extending
> >>>> 38 # interfaces.
> >>>>
> >>>>
> >>>>
> >>>> Thanks,
> >>>> Serguei
> >>>>
> >>>>
> >>>>
> >>>> On 8/16/18 14:13, Alex Menkov wrote:
> >>>>> Hi all,
> >>>>>
> >>>>> Please review next chunk of shell->java test conversion.
> >>>>> jira: https://bugs.openjdk.java.net/browse/JDK-8209604
> >>>>> webrev: http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/
> >>>>>
> >>>>> The fix contains some changes in library classes:
> >>>>> Jdb.java - timeouts are updated (as per Dan note in one of previous
> >>>>> review, timeouts should respect timeout factor, Utils.adjustTimeout
> >>>>> implements the functionality);
> >>>>> JdbCommand.java - new jdb commands (required by tests) are added.
> >>>>>
> >>>>> --alex
> >>>>
> >
>


-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180828/be9a83d2/attachment-0001.html>