RFR: JDK-8210560: [TEST] convert com/sun/jdi redefineClass-related tests
Alex Menkov
alexey.menkov at oracle.com
Wed Sep 12 18:12:41 UTC 2018
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
>
More information about the serviceability-dev
mailing list