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:18:14 UTC 2018 

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