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

[serguei.spit...@oracle.com]

Hi Alex, It looks good. Just one minor question. http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.04/test/jdk/com/sun/jdi/NullLocalVariable.java.udiff.html -dojdbCmds() -{ - #set -x - cmd stop at badscope:4 ; $sleepcmd - runToBkpt ; $sleepcmd - cmd next ; $sleepcmd - cmd next ; $sleepcmd - cmd locals ; $sleepcmd - cmd allowExit cont -} The original test had a $sleepcmd after each jdb command. New code does not have anything like this. Is it intentional or overlooked? In fact, I do not remember and have just some guess what is the $sleepcmd. Thanks, Serguei On 9/4/18 11:01, Alex Menkov wrote: Hi Chris, fixed. Updated webrev: http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.04/ On 08/31/2018 15:53, Chris Plummer wrote: Hi Alex, Overall it looks good.   159     // jdb prompt when debuggee is not started nor suspended after breakpoint How about "and is not suspended". And I'm not so sure the "after breakpoint" is needed. Jdb always suspends after a breakpoint. Eventually the user does a "cont", and after that I think you always see the simple prompt "> " because after the "cont" the debuggee is not suspended anymore. I'm just now realizing that there is a lot of replication of prompt related code in the vmTestBase version of Jdb.java. Maybe that's something we can address in the future. Because the classes have similar functionality. Initially it was planned to use vmTestBase classes to shell->java test conversion, but I was told by SQE that vmTestBase tests have too many issues and need to be reworked (moving them to appropriate locations). --alex thanks, Chris On 8/31/18 11:46 AM, Alex Menkov wrote: The latest webrev: http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.03/ --alex On 08/31/2018 10:49, Alex Menkov wrote: Hi Jc, On 08/31/2018 10:25, JC Beyler wrote: 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 <http://cr.openjdk.java.net/%7Eamenkov/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?) Yes, agree - there is no sense to have the field. 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 <http://cr.openjdk.java.net/%7Eamenkov/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? The original issue (https://bugs.openjdk.java.net/browse/JDK-4467887) is about "more graceful message when user tries to evaluate a method such as length() or toString() as a field". So actually this "correct" command is not needed (I suppose the command is executed just because test example in the bug contains it for comparison) I'll do a test run and then upload new webrev. --alex Thanks, Jc On Thu, Aug 30, 2018 at 11:13 PM serguei.spit...@oracle.com <mailto:serguei.spit...@oracle.com> <serguei.spit...@oracle.com <mailto:serguei.spit...@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 <http://cr.openjdk.java.net/%7Eamenkov/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/ <http://cr.openjdk.java.net/%7Eamenkov/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