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

[JC Beyler]

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/~amenkov/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

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


-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180817/7d0c872a/attachment-0001.html>