Ping: RFR: JDK-8209604: [TEST] rewrite com/sun/jdi shell tests to java version - step2
Chris Plummer
chris.plummer at oracle.com
Thu Aug 30 02:08:20 UTC 2018
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 at 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 at 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 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
>>>>>>>>>>
>>>>>>>
>>>>>
>>>
>>
>>
More information about the serviceability-dev
mailing list