RFR: JDK-8209604: [TEST] rewrite com/sun/jdi shell tests to java version - step2
Alex Menkov
alexey.menkov at oracle.com
Tue Aug 21 00:42:25 UTC 2018
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
More information about the serviceability-dev
mailing list