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


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

Reply via email to