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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Tue Sep 4 22:30:11 UTC 2018 

Okay, thanks!
Serguei


On 9/4/18 14:30, Alex Menkov wrote:
> Hi Serguei,
>
> sleepcmd is empty, so it's NOP.
>
> --alex
>
> On 09/04/2018 12:28, serguei.spitsyn at oracle.com wrote:
>> 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.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
>>>>
>>>>
>>>>
>>

More information about the serviceability-dev mailing list