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.men...@oracle.com
<mailto:alexey.men...@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
