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

Chris Plummer chris.plummer at oracle.com
Wed Aug 29 06:58:02 UTC 2018 

Oops. Alex, not Daniil.

Chris

On 8/28/18 11:18 PM, 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?
>
>  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.
>
>   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.
>
>   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.
>
>   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".
>
> 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.
>
> 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