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

[serguei.spitsyn at oracle.com]

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
>>>