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

[Alex Menkov]

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