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

[serguei.spit...@oracle.com]

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.men...@oracle.com <mailto:alexey.men...@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