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

[Alex Menkov]

Hi Jc,

thank you for the review.
I've integrated proposed changes, going to send updated review request 
after addressing Serguei's notes.

--alex

On 08/17/2018 12:54, JC Beyler wrote:
> Hi Alex,
> 
> I looked at the webrev and saw these few nits:
> 
> 1) You could merge the eval methods no and make them into two categories 
> it seems: evaluationContains and evaluationDoesNotContain, for example:
> 
> + private void evaluationShouldContain(String expr, String pattern) {
> + List<String> reply = jdb.command(JdbCommand.eval(expr));
> + new 
> OutputAnalyzer(reply.stream().collect(Collectors.joining(lineSeparator)))
> + .shouldContain(pattern);
> + }
> 
> (and put that in JdbTest.java)
> 
>     - For 
> http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalInterfaceStatic.java.udiff.html 
> <http://cr.openjdk.java.net/%7Eamenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalInterfaceStatic.java.udiff.html>
> http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalArraysAsList.java.udiff.html 
> <http://cr.openjdk.java.net/%7Eamenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalArraysAsList.java.udiff.html>
> http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalArgs.java.udiff.html 
> <http://cr.openjdk.java.net/%7Eamenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalArgs.java.udiff.html>
> 
>        -> This would simplify these tests to only be using the same 
> methods instead of redefining them every time (though there is a need 
> for fixing one of the tests which I bring up in (2)
> 
> 2) 
> http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalArgs.java.udiff.html 
> <http://cr.openjdk.java.net/%7Eamenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalArgs.java.udiff.html> 
> does a replace before the evaluation, seems like it is a constant replace:
> 
> + List<String> reply = 
> jdb.command(JdbCommand.eval(expr.replace("$classname", DEBUGGEE_CLASS)));
> 
> Why not just replace the string directly in the test and be done with it once and for all instead of making the test more complex?
> 
> 
> Otherwise, it looks good to me (though I'm not an official reviewer),
> 
> Jc
> 
> 
> 
> On Thu, Aug 16, 2018 at 2:13 PM Alex Menkov <alexey.menkov at oracle.com 
> <mailto:alexey.menkov at oracle.com>> 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/
>     <http://cr.openjdk.java.net/%7Eamenkov/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
> 
> 
> 
> -- 
> 
> Thanks,
> Jc