[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR JDK-8210243: [TEST] rewrite com/sun/jdi shell tests to java version - step3
From:       Chris Plummer <chris.plummer () oracle ! com>
Date:       2018-08-31 22:53:31
Message-ID: a9bd978c-97af-44bb-ff01-b7ac2a6b8be6 () oracle ! com
[Download RAW message or body]

Hi Alex,

Overall it looks good.

   159         // jdb prompt when debuggee is not started nor suspended after 
breakpoint

How about "and is not suspended". And I'm not so sure the "after 
breakpoint" is needed. Jdb always suspends after a breakpoint. 
Eventually the user does a "cont", and after that I think you always see 
the simple prompt "> " because after the "cont" the debuggee is not 
suspended anymore.

I'm just now realizing that there is a lot of replication of prompt 
related code in the vmTestBase version of Jdb.java. Maybe that's 
something we can address in the future.

thanks,

Chris

On 8/31/18 11:46 AM, Alex Menkov wrote:
> 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@oracle.com 
> > > <mailto:serguei.spitsyn@oracle.com> <serguei.spitsyn@oracle.com 
> > > <mailto:serguei.spitsyn@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


[prev in list] [next in list] [prev in thread] [next in thread]