[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: Ping: RFR: JDK-8209604: [TEST] rewrite com/sun/jdi shell tests to java version - step2
From: Chris Plummer <chris.plummer () oracle ! com>
Date: 2018-08-30 2:08:20
Message-ID: c0635895-ce91-3d0e-89c9-6fe258153c7b () oracle ! com
[Download RAW message or body]
Hi Alex,
Overall looks good. Just a couple minor comment suggestions below.
On 8/29/18 3:04 PM, Alex Menkov wrote:
> Hi Chris,
>
> Updated fix:
> http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev.02/
>
> see replies inline.
>
> On 08/28/2018 23:18, Chris Plummer wrote:
> > Hi Daniil,
> >
> > Overall the changes look good. A few cleanup suggestions are below:
> >
> > 63 setBreakpoints(System.getProperty("test.src") +
> > "/CatchAllTest.java", 1);
> >
> > This code is repeated a lot, and all current uses always use
> > test.src. Can the getProperty part just be moved into setBreakpoints?
>
> Done.
> Renamed the method name to ensure all calls are updated.
> Had to update some other tests which use the method.
> Also replaced calls of jdb.getJdbOutput()/jdb.getDebuggeeOutput() with
> JdbTest.getJdbOutput()/JdbTest.getDebuggeeOutput() as it's required
> for upcoming fixes to decrease noise in the future.
>
> >
> > 280 final String invalidTypeException =
> > "InvalidTypeException";
> > 281
> > 282
> > evalShouldContain("EvalArgsTarg.ffboolean(EvalArgsTarg.jjbyte)",
> > invalidTypeException);
> > 283
> > 284
> > evalShouldContain("EvalArgsTarg.ffintArray(EvalArgsTarg.jjint)",
> > invalidTypeException);
> > 285
> > 286
> > evalShouldContain("EvalArgsTarg.ffintArray(EvalArgsTarg.jjfloatArray)",
> > invalidTypeException);
> > 287
> > 288 evalShouldContain("EvalArgsTarg.ffjj2(EvalArgsTarg.myjj1)",
> > invalidTypeException);
> > 289
> > 290 evalShouldContain("EvalArgsTarg.ffjj2(EvalArgsTarg.myoranges)",
> > invalidTypeException);
> >
> > I think the invalidTypeException variable is unnecessary and just
> > causes the reader to go find what it is actually set to. Also, line
> > spacing is not needed here.
>
> The idea of the constant is to avoid typos in string constants (we
> need to ensure the string is correct only in one place, then compiler
> doesn't allow to mistype variable name).
> Line spacing is dropped so variable definition is close to the calls.
>
> >
> > 42 * The test makes sure that it is, at all, possible to invoke
> > an interface
> > 43 * static method and that the static methods are not inherited
> > by extending
> > 44 * interfaces.
> >
> > I think the "at all" comment should be removed. No idea why it is there.
>
> Done.
>
> >
> > 40 /*
> > 41 * This is another test of the same bug. The bug occurs when
> > trying
> > 42 * to walk the stack of a deoptimized thread. We can do this
> > 43 * by running in -Xcomp mode and by doing a step which causes
> > deopt,
> > 44 * and then a 'where'. This will cause not all the frames to
> > be shown.
> > 45 */
> >
> > I know you didn't change this comment, but what is "the same bug" in
> > reference too. Also, is says tested using -Xcomp, but the original
> > shell script version never seemed to do this. I guess it relied on
> > -Xcomp testing for the whole test run. You seem to have fixed this in
> > the java version so the debugee is always run with -Xcomp, which
> > seems like the right thing to do.
>
> In the test header:
> * @test
> * @bug 4525714
> * @summary jtreg test PopAsynchronousTest fails in build 85 with -Xcomp
> * @comment converted from test/jdk/com/sun/jdi/DeoptimizeWalk.sh
> I.e. the bug (4525714) is about failing PopAsynchronousTest test, but
> this test (DeoptimizeWalk) performs additional testing for the issue.
Can you capture this in the comment?
>
> >
> > 39 * The bug is that, for example, if a String is passed
> > 40 * as an arg to a func where an Object is expected,
> > 41 * the above error occurs. jdb doesnt notice that this is
> > 42 * legal because String is an instance of Object.
> >
> > Another unmodified comment that could use some cleaning up. What is
> > "the above error"? A better description of what is actually being
> > tested would be useful. Change "doesnt" to "doesn't".
>
> This case is similar - look at the test header
> * @test
> * @bug 4663146
> * @summary Arguments match no method error
> * @comment converted from test/jdk/com/sun/jdi/EvalArgs.sh
>
> So "the above error" is "Arguments match no method" error.
Also capture this in the comment.
>
> > Change "doesnt" to "doesn't"
> Done.
>
> >
> > BTW, the navigation buttons at the bottom of your webrev don't seem
> > to work starting with the 3rd file. The URL gets messed up.
>
> As far as I see the problem is renamed files - for some reason webrev
> doesn't correctly handle theirs URLs (the URLs contain both old and
> new names).
> Sorry, have no idea how to fix it.
Ok.
thanks,
Chris
>
> --alex
>
> >
> > thanks,
> >
> > Chris
> >
> > On 8/28/18 5:06 PM, serguei.spitsyn@oracle.com wrote:
> > > Sorry, forgot to add Chris - fixed now.
> > > Still it'd be good to get a confirmation from Chris.
> > >
> > > Thanks,
> > > Serguei
> > >
> > > On 8/28/18 16:38, Alex Menkov wrote:
> > > > Looks like you sent the message only to me :)
> > > > Anyway I've got 2nd review from Jc.
> > > >
> > > > --alex
> > > >
> > > > On 08/28/2018 16:20, serguei.spitsyn@oracle.com wrote:
> > > > > Hi Chris,
> > > > >
> > > > > I guess, Alex is waiting for your final thumbs up.
> > > > > Alex, please, fix me if it is wrong.
> > > > >
> > > > > Thanks,
> > > > > Serguei
> > > > >
> > > > >
> > > > > On 8/28/18 14:03, Alex Menkov wrote:
> > > > > > Need one more reviewer for the fix.
> > > > > >
> > > > > > --alex
> > > > > >
> > > > > > On 08/22/2018 10:18, serguei.spitsyn@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@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
> > > > > > > > > >
> > > > > > >
> > > > >
> > >
> >
> >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic