RFR: JDK-8210560: [TEST] convert com/sun/jdi redefineClass-related tests

[serguei.spitsyn at oracle.com]

Hi Alex,

Looks good.
Thank you for the update!

Thanks,
Serguei


On 9/12/18 11:12, Alex Menkov wrote:
> Hi Serguei,
>
> Updated webrev:
> http://cr.openjdk.java.net/~amenkov/sh2java/redefineClasses1/webrev.03/
>
> --alex
>
> On 09/11/2018 22:15, serguei.spitsyn at oracle.com wrote:
>> Hi Alex,
>>
>> It looks good.
>>
>> Just some minor comments.
>>
>> http://cr.openjdk.java.net/~amenkov/sh2java/redefineClasses1/webrev.02/test/jdk/com/sun/jdi/lib/jdb/JdbTest.java.udiff.html 
>>
>>
>> + public LaunchOptions sourceFilename(String name) {
>> + sourceFilename = name;
>> + return this;
>> + }
>>
>>    A suggestion is to rename this method to setSourceFilename.
>>
>>
>> http://cr.openjdk.java.net/~amenkov/sh2java/redefineClasses1/webrev.02/test/jdk/com/sun/jdi/lib/jdb/ClassTransformer.java.html 
>>
>>
>>    53     public ClassTransformer fileName(String fileName) {
>>    54         this.fileName = fileName;
>>    55         return this;
>>    56     }
>>
>>    A suggestion is to rename this method to setFileName;
>>
>>    It would be better for the comment at lines 107-135 to use the form:
>>    /*
>>     *  ...
>>     */
>>
>> Thanks,
>> Serguei
>>
>>
>> On 9/11/18 12:28, Alex Menkov wrote:
>>> Hi Jc,
>>>
>>> Thank you for review.
>>> Updated webrev (fixed both issue noted):
>>> http://cr.openjdk.java.net/~amenkov/sh2java/redefineClasses1/webrev.02/
>>>
>>> --alex
>>>
>>> On 09/10/2018 21:00, JC Beyler wrote:
>>>> Hi Alex,
>>>>
>>>> I was looking at this and I have a few nits and questions:
>>>> - I saw two spots in the patch that had weird spaces:
>>>> test/jdk/com/sun/jdi/RedefineG.java
>>>>
>>>> +        super(  DEBUGGEE_CLASS,
>>>> +                "RedefineG.java");
>>>>
>>>> - Then test/jdk/com/sun/jdi/RedefineImplementor.java
>>>>
>>>> +        super(  RedefineImplementorTarg.class.getName(),
>>>> +                "RedefineImplementor.java");
>>>>
>>>>
>>>> - My only question is should we not be worried or care if both 
>>>> test/jdk/com/sun/jdi/lib/jdb/JdbTest.java and 
>>>> test/jdk/com/sun/jdi/lib/jdb/ClassTransformer.java have now ways 
>>>> that could provoke NPEs? Should the code not try to throw an 
>>>> exception and say: careful, you called this but didn't call that 
>>>> first? It would make misuse of the class easier to debug when 
>>>> writing new tests.
>>>>
>>>> (Example being: calling redefineClass but not using the right 
>>>> constructor)
>>>>
>>>> I know it is for testing only but if we start writing new tests, it 
>>>> could be useful to be a bit defensive?
>>>>
>>>>
>>>> Apart from that, looked good to me.
>>>>
>>>> Jc
>>>>
>>>>
>>>>
>>>> On Mon, Sep 10, 2018 at 4:40 PM Alex Menkov 
>>>> <alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com>> wrote:
>>>>
>>>>     Hi,
>>>>
>>>>     Please review a fix for
>>>> https://bugs.openjdk.java.net/browse/JDK-8210560
>>>>     webrev:
>>>> http://cr.openjdk.java.net/~amenkov/sh2java/redefineClasses1/webrev.01/ 
>>>>
>>>> <http://cr.openjdk.java.net/%7Eamenkov/sh2java/redefineClasses1/webrev.01/> 
>>>>
>>>>
>>>>     New class (ClassTransformer) was developed to implement simple 
>>>> class
>>>>     transformer for class redefinition (the same functionality as
>>>>     implemented by ShellScaffold.sh).
>>>>
>>>>     Both setBreakpoint and redefineClass require source file name, so
>>>>     JdbTest class was updated to accept it as ctor argument and use by
>>>>     corresponding methods.
>>>>
>>>>     --alex
>>>>
>>>>
>>>>
>>>> -- 
>>>>
>>>> Thanks,
>>>> Jc
>>