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

[Alex Menkov]

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
>