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

Alex Menkov alexey.menkov at oracle.com
Tue Sep 4 21:30:25 UTC 2018 

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