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

[JC Beyler]

Hi Alexey,

Apart from Serguei's comment above, I only saw two nits:

A) The shorthand

http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.01/test/jdk/com/sun/jdi/lib/jdb/JdbTest.java.udiff.html

  1) typo shortland -> shorthand
  2) could we just have a method instead of the shorthand? ie:
instead of:
+    protected final String debuggeeClass;   // shortland for
launchOptions.debuggeeClass

we have:
+    protected String debuggeeClass() { return launchOptions.debuggeeClass;
}

Somehow, even as a final, duplication of data seems weird to me. (On the
other hand, it seems you only use launchOptions.debuggeeClass in two spots
in the code and once you used the shorthand and once you didn't. So perhaps
no shorthand at all makes sense?)

B) Not testing the command that works?

http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.01/test/jdk/com/sun/jdi/NotAField.java.udiff.html
-> I'm surprised we are testing a command that should work and then a
command that should not but only are checking the failing command's output.
I know this is a direct translation of the test but it seems weird to not
be testing also the hashCode() call, no?

Thanks,
Jc



On Thu, Aug 30, 2018 at 11:13 PM serguei.spitsyn at oracle.com <
serguei.spitsyn at oracle.com> wrote:

> Hi Alex,
>
> It looks good in general but not sure I understand all the changes.
> Could you, please, tell a little bit more about the refactoring in the
> Jdb.java and JdbTest.java?
> I understand that you moved some code from Jdb.java to JdbTest.java.
> But it is hard to track all of these move details.
> What was the main refactoring logic?
>
> Some comments on this update:
>
>
> http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.01/test/jdk/com/sun/jdi/NullLocalVariable.java.frames.html
>
>   30  * @library /test/lib  31  * @compile -g JdbExprTest.java  32  * @run main/othervm JdbExprTest
>
>   Why the class 'JdbExprTest' is used above?
>   Should it be the 'NullLocalVariable' instead?
>
> Thanks,
> Serguei
>
>
> On 8/30/18 16:27, Alex Menkov wrote:
>
> Hi all,
>
> Please review a fix for
> https://bugs.openjdk.java.net/browse/JDK-8210243
> webrev:
> http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.01/
>
> The fix converts the following tests:
> - test/jdk/com/sun/jdi/JdbArgTest.sh
> - test/jdk/com/sun/jdi/JdbLockTest.sh
> - test/jdk/com/sun/jdi/JdbMissStep.sh
> - test/jdk/com/sun/jdi/JdbVarargsTest.sh
> - test/jdk/com/sun/jdi/MixedSuspendTest.sh
> - test/jdk/com/sun/jdi/NotAField.sh
> - test/jdk/com/sun/jdi/NullLocalVariable.sh
>
> JdbArgTest requires to run only jdb (without debuggee), so Jdb class was
> decoupled - it now contains only jdb logic, debuggee logic was moved to
> JdbTest.
>
> --alex
>
>
>

-- 

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