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

[Chris Plummer]

Looks good.

Chris


On 9/4/18 11:01 AM, 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.spitsyn at oracle.com 
>>>>> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com 
>>>>> <mailto: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 
>>>>>
>>>>> <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
>>
>>
>>