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

Tue Sep 4 18:01:09 UTC 2018

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
> 
> 
> 