Ping: RFR: JDK-8209604: [TEST] rewrite com/sun/jdi shell tests to java version - step2
JC Beyler
jcbeyler at google.com
Wed Aug 29 00:02:30 UTC 2018
Hi Alex,
Fair enough and good to know! Looks good to me then :)
Jc
On Tue, Aug 28, 2018 at 4:55 PM Alex Menkov <alexey.menkov at oracle.com>
wrote:
> Hi Jc,
>
> The coding conventions say:
> Constant names in upper-case or mixed-case are tolerated, according to
> historical necessity.
> I didn't find special rule for enums.
>
> The values are converted to string by using Enum.toString, so lowercase
> enum values are used to get lowercase strings.
> I don't see much sense to use extra toLowerCase();
>
> --alex
>
> On 08/28/2018 15:03, JC Beyler wrote:
> > 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
> > <
> http://cr.openjdk.java.net/%7Eamenkov/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
> > <mailto: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
> > <mailto: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/
> > <http://cr.openjdk.java.net/%7Eamenkov/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
> > <mailto: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
> > <
> http://cr.openjdk.java.net/%7Eamenkov/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
> > <
> http://cr.openjdk.java.net/%7Eamenkov/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
> > <
> http://cr.openjdk.java.net/%7Eamenkov/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
> > <
> http://cr.openjdk.java.net/%7Eamenkov/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/
> > <http://cr.openjdk.java.net/%7Eamenkov/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
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180828/153fdd3f/attachment-0001.html>
More information about the serviceability-dev
mailing list