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

[David Holmes]

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
>