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

[Alex Menkov]

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